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

[Marcus Wolf]

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 :)
>