linux-kernel - Re: [PATCH] leds: simatic-ipc-leds-gpio: do not run into endless EPROBE

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 25 Jan 2023 19:30:34 +0100
From:   Henning Schild <henning.schild@...mens.com>
To:     Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        <linux-leds@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] leds: simatic-ipc-leds-gpio: do not run into endless
 EPROBE_DEFER loop

This is a replacement of what was sent as

[PATCH v4] leds: simatic-ipc-leds-gpio: make sure we have the GPIO
providing driver

earlier.

While that one is still valid, what i propose here is based on a
hopefully better understanding of the problem i was trying to solve.

That other one already entered some "about to merge" stages, so not
sure it can or should be stopped. If this one makes it the other one
can be reverted should it ever be merged as well.

Am Wed, 25 Jan 2023 19:17:11 +0100
schrieb Henning Schild <henning.schild@...mens.com>:

> Should the driver providing our GPIOs not be available we used to
> return -EPORBE_DEFER out of the probe function and cause a
> potentially endless loop that also printed a lot to the kernel log.
> 
> ...
> leds-gpio leds-gpio: cannot find GPIO chip igpio-f7188x-2, deferring
> leds-gpio leds-gpio: Skipping unavailable LED gpio 0 (red:status-1)
> ...
> 
> The "leds-gpio" just ignores all entries and would never try again
> even if the GPIOs show up later. But our extra two GPIOs could cause
> that loop, in which we would even register/unregister "leds-gpio" and
> cause all the printing.
> 
> If any of those two extra GPIOs is not there, return with -ENODEV
> instead of -EPROBE_DEFER.
> 
> Fixes: a6c80bec3c93 ("leds: simatic-ipc-leds-gpio: Add GPIO version
> of Siemens driver") Signed-off-by: Henning Schild
> <henning.schild@...mens.com> ---
>  drivers/leds/simple/simatic-ipc-leds-gpio.c | 29
> +++++++++------------ 1 file changed, 13 insertions(+), 16
> deletions(-)
> 
> diff --git a/drivers/leds/simple/simatic-ipc-leds-gpio.c
> b/drivers/leds/simple/simatic-ipc-leds-gpio.c index
> 07f0d79d604d..4e2595e684c6 100644 ---
> a/drivers/leds/simple/simatic-ipc-leds-gpio.c +++
> b/drivers/leds/simple/simatic-ipc-leds-gpio.c @@ -74,14 +74,13 @@
> static int simatic_ipc_leds_gpio_probe(struct platform_device *pdev)
> const struct simatic_ipc_platform *plat = pdev->dev.platform_data;
> struct gpio_desc *gpiod; int err;
> +	int i;
>  
>  	switch (plat->devmode) {
>  	case SIMATIC_IPC_DEVICE_127E:
>  		simatic_ipc_led_gpio_table =
> &simatic_ipc_led_gpio_table_127e; break;
>  	case SIMATIC_IPC_DEVICE_227G:
> -		if (!IS_ENABLED(CONFIG_GPIO_F7188X))
> -			return -ENODEV;
>  		request_module("gpio-f7188x");
>  		simatic_ipc_led_gpio_table =
> &simatic_ipc_led_gpio_table_227g; break;
> @@ -99,21 +98,19 @@ static int simatic_ipc_leds_gpio_probe(struct
> platform_device *pdev) goto out;
>  	}
>  
> -	/* PM_BIOS_BOOT_N */
> -	gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 6,
> GPIOD_OUT_LOW);
> -	if (IS_ERR(gpiod)) {
> -		err = PTR_ERR(gpiod);
> -		goto out;
> -	}
> -	gpiod_put(gpiod);
> -
> -	/* PM_WDT_OUT */
> -	gpiod = gpiod_get_index(&simatic_leds_pdev->dev, NULL, 7,
> GPIOD_OUT_LOW);
> -	if (IS_ERR(gpiod)) {
> -		err = PTR_ERR(gpiod);
> -		goto out;
> +	for (i = 0; i < 2; i++) {
> +		gpiod = gpiod_get_index(&simatic_leds_pdev->dev,
> NULL,
> +
> ARRAY_SIZE(simatic_ipc_gpio_leds) + i, GPIOD_OUT_LOW);
> +		if (IS_ERR(gpiod)) {
> +			err = PTR_ERR(gpiod);
> +			if (err == -EPROBE_DEFER) {
> +				dev_err(&pdev->dev, "GPIO driver
> seems missing\n");
> +				err = -ENODEV;

I tested this and it does work. But so far i could only test it on the
Super-IO based tigerlake box which uses
"request_module("gpio-f7188x");", not on the broxton where the pinctrl
driver gets loaded via some P2SB platform detection magic.

Not sure the probe order i want is guaranteed here. I always need the
probe order so that the pinctl driver is probed and create the GPIOs
before this driver probes. Otherwise this driver will give up early and
never try again.

Maybe it is already guaranteed or maybe i can also use request_module
for pinctrl-broxton as well to express the order i want. The
request_module for that Super-IO is actually not there for "ordering"
but to actually load a module which otherwise would not probe at all.

If request_module is an option and anything needs to be done, i will
add a comment to the two calls to say why there are there.

Henning

> +			}
> +			goto out;
> +		}
> +		gpiod_put(gpiod);
>  	}
> -	gpiod_put(gpiod);
>  
>  	return 0;
>  out: