Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Wed, 15 Nov 2023 14:57:06 +0000
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     andy.shevchenko@gmail.com
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Bartosz Golaszewski <bartosz.golaszewski@linaro.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        linux-gpio@vger.kernel.org, linux-acpi@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Hans de Goede <hdegoede@redhat.com>,
        Ferry Toth <ftoth@exalondelft.nl>
Subject: Re: [PATCH v1 2/3] gpiolib: Fix debug messaging in
 gpiod_find_and_request()
Message-ID: <ZVTcQnH_BEEFJ2Ut@google.com>
References: <20231019173457.2445119-1-andriy.shevchenko@linux.intel.com>
 <20231019173457.2445119-3-andriy.shevchenko@linux.intel.com>
 <ZTGBqEUzgGCcZP1B@google.com>
 <ZVOSd62yCz4lFIP1@smile.fi.intel.com>
 <ZVPkIOk6gvnwkp9F@google.com>
 <ZVQg9hTQwGtNwIhv@surfacebook.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZVQg9hTQwGtNwIhv@surfacebook.localdomain>
Precedence: bulk

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