[PATCH v2] leds: Mark GPIO LED trigger broken

[PATCH v2] leds: Mark GPIO LED trigger broken

[Linus Walleij]

The GPIO LED trigger exposes a userspace ABI where a user
can echo a GPIO number from the global GPIO numberspace into
a file that will trigger a certain LED when active.

This is problematic because the global GPIO numberspace is
inherently instable. The trigger came about at a time when
systems had one GPIO controller that defined hard-wired
GPIOs numbered 0..N and this number space was stable.

We have since moved to dynamic allocation of GPIO numbers
and there is no real guarantee that a GPIO number will stay
consistent even across a reboot: consider a USB attached
GPIO controller for example. Or two. Or the effect of
probe order after adding -EPROBE_DEFER to the kernel.

The trigger was added to support keypad LEDs on the Nokia
n810 from the GPIO event when a user slides up/down the
keypad. This is arch/arm/boot/dts/omap2420-n810.dts.
A userspace script is needed to activate the trigger.
This will be broken unless the script was updated recently
since the OMAP GPIO controller now uses dynamic GPIO
number allocations.

I want to know that this trigger has active users that
cannot live without it if we are to continue to support it.

Option if this is really needed: I can develop a new trigger
that can associate GPIOs with LEDs as triggers using device
tree, which should also remove the use of userspace custom
scripts to achieve this and be much more trustworthy, if
someone with the Nokia n810 or a device with a similar need
is willing to test it.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: linux-omap@vger.kernel.org
Cc: linux-gpio@vger.kernel.org

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Be less intrusive and just mark the feature broken
  for now.
---
 drivers/leds/trigger/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index dc6816d36d06..2a57328eca20 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -83,6 +83,7 @@ config LEDS_TRIGGER_ACTIVITY
 config LEDS_TRIGGER_GPIO
 	tristate "LED GPIO Trigger"
 	depends on GPIOLIB || COMPILE_TEST
+	depends on BROKEN
 	help
 	  This allows LEDs to be controlled by gpio events. It's good
 	  when using gpios as switches and triggering the needed LEDs
-- 
2.34.1

Re: [PATCH v2] leds: Mark GPIO LED trigger broken

[Lee Jones]

On Tue, 14 Mar 2023, Linus Walleij wrote:

> The GPIO LED trigger exposes a userspace ABI where a user
> can echo a GPIO number from the global GPIO numberspace into
> a file that will trigger a certain LED when active.
>
> This is problematic because the global GPIO numberspace is
> inherently instable. The trigger came about at a time when
> systems had one GPIO controller that defined hard-wired
> GPIOs numbered 0..N and this number space was stable.
>
> We have since moved to dynamic allocation of GPIO numbers
> and there is no real guarantee that a GPIO number will stay
> consistent even across a reboot: consider a USB attached
> GPIO controller for example. Or two. Or the effect of
> probe order after adding -EPROBE_DEFER to the kernel.
>
> The trigger was added to support keypad LEDs on the Nokia
> n810 from the GPIO event when a user slides up/down the
> keypad. This is arch/arm/boot/dts/omap2420-n810.dts.
> A userspace script is needed to activate the trigger.
> This will be broken unless the script was updated recently
> since the OMAP GPIO controller now uses dynamic GPIO
> number allocations.
>
> I want to know that this trigger has active users that
> cannot live without it if we are to continue to support it.
>
> Option if this is really needed: I can develop a new trigger
> that can associate GPIOs with LEDs as triggers using device
> tree, which should also remove the use of userspace custom
> scripts to achieve this and be much more trustworthy, if
> someone with the Nokia n810 or a device with a similar need
> is willing to test it.
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: linux-omap@vger.kernel.org
> Cc: linux-gpio@vger.kernel.org
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Be less intrusive and just mark the feature broken
>   for now.
> ---
>  drivers/leds/trigger/Kconfig | 1 +
>  1 file changed, 1 insertion(+)

Added Pavel's Suggested-by:

Applied, thanks

--
Lee Jones [李琼斯]

Re: [PATCH v2] leds: Mark GPIO LED trigger broken

[Jan Kundrát]

> I want to know that this trigger has active users that
> cannot live without it if we are to continue to support it.

We're using this feature. Our use case is a LED at the front panel which 
shows whether a signal is present at an input LC optical connector (DWDM 
network stuff). Here's how we're setting it up:

https://gerrit.cesnet.cz/plugins/gitiles/CzechLight/br2-external/+/6570b571bbf3f53cf24ef2be3079bc282c445b9e/package/czechlight-clearfog-leds/init-leds-edfa.sh

I understand that the GPIO numeric namespace is racy, but it's never been a 
problem for us in the past 5 years since this script runs much later during 
boot than any driver probing.

> Option if this is really needed: I can develop a new trigger
> that can associate GPIOs with LEDs as triggers using device
> tree, which should also remove the use of userspace custom
> scripts to achieve this and be much more trustworthy, if
> someone with the Nokia n810 or a device with a similar need
> is willing to test it.

I'll be happy to test a patch like that.

However, the GPIO in question on our board is connected to a MCP23S18, and 
we have a pair of these. When used in this configuration (two chips at the 
same SPI CS, differing by a chip-specific "HW address" on a HW level), 
there are some impedance mismatches because it's essentially two 
independent pinctrl instances on the same SPI address. This causes 
problems, e.g. the debugfs pinctrl instance won't be created for the second 
chip because of a naming conflict. We also carry this out-of-tree patch to 
make the GPIO labels work when set from DTS:

https://patchwork.ozlabs.org/project/linux-gpio/patch/517dcdda21ea0b0df884bc6adcba1dadb78b66b1.1551966077.git.jan.kundrat@cesnet.cz/

(Feedback on how to solve that problem is welcome, btw.)

Since I am not that much familiar with pinctrl/gpio stuff in kernel, I 
wanted to bring this up to make sure that your future trigger works even on 
a setup like ours. Here's how it's used via DTS in case it's relevant:

https://gerrit.cesnet.cz/plugins/gitiles/CzechLight/br2-external/+/6570b571bbf3f53cf24ef2be3079bc282c445b9e/board/czechlight/clearfog/sdn-roadm-clearfog.dtsi#151

With kind regards,
Jan

Re: [PATCH v2] leds: Mark GPIO LED trigger broken

[Linus Walleij]

On Thu, Aug 24, 2023 at 8:32 PM Jan Kundrát <jan.kundrat@cesnet.cz> wrote:

> > I want to know that this trigger has active users that
> > cannot live without it if we are to continue to support it.
>
> We're using this feature. Our use case is a LED at the front panel which
> shows whether a signal is present at an input LC optical connector (DWDM
> network stuff). Here's how we're setting it up:
>
> https://gerrit.cesnet.cz/plugins/gitiles/CzechLight/br2-external/+/6570b571bbf3f53cf24ef2be3079bc282c445b9e/package/czechlight-clearfog-leds/init-leds-edfa.sh

Interesting!

> I understand that the GPIO numeric namespace is racy, but it's never been a
> problem for us in the past 5 years since this script runs much later during
> boot than any driver probing.

Hm OK that does make sequential sense in a way.

> > Option if this is really needed: I can develop a new trigger
> > that can associate GPIOs with LEDs as triggers using device
> > tree, which should also remove the use of userspace custom
> > scripts to achieve this and be much more trustworthy, if
> > someone with the Nokia n810 or a device with a similar need
> > is willing to test it.
>
> I'll be happy to test a patch like that.

OK let's just do it. I'll cook something up.

> However, the GPIO in question on our board is connected to a MCP23S18, and
> we have a pair of these. When used in this configuration (two chips at the
> same SPI CS, differing by a chip-specific "HW address" on a HW level),
> there are some impedance mismatches because it's essentially two
> independent pinctrl instances on the same SPI address. This causes
> problems, e.g. the debugfs pinctrl instance won't be created for the second
> chip because of a naming conflict. We also carry this out-of-tree patch to
> make the GPIO labels work when set from DTS:
>
> https://patchwork.ozlabs.org/project/linux-gpio/patch/517dcdda21ea0b0df884bc6adcba1dadb78b66b1.1551966077.git.jan.kundrat@cesnet.cz/
>
> (Feedback on how to solve that problem is welcome, btw.)

Instead of using the of_* prefixed device tree traversal
functions, use the fwnode API because we have Intel ACPI
users of this driver, so we need to use the right abstraction.
Just use the similar functions from include/linux/property.h
and grep around for some example that does this already.

Use the bits in spi-present-mask = <0x05>; to iterate perhaps?
Like use for_each_set_bit() from <linux/bitmap.h> and
then we should in theory make the YAML description warn
if that bitmask and the nodes don't match up (yeah...)
since the kernel is not a device tree (or other HW description)
validator.

Then I think you should just split up that patch in one DT binding patch
and one patch for the driver and send it out in a series
"support line names on MCP23S18" or somthing, the MCP
expander is really popular so this is highly desired.

I hope the DT people don't randomly ask you to rewrite the
bindings in YAML. It could happen that they do.

> Since I am not that much familiar with pinctrl/gpio stuff in kernel, I
> wanted to bring this up to make sure that your future trigger works even on
> a setup like ours. Here's how it's used via DTS in case it's relevant:
>
> https://gerrit.cesnet.cz/plugins/gitiles/CzechLight/br2-external/+/6570b571bbf3f53cf24ef2be3079bc282c445b9e/board/czechlight/clearfog/sdn-roadm-clearfog.dtsi#151

Looks reasonable to me.

Yours,
Linus Walleij