diff mbox series

[v3,23/24] gpio: remove the RW semaphore from the GPIO device

Message ID 20240208095920.8035-24-brgl@bgdev.pl
State New
Headers show
Series gpio: rework locking and object life-time control | expand

Commit Message

Bartosz Golaszewski Feb. 8, 2024, 9:59 a.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

With all accesses to gdev->chip being protected with SRCU, we can now
remove the RW-semaphore specific to the character device which
fullfilled the same role up to this point.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib-cdev.c | 1 -
 drivers/gpio/gpiolib.c      | 4 ----
 drivers/gpio/gpiolib.h      | 5 -----
 3 files changed, 10 deletions(-)

Comments

Kent Gibson Feb. 10, 2024, 5:37 a.m. UTC | #1
On Thu, Feb 08, 2024 at 10:59:19AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> With all accesses to gdev->chip being protected with SRCU, we can now
> remove the RW-semaphore specific to the character device which
> fullfilled the same role up to this point.
>

fulfilled

> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/gpiolib-cdev.c | 1 -
>  drivers/gpio/gpiolib.c      | 4 ----
>  drivers/gpio/gpiolib.h      | 5 -----
>  3 files changed, 10 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index ccdeed013f6b..9323b357df43 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -24,7 +24,6 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/poll.h>
>  #include <linux/rbtree.h>
> -#include <linux/rwsem.h>
>  #include <linux/seq_file.h>
>  #include <linux/spinlock.h>
>  #include <linux/timekeeping.h>

Shouldn't this be part of the rwsem -> srcu switch in the previous
patch?

Other than those nits, FWIW the series looks good to me.

Cheers,
Kent.
Bartosz Golaszewski Feb. 12, 2024, 9:53 a.m. UTC | #2
On Sat, Feb 10, 2024 at 6:37 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Feb 08, 2024 at 10:59:19AM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > With all accesses to gdev->chip being protected with SRCU, we can now
> > remove the RW-semaphore specific to the character device which
> > fullfilled the same role up to this point.
> >
>
> fulfilled
>
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> >  drivers/gpio/gpiolib-cdev.c | 1 -
> >  drivers/gpio/gpiolib.c      | 4 ----
> >  drivers/gpio/gpiolib.h      | 5 -----
> >  3 files changed, 10 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index ccdeed013f6b..9323b357df43 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -24,7 +24,6 @@
> >  #include <linux/pinctrl/consumer.h>
> >  #include <linux/poll.h>
> >  #include <linux/rbtree.h>
> > -#include <linux/rwsem.h>
> >  #include <linux/seq_file.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/timekeeping.h>
>
> Shouldn't this be part of the rwsem -> srcu switch in the previous
> patch?
>

That other patch was already huge. I figured this should be separate.

Bart

> Other than those nits, FWIW the series looks good to me.
>
> Cheers,
> Kent.
Kent Gibson Feb. 12, 2024, 9:57 a.m. UTC | #3
On Mon, Feb 12, 2024 at 10:53:07AM +0100, Bartosz Golaszewski wrote:
> On Sat, Feb 10, 2024 at 6:37 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Feb 08, 2024 at 10:59:19AM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > With all accesses to gdev->chip being protected with SRCU, we can now
> > > remove the RW-semaphore specific to the character device which
> > > fullfilled the same role up to this point.
> > >
> >
> > fulfilled
> >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > ---
> > >  drivers/gpio/gpiolib-cdev.c | 1 -
> > >  drivers/gpio/gpiolib.c      | 4 ----
> > >  drivers/gpio/gpiolib.h      | 5 -----
> > >  3 files changed, 10 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > > index ccdeed013f6b..9323b357df43 100644
> > > --- a/drivers/gpio/gpiolib-cdev.c
> > > +++ b/drivers/gpio/gpiolib-cdev.c
> > > @@ -24,7 +24,6 @@
> > >  #include <linux/pinctrl/consumer.h>
> > >  #include <linux/poll.h>
> > >  #include <linux/rbtree.h>
> > > -#include <linux/rwsem.h>
> > >  #include <linux/seq_file.h>
> > >  #include <linux/spinlock.h>
> > >  #include <linux/timekeeping.h>
> >
> > Shouldn't this be part of the rwsem -> srcu switch in the previous
> > patch?
> >
>
> That other patch was already huge. I figured this should be separate.
>

To be clear, I mean just this header removal, not the whole patch.

Cheers,
Kent.
Bartosz Golaszewski Feb. 12, 2024, 9:59 a.m. UTC | #4
On Mon, 12 Feb 2024 at 10:57, Kent Gibson <warthog618@gmail.com> wrote:
>
> On Mon, Feb 12, 2024 at 10:53:07AM +0100, Bartosz Golaszewski wrote:
> > On Sat, Feb 10, 2024 at 6:37 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Thu, Feb 08, 2024 at 10:59:19AM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > With all accesses to gdev->chip being protected with SRCU, we can now
> > > > remove the RW-semaphore specific to the character device which
> > > > fullfilled the same role up to this point.
> > > >
> > >
> > > fulfilled
> > >
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > > > ---
> > > >  drivers/gpio/gpiolib-cdev.c | 1 -
> > > >  drivers/gpio/gpiolib.c      | 4 ----
> > > >  drivers/gpio/gpiolib.h      | 5 -----
> > > >  3 files changed, 10 deletions(-)
> > > >
> > > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > > > index ccdeed013f6b..9323b357df43 100644
> > > > --- a/drivers/gpio/gpiolib-cdev.c
> > > > +++ b/drivers/gpio/gpiolib-cdev.c
> > > > @@ -24,7 +24,6 @@
> > > >  #include <linux/pinctrl/consumer.h>
> > > >  #include <linux/poll.h>
> > > >  #include <linux/rbtree.h>
> > > > -#include <linux/rwsem.h>
> > > >  #include <linux/seq_file.h>
> > > >  #include <linux/spinlock.h>
> > > >  #include <linux/timekeeping.h>
> > >
> > > Shouldn't this be part of the rwsem -> srcu switch in the previous
> > > patch?
> > >
> >
> > That other patch was already huge. I figured this should be separate.
> >
>
> To be clear, I mean just this header removal, not the whole patch.
>
> Cheers,
> Kent.

Ah, then it makes sense indeed. I'll fix it in tree.

Bart
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index ccdeed013f6b..9323b357df43 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -24,7 +24,6 @@ 
 #include <linux/pinctrl/consumer.h>
 #include <linux/poll.h>
 #include <linux/rbtree.h>
-#include <linux/rwsem.h>
 #include <linux/seq_file.h>
 #include <linux/spinlock.h>
 #include <linux/timekeeping.h>
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a14717a3e222..97829f0c8487 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -963,7 +963,6 @@  int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->line_state_notifier);
 	BLOCKING_INIT_NOTIFIER_HEAD(&gdev->device_notifier);
-	init_rwsem(&gdev->sem);
 
 	ret = init_srcu_struct(&gdev->srcu);
 	if (ret)
@@ -1102,8 +1101,6 @@  void gpiochip_remove(struct gpio_chip *gc)
 	struct gpio_device *gdev = gc->gpiodev;
 	unsigned int i;
 
-	down_write(&gdev->sem);
-
 	/* FIXME: should the legacy sysfs handling be moved to gpio_device? */
 	gpiochip_sysfs_unregister(gdev);
 	gpiochip_free_hogs(gc);
@@ -1142,7 +1139,6 @@  void gpiochip_remove(struct gpio_chip *gc)
 	 * gone.
 	 */
 	gcdev_unregister(gdev);
-	up_write(&gdev->sem);
 	gpio_device_put(gdev);
 }
 EXPORT_SYMBOL_GPL(gpiochip_remove);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index b3810f7d286a..07443d26cbca 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -16,7 +16,6 @@ 
 #include <linux/gpio/driver.h>
 #include <linux/module.h>
 #include <linux/notifier.h>
-#include <linux/rwsem.h>
 #include <linux/srcu.h>
 
 #define GPIOCHIP_NAME	"gpiochip"
@@ -46,9 +45,6 @@ 
  *                       requested, released or reconfigured
  * @device_notifier: used to notify character device wait queues about the GPIO
  *                   device being unregistered
- * @sem: protects the structure from a NULL-pointer dereference of @chip by
- *       user-space operations when the device gets unregistered during
- *       a hot-unplug event
  * @srcu: protects the pointer to the underlying GPIO chip
  * @pin_ranges: range of pins served by the GPIO driver
  *
@@ -73,7 +69,6 @@  struct gpio_device {
 	struct list_head        list;
 	struct blocking_notifier_head line_state_notifier;
 	struct blocking_notifier_head device_notifier;
-	struct rw_semaphore	sem;
 	struct srcu_struct	srcu;
 
 #ifdef CONFIG_PINCTRL