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