Re: [PATCH] staging: pi433: add descriptions for mutex locks

[Valentin Vidic]

On Fri, Mar 23, 2018 at 07:00:27PM +0100, Valentin Vidic wrote:
> You are right, here is what kfifo.h says:
> 
> /*
>  * Note about locking : There is no locking required until only * one reader
>  * and one writer is using the fifo and no kfifo_reset() will be * called
>  *  kfifo_reset_out() can be safely used, until it will be only called
>  * in the reader thread.
>  *  For multiple writer and one reader there is only a need to lock the writer.
>  * And vice versa for only one writer and multiple reader there is only a need
>  * to lock the reader.
>  */
> 
> In the case of pi433 there is only one reader (pi433_tx_thread) and
> there is no need for a lock there. But the char device (pi433_write)
> might have multiple writers so we leave the mutex just in that function?

Ah, but there is a kfifo_reset call in pi433_write that requires a
mutex for both readers and writers:

"Usage of kfifo_reset is dangerous. It should be only called when the
fifo is exclusived locked or when it is secured that no other thread is
accessing the fifo."

Also kfifo_reset_out would probably not help here:

"The usage of kfifo_reset_out is safe until it will be only called from
the reader thread and there is only one concurrent reader. Otherwise it
is dangerous and must be handled in the same way as kfifo_reset."

But I have an idea to remove this kfifo_reset call in pi433_write
handling partial message writes:

  kfifo_reset(&device->tx_fifo); // TODO: maybe find a solution, not to discard already stored, valid entries

The writer could acquire the lock and than use kfifo_avail to check if
there is enough space to write the whole message.  What do you think?

-- 
Valentin