| Message ID | 20231019173457.2445119-1-andriy.shevchenko@linux.intel.com |
|---|---|
| Headers | show |
| Series | gpiolib: acpi: More fixes to the consolidation rework | expand |
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.
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
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(-) >
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.
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.
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.
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.
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.