Re: [PATCH v2 16/17] leds: leds-nuc: add support for changing the ethernet type indicator

[Marek Behún]

On Thu, 20 May 2021 18:00:28 +0200
Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:

> Em Thu, 20 May 2021 13:00:14 +0200
> Marek Behún <marek.behun@xxxxxx> escreveu:
> 
> > On Wed, 19 May 2021 20:30:14 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:
> >   
> > > Em Wed, 19 May 2021 17:55:03 +0200
> > > Marek Behún <kabel@xxxxxxxxxx> escreveu:
> > >     
> > > > On Wed, 19 May 2021 16:24:13 +0200
> > > > Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:
> > > >       
> > > > > On other words, if no extra care is taken, it could have bad side 
> > > > > effects at the machine's performance and affect system's latency,
> > > > > eventually resulting on things like audio clicks and pops, if some
> > > > > audio is playing while such calls keep happening.        
> > > > 
> > > > In general we want for every LED that is registered into kernel as
> > > > a LED classdev to be possible to control the brightness by
> > > > software. If the hardware supports it, it should be available.       
> > > 
> > > This is supported, but maybe not the same way as on other drivers.
> > > 
> > > There are two separate things: ON/OFF and LED brightness, when turned
> > > ON.
> > > 
> > > On other words, NUC leds allow to set the brightness ranging from 0
> > > to 100, but if the brightness is, let's say 50%, it means that, when
> > > the LED is triggered by the hardware:
> > > 
> > > 	- ON would mean 50%; and 
> > > 	- OFF would mean 0%.    
> > 
> > Not a problem, there are other controller which also work this way,
> > leds-turris-omnia for example.   
> 
> OK.
> 
> > Also LED triggers are supposed to work
> > this way: if a LED supports non-binary brightness (for exmaple 0-100),
> > and the user sets brightness 50, and then a trigger, then the trigger
> > should blink the LED with brightness 50.
> >   
> > > On other words, it actually adjusts the maximum brightness level.
> > > 
> > > Btw, this also applies to software control, as the hardware can still
> > > blink the LED, the available properties for software control indicator
> > > are:
> > > 	- brightness.
> > > 	- blink behavior and frequency;
> > > 	- led color (available only if BIOS says that it is a 
> > > 	  multi-colored led);    
> > 
> > - if the hw supports setting the LED to blink with a specific frequency
> >   (not depending on any other HW like disk or ethernet, just blinking),
> >   you should also implement the .blink_set method (look at
> >   Documentation/leds/leds-class.rst section Hardware accelerated blink
> >   of LEDs)  
> 
> Ok.
> 
> How the blink pattern is specified? NUC supports 3 different
> patterns (breathing, pulsing, strobing).
> 
> > - if BIOS says the LED is multi-colored, you should register it as
> >   multi-colored LED via multicolor framework  
> 
> Ok. I'll check how much work this will lead after we finish the API
> discussions, as all sysfs attributes that won't fit at the triggers
> will need to be implemented twice - one for mono-colored and another one
> for multicolored, as the priv pointer will use different structures.
> 
> > > The exported attributes:
> > > 
> > > 	static struct attribute *netdev_trig_attrs[] = {
> > > 		&dev_attr_device_name.attr,
> > > 		&dev_attr_link.attr,
> > > 		&dev_attr_rx.attr,
> > > 		&dev_attr_tx.attr,
> > > 		&dev_attr_interval.attr,
> > > 		NULL
> > > 	};
> > > 	ATTRIBUTE_GROUPS(netdev_trig);
> > > 
> > > also won't apply, as the NUC API doesn't support setting device_name, 
> > > RX, TX, link or interval.
> > > 
> > > Instead, it allows to set:
> > > - the maximum brightness;
> > > - the color (if the LED is multi-colored);
> > > - the physical port(s) that will be monitored:
> > > 	- LAN1
> > > 	- LAN2
> > > 	- LAN1+LAN2
> > > 
> > > where LAN1 and LAN2 are two physical ports behind the NUC device.
> > > The netdev layer knows those as "eno1" and "enp5s0" (not 
> > > necessarily at the same order).    
> > 
> > The only problem I see with trigger_data in this case is that only one
> > netdevice can be set, while your LED can be configured to blink on
> > activity on two netdevices.
> > 
> > Otherwise all these settings are relevant.
> > What your driver offload callback should do (once HW offloading of LED
> > triggers is merged) is this:
> >   - the offload method is called by the netdev trigger
> >   - the offload method looks at the trigger_data structure. This
> >     contains settings rx, tx, link, interval, device_name/device. If the
> >     device_name is "eno1" or "end5s0" (or better, if the device points
> >     to one of the 2 netdevices that are supported by the HW), and if
> >     the rx, tx, link and interval parameters are configured to values
> >     that can be done by the LED controller, than put the LED into HW
> >     controlled state (to blink with these parameters on network
> >     activity on that netdevice) and return 0
> >   - otherwise the offload method should return -EOPNOTSUPP, and the
> >     netdev trigger will know that the requested settings are not
> >     supported by the HW, and the netdev trigger will blink the LED in
> >     SW  
> 
> See, there's nothing that the driver can possible do with
> rx, tx, link, interval, device_name/device, as the the BIOS allows
> to set to "LAN1", "LAN2" or "LAN1+LAN2". the WMI interface doesn't
> provide *any* information about what LAN1 means.

On the contrary, there is something the driver can do with these
attributes. If the specific combination is not supported, the driver
should return -EOPNOTSUPP in the trigger_offload method and let the
netdev trigger do the work in software. What exactly do the LEDs do
when configured to blink on activity on a network device? Do they just
blink on RX/TX, and otherwise are off? Or are they on when a cable is
plugged, blink on rx/tx and otherwise off?

>
> In the case of my NUC, there are even two different drivers:
> 
> 	00:1f.6 Ethernet controller: Intel Corporation Ethernet Connection (2) I219-LM (rev 31)
> 	Kernel modules: e1000e
> 
> 	05:00.0 Ethernet controller: Intel Corporation I210 Gigabit Network Connection (rev 03)
> 	Kernel modules: igb
> 
> So, even the probing order can be different after a reboot.
>
> So, there isn't a portable way to tell if LAN1 means to either
> "eno1" or "end5s0"(*), as netdev and the associated net drivers
> talk with the hardware directly, and not via BIOS. So, the BIOS
> internal name is not known. Even the DMI tables don't tell it:
> 
> 	Handle 0x000F, DMI type 8, 9 bytes
> 	Port Connector Information
> 	        Internal Reference Designator: CON4501
> 	        Internal Connector Type: None
> 	        External Reference Designator: LAN
> 	        External Connector Type: RJ-45
> 	        Port Type: Network Port
> 	
> 	Handle 0x0010, DMI type 8, 9 bytes
> 	Port Connector Information
> 	        Internal Reference Designator: CON4401
> 	        Internal Connector Type: None
> 	        External Reference Designator: LAN
> 	        External Connector Type: RJ-45
> 	        Port Type: Network Port
> 
> (*)  I'm using the interface names on this specific model, but
>      I'm pretty sure other models will have different names
>      and will even use different network drivers.

Have you looked into DSDT and SSDT tables?

> The point is, while the current netdev trigger API can be generic
> enough when the LED is controlled by netdev, it is *not*
> generic enough to cover the case where it is trigged by the
> firmware, as the information exposed by the firmware can be
> (and it is on this case) completely different than what netdev
> exposes.

If even DSDT data is not enough to reliably find out which of the 2
network interfaces belongs to which LED setting, the worst case scenario
here is for your driver to need to have a list containing this
information for specific motherboards, and other people can then extend
the driver to support their motherboards as well.

> 
> > > Also, while netdev trigger seems to use just one device name,
> > > the NUC allows to monitor both interfaces at the same time.    
> > 
> > Yes. This can be solved in the future by extending netdev trigger to
> > support blinking on activity on multiple netdevices. I also thought
> > about this for use with another HW (mv88e6xxx switch).
> >   
> > > See, unfortunately I can't see a common API that would fit
> > > nicely on both cases.    
> > 
> > Well I can.  
> 
> Then the API needs to change, in order to allow to abstract from
> netdev-centric view of Ethernet interfaces. Or, instead, some
> other trigger is needed for firmware-controlled events.

No. If the necessary information for determining which network
interface pairs to LED1 and which to LED2 cannot be reliably determined
from ACPI tables, then IMO the driver should specify this information
for each motherboard that wants to use this feature.

> -
> 
> One thing that it is not clear to me: let's say that the LED
> called "front1" is currently handling Ethernet events, but
> the user wants to use, instead, the "front2" LED, disabling
> the "front1" one (or using for another event, like wifi, which
> is not monitored on BIOS default).
> 
> How this can be done using the trigger's API?

cd /sys/class/leds/front1
echo none >trigger
cd /sys/class/leds/front2
echo netdev >trigger
echo ifname >device_name
echo 1 >rx
echo 1 >tx