1. Patchew
  2. linux
  3. View series

[PATCH] gpiolib: use a single SRCU struct for all GPIO descriptors

Bartosz Golaszewski posted 1 patch 1 week, 5 days ago 
drivers/gpio/gpiolib-cdev.c |  2 +-
drivers/gpio/gpiolib.c      | 41 +++++++++++++++++--------------------
drivers/gpio/gpiolib.h      | 10 ++++-----
3 files changed, 25 insertions(+), 28 deletions(-)
[PATCH] gpiolib: use a single SRCU struct for all GPIO descriptors
Posted by Bartosz Golaszewski 1 week, 5 days ago 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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 <paulmck@kernel.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 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
Re: [PATCH] gpiolib: use a single SRCU struct for all GPIO descriptors
Posted by Bartosz Golaszewski 1 week, 3 days ago 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


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 <bartosz.golaszewski@linaro.org>
Re: [PATCH] gpiolib: use a single SRCU struct for all GPIO descriptors
Posted by Paul E. McKenney 1 week, 4 days ago 
On Tue, May 07, 2024 at 07:24:14PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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 <paulmck@kernel.org>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Acked-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  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
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.