On Mon, Nov 08, 2021 at 07:41:42PM +0100, Marek Behún wrote: > 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. > Ok. > > 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. > offload triggers = blinking modes supported > > 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. > Are all these sysfs entry OK? I mean if we want to add support for he main blinking modes, the number will increase to at least 10 additional entry. > 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? > Expose only the supported blinking modes? (in conjunciong with a generic traffic blinking mode) The initial question was Should we support a mixed mode offloaed blinking modes and blinking modes simulated by sw? I assume no as i don't think a device that supports that exist. > Marek -- Ansuel
