| Message ID | 20230125181711.30844-1-henning.schild@siemens.com |
|---|---|
| State | New |
| Headers | show |
| Series | leds: simatic-ipc-leds-gpio: do not run into endless EPROBE_DEFER loop | expand |
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:
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:
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.
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:
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(-)