Reviewed-by: Marcus Wolf <marcus.wolf@xxxxxxxxxxxxxxxxxxxxx>
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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
