linux-kernel - RE: [PATCH] pps: clients: gpio: Continue after failing to get optional ECHO pin

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <SJ0PR84MB16523160FD193563BE3FF26789FF2@SJ0PR84MB1652.NAMPRD84.PROD.OUTLOOK.COM>
Date: Mon, 3 Jun 2024 22:32:00 +0000
From: "Klein, Curtis" <curtis.klein@....com>
To: Rodolfo Giometti <giometti@...eenne.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] pps: clients: gpio: Continue after failing to get
 optional ECHO pin

On Sunday, June 2, 2024 11:33 PM, Rodolfo Giometti wrote: 
> On 01/06/24 00:44, Curtis Klein wrote:
> > Warn but do not fail when devm_gpiod_get_optional returns an error when
> > trying to get the ECHO pin. When creating a pps-gpio device using
> > platform data and GPIO lookup tables, the call to gpiod_get_optional
> > will match on the unlabeled pin meant as the input when searching for
> > the "echo" pin. Since it is already in use as the PPS input, it will
> 
> I'm not sure to well understand what you mean here: why the "echo" pin
> should be
> already in use as the PPS input?
> 
> Can you please explain better this situation with a real example?
> 

Sure, sorry it's kind of hard to explain with words. Here's the gpio
lookup table I'm using: 

  static struct gpiod_lookup_table pps_gpio_table = {
      .dev_id = "pps-gpio.0",
      .table  = {
          GPIO_LOOKUP_IDX("gpio.0", 23, NULL, 0, GPIO_ACTIVE_HIGH),
          {} /* Terminating entry */
      }};

We have a single unnamed entry that is meant to be used as the input
pin. No "echo" pin is defined. We register this using 
"gpiod_add_lookup_table(&pps_gpio_table)".

So the first time the pps-gpio driver gets the input pin with 
"devm_gpiod_get(dev, NULL, GPIOD_IN);", we will get the one and only 
gpio entry in the lookup table. Then when we try to get the "echo" pin
with "devm_gpiod_get_optional(dev, "echo",  GPIOD_OUT_LOW);" we will
also match the same entry in the lookup table. This is because the
matching logic in gpio_find (in "drivers/gpio/gpiolib.c") will allow an
unnamed pin in a lookup table to match any request from a gpio_get
function. When this happens, "devm_gpiod_get_optional" returns -EBUSY
because the pin is in use (as the pps-gpio input) and the pps-gpio
driver fails to load.

Ideally, I think you'd use a swnode to define the gpios (if ACPI changes
are not possible). But this change would unblock those that need to use
gpio lookup tables. Continuing after an error on a optional operation
seems okay to me, but I understand if that goes against the normal
convention.

> > fail with -EBUSY. As the ECHO pin is optional, we just warn on the error
> > and continue the initialization. This allows us to support devices
> > created using GPIO lookup tables instead of ACPI, DT, swnode, etc.
> >
> > Signed-off-by: Curtis Klein <curtis.klein@....com>
> > ---
> >   drivers/pps/clients/pps-gpio.c | 9 ++++++---
> >   1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
> > index 2f4b11b4dfcd..b7db4a3ee97e 100644
> > --- a/drivers/pps/clients/pps-gpio.c
> > +++ b/drivers/pps/clients/pps-gpio.c
> > @@ -114,9 +114,12 @@ static int pps_gpio_setup(struct device *dev)
> >   		device_property_read_bool(dev, "assert-falling-edge");
> >
> >   	data->echo_pin = devm_gpiod_get_optional(dev, "echo",
> GPIOD_OUT_LOW);
> > -	if (IS_ERR(data->echo_pin))
> > -		return dev_err_probe(dev, PTR_ERR(data->echo_pin),
> > -				     "failed to request ECHO GPIO\n");
> > +	if (IS_ERR(data->echo_pin)) {
> > +		dev_warn(dev, "failed to request ECHO GPIO: %ld\n",
> > +			 PTR_ERR(data->echo_pin));
> > +		data->echo_pin = NULL;
> > +		return 0;
> > +	}
> >
> >   	if (!data->echo_pin)
> >   		return 0;
> 
> Thanks,
> 
> Rodolfo
> 
> --
> GNU/Linux Solutions                  e-mail: giometti@...eenne.com
> Linux Device Driver                          giometti@...ux.it
> Embedded Systems                     phone:  +39 349 2432127
> UNIX programming

-Curtis