[PATCH] leds: simatic-ipc-leds-gpio: do not run into endless EPROBE_DEFER loop

[PATCH] leds: simatic-ipc-leds-gpio: do not run into endless EPROBE_DEFER loop

[Henning Schild]

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@siemens.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;
+			}
+			goto out;
+		}
+		gpiod_put(gpiod);
 	}
-	gpiod_put(gpiod);
 
 	return 0;
 out:
-- 
2.39.1

Re: [PATCH] leds: simatic-ipc-leds-gpio: do not run into endless EPROBE_DEFER loop

[Henning Schild]

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@siemens.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@siemens.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:

Re: [PATCH] leds: simatic-ipc-leds-gpio: do not run into endless EPROBE_DEFER loop

[Henning Schild]

Am Wed, 25 Jan 2023 19:17:11 +0100
schrieb Henning Schild <henning.schild@siemens.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.

This is a really bad idea. The real fix for the future will be to
write individual drivers which clearly describe their dep chains.

This patch should not be merged.

Henning
 
> Fixes: a6c80bec3c93 ("leds: simatic-ipc-leds-gpio: Add GPIO version
> of Siemens driver") Signed-off-by: Henning Schild
> <henning.schild@siemens.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;
> +			}
> +			goto out;
> +		}
> +		gpiod_put(gpiod);
>  	}
> -	gpiod_put(gpiod);
>  
>  	return 0;
>  out:

Re: [PATCH] leds: simatic-ipc-leds-gpio: do not run into endless EPROBE_DEFER loop

[Lee Jones]

On Thu, 02 Feb 2023, Henning Schild wrote:

> Am Wed, 25 Jan 2023 19:17:11 +0100
> schrieb Henning Schild <henning.schild@siemens.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.
> 
> This is a really bad idea. The real fix for the future will be to
> write individual drivers which clearly describe their dep chains.
> 
> This patch should not be merged.

Dropped.

-- 
Lee Jones [李琼斯]