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

[Mauro Carvalho Chehab]

Em Thu, 20 May 2021 22:07:03 +0200
Marek Behún <kabel@xxxxxxxxxx> escreveu:

> On Thu, 20 May 2021 20:59:33 +0200
> Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> wrote:
> 
> > > 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.     
> > 
> > Letting netdev to trigger is something we don't want to allow, as this
> > can cause side effects, making it doing slow the system due to BIOS calls
> > for no good reason.
> >  
> > > 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?    
> > 
> > They are on when a cable is plugged, blink on rx/tx and otherwise off.
> > 
> > Worth mentioning that, besides the LEDs controlled by this driver, each
> > RJ-45 port also a couple leds that behave just like normal RJ-45 ones: 
> > a yellow led for Ethernet PHY detection and a green one for traffic.  
> 
> So what the LED does when configured for ethernet is almost equivalent
> to netdev setting [link=1, rx=1, activity=1]. Almost because we still have
> the correct device setting and interval setting.
> 
> Theoretically what you can do is deny the netdev trigger for every
> other netdev setting (since, according to you, it would use too much
> CPU time in BIOS via software control). This could be done by the
> offload method returning another error value, or maybe just returning 0
> and printing info about this into kernel log. I wonder what others
> think about this possible resolution.

IMO, it would be preferred to have a different trigger here, as this
is not a netdev-based trigger. So, its implementation should not call:

	register_netdevice_notifier()

nor set timers, etc.

See, the hardware won't use any information provided by the netdev,
trigger and the API is not the same, as the hardware trigger only
wants to know if just one interface will trigger the led or both.

> > > 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.    
> > 
> > Needing something like that sucks and it is hard to maintain,
> > and depends on people reporting issues.  
> 
> I don't see much difference between this and various drivers having
> different OF compatible strings for different chips all supported by
> one driver.

It is somewhat similar: on my experiences, the upstream OF compatibles
are almost always outdated uptream: only OOT Kernels have the full
OF stuff :-p

The major difference is that hardware vendors usually develop and
provide OF.

In this case, you want users to fill bug reports that will ask a
Kernel developer to add new entries for their machines to work
properly. While we do this on other drivers, doing that is time
consuming and may lead into errors. Believe me: this is needed
on media drivers, as there are things like GPIOs that are
device-specific. It is a pain to maintain those things.

> > > > 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.    
> > 
> > What's the gain of adding such extra complexity to the driver?  
> 
> Having a consistent API on different devices is a benefit in itself, I
> would think.

It wouldn't be consistent. Hardware sees the events on different
ways than netdev, and associating netdev's view of the interface with
the hardware's view will always be an issue, on any driver that would
trigger such kind of events.

See, let's assume someone would implement a hardware trigger for
the LEDs on an 48-ports Ethernet switch, and different versions of
such hub would use different Ethernet drivers.

No matter how netdev sees the hardware, or if some of the ports
can be replaced (some devices allow port hot-plugging), from userspace
perspective, what it really matter is the port number as seen at
the switch panel, no matter how netdev sees it.

So, for hardware-triggered events, the hardware "label" is a lot
more relevant than any linux-internal representation.

> > All the user wants is to blink a led only for one of the LAN ports.
> > 
> > Denying it and using a more complex API doesn't make much sense, IMO.  
> 
> As I see it you are the one wanting to introduce more complexity into
> the sysfs ABI. There is already a solution together with documentation
> and everything for when the user wants to "blink a led only for one of
> the LAN ports". It is the netdev trigger. And you want to complicate
> that ABI.

No. The existing in-kernel API is to blink a led based on software
events originated from netdev from a single network port. 

It could monitor an interface that doesn't physically exist 
(a virtual network interface, like tun0). It could even monitor traffic
on a single VLAN, if the interface is specified like eth0.100[1].

[1] As we're discussing API here, I didn't test/check if the current
    implementation allows monitoring virtual and VLAN interfaces.
    From API's perspective, it makes perfect sense to be able to
    monitor any physical or logical interface supported by netdev.

The HW trigger is different: led blinks if the hardware detects Ethernet
activity on one or more physical interfaces.

See, the netdev trigger monitors a netdev event with may or may not
be due to an Ethernet port.

An Ethernet HW trigger monitors activity on a set of physical
Ethernet ports.

In essence, the only thing in common is that both triggers are related to
network, but they're actually monitoring two different types of events.

Merging them into a single one sounds a conceptual mistake on my eyes.

> You say that for this controller it would be bad to do in SW, because it
> would take too much time in BIOS calls. (I wonder how much...) 

Yeah, it would be interesting to know how much. However, measuring BIOS
latency and time spent on such calls can be tricky: one needs to use a
high-res clock that it is not used anywhere else, in order to measure
it. 

Thanks,
Mauro