From: Bartosz Golaszewski <[email protected]>
We used a per-descriptor SRCU struct in order to not impose a wait with
synchronize_srcu() for descriptor X on read-only operations of
descriptor Y. Now that we no longer call synchronize_srcu() on
descriptor label change but only when releasing descriptor resources, we
can use a single SRCU structure for all GPIO descriptors in a given chip.
Suggested-by: Paul E. McKenney <[email protected]>
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 2 +-
drivers/gpio/gpiolib.c | 41 +++++++++++++++++--------------------
drivers/gpio/gpiolib.h | 10 ++++-----
3 files changed, 25 insertions(+), 28 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index d09c7d728365..fea149ae7774 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -2351,7 +2351,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
dflags = READ_ONCE(desc->flags);
- scoped_guard(srcu, &desc->srcu) {
+ scoped_guard(srcu, &desc->gdev->desc_srcu) {
label = gpiod_get_label(desc);
if (label && test_bit(FLAG_REQUESTED, &dflags))
strscpy(info->consumer, label,
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 2fa3756c9073..fa50db0c3605 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -112,8 +112,8 @@ const char *gpiod_get_label(struct gpio_desc *desc)
if (!test_bit(FLAG_REQUESTED, &flags))
return NULL;
- label = srcu_dereference_check(desc->label, &desc->srcu,
- srcu_read_lock_held(&desc->srcu));
+ label = srcu_dereference_check(desc->label, &desc->gdev->desc_srcu,
+ srcu_read_lock_held(&desc->gdev->desc_srcu));
return label->str;
}
@@ -138,7 +138,7 @@ static int desc_set_label(struct gpio_desc *desc, const char *label)
old = rcu_replace_pointer(desc->label, new, 1);
if (old)
- call_srcu(&desc->srcu, &old->rh, desc_free_label);
+ call_srcu(&desc->gdev->desc_srcu, &old->rh, desc_free_label);
return 0;
}
@@ -709,13 +709,10 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
static void gpiodev_release(struct device *dev)
{
struct gpio_device *gdev = to_gpio_device(dev);
- unsigned int i;
- for (i = 0; i < gdev->ngpio; i++) {
- /* Free pending label. */
- synchronize_srcu(&gdev->descs[i].srcu);
- cleanup_srcu_struct(&gdev->descs[i].srcu);
- }
+ /* Call pending kfree()s for descriptor labels. */
+ synchronize_srcu(&gdev->desc_srcu);
+ cleanup_srcu_struct(&gdev->desc_srcu);
ida_free(&gpio_ida, gdev->id);
kfree_const(gdev->label);
@@ -992,6 +989,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
if (ret)
goto err_remove_from_list;
+ ret = init_srcu_struct(&gdev->desc_srcu);
+ if (ret)
+ goto err_cleanup_gdev_srcu;
+
#ifdef CONFIG_PINCTRL
INIT_LIST_HEAD(&gdev->pin_ranges);
#endif
@@ -999,23 +1000,19 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
if (gc->names) {
ret = gpiochip_set_desc_names(gc);
if (ret)
- goto err_cleanup_gdev_srcu;
+ goto err_cleanup_desc_srcu;
}
ret = gpiochip_set_names(gc);
if (ret)
- goto err_cleanup_gdev_srcu;
+ goto err_cleanup_desc_srcu;
ret = gpiochip_init_valid_mask(gc);
if (ret)
- goto err_cleanup_gdev_srcu;
+ goto err_cleanup_desc_srcu;
for (desc_index = 0; desc_index < gc->ngpio; desc_index++) {
struct gpio_desc *desc = &gdev->descs[desc_index];
- ret = init_srcu_struct(&desc->srcu);
- if (ret)
- goto err_cleanup_desc_srcu;
-
if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
assign_bit(FLAG_IS_OUT,
&desc->flags, !gc->get_direction(gc, desc_index));
@@ -1027,7 +1024,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
ret = of_gpiochip_add(gc);
if (ret)
- goto err_cleanup_desc_srcu;
+ goto err_free_valid_mask;
ret = gpiochip_add_pin_ranges(gc);
if (ret)
@@ -1074,10 +1071,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
gpiochip_remove_pin_ranges(gc);
err_remove_of_chip:
of_gpiochip_remove(gc);
-err_cleanup_desc_srcu:
- while (desc_index--)
- cleanup_srcu_struct(&gdev->descs[desc_index].srcu);
+err_free_valid_mask:
gpiochip_free_valid_mask(gc);
+err_cleanup_desc_srcu:
+ cleanup_srcu_struct(&gdev->desc_srcu);
err_cleanup_gdev_srcu:
cleanup_srcu_struct(&gdev->srcu);
err_remove_from_list:
@@ -2407,7 +2404,7 @@ char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset)
if (!test_bit(FLAG_REQUESTED, &desc->flags))
return NULL;
- guard(srcu)(&desc->srcu);
+ guard(srcu)(&desc->gdev->desc_srcu);
label = kstrdup(gpiod_get_label(desc), GFP_KERNEL);
if (!label)
@@ -4798,7 +4795,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
}
for_each_gpio_desc(gc, desc) {
- guard(srcu)(&desc->srcu);
+ guard(srcu)(&desc->gdev->desc_srcu);
if (test_bit(FLAG_REQUESTED, &desc->flags)) {
gpiod_get_direction(desc);
is_out = test_bit(FLAG_IS_OUT, &desc->flags);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 69a353c789f0..8e0e211ebf08 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -31,6 +31,7 @@
* @chip: pointer to the corresponding gpiochip, holding static
* data for this device
* @descs: array of ngpio descriptors.
+ * @desc_srcu: ensures consistent state of GPIO descriptors exposed to users
* @ngpio: the number of GPIO lines on this GPIO device, equal to the size
* of the @descs array.
* @can_sleep: indicate whether the GPIO chip driver's callbacks can sleep
@@ -61,6 +62,7 @@ struct gpio_device {
struct module *owner;
struct gpio_chip __rcu *chip;
struct gpio_desc *descs;
+ struct srcu_struct desc_srcu;
int base;
u16 ngpio;
bool can_sleep;
@@ -150,7 +152,6 @@ struct gpio_desc_label {
* @label: Name of the consumer
* @name: Line name
* @hog: Pointer to the device node that hogs this line (if any)
- * @srcu: SRCU struct protecting the label pointer.
*
* These are obtained using gpiod_get() and are preferable to the old
* integer-based handles.
@@ -188,7 +189,6 @@ struct gpio_desc {
#ifdef CONFIG_OF_DYNAMIC
struct device_node *hog;
#endif
- struct srcu_struct srcu;
};
#define gpiod_not_found(desc) (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
@@ -256,7 +256,7 @@ static inline int gpio_chip_hwgpio(const struct gpio_desc *desc)
#define gpiod_err(desc, fmt, ...) \
do { \
- scoped_guard(srcu, &desc->srcu) { \
+ scoped_guard(srcu, &desc->gdev->desc_srcu) { \
pr_err("gpio-%d (%s): " fmt, desc_to_gpio(desc), \
gpiod_get_label(desc) ? : "?", ##__VA_ARGS__); \
} \
@@ -264,7 +264,7 @@ do { \
#define gpiod_warn(desc, fmt, ...) \
do { \
- scoped_guard(srcu, &desc->srcu) { \
+ scoped_guard(srcu, &desc->gdev->desc_srcu) { \
pr_warn("gpio-%d (%s): " fmt, desc_to_gpio(desc), \
gpiod_get_label(desc) ? : "?", ##__VA_ARGS__); \
} \
@@ -272,7 +272,7 @@ do { \
#define gpiod_dbg(desc, fmt, ...) \
do { \
- scoped_guard(srcu, &desc->srcu) { \
+ scoped_guard(srcu, &desc->gdev->desc_srcu) { \
pr_debug("gpio-%d (%s): " fmt, desc_to_gpio(desc), \
gpiod_get_label(desc) ? : "?", ##__VA_ARGS__); \
} \
--
2.40.1
On Tue, May 07, 2024 at 07:24:14PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> We used a per-descriptor SRCU struct in order to not impose a wait with
> synchronize_srcu() for descriptor X on read-only operations of
> descriptor Y. Now that we no longer call synchronize_srcu() on
> descriptor label change but only when releasing descriptor resources, we
> can use a single SRCU structure for all GPIO descriptors in a given chip.
>
> Suggested-by: Paul E. McKenney <[email protected]>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
Acked-by: Paul E. McKenney <[email protected]>
> ---
> drivers/gpio/gpiolib-cdev.c | 2 +-
> drivers/gpio/gpiolib.c | 41 +++++++++++++++++--------------------
> drivers/gpio/gpiolib.h | 10 ++++-----
> 3 files changed, 25 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index d09c7d728365..fea149ae7774 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -2351,7 +2351,7 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
>
> dflags = READ_ONCE(desc->flags);
>
> - scoped_guard(srcu, &desc->srcu) {
> + scoped_guard(srcu, &desc->gdev->desc_srcu) {
> label = gpiod_get_label(desc);
> if (label && test_bit(FLAG_REQUESTED, &dflags))
> strscpy(info->consumer, label,
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 2fa3756c9073..fa50db0c3605 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -112,8 +112,8 @@ const char *gpiod_get_label(struct gpio_desc *desc)
> if (!test_bit(FLAG_REQUESTED, &flags))
> return NULL;
>
> - label = srcu_dereference_check(desc->label, &desc->srcu,
> - srcu_read_lock_held(&desc->srcu));
> + label = srcu_dereference_check(desc->label, &desc->gdev->desc_srcu,
> + srcu_read_lock_held(&desc->gdev->desc_srcu));
>
> return label->str;
> }
> @@ -138,7 +138,7 @@ static int desc_set_label(struct gpio_desc *desc, const char *label)
>
> old = rcu_replace_pointer(desc->label, new, 1);
> if (old)
> - call_srcu(&desc->srcu, &old->rh, desc_free_label);
> + call_srcu(&desc->gdev->desc_srcu, &old->rh, desc_free_label);
>
> return 0;
> }
> @@ -709,13 +709,10 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
> static void gpiodev_release(struct device *dev)
> {
> struct gpio_device *gdev = to_gpio_device(dev);
> - unsigned int i;
>
> - for (i = 0; i < gdev->ngpio; i++) {
> - /* Free pending label. */
> - synchronize_srcu(&gdev->descs[i].srcu);
> - cleanup_srcu_struct(&gdev->descs[i].srcu);
> - }
> + /* Call pending kfree()s for descriptor labels. */
> + synchronize_srcu(&gdev->desc_srcu);
> + cleanup_srcu_struct(&gdev->desc_srcu);
>
> ida_free(&gpio_ida, gdev->id);
> kfree_const(gdev->label);
> @@ -992,6 +989,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> if (ret)
> goto err_remove_from_list;
>
> + ret = init_srcu_struct(&gdev->desc_srcu);
> + if (ret)
> + goto err_cleanup_gdev_srcu;
> +
> #ifdef CONFIG_PINCTRL
> INIT_LIST_HEAD(&gdev->pin_ranges);
> #endif
> @@ -999,23 +1000,19 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> if (gc->names) {
> ret = gpiochip_set_desc_names(gc);
> if (ret)
> - goto err_cleanup_gdev_srcu;
> + goto err_cleanup_desc_srcu;
> }
> ret = gpiochip_set_names(gc);
> if (ret)
> - goto err_cleanup_gdev_srcu;
> + goto err_cleanup_desc_srcu;
>
> ret = gpiochip_init_valid_mask(gc);
> if (ret)
> - goto err_cleanup_gdev_srcu;
> + goto err_cleanup_desc_srcu;
>
> for (desc_index = 0; desc_index < gc->ngpio; desc_index++) {
> struct gpio_desc *desc = &gdev->descs[desc_index];
>
> - ret = init_srcu_struct(&desc->srcu);
> - if (ret)
> - goto err_cleanup_desc_srcu;
> -
> if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) {
> assign_bit(FLAG_IS_OUT,
> &desc->flags, !gc->get_direction(gc, desc_index));
> @@ -1027,7 +1024,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>
> ret = of_gpiochip_add(gc);
> if (ret)
> - goto err_cleanup_desc_srcu;
> + goto err_free_valid_mask;
>
> ret = gpiochip_add_pin_ranges(gc);
> if (ret)
> @@ -1074,10 +1071,10 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> gpiochip_remove_pin_ranges(gc);
> err_remove_of_chip:
> of_gpiochip_remove(gc);
> -err_cleanup_desc_srcu:
> - while (desc_index--)
> - cleanup_srcu_struct(&gdev->descs[desc_index].srcu);
> +err_free_valid_mask:
> gpiochip_free_valid_mask(gc);
> +err_cleanup_desc_srcu:
> + cleanup_srcu_struct(&gdev->desc_srcu);
> err_cleanup_gdev_srcu:
> cleanup_srcu_struct(&gdev->srcu);
> err_remove_from_list:
> @@ -2407,7 +2404,7 @@ char *gpiochip_dup_line_label(struct gpio_chip *gc, unsigned int offset)
> if (!test_bit(FLAG_REQUESTED, &desc->flags))
> return NULL;
>
> - guard(srcu)(&desc->srcu);
> + guard(srcu)(&desc->gdev->desc_srcu);
>
> label = kstrdup(gpiod_get_label(desc), GFP_KERNEL);
> if (!label)
> @@ -4798,7 +4795,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
> }
>
> for_each_gpio_desc(gc, desc) {
> - guard(srcu)(&desc->srcu);
> + guard(srcu)(&desc->gdev->desc_srcu);
> if (test_bit(FLAG_REQUESTED, &desc->flags)) {
> gpiod_get_direction(desc);
> is_out = test_bit(FLAG_IS_OUT, &desc->flags);
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 69a353c789f0..8e0e211ebf08 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -31,6 +31,7 @@
> * @chip: pointer to the corresponding gpiochip, holding static
> * data for this device
> * @descs: array of ngpio descriptors.
> + * @desc_srcu: ensures consistent state of GPIO descriptors exposed to users
> * @ngpio: the number of GPIO lines on this GPIO device, equal to the size
> * of the @descs array.
> * @can_sleep: indicate whether the GPIO chip driver's callbacks can sleep
> @@ -61,6 +62,7 @@ struct gpio_device {
> struct module *owner;
> struct gpio_chip __rcu *chip;
> struct gpio_desc *descs;
> + struct srcu_struct desc_srcu;
> int base;
> u16 ngpio;
> bool can_sleep;
> @@ -150,7 +152,6 @@ struct gpio_desc_label {
> * @label: Name of the consumer
> * @name: Line name
> * @hog: Pointer to the device node that hogs this line (if any)
> - * @srcu: SRCU struct protecting the label pointer.
> *
> * These are obtained using gpiod_get() and are preferable to the old
> * integer-based handles.
> @@ -188,7 +189,6 @@ struct gpio_desc {
> #ifdef CONFIG_OF_DYNAMIC
> struct device_node *hog;
> #endif
> - struct srcu_struct srcu;
> };
>
> #define gpiod_not_found(desc) (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
> @@ -256,7 +256,7 @@ static inline int gpio_chip_hwgpio(const struct gpio_desc *desc)
>
> #define gpiod_err(desc, fmt, ...) \
> do { \
> - scoped_guard(srcu, &desc->srcu) { \
> + scoped_guard(srcu, &desc->gdev->desc_srcu) { \
> pr_err("gpio-%d (%s): " fmt, desc_to_gpio(desc), \
> gpiod_get_label(desc) ? : "?", ##__VA_ARGS__); \
> } \
> @@ -264,7 +264,7 @@ do { \
>
> #define gpiod_warn(desc, fmt, ...) \
> do { \
> - scoped_guard(srcu, &desc->srcu) { \
> + scoped_guard(srcu, &desc->gdev->desc_srcu) { \
> pr_warn("gpio-%d (%s): " fmt, desc_to_gpio(desc), \
> gpiod_get_label(desc) ? : "?", ##__VA_ARGS__); \
> } \
> @@ -272,7 +272,7 @@ do { \
>
> #define gpiod_dbg(desc, fmt, ...) \
> do { \
> - scoped_guard(srcu, &desc->srcu) { \
> + scoped_guard(srcu, &desc->gdev->desc_srcu) { \
> pr_debug("gpio-%d (%s): " fmt, desc_to_gpio(desc), \
> gpiod_get_label(desc) ? : "?", ##__VA_ARGS__); \
> } \
> --
> 2.40.1
>
From: Bartosz Golaszewski <[email protected]>
On Tue, 07 May 2024 19:24:14 +0200, Bartosz Golaszewski wrote:
> We used a per-descriptor SRCU struct in order to not impose a wait with
> synchronize_srcu() for descriptor X on read-only operations of
> descriptor Y. Now that we no longer call synchronize_srcu() on
> descriptor label change but only when releasing descriptor resources, we
> can use a single SRCU structure for all GPIO descriptors in a given chip.
>
>
> [...]
Applied, thanks!
[1/1] gpiolib: use a single SRCU struct for all GPIO descriptors
commit: 7765ffed533d4a9f0291a0edc660496d104396ec
Best regards,
--
Bartosz Golaszewski <[email protected]>