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
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 <[email protected]>
Fixes: 8eb1f71e7acc ("gpiolib: consolidate GPIO lookups")
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
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
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 <[email protected]>
> 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(-)
>
On Wed, Nov 15, 2023 at 03:37:58AM +0200, [email protected] 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
On Wed, Nov 15, 2023 at 02:57:06PM +0000, Dmitry Torokhov wrote:
> On Wed, Nov 15, 2023 at 03:37:58AM +0200, [email protected] 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