From: Marcus Wolf <marcus.wolf@smarthome-wolf.de>
To: Valentin Vidic <Valentin.Vidic@CARNet.hr>
Cc: devel@driverdev.osuosl.org, "Derek Robson" <robsonde@gmail.com>,
"Michael Panzlaff" <michael.panzlaff@fau.de>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Luca Söthe" <luca@acul.me>,
linux-kernel@vger.kernel.org,
"Marcin Ciupak" <marcin.s.ciupak@gmail.com>,
"Simon Sandström" <simon@nikanor.nu>
Subject: Re: [PATCH v2] staging: pi433: cleanup tx_fifo locking
Date: Thu, 19 Apr 2018 12:40:14 +0200 [thread overview]
Message-ID: <20b592c2-33b1-f2f6-875e-39402e5ab058@smarthome-wolf.de> (raw)
In-Reply-To: <20180419102530.5927-1-Valentin.Vidic@CARNet.hr>
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
prev parent reply other threads:[~2018-04-19 10:40 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-23 9:47 [PATCH] staging: pi433: add descriptions for mutex locks Valentin Vidic
2018-03-23 10:22 ` Marcus Wolf
2018-03-23 11:42 ` Valentin Vidic
2018-03-23 15:38 ` Marcus Wolf
2018-03-23 18:00 ` Valentin Vidic
2018-03-23 22:18 ` Valentin Vidic
2018-03-24 22:09 ` [PATCH] staging: pi433: cleanup tx_fifo locking Valentin Vidic
2018-03-25 13:00 ` [PATCH] staging: pi433: add descriptions for mutex locks Marcus Wolf
2018-03-25 13:09 ` Valentin Vidic
2018-03-25 13:12 ` Marcus Wolf
2018-03-25 14:24 ` Valentin Vidic
2018-04-08 14:15 ` Marcus Wolf
2018-04-12 17:15 ` Valentin Vidic
2018-04-19 9:25 ` Marcus Wolf
2018-04-19 9:47 ` Valentin Vidic
2018-04-19 9:55 ` Marcus Wolf
2018-04-08 15:22 ` Marcus Wolf
2018-04-12 18:46 ` Valentin Vidic
2018-04-19 9:33 ` Marcus Wolf
2018-04-19 10:25 ` [PATCH v2] staging: pi433: cleanup tx_fifo locking Valentin Vidic
2018-04-19 10:40 ` Marcus Wolf [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20b592c2-33b1-f2f6-875e-39402e5ab058@smarthome-wolf.de \
--to=marcus.wolf@smarthome-wolf.de \
--cc=Valentin.Vidic@CARNet.hr \
--cc=devel@driverdev.osuosl.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luca@acul.me \
--cc=marcin.s.ciupak@gmail.com \
--cc=michael.panzlaff@fau.de \
--cc=robsonde@gmail.com \
--cc=simon@nikanor.nu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).