[patch 6/7] staging: speakup: add send_xchar, tiocmset and input functionality for tty

[Okash Khawaja]

Hi,

Two of the mistakes are frankly embarassing (down_timeout and up
inside spk_ttyio_in) . At some point during testing with apollo and
ltlk harwdare, I had to import and modify the patches to suit 4.10
kernel as 4.11 was still not stable and giving problems with USB on my
computer. I remember fixing them during tests, which is why those
synths worked. But didn't merge them back.

I will fix this patch today.

Thanks,
Okash

On Thu, Apr 27, 2017 at 3:06 PM, Samuel Thibault
<samuel.thibault at ens-lyon.org> wrote:
> Hello,
>
> Okash Khawaja, on jeu. 13 avril 2017 18:41:34 +0100, wrote:
>> +static struct spk_synth *spk_ttyio_synth;
>> +/* TODO: make this part of synth and remove the global */
>
> Mmm, rather the other way round: add a synth pointer inside the
> spk_ldisc_data structure, so you can get the synth from
>
>> +static int spk_ttyio_receive_buf2(struct tty_struct *tty,
>
>
>> -             pr_err("Error registering line discipline.\n");
>> +             pr_err("speakup: Error registering line discipline.\n");
>
> Do not mix with unrelated changes :)
>
>> +static unsigned char ttyio_in(int timeout)
>> +{
>> +     struct spk_ldisc_data *ldisc_data =
>> +             (struct spk_ldisc_data *)speakup_tty->disc_data;
>> +     char rv;
>> +
>> +     if (!down_timeout(&ldisc_data->sem, usecs_to_jiffies(timeout))) {
>
> Err, this should actually be if (down_timeout(...) != 0) (error case),
> shouldn't it?
>
>> +             pr_warn("spk_ttyio: timeout (%d)  while waiting for input\n",
>> +                             timeout);
>> +             return 0xff;
>> +     }
>> +
>> +     rv = ldisc_data->buf;
>> +     /* Make sure we have read buf before we set buf_free to let
>> +      * the producer overwrite it */
>> +     mb();
>> +     ldisc_data->buf_free = true;
>> +     tty_schedule_flip(speakup_tty->port);
>
> Here, document that tty_schedule_flip lets the tty push more characters.
>
>> +     up(&ldisc_data->sem);
>
> Err, no, it's spk_ttyio_receive_buf2 (the producer) which does the up(),
> the consumer shouldn't do any up()!  This is really a resource-counting
> semaphore, not a mutex!
>
>> Index: linux-staging/drivers/staging/speakup/spk_priv.h
>> ===================================================================
>> --- linux-staging.orig/drivers/staging/speakup/spk_priv.h
>> +++ linux-staging/drivers/staging/speakup/spk_priv.h
>> @@ -39,6 +39,7 @@
>>  #endif
>>
>>  #define KT_SPKUP 15
>> +#define SPK_SYNTH_TIMEOUT 100000
>
> Please document the unit (us)
>
> Samuel