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

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

[Valentin Vidic]

Removes TODO for tx_fifo_lock as tx_fifo is modified from
both pi433_tx_thread and pi433_write.

Fixes checkpatch warning:

  CHECK: struct mutex definition without comment

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
 drivers/staging/pi433/pi433_if.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index d1e0ddbc79ce..f6f106a3ff8e 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -87,7 +87,7 @@ struct pi433_device {
 
 	/* tx related values */
 	STRUCT_KFIFO_REC_1(MSG_FIFO_SIZE) tx_fifo;
-	struct mutex		tx_fifo_lock; // TODO: check, whether necessary or obsolete
+	struct mutex		tx_fifo_lock; /* serialize access to tx_fifo */
 	struct task_struct	*tx_task_struct;
 	wait_queue_head_t	tx_wait_queue;
 	u8			free_in_fifo;
@@ -100,7 +100,7 @@ struct pi433_device {
 	u32			rx_bytes_to_drop;
 	u32			rx_bytes_dropped;
 	unsigned int		rx_position;
-	struct mutex		rx_lock;
+	struct mutex		rx_lock; /* serialize read requests */
 	wait_queue_head_t	rx_wait_queue;
 
 	/* fifo wait queue */
-- 
2.16.2

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Marcus Wolf]

Hi Valentin,

could you please decribe in short words, why you think, that hte lock
isn't obsolete?

I wasn't sure, but close to remove the lock. That's why I putted the
comment.

Thanks,

Marcus

Am 23.03.2018 um 10:47 schrieb Valentin Vidic:
> Removes TODO for tx_fifo_lock as tx_fifo is modified from
> both pi433_tx_thread and pi433_write.
> 
> Fixes checkpatch warning:
> 
>   CHECK: struct mutex definition without comment
> 
> Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
> ---
>  drivers/staging/pi433/pi433_if.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index d1e0ddbc79ce..f6f106a3ff8e 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -87,7 +87,7 @@ struct pi433_device {
>  
>  	/* tx related values */
>  	STRUCT_KFIFO_REC_1(MSG_FIFO_SIZE) tx_fifo;
> -	struct mutex		tx_fifo_lock; // TODO: check, whether necessary or obsolete
> +	struct mutex		tx_fifo_lock; /* serialize access to tx_fifo */
>  	struct task_struct	*tx_task_struct;
>  	wait_queue_head_t	tx_wait_queue;
>  	u8			free_in_fifo;
> @@ -100,7 +100,7 @@ struct pi433_device {
>  	u32			rx_bytes_to_drop;
>  	u32			rx_bytes_dropped;
>  	unsigned int		rx_position;
> -	struct mutex		rx_lock;
> +	struct mutex		rx_lock; /* serialize read requests */
>  	wait_queue_head_t	rx_wait_queue;
>  
>  	/* fifo wait queue */
> 

-- 
Smarthome-Wolf UG (haftungsbeschränkt)
Helene-Lange-Weg 23
80637 München
Amtsgericht München, HRB 223529
Umastzsteuer-ID: DE304719911
Geschäftsführer: Marcus Wolf

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

[Valentin Vidic]

On Fri, Mar 23, 2018 at 11:22:39AM +0100, Marcus Wolf wrote:
> could you please decribe in short words, why you think, that hte lock
> isn't obsolete?
> 
> I wasn't sure, but close to remove the lock. That's why I putted the
> comment.

Sure, if pi433_tx_thread runs on one CPU it might be possible
to call pi433_write concurrently on another CPU and they would
both modify tx_fifo.  But maybe there is some other protection
in place that would prevent this?

-- 
Valentin
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Marcus Wolf]

Hi Valentin,

I had no time to work on the code for monthes now and the memorisation
of my thoughts when I was programming that (approx. one year ago) is
quite pale.

As far as I remember, I read something, that the fifo has an integrated
protection, so no external protection is needed. But absolutely unsure.

If I will find some time within the next days, I'll have a look at the
code and try to recall.

But the most important thing already took place: We started thinking
about it :-)

Thanks,

Marcus

Am 23.03.2018 um 12:42 schrieb Valentin Vidic:
> On Fri, Mar 23, 2018 at 11:22:39AM +0100, Marcus Wolf wrote:
>> could you please decribe in short words, why you think, that hte lock
>> isn't obsolete?
>>
>> I wasn't sure, but close to remove the lock. That's why I putted the
>> comment.
> 
> Sure, if pi433_tx_thread runs on one CPU it might be possible
> to call pi433_write concurrently on another CPU and they would
> both modify tx_fifo.  But maybe there is some other protection
> in place that would prevent this?
> 

-- 
Smarthome-Wolf UG (haftungsbeschränkt)
Helene-Lange-Weg 23
80637 München
Amtsgericht München, HRB 223529
Umastzsteuer-ID: DE304719911
Geschäftsführer: Marcus Wolf
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Valentin Vidic]

On Fri, Mar 23, 2018 at 04:38:50PM +0100, Marcus Wolf wrote:
> I had no time to work on the code for monthes now and the memorisation
> of my thoughts when I was programming that (approx. one year ago) is
> quite pale.
> 
> As far as I remember, I read something, that the fifo has an integrated
> protection, so no external protection is needed. But absolutely unsure.
> 
> If I will find some time within the next days, I'll have a look at the
> code and try to recall.
> 
> But the most important thing already took place: We started thinking
> about it :-)

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?

-- 
Valentin
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: pi433: cleanup tx_fifo locking

[Valentin Vidic]

pi433_write requires locking due to multiple kfifo writers.  After
acquiring the lock check if enough free space is available in the kfifo
to write the whole message. This check should prevent partial writes to
kfifo so kfifo_reset is not needed anymore.

pi433_tx_thread is the only kfifo reader so it does not require locking
after kfifo_reset is also removed.

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
---
 drivers/staging/pi433/pi433_if.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index d1e0ddbc79ce..97239b0eb322 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -87,7 +87,7 @@ struct pi433_device {
 
 	/* tx related values */
 	STRUCT_KFIFO_REC_1(MSG_FIFO_SIZE) tx_fifo;
-	struct mutex		tx_fifo_lock; // TODO: check, whether necessary or obsolete
+	struct mutex		tx_fifo_lock; /* serialize multiple writers */
 	struct task_struct	*tx_task_struct;
 	wait_queue_head_t	tx_wait_queue;
 	u8			free_in_fifo;
@@ -589,19 +589,15 @@ pi433_tx_thread(void *data)
 		 * - size of message
 		 * - message
 		 */
-		mutex_lock(&device->tx_fifo_lock);
-
 		retval = kfifo_out(&device->tx_fifo, &tx_cfg, sizeof(tx_cfg));
 		if (retval != sizeof(tx_cfg)) {
 			dev_dbg(device->dev, "reading tx_cfg from fifo failed: got %d byte(s), expected %d", retval, (unsigned int)sizeof(tx_cfg));
-			mutex_unlock(&device->tx_fifo_lock);
 			continue;
 		}
 
 		retval = kfifo_out(&device->tx_fifo, &size, sizeof(size_t));
 		if (retval != sizeof(size_t)) {
 			dev_dbg(device->dev, "reading msg size from fifo failed: got %d, expected %d", retval, (unsigned int)sizeof(size_t));
-			mutex_unlock(&device->tx_fifo_lock);
 			continue;
 		}
 
@@ -634,7 +630,6 @@ pi433_tx_thread(void *data)
 				   sizeof(device->buffer) - position);
 		dev_dbg(device->dev,
 			"read %d message byte(s) from fifo queue.", retval);
-		mutex_unlock(&device->tx_fifo_lock);
 
 		/* if rx is active, we need to interrupt the waiting for
 		 * incoming telegrams, to be able to send something.
@@ -818,7 +813,7 @@ pi433_write(struct file *filp, const char __user *buf,
 	struct pi433_instance	*instance;
 	struct pi433_device	*device;
 	int                     retval;
-	unsigned int		copied;
+	unsigned int		required, available, copied;
 
 	instance = filp->private_data;
 	device = instance->device;
@@ -833,6 +828,16 @@ pi433_write(struct file *filp, const char __user *buf,
 	 * - message
 	 */
 	mutex_lock(&device->tx_fifo_lock);
+
+	required = sizeof(instance->tx_cfg) + sizeof(size_t) + count;
+	available = kfifo_avail(&device->tx_fifo);
+	if (required > available) {
+		dev_dbg(device->dev, "write to fifo failed: %d bytes required but %d available",
+			required, available);
+		mutex_unlock(&device->tx_fifo_lock);
+		return -EAGAIN;
+	}
+
 	retval = kfifo_in(&device->tx_fifo, &instance->tx_cfg,
 			  sizeof(instance->tx_cfg));
 	if (retval != sizeof(instance->tx_cfg))
@@ -856,7 +861,6 @@ pi433_write(struct file *filp, const char __user *buf,
 
 abort:
 	dev_dbg(device->dev, "write to fifo failed: 0x%x", retval);
-	kfifo_reset(&device->tx_fifo); // TODO: maybe find a solution, not to discard already stored, valid entries
 	mutex_unlock(&device->tx_fifo_lock);
 	return -EAGAIN;
 }
-- 
2.16.2

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

[Marcus Wolf]

Hi Valentin,

> 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?

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.

Cheers,

Marcus

-- 
Smarthome-Wolf UG (haftungsbeschränkt)
Helene-Lange-Weg 23
80637 München
Amtsgericht München, HRB 223529
Umastzsteuer-ID: DE304719911
Geschäftsführer: Marcus Wolf

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

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

-- 
Valentin

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

[Marcus Wolf]

Hi Valentin,

I am not at home the next two weeks. So I can do a codereading on
Easter, but testing will not take place earlier then mid/end of April :-(

If you are interested, I can provide you an engineering sample of Pi433.

Cheers,

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

-- 
Smarthome-Wolf UG (haftungsbeschränkt)
Helene-Lange-Weg 23
80637 München
Amtsgericht München, HRB 223529
Umastzsteuer-ID: DE304719911
Geschäftsführer: Marcus Wolf

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

[Valentin Vidic]

On Sun, Mar 25, 2018 at 03:12:52PM +0200, Marcus Wolf wrote:
> I am not at home the next two weeks. So I can do a codereading on
> Easter, but testing will not take place earlier then mid/end of April :-(
> 
> If you are interested, I can provide you an engineering sample of Pi433.

Sure, let me know which version of Rpi it supports and if I need two
to test communication.

-- 
Valentin
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Marcus Wolf]

Hi Valentin,

The hardware of Pi433 is working with every Raspberry Pi (on zero, you
need to solder the GPIO-pins) and with several other fruits like banana
pi. The only thing is, that you need different versions of the driver,
according to the processor, mounted on your fruit.

If you'd like to test more then ther is no hang up or crash, you will
need a second party. You could use a 433MHz socket for testing TX or a
433 thermometer for testing RX. An example code for communication with a
socket is available on the Pi433 productpage (www.pi433.de). The sample
for the thermometer isn't published yet (as always lack of time).

If you want to test more deeply (using different features of the Rf69
chip or even do some long time testing, you have more options, if you
use to Pis with Pi433.

Just let me know, what you'd like to do and what equipment, you need.

Cheers,

Marcus

Am 25.03.2018 um 16:24 schrieb Valentin Vidic:
> On Sun, Mar 25, 2018 at 03:12:52PM +0200, Marcus Wolf wrote:
>> I am not at home the next two weeks. So I can do a codereading on
>> Easter, but testing will not take place earlier then mid/end of April :-(
>>
>> If you are interested, I can provide you an engineering sample of Pi433.
> 
> Sure, let me know which version of Rpi it supports and if I need two
> to test communication.
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Valentin Vidic]

On Sun, Apr 08, 2018 at 04:15:30PM +0200, Marcus Wolf wrote:
> The hardware of Pi433 is working with every Raspberry Pi (on zero, you
> need to solder the GPIO-pins) and with several other fruits like banana
> pi. The only thing is, that you need different versions of the driver,
> according to the processor, mounted on your fruit.
> 
> If you'd like to test more then ther is no hang up or crash, you will
> need a second party. You could use a 433MHz socket for testing TX or a
> 433 thermometer for testing RX. An example code for communication with a
> socket is available on the Pi433 productpage (www.pi433.de). The sample
> for the thermometer isn't published yet (as always lack of time).
> 
> If you want to test more deeply (using different features of the Rf69
> chip or even do some long time testing, you have more options, if you
> use to Pis with Pi433.
> 
> Just let me know, what you'd like to do and what equipment, you need.

At the moment I have Rpi2 but not any other 433MHz equipment, so I
could only do some basic testing unfortunately.  In case I get the
new Rpi3 some time soon I would be able to do something better.

-- 
Valentin
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Marcus Wolf]

Am 12.04.2018 um 19:15 schrieb Valentin Vidic:
> On Sun, Apr 08, 2018 at 04:15:30PM +0200, Marcus Wolf wrote:
>> The hardware of Pi433 is working with every Raspberry Pi (on zero, you
>> need to solder the GPIO-pins) and with several other fruits like banana
>> pi. The only thing is, that you need different versions of the driver,
>> according to the processor, mounted on your fruit.
>>
>> If you'd like to test more then ther is no hang up or crash, you will
>> need a second party. You could use a 433MHz socket for testing TX or a
>> 433 thermometer for testing RX. An example code for communication with a
>> socket is available on the Pi433 productpage (www.pi433.de). The sample
>> for the thermometer isn't published yet (as always lack of time).
>>
>> If you want to test more deeply (using different features of the Rf69
>> chip or even do some long time testing, you have more options, if you
>> use to Pis with Pi433.
>>
>> Just let me know, what you'd like to do and what equipment, you need.
> 
> At the moment I have Rpi2 but not any other 433MHz equipment, so I
> could only do some basic testing unfortunately.  In case I get the
> new Rpi3 some time soon I would be able to do something better.
> 
Hi Valentin,



let me know, what you like to have. For sure with just one station and
no other 433MHz equipment, options for testing are quite limited.



Marcus

-- 
Smarthome-Wolf UG (haftungsbeschränkt)
Helene-Lange-Weg 23
80637 München
Amtsgericht München, HRB 223529
Umastzsteuer-ID: DE304719911
Geschäftsführer: Marcus Wolf

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

[Valentin Vidic]

On Thu, Apr 19, 2018 at 11:25:19AM +0200, Marcus Wolf wrote:
> let me know, what you like to have. For sure with just one station and
> no other 433MHz equipment, options for testing are quite limited.

I can get Rpi3 and with two shields test 433MHz communication between
Rpi2 and Rpi3.

-- 
Valentin
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Marcus Wolf]

Hi!

So if you like, give me your address, and I'll send you two development
samples of Pi433.

Cheers,

Marcus

Am 19.04.2018 um 11:47 schrieb Valentin Vidic:
> On Thu, Apr 19, 2018 at 11:25:19AM +0200, Marcus Wolf wrote:
>> let me know, what you like to have. For sure with just one station and
>> no other 433MHz equipment, options for testing are quite limited.
> 
> I can get Rpi3 and with two shields test 433MHz communication between
> Rpi2 and Rpi3.
> 

-- 
Smarthome-Wolf UG (haftungsbeschränkt)
Helene-Lange-Weg 23
80637 München
Amtsgericht München, HRB 223529
Umastzsteuer-ID: DE304719911
Geschäftsführer: Marcus Wolf
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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 :)
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Valentin Vidic]

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
kfifo_in.

> 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.

-- 
Valentin

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

[Marcus Wolf]

Am 12.04.2018 um 20:46 schrieb Valentin Vidic:
> 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
> kfifo_in.

You are right. The write function is open for multiple userspace programs.
So mutex_lock schould remain.

>> 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.

For my feeling, that's a safe solution but most probably oversized.

>> 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.

>From my point of veiew, the warning is (in combination of the
size-check) the by far better solution then the fifo reset.

So all in all, I think, we should go with your proposal.

Unfortunaly still no time to setup my test environment with a current
version of the driver in order to give it a try :-(
Sorry,

Marcus
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2] staging: pi433: cleanup tx_fifo locking

[Valentin Vidic]

pi433_write requires locking due to multiple writers.  After acquiring
the lock check if enough free space is available in the kfifo to write
the whole message. This check should prevent partial writes to tx_fifo
so kfifo_reset is not needed anymore.

pi433_tx_thread is the only reader so it does not require locking
after kfifo_reset is removed.

Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
Reviewed-by: Marcus Wolf <marcus.wolf@wolf-entwicklungen.de>
---
v2: print a warning if partial fifo write happens

 drivers/staging/pi433/pi433_if.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index d1e0ddbc79ce..2a05eff88469 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -87,7 +87,7 @@ struct pi433_device {
 
 	/* tx related values */
 	STRUCT_KFIFO_REC_1(MSG_FIFO_SIZE) tx_fifo;
-	struct mutex		tx_fifo_lock; // TODO: check, whether necessary or obsolete
+	struct mutex		tx_fifo_lock; /* serialize userspace writers */
 	struct task_struct	*tx_task_struct;
 	wait_queue_head_t	tx_wait_queue;
 	u8			free_in_fifo;
@@ -589,19 +589,15 @@ pi433_tx_thread(void *data)
 		 * - size of message
 		 * - message
 		 */
-		mutex_lock(&device->tx_fifo_lock);
-
 		retval = kfifo_out(&device->tx_fifo, &tx_cfg, sizeof(tx_cfg));
 		if (retval != sizeof(tx_cfg)) {
 			dev_dbg(device->dev, "reading tx_cfg from fifo failed: got %d byte(s), expected %d", retval, (unsigned int)sizeof(tx_cfg));
-			mutex_unlock(&device->tx_fifo_lock);
 			continue;
 		}
 
 		retval = kfifo_out(&device->tx_fifo, &size, sizeof(size_t));
 		if (retval != sizeof(size_t)) {
 			dev_dbg(device->dev, "reading msg size from fifo failed: got %d, expected %d", retval, (unsigned int)sizeof(size_t));
-			mutex_unlock(&device->tx_fifo_lock);
 			continue;
 		}
 
@@ -634,7 +630,6 @@ pi433_tx_thread(void *data)
 				   sizeof(device->buffer) - position);
 		dev_dbg(device->dev,
 			"read %d message byte(s) from fifo queue.", retval);
-		mutex_unlock(&device->tx_fifo_lock);
 
 		/* if rx is active, we need to interrupt the waiting for
 		 * incoming telegrams, to be able to send something.
@@ -818,7 +813,7 @@ pi433_write(struct file *filp, const char __user *buf,
 	struct pi433_instance	*instance;
 	struct pi433_device	*device;
 	int                     retval;
-	unsigned int		copied;
+	unsigned int		required, available, copied;
 
 	instance = filp->private_data;
 	device = instance->device;
@@ -833,6 +828,16 @@ pi433_write(struct file *filp, const char __user *buf,
 	 * - message
 	 */
 	mutex_lock(&device->tx_fifo_lock);
+
+	required = sizeof(instance->tx_cfg) + sizeof(size_t) + count;
+	available = kfifo_avail(&device->tx_fifo);
+	if (required > available) {
+		dev_dbg(device->dev, "write to fifo failed: %d bytes required but %d available",
+			required, available);
+		mutex_unlock(&device->tx_fifo_lock);
+		return -EAGAIN;
+	}
+
 	retval = kfifo_in(&device->tx_fifo, &instance->tx_cfg,
 			  sizeof(instance->tx_cfg));
 	if (retval != sizeof(instance->tx_cfg))
@@ -855,8 +860,8 @@ pi433_write(struct file *filp, const char __user *buf,
 	return copied;
 
 abort:
-	dev_dbg(device->dev, "write to fifo failed: 0x%x", retval);
-	kfifo_reset(&device->tx_fifo); // TODO: maybe find a solution, not to discard already stored, valid entries
+	dev_warn(device->dev,
+		 "write to fifo failed, non recoverable: 0x%x", retval);
 	mutex_unlock(&device->tx_fifo_lock);
 	return -EAGAIN;
 }
-- 
2.17.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: pi433: cleanup tx_fifo locking

[Marcus Wolf]

Reviewed-by: Marcus Wolf <marcus.wolf@wolf-entwicklungen.de>

Am 19.04.2018 um 12:25 schrieb Valentin Vidic:
> pi433_write requires locking due to multiple writers.  After acquiring
> the lock check if enough free space is available in the kfifo to write
> the whole message. This check should prevent partial writes to tx_fifo
> so kfifo_reset is not needed anymore.
> 
> pi433_tx_thread is the only reader so it does not require locking
> after kfifo_reset is removed.
> 
> Signed-off-by: Valentin Vidic <Valentin.Vidic@CARNet.hr>
> ---
> v2: print a warning if partial fifo write happens
> 
>  drivers/staging/pi433/pi433_if.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index d1e0ddbc79ce..2a05eff88469 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -87,7 +87,7 @@ struct pi433_device {
>  
>  	/* tx related values */
>  	STRUCT_KFIFO_REC_1(MSG_FIFO_SIZE) tx_fifo;
> -	struct mutex		tx_fifo_lock; // TODO: check, whether necessary or obsolete
> +	struct mutex		tx_fifo_lock; /* serialize userspace writers */
>  	struct task_struct	*tx_task_struct;
>  	wait_queue_head_t	tx_wait_queue;
>  	u8			free_in_fifo;
> @@ -589,19 +589,15 @@ pi433_tx_thread(void *data)
>  		 * - size of message
>  		 * - message
>  		 */
> -		mutex_lock(&device->tx_fifo_lock);
> -
>  		retval = kfifo_out(&device->tx_fifo, &tx_cfg, sizeof(tx_cfg));
>  		if (retval != sizeof(tx_cfg)) {
>  			dev_dbg(device->dev, "reading tx_cfg from fifo failed: got %d byte(s), expected %d", retval, (unsigned int)sizeof(tx_cfg));
> -			mutex_unlock(&device->tx_fifo_lock);
>  			continue;
>  		}
>  
>  		retval = kfifo_out(&device->tx_fifo, &size, sizeof(size_t));
>  		if (retval != sizeof(size_t)) {
>  			dev_dbg(device->dev, "reading msg size from fifo failed: got %d, expected %d", retval, (unsigned int)sizeof(size_t));
> -			mutex_unlock(&device->tx_fifo_lock);
>  			continue;
>  		}
>  
> @@ -634,7 +630,6 @@ pi433_tx_thread(void *data)
>  				   sizeof(device->buffer) - position);
>  		dev_dbg(device->dev,
>  			"read %d message byte(s) from fifo queue.", retval);
> -		mutex_unlock(&device->tx_fifo_lock);
>  
>  		/* if rx is active, we need to interrupt the waiting for
>  		 * incoming telegrams, to be able to send something.
> @@ -818,7 +813,7 @@ pi433_write(struct file *filp, const char __user *buf,
>  	struct pi433_instance	*instance;
>  	struct pi433_device	*device;
>  	int                     retval;
> -	unsigned int		copied;
> +	unsigned int		required, available, copied;
>  
>  	instance = filp->private_data;
>  	device = instance->device;
> @@ -833,6 +828,16 @@ pi433_write(struct file *filp, const char __user *buf,
>  	 * - message
>  	 */
>  	mutex_lock(&device->tx_fifo_lock);
> +
> +	required = sizeof(instance->tx_cfg) + sizeof(size_t) + count;
> +	available = kfifo_avail(&device->tx_fifo);
> +	if (required > available) {
> +		dev_dbg(device->dev, "write to fifo failed: %d bytes required but %d available",
> +			required, available);
> +		mutex_unlock(&device->tx_fifo_lock);
> +		return -EAGAIN;
> +	}
> +
>  	retval = kfifo_in(&device->tx_fifo, &instance->tx_cfg,
>  			  sizeof(instance->tx_cfg));
>  	if (retval != sizeof(instance->tx_cfg))
> @@ -855,8 +860,8 @@ pi433_write(struct file *filp, const char __user *buf,
>  	return copied;
>  
>  abort:
> -	dev_dbg(device->dev, "write to fifo failed: 0x%x", retval);
> -	kfifo_reset(&device->tx_fifo); // TODO: maybe find a solution, not to discard already stored, valid entries
> +	dev_warn(device->dev,
> +		 "write to fifo failed, non recoverable: 0x%x", retval);
>  	mutex_unlock(&device->tx_fifo_lock);
>  	return -EAGAIN;
>  }
> 


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel