On 10/16/22 16:58, Sander Vanheule wrote:
Hi Alex,
Hi
[snip]
+
+ if (current_trigger != rtl_trigger && !bitmap_empty(group->ports,
group->size)) {
+ dev_warn(ctrl->dev, "cannot map (%d,%d) to group %d: 0x%02x
!= 0x%02x\n",
+ led->port, led->index, group->index,
current_trigger, rtl_trigger);
+ return ERR_PTR(-ENOSPC);
+ }
It seems that the rtl_hw_trigger cannot be changed once any of the LEDs
are switched to "realtek-switchport".
# echo realtek-switchport | tee /sys/class/leds/green:lan*_1000/trigger
# echo 13 | tee /sys/class/leds/green:lan*_1000/rtl_hw_trigger
tee: /sys/class/leds/green:lan1_1000/rtl_hw_trigger: I/O error
[ 852.470000] realtek-switch-port-led realtek-switchcore-port-leds.0:
cannot map (15,1) to group 0: 0x1f != 0x0a
[ 852.480000] realtek-switch-port-led realtek-switchcore-port-leds.0:
cannot map (14,1) to group 0: 0x1f != 0x0a
...
However, doing things in opposite order works (after reboot):
-------------------------------------------------------------
# echo 13 | tee /sys/class/leds/green:lan*_1000/rtl_hw_trigger
# echo realtek-switchport | tee /sys/class/leds/green:lan*_1000/trigger
# grep . /sys/kernel/debug/rtl838x/led/led*
/sys/kernel/debug/rtl838x/led/led1_sw_p_en_ctrl:0x0fff00ff
/sys/kernel/debug/rtl838x/led/led_mode_ctrl:0x00838147
This behaviour is intentional, but I am open to suggestions on improving it, as
I agree it isn't very nice.
When using the "netdev" trigger, the sysfs configuration files are only exposed
when the trigger is selected. This makes sense if each LED can be configured
independently of the others.
The hardware trigger on RTL838x requires an LED to be part of a group (or "set"
in the SDK). As such, an LED itself cannot be configured to trigger on a certain
hardware state, but the group it belongs to must be configured. That is why I
opted to let the user set a HW trigger condition before activating the HW
trigger, since activation means assigning the LED to one of the limited number
of groups.
One alternative would be to only expose the rtl_hw_trigger file once the
"realtek-switchport" trigger is selected, but that would mean I have to either
* ignore configurations that cannot currently be mapped to any active group,
* change the behaviour of other LEDs belonging to an already existing group,
if
the HW trigger is overwritten.
The latter may work on RTL838x, where there is only one HW trigger setting for
any given LED. On RTL839x this is not possible though, because there is no way
to decide which of the 4 available groups should be used if there aren't any
available.
This is modeled in sysfs such that each LED appears to have its own
"rtl_hw_trigger". Instead, why not model one "rtl_hw_trigger" per LED
group? For example, symlink "rtl_hw_trigger" to a sysfs entry in the parent.
You can make the symlink appear or disappear when "realtek-switchport"
trigger is selected, or just leave it always on. When you change the
setting for one LED, it changes it for all the LEDs in the group. I
think it's more intuitive because it aligns more to how the LED
controller actually works.
Thus, "realtek-switchport" controls led{0,1,2}_sw_p_en_ctrl, and
"rtl_hw_trigger" only controls led_mode_ctrl. No need to intermingle
them with complicated logic.
But then there's yet another confusing point:
---------------------------------------------
# echo 13 > /sys/class/leds/green:lan1_1000/rtl_hw_trigger
# cat /sys/class/leds/green:lan1_1000/rtl_hw_trigger
13
# echo realtek-switchport | tee /sys/class/leds/green:lan*_1000/trigger
# grep . /sys/class/leds/green:lan*_1000/trigger
/sys/class/leds/green:lan1_1000/trigger:none timer heartbeat default-on
netdev [realtek-switchport]
/sys/class/leds/green:lan2_1000/trigger:[none] timer heartbeat
default-on netdev realtek-switchport
...
The other ports don't get switched to realtek-switchport, and there is
no noticeable indication of an error.
Did you run this after rebooting? I'm not able to reproduce this on my
(mainline) image. When only one port has trigger flags "13", this results in log
messages in my quick test.
Yes, that's right after reboot (rtl838x-based TL-SG2008P). I see the
dmesg messages now. Still, I would expect the 'tee' or 'echo' command to
show an error. It's not obvious that the answer lies in dmesg.
+static ssize_t rtl_hw_trigger_show(struct device *dev, struct
device_attribute *attr, char *buf)
+{
+ struct led_classdev *cdev = dev_get_drvdata(dev);
+ struct switch_port_led *pled = to_switch_port_led(cdev);
+
+ return sprintf(buf, "%x\n", pled->trigger_flags);
+}
# grep . /sys/class/leds/green:lan*_1000/*rigger
rtl_hw_trigger:13
trigger:none timer heartbeat default-on netdev [realtek-switchport]
The "trigger flags" number is meaningless for sysfs. I suggest this
should show a text field with a selected choice, similar to how
"trigger" behaves:
rtl_hw_trigger:none link_10-100 [act_10-100] link_1g act_1g ...
On RTL838x/RTL839x (i.e. the current patches), ca. 16 modes are documented.
These can be represented as an enum [1], which would be text-reprentable.
On RTL930x/RTL931x however, there are 13 bits that can be set independently [2].
Not all these bits are orthogonal though, but this would still result in >8000
combinations. I don't think anybody can process that kind of enumeration.
Maybe the flags could just be represented as text, but I'm not sure how common
that is for a sysfs file (instead of the typical enumeration of mutually
exclusive options).
I definitely wouldn't want 8000 options to represent a 13 bit bitfield.
Looking through sysfs, bitmasks are represented as integers. I agree the
enum idea only makes sense for rtl838x.
Alex
[1] https://svanheule.net/realtek/maple/register/led_mode_ctrl
[2] https://svanheule.net/realtek/longan/register/led_set0_0_ctrl
Best,
Sander
_______________________________________________
openwrt-devel mailing list
[email protected]
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
