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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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