[patch 6/6] staging: speakup: Migrate acntsa, bns, dectlk and txprt to ttyio

[Okash Khawaja]

Hi,

On Sun, Feb 26, 2017 at 11:07 AM, Samuel Thibault
<samuel.thibault at ens-lyon.org> wrote:
> Hello,
>
> Samuel Thibault, on dim. 26 févr. 2017 03:48:32 +0100, wrote:
>> Samuel Thibault, on dim. 26 févr. 2017 02:50:05 +0100, wrote:
>> > Please note in your todo for the input part, that ttyio will have to
>> > call read_buff_add for each received character, when that method is
>> > not NULL (that's actually the only driver using it, so we will really
>> > need a tester for this exact driver).
>>
>> More precisely, it's the spk_ttyio_ldisc_ops->receive_buf2 method which
>> should call read_buff_add for each received character.
>>
>>
>> The step further will be implementing spk_ttyio_in and
>> spk_ttyio_in_nowait.  I believe the easiest way is the following:
>
> I forgot the part that stops the tty layer from sending characters:
>
>> - define an spk_ldisc_data structure containing just one character (buf),
>> and a semaphore.
>
> and one int (buf_free).
>
>> - on ldisc_open, allocate a pointer to such structure, set
>> tty->disc_data to point to it, and initialized the semaphore to 0
>> tokens.
>
> and initialize buf_free to 1.
>
>> - in the receive_buf2 method,
>>   - if read_buff_add is defined, just call it for each character and be
>>   done
>>   - otherwise, store the first received character in
>>     ((struct spk_ldisc_data *)tty->disc_data)->buf
>>     , call up() on the semaphore, and return 1 (to tell that you ate the
>>     character).
>
> Here, *just before* storing the character in buf, check buf_free: if
> it is 0, return 0. otherwise, call mb(), and continue with what I wrote
> above.
>
>> - in spk_serial_in, call down_timeout(usecs_to_jiffies(SPK_SERIAL_TIMEOUT)),
>>   - on success, copy the character stored in buf, then call tty_schedule_flip() and return the copy
>>   - on failure (timed out), return 0xff.
>>
>> - in spk_serial_in_nowait, call down_trylock(),
>>   - on success, copy the character stored in buf, then call tty_schedule_flip() and return the copy
>>   - on failure, return 0.
>
> In each case, right after the copy, call mb() and set buf_free to 1.
>
>
> Actually, these two function could be factorized to just one, which
> takes a long timeout parameter (in jiffies) and returns an int
> instead of a char, and uses down_trylock when the timeout is 0 and
> down_timeout otherwise, and returns -1 instead of 0 on failure. And then
> make spk_serial_in use it and return 0xff when getting -1, and make
> spk_serial_in_nowait return 0 when getting -1. Cleaning that returned
> value convention can be another patch later.
>

Here's another approach using atomic_t instead of semaphore but still
employing same logic.

- spk_ldisc_data contains two members: char buf and atomic_t buf_free
- ldisc_open initialises buf_free to 1
- in receive_buf2, if read_buff_add is not defined:
        - check if buf_free is zero then return zero and finish
        - call mb()
        - copy first character to buf in spk_ldisc_data
        - call atomic_dec(&buf_free)
        - return 1
- in spk_ttyio_in:
        - countdown for SPK_SERIAL_TIMEOUT usecs - similar to
spk_serial_in - and check for atomic_read(&buf_free) == 0
        - if timed out, return 0xff
        - otherwise call mb()
        - read the char stored in buf and call atomic_inc(&buf_free)
        - call tty_schedule_flip()
        - return the char

spk_ttyio_in_nowait will be similar and can be factorised into
spk_ttyio_in as you suggested.

Using this approach we can avoid using down_timeout() which may need
justification when merging upstream. I might be wrong here but there
seems to be a somewhat less acceptance for timeout while acquiring
locks. For example in case of mutex_lock_timeout:
http://lkml.iu.edu/hypermail/linux/kernel/0611.3/0254.html.

If however there are plans of making buf bigger than one char, then
this will require reworking as the above scheme overloads buf_free
both for mutual exclusion and to count free bytes in buffer.

Okash