Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] gpio: don't use same lockdep class for all devm_gpiochip_add_data users
@ 2020-07-31 12:38 Ahmad Fatoum
  2020-07-31 15:19 ` Bartosz Golaszewski
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Ahmad Fatoum @ 2020-07-31 12:38 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Grygorii Strashko, Thierry Reding
  Cc: kernel, Ahmad Fatoum, linux-gpio, linux-kernel

Commit 959bc7b22bd2 ("gpio: Automatically add lockdep keys") documents
in its commits message its intention to "create a unique class key for
each driver".

It does so by having gpiochip_add_data add in-place the definition of
two static lockdep classes for LOCKDEP use. That way, every caller of
the macro adds their gpiochip with unique lockdep classes.

There are many indirect callers of gpiochip_add_data, however, via
use of devm_gpiochip_add_data. devm_gpiochip_add_data has external
linkage and all its users will share the same lockdep classes, which
probably is not intended.

Fix this by replicating the gpio_chip_add_data statics-in-macro for
the devm_ version as well.

Fixes: 959bc7b22bd2 ("gpio: Automatically add lockdep keys")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
This doesn't fix any particular problem I ran into, but the code
looked buggy, at least to my lockdep-user-not-developer eyes.
---
 drivers/gpio/gpiolib-devres.c | 13 ++++++++-----
 include/linux/gpio/driver.h   | 13 +++++++++++--
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
index 5c91c4365da1..7dbce4c4ebdf 100644
--- a/drivers/gpio/gpiolib-devres.c
+++ b/drivers/gpio/gpiolib-devres.c
@@ -487,10 +487,12 @@ static void devm_gpio_chip_release(struct device *dev, void *res)
 }
 
 /**
- * devm_gpiochip_add_data() - Resource managed gpiochip_add_data()
+ * devm_gpiochip_add_data_with_key() - Resource managed gpiochip_add_data_with_key()
  * @dev: pointer to the device that gpio_chip belongs to.
  * @gc: the GPIO chip to register
  * @data: driver-private data associated with this chip
+ * @lock_key: lockdep class for IRQ lock
+ * @request_key: lockdep class for IRQ request
  *
  * Context: potentially before irqs will work
  *
@@ -501,8 +503,9 @@ static void devm_gpio_chip_release(struct device *dev, void *res)
  * gc->base is invalid or already associated with a different chip.
  * Otherwise it returns zero as a success code.
  */
-int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
-			   void *data)
+int devm_gpiochip_add_data_with_key(struct device *dev, struct gpio_chip *gc, void *data,
+				    struct lock_class_key *lock_key,
+				    struct lock_class_key *request_key)
 {
 	struct gpio_chip **ptr;
 	int ret;
@@ -512,7 +515,7 @@ int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
 	if (!ptr)
 		return -ENOMEM;
 
-	ret = gpiochip_add_data(gc, data);
+	ret = gpiochip_add_data_with_key(gc, data, lock_key, request_key);
 	if (ret < 0) {
 		devres_free(ptr);
 		return ret;
@@ -523,4 +526,4 @@ int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(devm_gpiochip_add_data);
+EXPORT_SYMBOL_GPL(devm_gpiochip_add_data_with_key);
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index c4f272af7af5..e6217d8e2e9f 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -509,8 +509,16 @@ extern int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		gpiochip_add_data_with_key(gc, data, &lock_key, \
 					   &request_key);	  \
 	})
+#define devm_gpiochip_add_data(dev, gc, data) ({ \
+		static struct lock_class_key lock_key;	\
+		static struct lock_class_key request_key;	  \
+		devm_gpiochip_add_data_with_key(dev, gc, data, &lock_key, \
+					   &request_key);	  \
+	})
 #else
 #define gpiochip_add_data(gc, data) gpiochip_add_data_with_key(gc, data, NULL, NULL)
+#define devm_gpiochip_add_data(dev, gc, data) \
+	devm_gpiochip_add_data_with_key(dev, gc, data, NULL, NULL)
 #endif /* CONFIG_LOCKDEP */
 
 static inline int gpiochip_add(struct gpio_chip *gc)
@@ -518,8 +526,9 @@ static inline int gpiochip_add(struct gpio_chip *gc)
 	return gpiochip_add_data(gc, NULL);
 }
 extern void gpiochip_remove(struct gpio_chip *gc);
-extern int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
-				  void *data);
+extern int devm_gpiochip_add_data_with_key(struct device *dev, struct gpio_chip *gc, void *data,
+					   struct lock_class_key *lock_key,
+					   struct lock_class_key *request_key);
 
 extern struct gpio_chip *gpiochip_find(void *data,
 			      int (*match)(struct gpio_chip *gc, void *data));
-- 
2.27.0


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] gpio: don't use same lockdep class for all devm_gpiochip_add_data users
  2020-07-31 12:38 [PATCH] gpio: don't use same lockdep class for all devm_gpiochip_add_data users Ahmad Fatoum
@ 2020-07-31 15:19 ` Bartosz Golaszewski
  2020-08-01 11:51 ` Andy Shevchenko
  2020-08-03 23:23 ` Linus Walleij
  2 siblings, 0 replies; 4+ messages in thread
From: Bartosz Golaszewski @ 2020-07-31 15:19 UTC (permalink / raw)
  To: Ahmad Fatoum
  Cc: Linus Walleij, Grygorii Strashko, Thierry Reding, Sascha Hauer,
	linux-gpio, LKML

On Fri, Jul 31, 2020 at 2:39 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Commit 959bc7b22bd2 ("gpio: Automatically add lockdep keys") documents
> in its commits message its intention to "create a unique class key for
> each driver".
>
> It does so by having gpiochip_add_data add in-place the definition of
> two static lockdep classes for LOCKDEP use. That way, every caller of
> the macro adds their gpiochip with unique lockdep classes.
>
> There are many indirect callers of gpiochip_add_data, however, via
> use of devm_gpiochip_add_data. devm_gpiochip_add_data has external
> linkage and all its users will share the same lockdep classes, which
> probably is not intended.
>
> Fix this by replicating the gpio_chip_add_data statics-in-macro for
> the devm_ version as well.
>
> Fixes: 959bc7b22bd2 ("gpio: Automatically add lockdep keys")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> This doesn't fix any particular problem I ran into, but the code
> looked buggy, at least to my lockdep-user-not-developer eyes.
> ---
>  drivers/gpio/gpiolib-devres.c | 13 ++++++++-----
>  include/linux/gpio/driver.h   | 13 +++++++++++--
>  2 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
> index 5c91c4365da1..7dbce4c4ebdf 100644
> --- a/drivers/gpio/gpiolib-devres.c
> +++ b/drivers/gpio/gpiolib-devres.c
> @@ -487,10 +487,12 @@ static void devm_gpio_chip_release(struct device *dev, void *res)
>  }
>
>  /**
> - * devm_gpiochip_add_data() - Resource managed gpiochip_add_data()
> + * devm_gpiochip_add_data_with_key() - Resource managed gpiochip_add_data_with_key()
>   * @dev: pointer to the device that gpio_chip belongs to.
>   * @gc: the GPIO chip to register
>   * @data: driver-private data associated with this chip
> + * @lock_key: lockdep class for IRQ lock
> + * @request_key: lockdep class for IRQ request
>   *
>   * Context: potentially before irqs will work
>   *
> @@ -501,8 +503,9 @@ static void devm_gpio_chip_release(struct device *dev, void *res)
>   * gc->base is invalid or already associated with a different chip.
>   * Otherwise it returns zero as a success code.
>   */
> -int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
> -                          void *data)
> +int devm_gpiochip_add_data_with_key(struct device *dev, struct gpio_chip *gc, void *data,
> +                                   struct lock_class_key *lock_key,
> +                                   struct lock_class_key *request_key)
>  {
>         struct gpio_chip **ptr;
>         int ret;
> @@ -512,7 +515,7 @@ int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
>         if (!ptr)
>                 return -ENOMEM;
>
> -       ret = gpiochip_add_data(gc, data);
> +       ret = gpiochip_add_data_with_key(gc, data, lock_key, request_key);
>         if (ret < 0) {
>                 devres_free(ptr);
>                 return ret;
> @@ -523,4 +526,4 @@ int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
>
>         return 0;
>  }
> -EXPORT_SYMBOL_GPL(devm_gpiochip_add_data);
> +EXPORT_SYMBOL_GPL(devm_gpiochip_add_data_with_key);
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index c4f272af7af5..e6217d8e2e9f 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -509,8 +509,16 @@ extern int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>                 gpiochip_add_data_with_key(gc, data, &lock_key, \
>                                            &request_key);         \
>         })
> +#define devm_gpiochip_add_data(dev, gc, data) ({ \
> +               static struct lock_class_key lock_key;  \
> +               static struct lock_class_key request_key;         \
> +               devm_gpiochip_add_data_with_key(dev, gc, data, &lock_key, \
> +                                          &request_key);         \
> +       })
>  #else
>  #define gpiochip_add_data(gc, data) gpiochip_add_data_with_key(gc, data, NULL, NULL)
> +#define devm_gpiochip_add_data(dev, gc, data) \
> +       devm_gpiochip_add_data_with_key(dev, gc, data, NULL, NULL)
>  #endif /* CONFIG_LOCKDEP */
>
>  static inline int gpiochip_add(struct gpio_chip *gc)
> @@ -518,8 +526,9 @@ static inline int gpiochip_add(struct gpio_chip *gc)
>         return gpiochip_add_data(gc, NULL);
>  }
>  extern void gpiochip_remove(struct gpio_chip *gc);
> -extern int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
> -                                 void *data);
> +extern int devm_gpiochip_add_data_with_key(struct device *dev, struct gpio_chip *gc, void *data,
> +                                          struct lock_class_key *lock_key,
> +                                          struct lock_class_key *request_key);
>
>  extern struct gpio_chip *gpiochip_find(void *data,
>                               int (*match)(struct gpio_chip *gc, void *data));
> --
> 2.27.0
>

Looks good to me and the previous code indeed looks buggy.

Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] gpio: don't use same lockdep class for all devm_gpiochip_add_data users
  2020-07-31 12:38 [PATCH] gpio: don't use same lockdep class for all devm_gpiochip_add_data users Ahmad Fatoum
  2020-07-31 15:19 ` Bartosz Golaszewski
@ 2020-08-01 11:51 ` Andy Shevchenko
  2020-08-03 23:23 ` Linus Walleij
  2 siblings, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2020-08-01 11:51 UTC (permalink / raw)
  To: Ahmad Fatoum
  Cc: Linus Walleij, Bartosz Golaszewski, Grygorii Strashko,
	Thierry Reding, Sascha Hauer, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List

On Fri, Jul 31, 2020 at 3:40 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>
> Commit 959bc7b22bd2 ("gpio: Automatically add lockdep keys") documents
> in its commits message its intention to "create a unique class key for
> each driver".
>
> It does so by having gpiochip_add_data add in-place the definition of
> two static lockdep classes for LOCKDEP use. That way, every caller of
> the macro adds their gpiochip with unique lockdep classes.
>
> There are many indirect callers of gpiochip_add_data, however, via
> use of devm_gpiochip_add_data. devm_gpiochip_add_data has external
> linkage and all its users will share the same lockdep classes, which
> probably is not intended.
>
> Fix this by replicating the gpio_chip_add_data statics-in-macro for
> the devm_ version as well.

I ran into similar issues in another driver (not GPIO) and I agree with the fix.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Fixes: 959bc7b22bd2 ("gpio: Automatically add lockdep keys")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> This doesn't fix any particular problem I ran into, but the code
> looked buggy, at least to my lockdep-user-not-developer eyes.
> ---
>  drivers/gpio/gpiolib-devres.c | 13 ++++++++-----
>  include/linux/gpio/driver.h   | 13 +++++++++++--
>  2 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
> index 5c91c4365da1..7dbce4c4ebdf 100644
> --- a/drivers/gpio/gpiolib-devres.c
> +++ b/drivers/gpio/gpiolib-devres.c
> @@ -487,10 +487,12 @@ static void devm_gpio_chip_release(struct device *dev, void *res)
>  }
>
>  /**
> - * devm_gpiochip_add_data() - Resource managed gpiochip_add_data()
> + * devm_gpiochip_add_data_with_key() - Resource managed gpiochip_add_data_with_key()
>   * @dev: pointer to the device that gpio_chip belongs to.
>   * @gc: the GPIO chip to register
>   * @data: driver-private data associated with this chip
> + * @lock_key: lockdep class for IRQ lock
> + * @request_key: lockdep class for IRQ request
>   *
>   * Context: potentially before irqs will work
>   *
> @@ -501,8 +503,9 @@ static void devm_gpio_chip_release(struct device *dev, void *res)
>   * gc->base is invalid or already associated with a different chip.
>   * Otherwise it returns zero as a success code.
>   */
> -int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
> -                          void *data)
> +int devm_gpiochip_add_data_with_key(struct device *dev, struct gpio_chip *gc, void *data,
> +                                   struct lock_class_key *lock_key,
> +                                   struct lock_class_key *request_key)
>  {
>         struct gpio_chip **ptr;
>         int ret;
> @@ -512,7 +515,7 @@ int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
>         if (!ptr)
>                 return -ENOMEM;
>
> -       ret = gpiochip_add_data(gc, data);
> +       ret = gpiochip_add_data_with_key(gc, data, lock_key, request_key);
>         if (ret < 0) {
>                 devres_free(ptr);
>                 return ret;
> @@ -523,4 +526,4 @@ int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
>
>         return 0;
>  }
> -EXPORT_SYMBOL_GPL(devm_gpiochip_add_data);
> +EXPORT_SYMBOL_GPL(devm_gpiochip_add_data_with_key);
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index c4f272af7af5..e6217d8e2e9f 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -509,8 +509,16 @@ extern int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>                 gpiochip_add_data_with_key(gc, data, &lock_key, \
>                                            &request_key);         \
>         })
> +#define devm_gpiochip_add_data(dev, gc, data) ({ \
> +               static struct lock_class_key lock_key;  \
> +               static struct lock_class_key request_key;         \
> +               devm_gpiochip_add_data_with_key(dev, gc, data, &lock_key, \
> +                                          &request_key);         \
> +       })
>  #else
>  #define gpiochip_add_data(gc, data) gpiochip_add_data_with_key(gc, data, NULL, NULL)
> +#define devm_gpiochip_add_data(dev, gc, data) \
> +       devm_gpiochip_add_data_with_key(dev, gc, data, NULL, NULL)
>  #endif /* CONFIG_LOCKDEP */
>
>  static inline int gpiochip_add(struct gpio_chip *gc)
> @@ -518,8 +526,9 @@ static inline int gpiochip_add(struct gpio_chip *gc)
>         return gpiochip_add_data(gc, NULL);
>  }
>  extern void gpiochip_remove(struct gpio_chip *gc);
> -extern int devm_gpiochip_add_data(struct device *dev, struct gpio_chip *gc,
> -                                 void *data);
> +extern int devm_gpiochip_add_data_with_key(struct device *dev, struct gpio_chip *gc, void *data,
> +                                          struct lock_class_key *lock_key,
> +                                          struct lock_class_key *request_key);
>
>  extern struct gpio_chip *gpiochip_find(void *data,
>                               int (*match)(struct gpio_chip *gc, void *data));
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] gpio: don't use same lockdep class for all devm_gpiochip_add_data users
  2020-07-31 12:38 [PATCH] gpio: don't use same lockdep class for all devm_gpiochip_add_data users Ahmad Fatoum
  2020-07-31 15:19 ` Bartosz Golaszewski
  2020-08-01 11:51 ` Andy Shevchenko
@ 2020-08-03 23:23 ` Linus Walleij
  2 siblings, 0 replies; 4+ messages in thread
From: Linus Walleij @ 2020-08-03 23:23 UTC (permalink / raw)
  To: Ahmad Fatoum
  Cc: Bartosz Golaszewski, Grygorii Strashko, Thierry Reding,
	Sascha Hauer, open list:GPIO SUBSYSTEM, linux-kernel

On Fri, Jul 31, 2020 at 2:39 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:

> Commit 959bc7b22bd2 ("gpio: Automatically add lockdep keys") documents
> in its commits message its intention to "create a unique class key for
> each driver".
>
> It does so by having gpiochip_add_data add in-place the definition of
> two static lockdep classes for LOCKDEP use. That way, every caller of
> the macro adds their gpiochip with unique lockdep classes.
>
> There are many indirect callers of gpiochip_add_data, however, via
> use of devm_gpiochip_add_data. devm_gpiochip_add_data has external
> linkage and all its users will share the same lockdep classes, which
> probably is not intended.
>
> Fix this by replicating the gpio_chip_add_data statics-in-macro for
> the devm_ version as well.
>
> Fixes: 959bc7b22bd2 ("gpio: Automatically add lockdep keys")
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> This doesn't fix any particular problem I ran into, but the code
> looked buggy, at least to my lockdep-user-not-developer eyes.

Thanks patch applied, I think we had this fixed before
but managed to loose it in some API rewrite. Thanks for
fixing it up!

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-31 12:38 [PATCH] gpio: don't use same lockdep class for all devm_gpiochip_add_data users Ahmad Fatoum
2020-07-31 15:19 ` Bartosz Golaszewski
2020-08-01 11:51 ` Andy Shevchenko
2020-08-03 23:23 ` Linus Walleij

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