[WIP] treewide: prepare to drop deprecated LED label property

[adschm]

This PR is in very early WIP state, and I'm actually just posting it here to collect the discussion on it in a common place.

The choice of "migrated" devices is mainly based on what seems helpful for illustration and test.

Currently, status LEDs via aliases are broken, and much else probably is as well.

The kernel patches have been submitted upstream.

Don't worry: This is a project for after the 20.xx branch (we should also have dropped kernel 4.19 by then).

[adschm]

Current problem:
If label is removed, get_led_dt will just use the node name, e.g. "wlan2g"

In /sys/class/leds, the name will be constructed from "color:function", though. And since color is an integer in device tree, we cannot construct it easily.

Edit: Of course I could just name the node "green:wlan2g" or "green-wlan2g", but that defeats the purpose and this would require to rename almost all nodes (and would also deviate from typical names in kernel).

[chunkeey]

we can leave the basename there as a last resort. The led's common.yaml in 56 still states "If omitted, the label is taken from the node name [...]" and drivers like leds-pwm.c have not been converted. It's a barrel of fun.

Here's the output for the MR32 (which uses gpio-leds for and leds-pwm)

MR32# ls -sys/class/leds
amber:fault      blue             green            red              white:indicator

[chunkeey]

... And since I wrote it anyway. There's another worse way to do it. This is the leds.sh portion. It uses a lookup table and needs to convert the color big-endian (because DT is big endian) value to a string.

hex_be32_to_cpu() {
    [ "$(echo 10 | hexdump -v -n 2 -e '/2 "%x"')" = "3031" ] && {
            echo "${1:8:2}${1:6:2}${1:4:2}${1:2:2}${1:0:2}"
            return
    }
    echo "$@"
}

get_dt_color_by_id() {
    val=$(( 0x$(hex_be32_to_cpu $(hexdump -v -e '/4 "%08x"' "$1") ) +1 ))
    echo "white%red%green%blue%amber%violet%yellow%ir" | cut -d% -f "$val"
}

get_dt_name_from_color_function() {
    local ledpath="$1"
    local function
    local funcenum
    local color

    [ ! -f "$ledpath/color" ] && return 1

    color=$(get_dt_color_by_id "$ledpath/color")
    [ -z "$color" ] && return 1 # Something went wrong
    echo -n "$color"

    [ -f "$ledpath/function" ] && { 
            function=$(cat "$ledpath/function")
            [ -n "$function" ] && {
                    echo -n ":$function"

                    funcenum=$(cat "$ledpath/function-enumerator" 2>/dev/null)
                    [ -n "$funcenum" ] && echo -n "-$funcenum"
            }
    }
    return 0
}

get_dt_led_path() {
    local ledpath
    local basepath="/proc/device-tree"
    local nodepath="$basepath/aliases/led-$1"

    [ -f "$nodepath" ] && ledpath=$(cat "$nodepath")
    [ -n "$ledpath" ] && ledpath="$basepath$ledpath"

    echo "$ledpath"
}

get_dt_led() {
    local label
    local ledpath=$(get_dt_led_path $1)

    [ -n "$ledpath" ] && \
            label=$(cat "$ledpath/label" 2>/dev/null) || \
            label=$(cat "$ledpath/chan-name" 2>/dev/null) || \
            label=$(get_dt_name_from_color_function "$ledpath") || \
            label=$(basename "$ledpath")

    echo "$label"
}

[adschm]

we can leave the basename there as a last resort. The led's common.yaml in 56 still states "If omitted, the label is taken from the node name [...]" and drivers like leds-pwm.c have not been converted. It's a barrel of fun.

Yes, I actually just moved it to the get_dt_led_name function in my draft, as I had some problems with the true/false in the chained OR command.

There's another worse way to do it. This is the leds.sh portion. It uses a lookup table and needs to convert the color big-endian (because DT is big endian) value to a string.

I also thought about something like that; the problem is that there is not just color:function, function, color, function-number etc., but there might also be a device-name if the LED is linked to a parent device, and then you would have something like wlan0:green:activity for some leds. That was the point where I stopped to think about assembling the name myself :-(

[adschm]

I had some discussion with upstream about this (due to the submitted patches) and the result was actually quite discouraging:

1. The WLAN2G/WLAN5G functions were effectively rejected, and I was told the proper solution would be to link this to the corresponding wlan device (so that the driver prepends an identifier, ending up with something like wlan0:green:activity instead of device:green:wlan2g. However, that solution is still in the planning/concept phase upstream, so we probably won't even get it for kernel 5.10. (And though I understand why that's desirable conceptually, I don't think it's convenient for us)

2. So far, we are used to relying on the label to get the LED name. The new concept makes this much more complicated by only assembling the name in the driver, so we cannot determine it based on device tree. This will effectively make all solutions based on led aliases in DTS files worse to implement and handle.

Based on that, I personally must say that the color/function scheme for LEDs does not seem ready yet at all, and we will be better off by (or even require to) keep the labels for at least one release/kernel version.
An alternative might be to drop the label and just use the node name instead (without using color/function), but I have not made up my mind on that yet.

[adschm]

Another option would be to just use WLAN2G/WLAN5G/RSSI functions anyway, in contrast to what kernel recommends. This would allow us to migrate the labels earlier (and e.g. get rid of the "useless" model prefix we currently use for the labels). We would still have to deal with the name assembly, though.

[chunkeey]

Sorry, I'm mostly away during the week and only have so much time on the weekends.

As you've said it's not really ready... and I don't think it will for the next -stable. But it's
something to keep in mind when up-streaming boards. In my case I came across this
label deprecation with the MR32. But I've no idea if this is stays this way or is just a
phase/iteration.

[mans0n]

I've investigated it when 5.4 first came into OpenWrt, and I also concluded that we are not ready for it (yet).

It seems (if not all) most led names are composed in led_compose_name(). I'm not sure if it's feasible to implement the algorithm correctly in leds.sh.

If that is not possible, then we should devise a way to ask kernel to translate dts led node to actual led name and vice versa. Perhaps we can leverage the result of ls -l /sys/class/leds/*/device/of_node, but I don't know if it provides enough information.

And I think using non-standard led functions is inevitable anyway. There's a lot of leds out there which can't be confined to a small set of LED_FUNCTION_* macros (as shown in git grep "label = ".)

[adschm]

It seems (if not all) most led names are composed in led_compose_name(). I'm not sure if it's feasible to implement the algorithm correctly in leds.sh.

This has two levels for me:

1. The color/function part of the name should be relatively easy to rewrite in leds.sh, I just think that conceptually it's not the way to go to implement the same code again (What happens if kernel changes the code? And generally, is that additional complication really necessary?)
2. As far as I understood it in the discussion, the devicename part will be the bigger problems, as it is supposed to be delivered by the different drivers. So, we cannot assume any shared logic on the name there. This latter point is the main reason why I gave up on trying to assemble the name ourselves and want to read the final string somewhere.

If that is not possible, then we should devise a way to ask kernel to translate dts led node to actual led name and vice versa. Perhaps we can leverage the result of ls -l /sys/class/leds/*/device/of_node, but I don't know if it provides enough information.

I had that very same discussion with the people on the list, the also pointed me towards of_node. However, the problem is that we would need a single led linked to a single entry in of_node. Practically, we only have two lists though, where the individual elements are not linked to each other. The only link I found is that uevent file I used for the current patchset, and that one is really ugly.

And I think using non-standard led functions is inevitable anyway. There's a lot of leds out there which can't be confined to a small set of LED_FUNCTION_* macros (as shown in git grep "label = ".)

That's what I'm thinking about right now. The more I discussed with the upstream people, the more I built the opinion that their concept does not work for us (though I feel like I understand why the concept makes sense for them).

From my point, the motivation for us to touch that is like the following:

1. The fact that label is deprecated itself (more idealistic that practical)

2. The uselessness of the modelname prefix: That's actually my personal motivation for dealing with this. I always had the impression that adding the model name to the label was completely useless and only had drawbacks; if we just used "green:wlan2g" instead of "tl-wdr4300-v1:green:wlan2g" we could have so much less code (particularly duplicated stuff in the device DTS files, no $boardname in 01_leds etc.). If we just dropped that, the only downside would be that it does not match kernel guidelines.

3. The label is redundant if you set the node name properly: If you name the node "wlan2g_green", having a label "green:wlan2g" is just useless redundancy (or, even worse, inconsistency, if the names deviate).

Due to these reasons, I'm still wondering whether we can find a way which works for us, though we don't follow upstream, e.g.

a) drop label, don't use color/function and just use the node name: The node name would then became the LED name, so we could use it directly.

b) stick to label property, but just remove the modelname part everywhere: that would keep us the LED "function" names as we handle them now, but we could get rid of the totally useless model name.

The bigger problem is, no matter what we do, we will effectively have to support both label and the new color/function approach in OpenWrt at some point in the future. But as it appears to me, that point in the future might be far away and it still might make sense to discuss an interim solution.

[adschm]

BTW discussion is found here:
https://www.spinics.net/lists/linux-leds/msg16592.html
https://www.spinics.net/lists/linux-leds/msg16594.html

[mans0n]

The uselessness of the modelname prefix

I totally agree with you. I don't see much benefit from using modelname prefix there.
It might be useful for some stackable dev boards, but well... there will be other ways to deal with it.

I like both a) and b) options, but a node name is the last resort the composer will take, so we will have to remove label/function/color properties if present (which might be cumbersome for upstream dts.)

And... the third option might be c) do not use led aliases and revert back to userspace. ;)

[adschm]

And... the third option might be c) do not use led aliases and revert back to userspace. ;)

Indeed, actually, for the color/function approach this would be by far the easiest approach.

The aliases approach simply does not work with the upstream color/function scheme, and I also wondered whether it's really worth so much ugly and error-prone code when we could simple define the relevant LED in user-space instead.
/etc/diag.sh is user-space anyway, so we are just talking about the location of the definition.

Even if we stick to the labels for now as I'm working on currently, we will at some point have to deal with color/function definitions on devices we take from upstream (unless we just want to patch in a label there ...).