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