Re: Leds-gpio discarding the entries in /sys/class/leds : Linux 5.4.38

Jacek Anaszewski
2020-06-22T20:08:50Z

On 6/22/20 3:12 PM, Vishwanatha Subbanna wrote:
> Hi Jacek,
>
>> On 22-Jun-2020, at 5:06 PM, Jacek Anaszewski
>> <jacek.anaszewski@gmail.com> wrote:
>>
>> On 6/22/20 1:07 PM, Vishwanatha Subbanna wrote:
>>> Hi Jacek,
>>>> On 22-Jun-2020, at 4:24 PM, Jacek Anaszewski
>>>> <jacek.anaszewski@gmail.com> wrote:
>>>>
>>>> Hi Vishwanatha,
>>>>
>>>> On 6/22/20 8:58 AM, Vishwanatha Subbanna wrote:
>>>>> Thank you very much Jacek.
>>>>>> On 22-Jun-2020, at 3:12 AM, Jacek Anaszewski
>>>>>> <jacek.anaszewski@gmail.com> wrote:
>>>>>>
>>>>>> Hi Vishwanatha,
>>>>>>
>>>>>> On 6/20/20 7:25 PM, Vishwanatha Subbanna wrote:
>>>>>>> Hi Jacek,
>>>>>>> Thank you very much for the quick response. Greatly appreciate that.
>>>>>>
>>>>>> You're welcome.
>>>>>>
>>>>>>>> On 20-Jun-2020, at 3:27 AM, Jacek Anaszewski
>>>>>>>> <jacek.anaszewski@gmail.com> wrote:
>>>>>>>>
>>>>>>>> Hi Vishwanatha,
>>>>>>>>
>>>>>>>> Please refer to
>>>>>>>> Documentation/devicetree/bindings/leds/leds-pca955x.txt.
>>>>>>>>
>>>>>>>> At first glance I don't get why you have gpio-leds node, which
>>>>>>>> is for
>>>>>>>> leds-gpio driver.
>>>>>>> Not sure I understood it right.. But if you are asking me why I
>>>>>>> have "leds {"  and “gpio-leds” in there, then it is to get the
>>>>>>> entries in /sys/class/leds.
>>>>>>> The GPIOs from PCA9552 are connected to LED. Also, that is how we
>>>>>>> have had in the past, and that worked.
>>>>>>> Example:
>>>>>>> https://github.com/openbmc/linux/blob/dev-5.4/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts#L115
>>>>>>
>>>>>> Thanks. Yeah, that looks OK, I had to take closer look at the driver.
>>>>>>
>>>>>>> The problem I am running into is for :
>>>>>>> https://github.com/openbmc/linux/blob/dev-5.4/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
>>>>>>>>
>>>>>>>> On 6/19/20 3:34 PM, Vishwanatha Subbanna wrote:
>>>>>>>>> Hello,
>>>>>>>>> I am Vishwanath, working with IBM and looking for your help on
>>>>>>>>> one of the issues that I am running into. Would really
>>>>>>>>> appreciate help on this. I run Linux 5.4.38
>>>>>>>>> I have 2 number of PCA9552 chips, one on the Planar and other
>>>>>>>>> on the card that is optionally pluggable. The optional card
>>>>>>>>> must be plugged prior to booting and is not hot pluggable. In
>>>>>>>>> my experiment, I am running *without* the optional card plugged in.
>>>>>>>>> In the device tree, I have a "leds {" section that looks like
>>>>>>>>> below for the PCA9552 that is on the planar and everything
>>>>>>>>> works fine and I can see /sys/class/leds/fan0
>>>>>>>>> leds {
>>>>>>>>>        compatible = "gpio-leds”;
>>>>>>>>>        fan0 {
>>>>>>>>>            retain-state-shutdown;
>>>>>>>>>            default-state = "keep";
>>>>>>>>>            gpios = <&pca0 0 GPIO_ACTIVE_LOW>;
>>>>>>>>>        };
>>>>>>>>> };
>>>>>>>>> &i2c7 {
>>>>>>>>>    status = "okay”;
>>>>>>>>>    pca0: pca9552@61 {
>>>>>>>>>       compatible = "nxp,pca9552";
>>>>>>>>>        reg = <0x61>;
>>>>>>>>>        #address-cells = <1>;
>>>>>>>>>        #size-cells = <0>;
>>>>>>>>>        gpio-controller;
>>>>>>>>>        #gpio-cells = <2>;
>>>>>>>>>        gpio@0 {
>>>>>>>>>            reg = <0>;
>>>>>>>>>            type = <PCA955X_TYPE_GPIO>;
>>>>>>>>> };
>>>>>>>>>    };
>>>>>>>>> };
>>>>>>>>> Similarly, if I update the device tree entry for PCA9552 for
>>>>>>>>> the card that is optionally pluggable, then I don’t see any
>>>>>>>>> leds entries in /sys/class/leds.
>>>>>>>>
>>>>>>>> Please share your DT node after the update.
>>>>>>>>
>>>>>>> Pasting the GPIO_0 entry only here for brevity.
>>>>>>> leds {
>>>>>>>        compatible = "gpio-leds”;
>>>>>>>        fan0 {
>>>>>>>            retain-state-shutdown;
>>>>>>>            default-state = "keep";
>>>>>>>            gpios = <&pca0 0 GPIO_ACTIVE_LOW>;
>>>>>>>        };
>>>>>>>        nvmeslot0 {
>>>>>>>            retain-state-shutdown;
>>>>>>>            default-state = "keep";
>>>>>>>            gpios = <&pca1 0 GPIO_ACTIVE_LOW>;
>>>>>>>        };
>>>>>>> };
>>>>>>> &i2c7 {
>>>>>>>    status = "okay”;
>>>>>>>    pca0: pca9552@61 {
>>>>>>>       compatible = "nxp,pca9552";
>>>>>>>        reg = <0x61>;
>>>>>>>        #address-cells = <1>;
>>>>>>>        #size-cells = <0>;
>>>>>>>        gpio-controller;
>>>>>>>        #gpio-cells = <2>;
>>>>>>>        gpio@0 {
>>>>>>>            reg = <0>;
>>>>>>>            type = <PCA955X_TYPE_GPIO>;
>>>>>>> };
>>>>>>>    };
>>>>>>> };
>>>>>>> &i2c13
>>>>>>> {
>>>>>>>    pca1: pca9552@60 {
>>>>>>>       compatible = "nxp,pca9552";
>>>>>>>       reg = <0x60>;
>>>>>>>       #address-cells = <1>;
>>>>>>>       #size-cells = <0>;
>>>>>>>       gpio-controller;
>>>>>>>       #gpio-cells = <2>;
>>>>>>>       gpio@0 {
>>>>>>>           reg = <0>;
>>>>>>>           type = <PCA955X_TYPE_GPIO>;
>>>>>>>       };
>>>>>>>    };
>>>>>>> };
>>>>>>> Thanks
>>>>>>> !! Vishwa !!
>>>>>>>>> I don’t even see “fan0” that is on the PCA9552 on planar also.
>>>>>>>>> I was expecting that I should see “/sys/class/leds/fan0”.
>>>>>>>>> However, I could see all the entries in “/proc/device-tree/leds”.
>>>>>>>>> Data from the failure.
>>>>>>>>> [    7.895757] leds-pca955x 7-0061: leds-pca955x: Using pca9552
>>>>>>>>> 16-bit LED driver at slave address 0x61
>>>>>>>>> [    7.907659] leds-pca955x 7-0061: gpios 168...183
>>>>>>
>>>>>> It is weird that you don't see "fan0" LED since this gpio seems to
>>>>>> have
>>>>>> been properly registered according to this log.
>>>>>>
>>>>> This is exactly what I don’t understand. I would expect “fan0” to
>>>>> appear in /sys/class/leds. Is there any reason why this might not
>>>>> be appearing ?..
>>>>
>>>> OK, now the reason is clear to me. If leds-gpio driver fails to register
>>>> any of the LEDs found in DT node it returns with an error from the
>>>> probe(), which results in unregistering any of the LEDs registered in
>>>> the previous iteration steps.
>>>>
>>>> Look at the function gpio_leds_create() in
>>>> drivers/leds/leds-gpio.c.
>>>>
>>>> Probably it is devm_fwnode_get_gpiod_from_child() that fails
>>>> while parsing nvmeslot0 node.
>>> Is this how it is designed or a bug ?.. From a system standpoint, not
>>> having an optional card results in not seeing the ones that are
>>> present on the system.
>>> Would you think it is worthwhile to modify to not chuck off what is
>>> existing because something optional is not plugged in ?.. I believe
>>> the I2C driver handles this scenario by putting an error message but
>>> still consumes what is present.
>>
>> Well, this code is in mainline for some time and we cannot guarantee
>> the someone does not rely on this behavior.
>>
>
> Should someone assume those behaviours ?.. Would it be okay to put an
> email to the gpio-leds community ?. Just in my opinion, I see a lot of
> value in modifying it. Also, is there an IRC where we can discuss this
> than the email ?

Changing this behavior could break someone's userspace, which is one of
the most vital no-nos in kernel development. Also, there is no such
entity like gpio-leds community - these are two separate kernel
subsystems, but leds-gpio driver is under LED maintainer's jurisdiction.

I don't think we need IRC discussion yet. Let's wait for the other
LED maintainer's opinion. Pavel?

>> You mentioned, that your card is not hot-pluggable so it is even more
>> justified to treat the two hardware setups as demanding a separate DT.
>>
>
> I mean, it is the same system that can either have a card on the slot or
> don’t have it. So, it’s not really a different system needing different DT.
> Also, it has 3 slots and we can have multiple combinations :)
>
>> Otherwise you could probably employ DT overlays mechanism.
>>
>
> Hmm.. this looks interesting in a quick glance. However, I feel the
> current leds-gpio driver needs to be updated to not discard the valid ones.
> Besides someone may be counting on the existing behaviour, is there any
> reason why we want to maintain leds-gpio the way it is and not do what
> I2C driver does for example.

This is perfectly sufficient reason to not accept such a modification.