On Mon, 8 Nov 2021 18:58:38 +0100 Ansuel Smith <ansuelsmth@xxxxxxxxx> wrote: > Are you aware of any device that can have some trigger offloaded and > still have the led triggered manually? I don't understand why we would need such a thing. Look, just to make it clear via an example: I have a device with a Marvell PHY chip inside. There is a LED connected to one of the PHY LED pins. Marvell PHY has LED[0] control register, which supports the following modes: LED is OFF LED is ON LED is ON when Link is up LED blinks on RX activity LED blinks on TX activity LED blinks on RX/TX activity LED is ON and blinks on RX/TX activity ... I have code that exports this LED as a LED classdev When I activate netdev trigger on this LED, the netdev trigger currently just blinks the LED in software, by calling the .brightness_set() method, which configures LED[0] control register to one of the first two modes above (LED is OFF, LED is ON). But I have also another patch that adds support to offloading netdev trigger upon offloadable settings. The netdev trigger code calls the .trigger_offload() method, which is implemented in PHY driver. This method checks whether it is a netdev trigger that is to be offloaded, and whether device_name is the name of the device attached to the PHY, and then chooses one of the modes above, according to netdev trigger settings. So when I request netdev trigger for eth0, to indicate link and blink on activity, the netdev trigger doesn't do anything in software. It just calls the offload method ONCE (at the moment I am changing netdev trigger settings). The blinking is then done by the PHY chip. Netdev trigger doesn't do anything, at least not until I change the settings again. > Talking about mixed mode, so HW and SW. What exactly do you mean by mixed mode? There is no mixed mode. > Asking to understand as currently the only way to impement all > of this in netdev trigger is that: > IF any hw offload trigger is supported (and enabled) then the entire > netdev trigger can't work as it won't be able to simulate missing > trigger in SW. And that would leave some flexibility. What do you mean by missing trigger here? I think we need to clarify what we mean by the word "trigger". Are you talking about the various blinking modes that the PHY supports? If so, please let's call them HW control modes, and not triggers. By "triggers" I understand triggers that can be enabled on a LED via /sys/class/leds/<LED>/trigger. > We need to understand how to operate in this condition. Should netdev > detect that and ""hide"" the sysfs triggers? Should we report error? So if I understand you correctly, you are asking about what should we do if user asked for netdev trigger settings (currently only link, rx, tx, interval) that can't be offloaded to the PHY chip. Well, if the PHY allows to manipulate the LEDs ON/OFF state (in other words "full control by SW", or ability to implement brightness_set() method), then netdev trigger should blink the LED in SW via this mechanism (which is something it would do now). A new sysfs file, "offloaded", can indicate whether the trigger is offloaded to HW or not. If, on the other hand, the LED cannot be controlled by SW, and it only support some HW control modes, then there are multiple ways how to implement what should be done, and we need to discuss this. For example suppose that the PHY LED pin supports indicating LINK, blinking on activity, or both, but it doesn't support blinking on rx only, or tx only. Since the LED is always indicating something about one network device, the netdev trigger should be always activated for this LED and it should be impossible to deactivate it. Also, it should be impossible to change device_name. $ cd /sys/class/leds/<LED> $ cat device_name eth0 $ echo eth1 >device_name Operation not supported. $ echo none >trigger Operation not supported. Now suppose that the driver by default enabled link indication, so we have: $ cat link 1 $ cat rx 0 $ cat tx 0 We want to enable blink on activity, but the LED supports only blinking on both rx/tx activity, rx only or tx only is not supported. Currently the only way to enable this is to do $ echo 1 >rx $ echo 1 >tx but the first call asks for (link=1, rx=1, tx=0), which is impossible. There are multiple things which can be done: - "echo 1 >rx" indicates error, but remembers the setting - "echo 1 >rx" quietly fails, without error indication. Something can be written to dmesg about nonsupported mode - "echo 1 >rx" succeeds, but also sets tx=1 - rx and tx are non-writable, writing always fails. Another sysfs file is created, which lists modes that are actually supported, and allows to select between them. When a mode is selected, link,rx,tx are filled automatically, so that user may read them to know what the LED is actually doing - something different? Marek
