Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
From: Bartosz Golaszewski <brgl@bgdev.pl>
To: Kent Gibson <warthog618@gmail.com>,
	Linus Walleij <linus.walleij@linaro.org>
Cc: linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>
Subject: [PATCH v2 07/11] gpiolib: rework the locking mechanism for lineevent kfifo
Date: Wed,  4 Dec 2019 16:59:10 +0100
Message-ID: <20191204155912.17590-8-brgl@bgdev.pl> (raw)
In-Reply-To: <20191204155912.17590-1-brgl@bgdev.pl>

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The read_lock mutex is supposed to prevent collisions between reading
and writing to the line event kfifo but it's actually only taken when
the events are being read from it.

Drop the mutex entirely and reuse the spinlock made available to us in
the waitqueue struct. Take the lock whenever the fifo is modified or
inspected. Drop the call to kfifo_to_user() and instead first extract
the new element from kfifo when the lock is taken and only then pass
it on to the user after the spinlock is released.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpiolib.c | 54 ++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b7043946c029..43f90eca6d45 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -788,8 +788,6 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
  * @irq: the interrupt that trigger in response to events on this GPIO
  * @wait: wait queue that handles blocking reads of events
  * @events: KFIFO for the GPIO events
- * @read_lock: mutex lock to protect reads from colliding with adding
- * new events to the FIFO
  * @timestamp: cache for the timestamp storing it between hardirq
  * and IRQ thread, used to bring the timestamp close to the actual
  * event
@@ -802,7 +800,6 @@ struct lineevent_state {
 	int irq;
 	wait_queue_head_t wait;
 	DECLARE_KFIFO(events, struct gpioevent_data, 16);
-	struct mutex read_lock;
 	u64 timestamp;
 };
 
@@ -818,8 +815,10 @@ static __poll_t lineevent_poll(struct file *filep,
 
 	poll_wait(filep, &le->wait, wait);
 
+	spin_lock(&le->wait.lock);
 	if (!kfifo_is_empty(&le->events))
 		events = EPOLLIN | EPOLLRDNORM;
+	spin_unlock(&le->wait.lock);
 
 	return events;
 }
@@ -831,43 +830,47 @@ static ssize_t lineevent_read(struct file *filep,
 			      loff_t *f_ps)
 {
 	struct lineevent_state *le = filep->private_data;
-	unsigned int copied;
+	struct gpioevent_data event;
 	int ret;
 
-	if (count < sizeof(struct gpioevent_data))
+	if (count < sizeof(event))
 		return -EINVAL;
 
-	do {
+	for (;;) {
+		spin_lock(&le->wait.lock);
 		if (kfifo_is_empty(&le->events)) {
-			if (filep->f_flags & O_NONBLOCK)
+			if (filep->f_flags & O_NONBLOCK) {
+				spin_unlock(&le->wait.lock);
 				return -EAGAIN;
+			}
 
-			ret = wait_event_interruptible(le->wait,
+			ret = wait_event_interruptible_locked(le->wait,
 					!kfifo_is_empty(&le->events));
-			if (ret)
+			if (ret) {
+				spin_unlock(&le->wait.lock);
 				return ret;
-		}
+			}
 
-		if (mutex_lock_interruptible(&le->read_lock))
-			return -ERESTARTSYS;
-		ret = kfifo_to_user(&le->events, buf, count, &copied);
-		mutex_unlock(&le->read_lock);
+		}
 
-		if (ret)
-			return ret;
+		ret = kfifo_out(&le->events, &event, 1);
+		spin_unlock(&le->wait.lock);
+		if (ret == 1)
+			break;
 
 		/*
-		 * If we couldn't read anything from the fifo (a different
-		 * thread might have been faster) we either return -EAGAIN if
-		 * the file descriptor is non-blocking, otherwise we go back to
-		 * sleep and wait for more data to arrive.
+		 * We should never get here since we're holding the lock from
+		 * the moment we noticed a new event until calling kfifo_out()
+		 * but we must check the return value. On the off-chance - just
+		 * go back to waiting.
 		 */
-		if (copied == 0 && (filep->f_flags & O_NONBLOCK))
-			return -EAGAIN;
+	}
 
-	} while (copied == 0);
+	ret = copy_to_user(buf, &event, sizeof(event));
+	if (ret)
+		return -EFAULT;
 
-	return copied;
+	return sizeof(event);
 }
 
 static int lineevent_release(struct inode *inode, struct file *filep)
@@ -969,7 +972,7 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
 		return IRQ_NONE;
 	}
 
-	ret = kfifo_put(&le->events, ge);
+	ret = kfifo_in_spinlocked(&le->events, &ge, 1, &le->wait.lock);
 	if (ret)
 		wake_up_poll(&le->wait, EPOLLIN);
 
@@ -1084,7 +1087,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
 
 	INIT_KFIFO(le->events);
 	init_waitqueue_head(&le->wait);
-	mutex_init(&le->read_lock);
 
 	/* Request a thread to read the events */
 	ret = request_threaded_irq(le->irq,
-- 
2.23.0


  parent reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-04 15:59 [PATCH v2 00/11] gpiolib: add an ioctl() for monitoring line status changes Bartosz Golaszewski
2019-12-04 15:59 ` [PATCH v2 01/11] gpiolib: use 'unsigned int' instead of 'unsigned' in gpio_set_config() Bartosz Golaszewski
2019-12-04 22:18   ` Andy Shevchenko
2019-12-04 15:59 ` [PATCH v2 02/11] gpiolib: have a single place of calling set_config() Bartosz Golaszewski
2019-12-04 22:18   ` Andy Shevchenko
2019-12-04 15:59 ` [PATCH v2 03/11] gpiolib: convert the type of hwnum to unsigned int in gpiochip_get_desc() Bartosz Golaszewski
2019-12-04 22:19   ` Andy Shevchenko
2019-12-04 15:59 ` [PATCH v2 04/11] gpiolib: use gpiochip_get_desc() in linehandle_create() Bartosz Golaszewski
2019-12-04 22:20   ` Andy Shevchenko
2019-12-04 15:59 ` [PATCH v2 05/11] gpiolib: use gpiochip_get_desc() in lineevent_create() Bartosz Golaszewski
2019-12-04 22:20   ` Andy Shevchenko
2019-12-04 15:59 ` [PATCH v2 06/11] gpiolib: use gpiochip_get_desc() in gpio_ioctl() Bartosz Golaszewski
2019-12-04 16:25   ` Rainer Sickinger
2019-12-04 22:21   ` Andy Shevchenko
2019-12-04 15:59 ` Bartosz Golaszewski [this message]
2019-12-04 22:25   ` [PATCH v2 07/11] gpiolib: rework the locking mechanism for lineevent kfifo Andy Shevchenko
2019-12-05  9:31     ` Bartosz Golaszewski
2019-12-05 10:22       ` Andy Shevchenko
2019-12-04 15:59 ` [PATCH v2 08/11] gpiolib: emit a debug message when adding events to a full kfifo Bartosz Golaszewski
2019-12-04 22:28   ` Andy Shevchenko
2019-12-19 16:22     ` Bartosz Golaszewski
2019-12-04 15:59 ` [PATCH v2 09/11] gpiolib: provide a dedicated function for setting lineinfo Bartosz Golaszewski
2019-12-04 22:30   ` Andy Shevchenko
2019-12-05  9:28     ` Bartosz Golaszewski
2019-12-05 10:20       ` Andy Shevchenko
2019-12-05 13:45         ` Bartosz Golaszewski
2019-12-05 16:47           ` Andy Shevchenko

Reply instructions:

You may reply publically 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=20191204155912.17590-8-brgl@bgdev.pl \
    --to=brgl@bgdev.pl \
    --cc=bgolaszewski@baylibre.com \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=warthog618@gmail.com \
    /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

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org
	public-inbox-index linux-gpio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git