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

[Marcus Wolf]

Reviewed-by: Marcus Wolf 

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 
> ---
> 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 mutextx_fifo_lock; // TODO: check, whether necessary 
> or obsolete
> + struct mutextx_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(>tx_fifo_lock);
> -
>   retval = kfifo_out(>tx_fifo, _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(>tx_fifo_lock);
>   continue;
>   }
>  
>   retval = kfifo_out(>tx_fifo, , 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(>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(>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 intcopied;
> + unsigned intrequired, 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(>tx_fifo_lock);
> +
> + required = sizeof(instance->tx_cfg) + sizeof(size_t) + count;
> + available = kfifo_avail(>tx_fifo);
> + if (required > available) {
> + dev_dbg(device->dev, "write to fifo failed: %d bytes required 
> but %d available",
> + required, available);
> + mutex_unlock(>tx_fifo_lock);
> + return -EAGAIN;
> + }
> +
>   retval = kfifo_in(>tx_fifo, >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(>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(>tx_fifo_lock);
>   return -EAGAIN;
>  }
> 


___
devel mailing list
de...@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 
---
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 mutextx_fifo_lock; // TODO: check, whether necessary 
or obsolete
+   struct mutextx_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(>tx_fifo_lock);
-
retval = kfifo_out(>tx_fifo, _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(>tx_fifo_lock);
continue;
}
 
retval = kfifo_out(>tx_fifo, , 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(>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(>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 intcopied;
+   unsigned intrequired, 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(>tx_fifo_lock);
+
+   required = sizeof(instance->tx_cfg) + sizeof(size_t) + count;
+   available = kfifo_avail(>tx_fifo);
+   if (required > available) {
+   dev_dbg(device->dev, "write to fifo failed: %d bytes required 
but %d available",
+   required, available);
+   mutex_unlock(>tx_fifo_lock);
+   return -EAGAIN;
+   }
+
retval = kfifo_in(>tx_fifo, >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(>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(>tx_fifo_lock);
return -EAGAIN;
 }
-- 
2.17.0

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