Hi Valentin, like promissed, I finally had a deeper look to your proposal with kfifo_avail and your patch from 24th of March. In principle, I think it is a very nice idea, and we should try to implement it. But there is a snag: There is no guarantee, that kfifo_in will only fail, if the fifo is full. If there will be any another reason for kfifo_in to fail, with the new implementation there will be no handling for that. But I think the chance of such an situation is low to impossible and the win in simplicity of the code is really great. 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? 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? 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. static ssize_t pi433_write(struct file *filp, const char __user *buf, size_t count, loff_t *f_pos) { struct pi433_instance *instance; struct pi433_device *device; int required, copied, retval; instance = filp->private_data; device = instance->device; /* check whether there is enough space available in tx_fifo */ required = sizeof(instance->tx_cfg) + sizeof(size_t) + count; if ( num_of_bytes_to_store_in_fifo > kfifo_avail(&device->tx_fifo) ) { dev_dbg(device->dev, "Not enough space in fifo. Need %d, but only have" , required , kfifo_avail(&device->tx_fifo) ); return -EMSGSIZE; } /* write the following sequence into fifo: * - tx_cfg * - size of message * - message */ retval = kfifo_in(&device->tx_fifo, &instance->tx_cfg, sizeof(instance->tx_cfg)); retval += kfifo_in (&device->tx_fifo, &count, sizeof(size_t)); retval += kfifo_from_user(&device->tx_fifo, buf, count, &copied); if (retval != required ) { dev_dbg(device->dev, "write to fifo failed, reason unknown, non recoverable."); return -EAGAIN; } /* start transfer */ wake_up_interruptible(&device->tx_wait_queue); dev_dbg(device->dev, "write: generated new msg with %d bytes.", copied); return 0; } Hope this helps :-) Marcus Am 25.03.2018 um 15:09 schrieb Valentin Vidic: > On Sun, Mar 25, 2018 at 03:00:09PM +0200, Marcus Wolf wrote: >> Unfortunaly I can't find the time to have a closer look on the code this >> weekend - still busy with tax stuff :-( >> >> Idea sounds great. I'll try to look at the code and think about it >> during Easter hollidays. > > No problem, there is no hurry. But please do test the patch I sent yesterday: > > [PATCH] staging: pi433: cleanup tx_fifo locking > > As I don't have the hardware this is just compile tested now :) > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel