On Sun, Apr 08, 2018 at 05:22:46PM +0200, Marcus Wolf wrote:
> Regarding your patch, I did not understand, why you did not remove
> the mutex_lock in pi433_write. Wasn't it the goal to remove it?

Is it possible for more than one userspace program to open the pi433
device and send messages?  In that case more than one pi433_write
could be running and it needs to hold a mutex_lock before calling

> Below find a proposal of pi433_write function, I wrote on base
> of my outdated (!), private repo. It is not compiled and not tested.
> Since there is no more handling in case of an error (as well in the
> propsal as in your patch), I removed the error handling completely.
> I only do a test to detect proplems while writing to the tx_fifo,
> but (like in your patch) do nothing for solving, just printing a line.
> If this unexpected situation will occur (most probably never),
> the tx_fifo will be (and stay) out of sync until driver gets unloaded.
> We have to decide, whether we can stay with that. Like written above,
> I thinkt the benefits are great, the chance of such kind of error
> very low.
> What do you think?

Yes, if there is only one writer and it checks the available size,
kfifo_in should not fail.  The only problem might be copy_from_user
but perhaps that is also quite unlikely.  A workaround for that could
be to copy the data into a temporary kernel buffer first and than
start kfifo writes using only kernel memory.

> It could be discussed, whether it is better to return EMSGSIZE or
> EAGAIN on the first check. On the one hand, there is a problem with
> the message size, on the other hand (if message isn't generally too
> big) after a while, there should be some more space available in
> fifo, so EAGAIN may be better choice.

EAGAIN does seem better unless the message is too big to ever fit
in the kfifo.

>       if (retval != required ) {
>               dev_dbg(device->dev, "write to fifo failed, reason unknown, non 
> recoverable.");
>               return -EAGAIN;
>       }

Maybe this should be dev_warn or even dev_crit if the driver is not
usable anymore when this happens?  The error message should than also
be adjusted to EBADF or something similar.

devel mailing list

Reply via email to