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

[Mauro Carvalho Chehab]

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.

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.

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.


> > 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.

-

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?

> The only problem here is that it is not supported yet. I will try to
> send patches ASAP and poke Pavel to review them.

Ok. Please c/c on such patches.

> > > Currently the netdev trigger does the blinking in software only
> > > (code in "ledtrig-netdev.c" file). There is a WIP to add the
> > > necessary support for the netdev trigger to have the ability to
> > > offload blinking to HW. I will try to respin this WIP and send
> > > patches for review.
> > > 
> > > Once netdev trigger supports this feature, you can implement your
> > > driver in this way. You can even make your driver depend on netdev
> > > trigger     
> >   
> > > and set the specific LED into netdev triggering by default, and
> > > even forbidding anything else.     
> > 
> > This is also probably one of the differences from other hardware:
> > In principle, *any* led can monitor *any* hardware event[1].
> > 
> > [1] There are some bitmaps at the interface that would allow the
> >     BIOS to restrict it, but, at least on the device I have
> >     (Hades Canyon), there's no such restriction: the same bitmap
> >     masks are returned for all LEDs.
> >   
> > > But this is the corrent way to do this,
> > > instead of creating new sysfs API that is non-extendable.
> > > 
> > > I am sorry that I did not explain this thoroughly in previous mails.
> > > Hopefully this explanation is better.    
> > 
> > Yes, it is a lot better. Thanks for the explanation!
> > 
> > Still, as I pointed above, I'm so far unable to see much in common 
> > with the way the existing LED drivers work and the way NUC LEDS are
> > controlled.
> > 
> > So, as much I would love to just reuse something that already exists,
> > perhaps it would make more sense to create a separate class for such
> > kind of usage.  
> 
> The problem is that this API is not yet available in upstream kernel. I
> will try to push it and return to you.

Ok.

> 
> In the meantime let's see what others think about your code.

Ok, thank you!

Thanks,
Mauro