[PATCH v1 0/3] gpiolib: acpi: More fixes to the consolidation rework

[PATCH v1 0/3] gpiolib: acpi: More fixes to the consolidation rework

[Andy Shevchenko]

On top what Hans already fixed, Ferry reported a few bugs that pointed
out to the same consolidation rework done in v6.2.

The first is most serious issue, that needs to be fixed ASAP.

The second is good to have.

And the third one I'm not fully okay with, so open for advice on
how to improve.

Note, that long list of parameters to a _find_gpio() functions
can be hidden in the specifically crafted a new data structure,
but this is out of scope of the _fixes_ series. I'm all ears as
well for that one.

Andy Shevchenko (3):
  gpiolib: acpi: Add missing memset(0) to acpi_get_gpiod_from_data()
  gpiolib: Fix debug messaging in gpiod_find_and_request()
  gpiolib: Make debug messages in gpiod_find_by_fwnode() less confusing

 drivers/gpio/gpiolib-acpi.c   | 10 ++++-----
 drivers/gpio/gpiolib-acpi.h   | 13 ++++++------
 drivers/gpio/gpiolib-of.c     | 13 ++++++------
 drivers/gpio/gpiolib-of.h     |  8 ++++----
 drivers/gpio/gpiolib-swnode.c |  4 ++--
 drivers/gpio/gpiolib-swnode.h |  1 +
 drivers/gpio/gpiolib.c        | 38 ++++++++++++++++++++---------------
 7 files changed, 48 insertions(+), 39 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 1/3] gpiolib: acpi: Add missing memset(0) to acpi_get_gpiod_from_data()

[Andy Shevchenko]

When refactoring the acpi_get_gpiod_from_data() the change missed
cleaning up the variable on stack. Add missing memset().

Reported-by: Ferry Toth <ftoth@exalondelft.nl>
Fixes: 16ba046e86e9 ("gpiolib: acpi: teach acpi_find_gpio() to handle data-only nodes")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index fbda452fb4d6..51e41676de0b 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -951,6 +951,7 @@ static struct gpio_desc *acpi_get_gpiod_from_data(struct fwnode_handle *fwnode,
 	if (!propname)
 		return ERR_PTR(-EINVAL);
 
+	memset(&lookup, 0, sizeof(lookup));
 	lookup.index = index;
 
 	ret = acpi_gpio_property_lookup(fwnode, propname, index, &lookup);
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 2/3] gpiolib: Fix debug messaging in gpiod_find_and_request()

[Andy Shevchenko]

When consolidating GPIO lookups in ACPI code, the debug messaging
had been broken and hence lost a bit of sense. Restore debug
messaging in gpiod_find_and_request() when configuring the GPIO
line via gpiod_configure_flags().

Reported-by: Ferry Toth <ftoth@exalondelft.nl>
Fixes: 8eb1f71e7acc ("gpiolib: consolidate GPIO lookups")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c   |  9 ++++-----
 drivers/gpio/gpiolib-acpi.h   | 13 +++++++------
 drivers/gpio/gpiolib-of.c     | 13 +++++++------
 drivers/gpio/gpiolib-of.h     |  8 ++++----
 drivers/gpio/gpiolib-swnode.c |  4 ++--
 drivers/gpio/gpiolib-swnode.h |  1 +
 drivers/gpio/gpiolib.c        | 21 ++++++++++++++-------
 7 files changed, 39 insertions(+), 30 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 51e41676de0b..a2899b2a58bd 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -973,24 +973,23 @@ static bool acpi_can_fallback_to_crs(struct acpi_device *adev,
 }
 
 struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
-				 const char *con_id,
-				 unsigned int idx,
+				 const char *con_id, unsigned int idx,
+				 char *propname, size_t propsize,
 				 enum gpiod_flags *dflags,
 				 unsigned long *lookupflags)
 {
 	struct acpi_device *adev = to_acpi_device_node(fwnode);
 	struct acpi_gpio_info info;
 	struct gpio_desc *desc;
-	char propname[32];
 	int i;
 
 	/* Try first from _DSD */
 	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
 		if (con_id) {
-			snprintf(propname, sizeof(propname), "%s-%s",
+			snprintf(propname, propsize, "%s-%s",
 				 con_id, gpio_suffixes[i]);
 		} else {
-			snprintf(propname, sizeof(propname), "%s",
+			snprintf(propname, propsize, "%s",
 				 gpio_suffixes[i]);
 		}
 
diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h
index 0fcd7e14d7f9..b5f0de249765 100644
--- a/drivers/gpio/gpiolib-acpi.h
+++ b/drivers/gpio/gpiolib-acpi.h
@@ -28,8 +28,8 @@ void acpi_gpiochip_request_interrupts(struct gpio_chip *chip);
 void acpi_gpiochip_free_interrupts(struct gpio_chip *chip);
 
 struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
-				 const char *con_id,
-				 unsigned int idx,
+				 const char *con_id, unsigned int idx,
+				 char *propname, size_t propsize,
 				 enum gpiod_flags *dflags,
 				 unsigned long *lookupflags);
 
@@ -44,10 +44,11 @@ acpi_gpiochip_request_interrupts(struct gpio_chip *chip) { }
 static inline void
 acpi_gpiochip_free_interrupts(struct gpio_chip *chip) { }
 
-static inline struct gpio_desc *
-acpi_find_gpio(struct fwnode_handle *fwnode, const char *con_id,
-	       unsigned int idx, enum gpiod_flags *dflags,
-	       unsigned long *lookupflags)
+static inline struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
+					       const char *con_id, unsigned int idx,
+					       char *propname, size_t propsize,
+					       enum gpiod_flags *dflags,
+					       unsigned long *lookupflags)
 {
 	return ERR_PTR(-ENOENT);
 }
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 531faabead0f..017ee5cbfb60 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -621,10 +621,11 @@ static const of_find_gpio_quirk of_find_gpio_quirks[] = {
 	NULL
 };
 
-struct gpio_desc *of_find_gpio(struct device_node *np, const char *con_id,
-			       unsigned int idx, unsigned long *flags)
+struct gpio_desc *of_find_gpio(struct device_node *np,
+			       const char *con_id, unsigned int idx,
+			       char *propname, size_t propsize,
+			       unsigned long *flags)
 {
-	char prop_name[32]; /* 32 is max size of property name */
 	enum of_gpio_flags of_flags;
 	const of_find_gpio_quirk *q;
 	struct gpio_desc *desc;
@@ -633,13 +634,13 @@ struct gpio_desc *of_find_gpio(struct device_node *np, const char *con_id,
 	/* Try GPIO property "foo-gpios" and "foo-gpio" */
 	for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
 		if (con_id)
-			snprintf(prop_name, sizeof(prop_name), "%s-%s", con_id,
+			snprintf(propname, propsize, "%s-%s", con_id,
 				 gpio_suffixes[i]);
 		else
-			snprintf(prop_name, sizeof(prop_name), "%s",
+			snprintf(propname, propsize, "%s",
 				 gpio_suffixes[i]);
 
-		desc = of_get_named_gpiod_flags(np, prop_name, idx, &of_flags);
+		desc = of_get_named_gpiod_flags(np, propname, idx, &of_flags);
 
 		if (!gpiod_not_found(desc))
 			break;
diff --git a/drivers/gpio/gpiolib-of.h b/drivers/gpio/gpiolib-of.h
index 6b3a5347c5d9..c2517e7913ee 100644
--- a/drivers/gpio/gpiolib-of.h
+++ b/drivers/gpio/gpiolib-of.h
@@ -16,16 +16,16 @@ struct gpio_device;
 
 #ifdef CONFIG_OF_GPIO
 struct gpio_desc *of_find_gpio(struct device_node *np,
-			       const char *con_id,
-			       unsigned int idx,
+			       const char *con_id, unsigned int idx,
+			       char *propname, size_t propsize,
 			       unsigned long *lookupflags);
 int of_gpiochip_add(struct gpio_chip *gc);
 void of_gpiochip_remove(struct gpio_chip *gc);
 int of_gpio_get_count(struct device *dev, const char *con_id);
 #else
 static inline struct gpio_desc *of_find_gpio(struct device_node *np,
-					     const char *con_id,
-					     unsigned int idx,
+					     const char *con_id, unsigned int idx,
+					     char *propname, size_t propsize,
 					     unsigned long *lookupflags)
 {
 	return ERR_PTR(-ENOENT);
diff --git a/drivers/gpio/gpiolib-swnode.c b/drivers/gpio/gpiolib-swnode.c
index b5a6eaf3729b..12c94f620f6b 100644
--- a/drivers/gpio/gpiolib-swnode.c
+++ b/drivers/gpio/gpiolib-swnode.c
@@ -51,20 +51,20 @@ static struct gpio_chip *swnode_get_chip(struct fwnode_handle *fwnode)
 
 struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
 				   const char *con_id, unsigned int idx,
+				   char *propname, size_t propsize,
 				   unsigned long *flags)
 {
 	const struct software_node *swnode;
 	struct fwnode_reference_args args;
 	struct gpio_chip *chip;
 	struct gpio_desc *desc;
-	char propname[32]; /* 32 is max size of property name */
 	int error;
 
 	swnode = to_software_node(fwnode);
 	if (!swnode)
 		return ERR_PTR(-EINVAL);
 
-	swnode_format_propname(con_id, propname, sizeof(propname));
+	swnode_format_propname(con_id, propname, propsize);
 
 	/*
 	 * We expect all swnode-described GPIOs have GPIO number and
diff --git a/drivers/gpio/gpiolib-swnode.h b/drivers/gpio/gpiolib-swnode.h
index af849e56f6bc..b75fbb1fa7a6 100644
--- a/drivers/gpio/gpiolib-swnode.h
+++ b/drivers/gpio/gpiolib-swnode.h
@@ -8,6 +8,7 @@ struct gpio_desc;
 
 struct gpio_desc *swnode_find_gpio(struct fwnode_handle *fwnode,
 				   const char *con_id, unsigned int idx,
+				   char *propname, size_t propsize,
 				   unsigned long *flags);
 int swnode_gpio_count(const struct fwnode_handle *fwnode, const char *con_id);
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 40a0022ea719..beac3031246e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3938,8 +3938,8 @@ static int platform_gpio_count(struct device *dev, const char *con_id)
 
 static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode,
 					      struct device *consumer,
-					      const char *con_id,
-					      unsigned int idx,
+					      const char *con_id, unsigned int idx,
+					      char *propname, size_t propsize,
 					      enum gpiod_flags *flags,
 					      unsigned long *lookupflags)
 {
@@ -3948,15 +3948,18 @@ static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode,
 	if (is_of_node(fwnode)) {
 		dev_dbg(consumer, "using DT '%pfw' for '%s' GPIO lookup\n",
 			fwnode, con_id);
-		desc = of_find_gpio(to_of_node(fwnode), con_id, idx, lookupflags);
+		desc = of_find_gpio(to_of_node(fwnode), con_id, idx, propname, propsize,
+				    lookupflags);
 	} else if (is_acpi_node(fwnode)) {
 		dev_dbg(consumer, "using ACPI '%pfw' for '%s' GPIO lookup\n",
 			fwnode, con_id);
-		desc = acpi_find_gpio(fwnode, con_id, idx, flags, lookupflags);
+		desc = acpi_find_gpio(fwnode, con_id, idx, propname, propsize,
+				      flags, lookupflags);
 	} else if (is_software_node(fwnode)) {
 		dev_dbg(consumer, "using swnode '%pfw' for '%s' GPIO lookup\n",
 			fwnode, con_id);
-		desc = swnode_find_gpio(fwnode, con_id, idx, lookupflags);
+		desc = swnode_find_gpio(fwnode, con_id, idx, propname, propsize,
+					lookupflags);
 	}
 
 	return desc;
@@ -3970,11 +3973,15 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
 						const char *label,
 						bool platform_lookup_allowed)
 {
+	char propname[32] = ""; /* 32 is max size of property name */
+	const char *funcname = con_id ?: propname;
 	unsigned long lookupflags = GPIO_LOOKUP_FLAGS_DEFAULT;
 	struct gpio_desc *desc;
 	int ret;
 
-	desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx, &flags, &lookupflags);
+	desc = gpiod_find_by_fwnode(fwnode, consumer, con_id, idx,
+				    propname, sizeof(propname),
+				    &flags, &lookupflags);
 	if (gpiod_not_found(desc) && platform_lookup_allowed) {
 		/*
 		 * Either we are not using DT or ACPI, or their lookup did not
@@ -4012,7 +4019,7 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
 		return desc;
 	}
 
-	ret = gpiod_configure_flags(desc, con_id, lookupflags, flags);
+	ret = gpiod_configure_flags(desc, funcname, lookupflags, flags);
 	if (ret < 0) {
 		dev_dbg(consumer, "setup of GPIO %s failed\n", con_id);
 		gpiod_put(desc);
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 3/3] gpiolib: Make debug messages in gpiod_find_by_fwnode() less confusing

[Andy Shevchenko]

Currently the extended debug messages have added confusion,
but value when con_id is NULL, which is the case for, e.g.,
GPIO LEDs.

Improve the messaging by using GPIO function name rather than con_id.
This requires to split and move the second part after the respective
calls.

Reported-by: Ferry Toth <ftoth@exalondelft.nl>
Fixes: 8eb1f71e7acc ("gpiolib: consolidate GPIO lookups")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index beac3031246e..2cc275fb62b6 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3946,18 +3946,15 @@ static struct gpio_desc *gpiod_find_by_fwnode(struct fwnode_handle *fwnode,
 	struct gpio_desc *desc = ERR_PTR(-ENOENT);
 
 	if (is_of_node(fwnode)) {
-		dev_dbg(consumer, "using DT '%pfw' for '%s' GPIO lookup\n",
-			fwnode, con_id);
+		dev_dbg(consumer, "using DT '%pfw' for GPIO lookup\n", fwnode);
 		desc = of_find_gpio(to_of_node(fwnode), con_id, idx, propname, propsize,
 				    lookupflags);
 	} else if (is_acpi_node(fwnode)) {
-		dev_dbg(consumer, "using ACPI '%pfw' for '%s' GPIO lookup\n",
-			fwnode, con_id);
+		dev_dbg(consumer, "using ACPI '%pfw' for GPIO lookup\n", fwnode);
 		desc = acpi_find_gpio(fwnode, con_id, idx, propname, propsize,
 				      flags, lookupflags);
 	} else if (is_software_node(fwnode)) {
-		dev_dbg(consumer, "using swnode '%pfw' for '%s' GPIO lookup\n",
-			fwnode, con_id);
+		dev_dbg(consumer, "using swnode '%pfw' for GPIO lookup\n", fwnode);
 		desc = swnode_find_gpio(fwnode, con_id, idx, propname, propsize,
 					lookupflags);
 	}
@@ -3993,10 +3990,12 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
 	}
 
 	if (IS_ERR(desc)) {
-		dev_dbg(consumer, "No GPIO consumer %s found\n", con_id);
+		dev_dbg(consumer, "No GPIO descriptor for '%s' found\n", funcname);
 		return desc;
 	}
 
+	dev_dbg(consumer, "Found GPIO descriptor for '%s'\n", funcname);
+
 	/*
 	 * If a connection label was passed use that, else attempt to use
 	 * the device name as label
@@ -4015,13 +4014,13 @@ static struct gpio_desc *gpiod_find_and_request(struct device *consumer,
 		 * FIXME: Make this more sane and safe.
 		 */
 		dev_info(consumer,
-			 "nonexclusive access to GPIO for %s\n", con_id);
+			 "nonexclusive access to GPIO for %s\n", funcname);
 		return desc;
 	}
 
 	ret = gpiod_configure_flags(desc, funcname, lookupflags, flags);
 	if (ret < 0) {
-		dev_dbg(consumer, "setup of GPIO %s failed\n", con_id);
+		dev_dbg(consumer, "setup of GPIO %s failed\n", funcname);
 		gpiod_put(desc);
 		return ERR_PTR(ret);
 	}
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 1/3] gpiolib: acpi: Add missing memset(0) to acpi_get_gpiod_from_data()

[Dmitry Torokhov]

On Thu, Oct 19, 2023 at 08:34:55PM +0300, Andy Shevchenko wrote:
> When refactoring the acpi_get_gpiod_from_data() the change missed
> cleaning up the variable on stack. Add missing memset().
> 
> Reported-by: Ferry Toth <ftoth@exalondelft.nl>
> Fixes: 16ba046e86e9 ("gpiolib: acpi: teach acpi_find_gpio() to handle data-only nodes")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Although I think it would be better to change
acpi_gpio_resource_lookup() to take an index and return a gpiod
descriptor and have a local copy of the lookup structure. 

> ---
>  drivers/gpio/gpiolib-acpi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index fbda452fb4d6..51e41676de0b 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -951,6 +951,7 @@ static struct gpio_desc *acpi_get_gpiod_from_data(struct fwnode_handle *fwnode,
>  	if (!propname)
>  		return ERR_PTR(-EINVAL);
>  
> +	memset(&lookup, 0, sizeof(lookup));
>  	lookup.index = index;
>  
>  	ret = acpi_gpio_property_lookup(fwnode, propname, index, &lookup);
> -- 
> 2.40.0.1.gaa8946217a0b
> 

Thanks.

-- 
Dmitry

Re: [PATCH v1 2/3] gpiolib: Fix debug messaging in gpiod_find_and_request()

[Dmitry Torokhov]

On Thu, Oct 19, 2023 at 08:34:56PM +0300, Andy Shevchenko wrote:
> When consolidating GPIO lookups in ACPI code, the debug messaging
> had been broken and hence lost a bit of sense. Restore debug
> messaging in gpiod_find_and_request() when configuring the GPIO
> line via gpiod_configure_flags().

Could you give an example of the before/after messages to show exavtly
what is being improved?

Thanks.

-- 
Dmitry

Re: [PATCH v1 1/3] gpiolib: acpi: Add missing memset(0) to acpi_get_gpiod_from_data()

[Bartosz Golaszewski]

On Thu, Oct 19, 2023 at 9:20 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Thu, Oct 19, 2023 at 08:34:55PM +0300, Andy Shevchenko wrote:
> > When refactoring the acpi_get_gpiod_from_data() the change missed
> > cleaning up the variable on stack. Add missing memset().
> >
> > Reported-by: Ferry Toth <ftoth@exalondelft.nl>
> > Fixes: 16ba046e86e9 ("gpiolib: acpi: teach acpi_find_gpio() to handle data-only nodes")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Although I think it would be better to change
> acpi_gpio_resource_lookup() to take an index and return a gpiod
> descriptor and have a local copy of the lookup structure.
>

I queued it for fixes as this is a bug, we can improve it later.

Bart

> > ---
> >  drivers/gpio/gpiolib-acpi.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > index fbda452fb4d6..51e41676de0b 100644
> > --- a/drivers/gpio/gpiolib-acpi.c
> > +++ b/drivers/gpio/gpiolib-acpi.c
> > @@ -951,6 +951,7 @@ static struct gpio_desc *acpi_get_gpiod_from_data(struct fwnode_handle *fwnode,
> >       if (!propname)
> >               return ERR_PTR(-EINVAL);
> >
> > +     memset(&lookup, 0, sizeof(lookup));
> >       lookup.index = index;
> >
> >       ret = acpi_gpio_property_lookup(fwnode, propname, index, &lookup);
> > --
> > 2.40.0.1.gaa8946217a0b
> >
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH v1 0/3] gpiolib: acpi: More fixes to the consolidation rework

[Ferry Toth]

Op 19-10-2023 om 19:34 schreef Andy Shevchenko:
> On top what Hans already fixed, Ferry reported a few bugs that pointed
> out to the same consolidation rework done in v6.2.
> 
> The first is most serious issue, that needs to be fixed ASAP.
> 
> The second is good to have.
> 
> And the third one I'm not fully okay with, so open for advice on
> how to improve.
> 
> Note, that long list of parameters to a _find_gpio() functions
> can be hidden in the specifically crafted a new data structure,
> but this is out of scope of the _fixes_ series. I'm all ears as
> well for that one.
> 
> Andy Shevchenko (3):
>    gpiolib: acpi: Add missing memset(0) to acpi_get_gpiod_from_data()
>    gpiolib: Fix debug messaging in gpiod_find_and_request()
>    gpiolib: Make debug messages in gpiod_find_by_fwnode() less confusing

For the series
Tested-by: Ferry Toth <fntoth@gmail.com>

>   drivers/gpio/gpiolib-acpi.c   | 10 ++++-----
>   drivers/gpio/gpiolib-acpi.h   | 13 ++++++------
>   drivers/gpio/gpiolib-of.c     | 13 ++++++------
>   drivers/gpio/gpiolib-of.h     |  8 ++++----
>   drivers/gpio/gpiolib-swnode.c |  4 ++--
>   drivers/gpio/gpiolib-swnode.h |  1 +
>   drivers/gpio/gpiolib.c        | 38 ++++++++++++++++++++---------------
>   7 files changed, 48 insertions(+), 39 deletions(-)
> 

Re: [PATCH v1 2/3] gpiolib: Fix debug messaging in gpiod_find_and_request()

[Andy Shevchenko]

On Thu, Oct 19, 2023 at 12:21:12PM -0700, Dmitry Torokhov wrote:
> On Thu, Oct 19, 2023 at 08:34:56PM +0300, Andy Shevchenko wrote:
> > When consolidating GPIO lookups in ACPI code, the debug messaging
> > had been broken and hence lost a bit of sense. Restore debug
> > messaging in gpiod_find_and_request() when configuring the GPIO
> > line via gpiod_configure_flags().
> 
> Could you give an example of the before/after messages to show exavtly
> what is being improved?

Before your patch:

[    5.266823] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
[   14.182994] gpio-40 (?): no flags found for gpios

After your patch:

[    5.085048] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
[   13.401402] gpio-40 (?): no flags found for (null)

After this patch:

[    3.871185] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
[   12.491998] gpio-40 (?): no flags found for gpios

...

Looking at this it's definitely a fix.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] gpiolib: Fix debug messaging in gpiod_find_and_request()

[Linus Walleij]

On Thu, Oct 19, 2023 at 7:35 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> When consolidating GPIO lookups in ACPI code, the debug messaging
> had been broken and hence lost a bit of sense. Restore debug
> messaging in gpiod_find_and_request() when configuring the GPIO
> line via gpiod_configure_flags().
>
> Reported-by: Ferry Toth <ftoth@exalondelft.nl>
> Fixes: 8eb1f71e7acc ("gpiolib: consolidate GPIO lookups")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Please copy the example dmesg outputs you sent in reply to
Dmitry and with that:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v1 3/3] gpiolib: Make debug messages in gpiod_find_by_fwnode() less confusing

[Linus Walleij]

On Thu, Oct 19, 2023 at 7:35 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> Currently the extended debug messages have added confusion,
> but value when con_id is NULL, which is the case for, e.g.,
> GPIO LEDs.
>
> Improve the messaging by using GPIO function name rather than con_id.
> This requires to split and move the second part after the respective
> calls.
>
> Reported-by: Ferry Toth <ftoth@exalondelft.nl>
> Fixes: 8eb1f71e7acc ("gpiolib: consolidate GPIO lookups")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

It sounds nice and looks reasonable so:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

A dmesg example before/after here would be nice as
well, if possible.

Yours,
Linus Walleij

Re: [PATCH v1 2/3] gpiolib: Fix debug messaging in gpiod_find_and_request()

[Dmitry Torokhov]

On Tue, Nov 14, 2023 at 05:29:59PM +0200, Andy Shevchenko wrote:
> On Thu, Oct 19, 2023 at 12:21:12PM -0700, Dmitry Torokhov wrote:
> > On Thu, Oct 19, 2023 at 08:34:56PM +0300, Andy Shevchenko wrote:
> > > When consolidating GPIO lookups in ACPI code, the debug messaging
> > > had been broken and hence lost a bit of sense. Restore debug
> > > messaging in gpiod_find_and_request() when configuring the GPIO
> > > line via gpiod_configure_flags().
> > 
> > Could you give an example of the before/after messages to show exavtly
> > what is being improved?
> 
> Before your patch:
> 
> [    5.266823] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
> [   14.182994] gpio-40 (?): no flags found for gpios
> 
> After your patch:
> 
> [    5.085048] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
> [   13.401402] gpio-40 (?): no flags found for (null)
> 
> After this patch:
> 
> [    3.871185] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
> [   12.491998] gpio-40 (?): no flags found for gpios
> 
> ...
> 
> Looking at this it's definitely a fix.

If this ("(null)" vs static "gpios" string) is important, can we reduce
the patch to:

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 76e0c38026c3..b868c016a9be 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4151,7 +4151,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 
 	/* No particular flag request, return here... */
 	if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
-		gpiod_dbg(desc, "no flags found for %s\n", con_id);
+		gpiod_dbg(desc, "no flags found for %s\n", con_id ?: "gpios");
 		return 0;
 	}
 

instead of plumbing the names through?

Although this (and the original fix patch) are losing information, in
the sense that "(null)" explicitly communicates that caller used
default/NULL conn_id, and not something like "gpios-gpios".

Thanks.

-- 
Dmitry

Re: [PATCH v1 2/3] gpiolib: Fix debug messaging in gpiod_find_and_request()

[andy.shevchenko]

Tue, Nov 14, 2023 at 09:18:24PM +0000, Dmitry Torokhov kirjoitti:
> On Tue, Nov 14, 2023 at 05:29:59PM +0200, Andy Shevchenko wrote:
> > On Thu, Oct 19, 2023 at 12:21:12PM -0700, Dmitry Torokhov wrote:
> > > On Thu, Oct 19, 2023 at 08:34:56PM +0300, Andy Shevchenko wrote:


> > > > When consolidating GPIO lookups in ACPI code, the debug messaging
> > > > had been broken and hence lost a bit of sense. Restore debug
> > > > messaging in gpiod_find_and_request() when configuring the GPIO
> > > > line via gpiod_configure_flags().
> > > 
> > > Could you give an example of the before/after messages to show exavtly
> > > what is being improved?
> > 
> > Before your patch:
> > 
> > [    5.266823] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
> > [   14.182994] gpio-40 (?): no flags found for gpios
> > 
> > After your patch:
> > 
> > [    5.085048] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
> > [   13.401402] gpio-40 (?): no flags found for (null)
> > 
> > After this patch:
> > 
> > [    3.871185] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
> > [   12.491998] gpio-40 (?): no flags found for gpios
> > 
> > ...
> > 
> > Looking at this it's definitely a fix.
> 
> If this ("(null)" vs static "gpios" string) is important, can we reduce
> the patch to:
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 76e0c38026c3..b868c016a9be 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -4151,7 +4151,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
>  
>  	/* No particular flag request, return here... */
>  	if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
> -		gpiod_dbg(desc, "no flags found for %s\n", con_id);
> +		gpiod_dbg(desc, "no flags found for %s\n", con_id ?: "gpios");
>  		return 0;
>  	}
>  
> 
> instead of plumbing the names through?

Definitely no, because how can you guess that this is "gpios" and not "gpio"?

> Although this (and the original fix patch) are losing information, in
> the sense that "(null)" explicitly communicates that caller used
> default/NULL conn_id, and not something like "gpios-gpios".

This is not true, there was no such information before your patch and NULL
pointer printing is simply a bad style programming. We already had the cases
when users were scary by "NULL device *" and other similar stuff when it's
practically no problems in the flow. This has to be fixed anyway.

And what's the practical meaning of gpios-gpios / gpio-gpios / gpios-gpio /
gpio-gpio? I believe they are so weird that thinking about them would be lowest
priority over the issues with the messaging there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] gpiolib: Fix debug messaging in gpiod_find_and_request()

[Dmitry Torokhov]

On Wed, Nov 15, 2023 at 03:37:58AM +0200, andy.shevchenko@gmail.com wrote:
> Tue, Nov 14, 2023 at 09:18:24PM +0000, Dmitry Torokhov kirjoitti:
> > On Tue, Nov 14, 2023 at 05:29:59PM +0200, Andy Shevchenko wrote:
> > > On Thu, Oct 19, 2023 at 12:21:12PM -0700, Dmitry Torokhov wrote:
> > > > On Thu, Oct 19, 2023 at 08:34:56PM +0300, Andy Shevchenko wrote:
> 
> 
> > > > > When consolidating GPIO lookups in ACPI code, the debug messaging
> > > > > had been broken and hence lost a bit of sense. Restore debug
> > > > > messaging in gpiod_find_and_request() when configuring the GPIO
> > > > > line via gpiod_configure_flags().
> > > > 
> > > > Could you give an example of the before/after messages to show exavtly
> > > > what is being improved?
> > > 
> > > Before your patch:
> > > 
> > > [    5.266823] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
> > > [   14.182994] gpio-40 (?): no flags found for gpios
> > > 
> > > After your patch:
> > > 
> > > [    5.085048] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
> > > [   13.401402] gpio-40 (?): no flags found for (null)
> > > 
> > > After this patch:
> > > 
> > > [    3.871185] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
> > > [   12.491998] gpio-40 (?): no flags found for gpios
> > > 
> > > ...
> > > 
> > > Looking at this it's definitely a fix.
> > 
> > If this ("(null)" vs static "gpios" string) is important, can we reduce
> > the patch to:
> > 
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 76e0c38026c3..b868c016a9be 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -4151,7 +4151,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> >  
> >  	/* No particular flag request, return here... */
> >  	if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
> > -		gpiod_dbg(desc, "no flags found for %s\n", con_id);
> > +		gpiod_dbg(desc, "no flags found for %s\n", con_id ?: "gpios");
> >  		return 0;
> >  	}
> >  
> > 
> > instead of plumbing the names through?
> 
> Definitely no, because how can you guess that this is "gpios" and not "gpio"?
> 
> > Although this (and the original fix patch) are losing information, in
> > the sense that "(null)" explicitly communicates that caller used
> > default/NULL conn_id, and not something like "gpios-gpios".
> 
> This is not true, there was no such information before your patch and NULL
> pointer printing is simply a bad style programming. We already had the cases
> when users were scary by "NULL device *" and other similar stuff when it's
> practically no problems in the flow. This has to be fixed anyway.
> 
> And what's the practical meaning of gpios-gpios / gpio-gpios / gpios-gpio /
> gpio-gpio? I believe they are so weird that thinking about them would be lowest
> priority over the issues with the messaging there.

Well, I think we should try to communicate better what it is that we are
printing. Consider your example:

	"gpio-40 (?): no flags found for gpios"

what gpios mean here? You need to go into the code to figure out that it
is connection id (whatever it is for a person who is not ultimately
familiar with gpio subsystem) and not "gpios" in a generic sense. Plus
with your patch you need to ascend a couple of layers up to figure out
that it is connection id and not something else. With "(null)" we at
least did not obfuscate things just so they are visually pleasing to a
random user.

How about we change a message a bit:

	gpiod_dbg(desc, "no flags found for %s gpios\n",
		  con_id ?: "default");

We can argue if "default" should be "unnamed" or "unspecified" or
something else.

And finally what would help most is having a consumer device for which
we are carrying out the operation. You can figure it out from the
sequence of debug messages, but having it right here would be better.

Thanks.

-- 
Dmitry

Re: [PATCH v1 2/3] gpiolib: Fix debug messaging in gpiod_find_and_request()

[Andy Shevchenko]

On Wed, Nov 15, 2023 at 02:57:06PM +0000, Dmitry Torokhov wrote:
> On Wed, Nov 15, 2023 at 03:37:58AM +0200, andy.shevchenko@gmail.com wrote:
> > Tue, Nov 14, 2023 at 09:18:24PM +0000, Dmitry Torokhov kirjoitti:
> > > On Tue, Nov 14, 2023 at 05:29:59PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Oct 19, 2023 at 12:21:12PM -0700, Dmitry Torokhov wrote:
> > > > > On Thu, Oct 19, 2023 at 08:34:56PM +0300, Andy Shevchenko wrote:

Sorry for the late reply. Took me a bit to go through other things first.

...

> > > > > > When consolidating GPIO lookups in ACPI code, the debug messaging
> > > > > > had been broken and hence lost a bit of sense. Restore debug
> > > > > > messaging in gpiod_find_and_request() when configuring the GPIO
> > > > > > line via gpiod_configure_flags().
> > > > > 
> > > > > Could you give an example of the before/after messages to show exavtly
> > > > > what is being improved?
> > > > 
> > > > Before your patch:
> > > > 
> > > > [    5.266823] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
> > > > [   14.182994] gpio-40 (?): no flags found for gpios
> > > > 
> > > > After your patch:
> > > > 
> > > > [    5.085048] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
> > > > [   13.401402] gpio-40 (?): no flags found for (null)
> > > > 
> > > > After this patch:
> > > > 
> > > > [    3.871185] gpio-96 (ACPI:OpRegion): no flags found for ACPI:OpRegion
> > > > [   12.491998] gpio-40 (?): no flags found for gpios
> > > > 
> > > > ...
> > > > 
> > > > Looking at this it's definitely a fix.
> > > 
> > > If this ("(null)" vs static "gpios" string) is important, can we reduce
> > > the patch to:
> > > 
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index 76e0c38026c3..b868c016a9be 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -4151,7 +4151,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
> > >  
> > >  	/* No particular flag request, return here... */
> > >  	if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
> > > -		gpiod_dbg(desc, "no flags found for %s\n", con_id);
> > > +		gpiod_dbg(desc, "no flags found for %s\n", con_id ?: "gpios");
> > >  		return 0;
> > >  	}
> > >  
> > > 
> > > instead of plumbing the names through?
> > 
> > Definitely no, because how can you guess that this is "gpios" and not "gpio"?
> > 
> > > Although this (and the original fix patch) are losing information, in
> > > the sense that "(null)" explicitly communicates that caller used
> > > default/NULL conn_id, and not something like "gpios-gpios".
> > 
> > This is not true, there was no such information before your patch and NULL
> > pointer printing is simply a bad style programming. We already had the cases
> > when users were scary by "NULL device *" and other similar stuff when it's
> > practically no problems in the flow. This has to be fixed anyway.
> > 
> > And what's the practical meaning of gpios-gpios / gpio-gpios / gpios-gpio /
> > gpio-gpio? I believe they are so weird that thinking about them would be lowest
> > priority over the issues with the messaging there.
> 
> Well, I think we should try to communicate better what it is that we are
> printing. Consider your example:
> 
> 	"gpio-40 (?): no flags found for gpios"
> 
> what gpios mean here? You need to go into the code to figure out that it
> is connection id (whatever it is for a person who is not ultimately
> familiar with gpio subsystem) and not "gpios" in a generic sense. Plus
> with your patch you need to ascend a couple of layers up to figure out
> that it is connection id and not something else. With "(null)" we at
> least did not obfuscate things just so they are visually pleasing to a
> random user.
> 
> How about we change a message a bit:
> 
> 	gpiod_dbg(desc, "no flags found for %s gpios\n",
> 		  con_id ?: "default");
> 
> We can argue if "default" should be "unnamed" or "unspecified" or
> something else.

We can use something with a space that would definitely may not be a connection
ID (in the DT/ACPI/swnode[?]).

Let me figure out, but yes, can be a workaround as a quickfix.

> And finally what would help most is having a consumer device for which
> we are carrying out the operation. You can figure it out from the
> sequence of debug messages, but having it right here would be better.

Maybe, but it's out of scope of this fix.

-- 
With Best Regards,
Andy Shevchenko