lets settle the LED `function` property regarding the netdev trigger

lets settle the LED `function` property regarding the netdev trigger

[Marek Behún]

Hello Pavel, Jacek, Rob and others,

I'd like to settle DT binding for the LED function property regarding
the netdev LED trigger.

Currently we have, in include/dt-bindings/leds/common.h, the following
functions defined that could be interpreted as request to enable netdev
trigger on given LEDs:
  activity
  lan
  rx tx
  wan
  wlan

The "activity" function was originally meant to imply the CPU
activity trigger, while "rx" and "tx" are AFAIK meant as UART indicators
(tty LED trigger), see
https://lore.kernel.org/linux-leds/20190609190803.14815-27-jacek.anaszewski@gmail.com/

The netdev trigger supports different settings:
- indicate link
- blink on rx, blink on tx, blink on both

The current scheme does not allow for implying these.

I therefore propose that when a LED has a network device handle in the
trigger-sources property, the "rx", "tx" and "activity" functions
should also imply netdev trigger (with the corresponding setting).
A "link" function should be added, also implying netdev trigger.

What about if a LED is meant by the device vendor to indicate both link
(on) and activity (blink)?
The function property is currently a string. This could be changed to
array of strings, and then we can have
  function = "link", "activity";
Since the function property is also used for composing LED classdev
names, I think only the first member should be used for that.

This would allow for ethernet LEDs with names
  ethphy-0:green:link
  ethphy-0:yellow:activity
to be controlled by netdev trigger in a specific setting without the
need to set the trigger in /sys/class/leds.

Opinions?

Marek

Re: lets settle the LED `function` property regarding the netdev trigger

[Andrew Lunn]

On Fri, Oct 01, 2021 at 02:36:01PM +0200, Marek Behún wrote:
> Hello Pavel, Jacek, Rob and others,
> 
> I'd like to settle DT binding for the LED function property regarding
> the netdev LED trigger.
> 
> Currently we have, in include/dt-bindings/leds/common.h, the following
> functions defined that could be interpreted as request to enable netdev
> trigger on given LEDs:
>   activity
>   lan
>   rx tx
>   wan
>   wlan
> 
> The "activity" function was originally meant to imply the CPU
> activity trigger, while "rx" and "tx" are AFAIK meant as UART indicators
> (tty LED trigger), see
> https://lore.kernel.org/linux-leds/20190609190803.14815-27-jacek.anaszewski@gmail.com/
> 
> The netdev trigger supports different settings:
> - indicate link
> - blink on rx, blink on tx, blink on both
> 
> The current scheme does not allow for implying these.
> 
> I therefore propose that when a LED has a network device handle in the
> trigger-sources property, the "rx", "tx" and "activity" functions
> should also imply netdev trigger (with the corresponding setting).
> A "link" function should be added, also implying netdev trigger.
> 
> What about if a LED is meant by the device vendor to indicate both link
> (on) and activity (blink)?
> The function property is currently a string. This could be changed to
> array of strings, and then we can have
>   function = "link", "activity";
> Since the function property is also used for composing LED classdev
> names, I think only the first member should be used for that.
> 
> This would allow for ethernet LEDs with names
>   ethphy-0:green:link
>   ethphy-0:yellow:activity
> to be controlled by netdev trigger in a specific setting without the
> need to set the trigger in /sys/class/leds.

Hi Marek

There is no real standardization here. Which means PHYs differ a lot
in what they can do. We need to strike a balance between over
simplifying and only supporting a very small set of PHY LED features,
and allowing great flexibility and having each PHY implement its own
specific features and having little in common.

I think your current proposal is currently on the too simple side.

One common feature is that there are multiple modes for indicating
link, which take into account the link speed. Look at for example
include/dt-bindings/net/microchip-lan78xx.h

#define LAN78XX_LINK_ACTIVITY           0
#define LAN78XX_LINK_1000_ACTIVITY      1
#define LAN78XX_LINK_100_ACTIVITY       2
#define LAN78XX_LINK_10_ACTIVITY        3
#define LAN78XX_LINK_100_1000_ACTIVITY  4
#define LAN78XX_LINK_10_1000_ACTIVITY   5
#define LAN78XX_LINK_10_100_ACTIVITY    6

And:

intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK10	0x0010
intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK100	0x0020
intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK10X	0x0030
intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK1000	0x0040
intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK10_0	0x0050
intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK100X	0x0060
intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK10XX	0x0070

Marvell PHYs have similar LINK modes which can either be one specific
speed, or a combination of speeds.

This is a common enough feature, and a frequently used feature, we
need to support it. We also need to forward looking. We should not
limit ourselves to 10/100/1G. We have 3 PHY drivers which support
2.5G, 5G and 10G. 25G and 40G are standardized so are likely to come
along at some point.

One way we could support this is:

function = "link100", "link1G", "activity";

for LAN78XX_LINK_100_1000_ACTIVITY, etc.

    Andrew

Re: lets settle the LED `function` property regarding the netdev trigger

[Marek Behún]

On Sun, 3 Oct 2021 20:56:23 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Fri, Oct 01, 2021 at 02:36:01PM +0200, Marek Behún wrote:
> > Hello Pavel, Jacek, Rob and others,
> > 
> > I'd like to settle DT binding for the LED function property regarding
> > the netdev LED trigger.
> > 
> > Currently we have, in include/dt-bindings/leds/common.h, the following
> > functions defined that could be interpreted as request to enable netdev
> > trigger on given LEDs:
> >   activity
> >   lan
> >   rx tx
> >   wan
> >   wlan
> > 
> > The "activity" function was originally meant to imply the CPU
> > activity trigger, while "rx" and "tx" are AFAIK meant as UART indicators
> > (tty LED trigger), see
> > https://lore.kernel.org/linux-leds/20190609190803.14815-27-jacek.anaszewski@gmail.com/
> > 
> > The netdev trigger supports different settings:
> > - indicate link
> > - blink on rx, blink on tx, blink on both
> > 
> > The current scheme does not allow for implying these.
> > 
> > I therefore propose that when a LED has a network device handle in the
> > trigger-sources property, the "rx", "tx" and "activity" functions
> > should also imply netdev trigger (with the corresponding setting).
> > A "link" function should be added, also implying netdev trigger.
> > 
> > What about if a LED is meant by the device vendor to indicate both link
> > (on) and activity (blink)?
> > The function property is currently a string. This could be changed to
> > array of strings, and then we can have
> >   function = "link", "activity";
> > Since the function property is also used for composing LED classdev
> > names, I think only the first member should be used for that.
> > 
> > This would allow for ethernet LEDs with names
> >   ethphy-0:green:link
> >   ethphy-0:yellow:activity
> > to be controlled by netdev trigger in a specific setting without the
> > need to set the trigger in /sys/class/leds.  
> 
> Hi Marek
> 
> There is no real standardization here. Which means PHYs differ a lot
> in what they can do. We need to strike a balance between over
> simplifying and only supporting a very small set of PHY LED features,
> and allowing great flexibility and having each PHY implement its own
> specific features and having little in common.
> 
> I think your current proposal is currently on the too simple side.
> 
> One common feature is that there are multiple modes for indicating
> link, which take into account the link speed. Look at for example
> include/dt-bindings/net/microchip-lan78xx.h
> 
> #define LAN78XX_LINK_ACTIVITY           0
> #define LAN78XX_LINK_1000_ACTIVITY      1
> #define LAN78XX_LINK_100_ACTIVITY       2
> #define LAN78XX_LINK_10_ACTIVITY        3
> #define LAN78XX_LINK_100_1000_ACTIVITY  4
> #define LAN78XX_LINK_10_1000_ACTIVITY   5
> #define LAN78XX_LINK_10_100_ACTIVITY    6
> 
> And:
> 
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK10	0x0010
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK100	0x0020
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK10X	0x0030
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK1000	0x0040
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK10_0	0x0050
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK100X	0x0060
> intel-xway.c:#define  XWAY_MMD_LEDxL_BLINKS_LINK10XX	0x0070
> 
> Marvell PHYs have similar LINK modes which can either be one specific
> speed, or a combination of speeds.
> 
> This is a common enough feature, and a frequently used feature, we
> need to support it. We also need to forward looking. We should not
> limit ourselves to 10/100/1G. We have 3 PHY drivers which support
> 2.5G, 5G and 10G. 25G and 40G are standardized so are likely to come
> along at some point.
> 
> One way we could support this is:
> 
> function = "link100", "link1G", "activity";
> 
> for LAN78XX_LINK_100_1000_ACTIVITY, etc.
> 
>     Andrew

Hello Andrew,

I am aware of this, and in fact am working on a proposal for an
extension of netdev LED extension, to support the different link
modes. (And also to support for multi-color LEDs.)

But I am not entirely sure whether these different link modes should be
also definable via device-tree. Are there devices with ethernet LEDs
dedicated for a specific speed? (i.e. the manufacturer says in the
documentation of the device, or perhaps on the device's case, that this
LED shows 100M/1000M link, and that other LED is shows 10M link?)
If so, that this should be specified in the devicetree, IMO. But are
such devices common?

And what about multi-color LEDs? There are ethernet ports where one LED
is red-green, and so can generate red, green, and yellow color. Should
device tree also define which color indicates which mode?

Marek

Re: lets settle the LED `function` property regarding the netdev trigger

[Andrew Lunn]

> Hello Andrew,
> 
> I am aware of this, and in fact am working on a proposal for an
> extension of netdev LED extension, to support the different link
> modes. (And also to support for multi-color LEDs.)
> 
> But I am not entirely sure whether these different link modes should be
> also definable via device-tree. Are there devices with ethernet LEDs
> dedicated for a specific speed? (i.e. the manufacturer says in the
> documentation of the device, or perhaps on the device's case, that this
> LED shows 100M/1000M link, and that other LED is shows 10M link?)
> If so, that this should be specified in the devicetree, IMO. But are
> such devices common?

I have a dumb 5 port switch next to me. One port is running at 1G. Its
left green LED is on and blinks with traffic. Another port of the
switch is running at 100Mbps and its right orange LED is on, blinks
for traffic. And there is text on the case saying 10/100 orange, 1G
green.

I think this is pretty common. You generally do want to know if 10/100
is being used, it can indicate problems. Same for a 10G port running
at 1G, etc.

> And what about multi-color LEDs? There are ethernet ports where one LED
> is red-green, and so can generate red, green, and yellow color. Should
> device tree also define which color indicates which mode?

There are two different ways this can be implemented. There can be two
independent LEDs within the same package. So you can generate three
colours. Or there can be two cross connected LEDs within the
package. Apply +ve you get one colour, apply -ve you get a different
colour. Since you cannot apply both -ve and +ve at the same time, you
cannot get both colours at once.

If you have two independent LEDs, I would define two LEDs in DT.

Things get tricky for the two dependency LEDs. Does the LED core have
support for such LEDs?

This is where we need to strike a balance between too simple and too
complex. Implement most of the common features, but don't support
exotic stuff, like two dependency LEDs?

       Andrew

Re: lets settle the LED `function` property regarding the netdev trigger

[Marek Behún]

On Mon, 4 Oct 2021 16:50:09 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > Hello Andrew,
> > 
> > I am aware of this, and in fact am working on a proposal for an
> > extension of netdev LED extension, to support the different link
> > modes. (And also to support for multi-color LEDs.)
> > 
> > But I am not entirely sure whether these different link modes should be
> > also definable via device-tree. Are there devices with ethernet LEDs
> > dedicated for a specific speed? (i.e. the manufacturer says in the
> > documentation of the device, or perhaps on the device's case, that this
> > LED shows 100M/1000M link, and that other LED is shows 10M link?)
> > If so, that this should be specified in the devicetree, IMO. But are
> > such devices common?  
> 
> I have a dumb 5 port switch next to me. One port is running at 1G. Its
> left green LED is on and blinks with traffic. Another port of the
> switch is running at 100Mbps and its right orange LED is on, blinks
> for traffic. And there is text on the case saying 10/100 orange, 1G
> green.
> 
> I think this is pretty common. You generally do want to know if 10/100
> is being used, it can indicate problems. Same for a 10G port running
> at 1G, etc.

OK then. I will work no a proposal for device tree bindings for this.

> > And what about multi-color LEDs? There are ethernet ports where one LED
> > is red-green, and so can generate red, green, and yellow color. Should
> > device tree also define which color indicates which mode?  
> 
> There are two different ways this can be implemented. There can be two
> independent LEDs within the same package. So you can generate three
> colours. Or there can be two cross connected LEDs within the
> package. Apply +ve you get one colour, apply -ve you get a different
> colour. Since you cannot apply both -ve and +ve at the same time, you
> cannot get both colours at once.
> 
> If you have two independent LEDs, I would define two LEDs in DT.

No, we have multicolor LED API which is meant for exactly this
situation: a multicolor LED.
(I am talking about something like the KJ2518D-262 from
 http://www.rego.com.tw/product_detail.php?prdt_id=258
 which has Green/Orange on left and Yellow on right side.
 The left Green/Orange LED has 3 pins, and so it can mix the colors into
 yellow.)

> Things get tricky for the two dependency LEDs. Does the LED core have
> support for such LEDs?

Unfortunately not yet. The multicolor API supports LEDs where the
sub-leds are independent.

> This is where we need to strike a balance between too simple and too
> complex. Implement most of the common features, but don't support
> exotic stuff, like two dependency LEDs?

I think the best solution here would be a subclass "enumcolor" (or
different name), where you can choose between several pre-defined colors.
In sysfs you could then do
  echo 1 >brightness
  echo green >color
  echo yellow >color

There already are other people who need to register such LEDs.

Marek

Re: lets settle the LED `function` property regarding the netdev trigger

[Andrew Lunn]

> > There are two different ways this can be implemented. There can be two
> > independent LEDs within the same package. So you can generate three
> > colours. Or there can be two cross connected LEDs within the
> > package. Apply +ve you get one colour, apply -ve you get a different
> > colour. Since you cannot apply both -ve and +ve at the same time, you
> > cannot get both colours at once.
> > 
> > If you have two independent LEDs, I would define two LEDs in DT.
> 
> No, we have multicolor LED API which is meant for exactly this
> situation: a multicolor LED.
> (I am talking about something like the KJ2518D-262 from
>  http://www.rego.com.tw/product_detail.php?prdt_id=258
>  which has Green/Orange on left and Yellow on right side.
>  The left Green/Orange LED has 3 pins, and so it can mix the colors into
>  yellow.)

But here you are talking about the LED, not the controller in the
PHY. The controller might control it as two independent LEDs. It has
no idea it can get a third colour by enabling two LEDs at the same
time. Or maybe the controller does know it can combine colours.

So you need to know about both the controller and the LED. And the
same controller can be used either way. Plus you need to think about
the non DT case, when you have no idea about the LED connected to the
controller.

> I think the best solution here would be a subclass "enumcolor" (or
> different name), where you can choose between several pre-defined colors.
> In sysfs you could then do
>   echo 1 >brightness
>   echo green >color
>   echo yellow >color

I'm not sure it is as simple as that. In the general case, you have no
idea what the colours actually are. You only know the colours if you
have DT and DT lists the colours. And you only know if LEDs are
combined if you have DT. You need a basic sysfs API based on knowing
the PHY can control X LEDs. You can then extend that API if you have
additional information via DT, like colour and if LEDs are combined,
that only LEDs numbered 2 and 3 are used, etc.

	   Andrew

Re: lets settle the LED `function` property regarding the netdev trigger

[Marek Behún]

On Mon, 4 Oct 2021 19:28:19 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > > There are two different ways this can be implemented. There can be two
> > > independent LEDs within the same package. So you can generate three
> > > colours. Or there can be two cross connected LEDs within the
> > > package. Apply +ve you get one colour, apply -ve you get a different
> > > colour. Since you cannot apply both -ve and +ve at the same time, you
> > > cannot get both colours at once.
> > > 
> > > If you have two independent LEDs, I would define two LEDs in DT.  
> > 
> > No, we have multicolor LED API which is meant for exactly this
> > situation: a multicolor LED.
> > (I am talking about something like the KJ2518D-262 from
> >  http://www.rego.com.tw/product_detail.php?prdt_id=258
> >  which has Green/Orange on left and Yellow on right side.
> >  The left Green/Orange LED has 3 pins, and so it can mix the colors into
> >  yellow.)  
> 
> But here you are talking about the LED, not the controller in the
> PHY. The controller might control it as two independent LEDs. It has
> no idea it can get a third colour by enabling two LEDs at the same
> time. Or maybe the controller does know it can combine colours.
> 
> So you need to know about both the controller and the LED. And the
> same controller can be used either way. Plus you need to think about
> the non DT case, when you have no idea about the LED connected to the
> controller.
> 
> > I think the best solution here would be a subclass "enumcolor" (or
> > different name), where you can choose between several pre-defined colors.
> > In sysfs you could then do
> >   echo 1 >brightness
> >   echo green >color
> >   echo yellow >color  
> 
> I'm not sure it is as simple as that. In the general case, you have no
> idea what the colours actually are. You only know the colours if you
> have DT and DT lists the colours. And you only know if LEDs are
> combined if you have DT. You need a basic sysfs API based on knowing
> the PHY can control X LEDs. You can then extend that API if you have
> additional information via DT, like colour and if LEDs are combined,
> that only LEDs numbered 2 and 3 are used, etc.
> 
> 	   Andrew

I really don't think we should be registering any LEDs in the PHY driver
if the driver does not know whether there are LEDs connected to the PHY.

If this information is not available (via device-tree or some other
method, for example USB vendor/device table), then we can't register a
LED and let user control it.

What if the pin is used for something different on a board?

Marek

Re: lets settle the LED `function` property regarding the netdev trigger

[Andrew Lunn]

> I really don't think we should be registering any LEDs in the PHY driver
> if the driver does not know whether there are LEDs connected to the PHY.
> 
> If this information is not available (via device-tree or some other
> method, for example USB vendor/device table), then we can't register a
> LED and let user control it.
> 
> What if the pin is used for something different on a board?

There is some danger here. Some hardware misuse LED outputs for WoL
interrupts. There is even m88e1318_set_wol() which sets up LED2 for
WoL. So i will need to review the PHY drivers to look out for this,
and maybe add some restrictions.

But i think we have little choice but to export all the LEDs a PHY
supports. USB vendor/product, PCI vendor/product does not give us
anything useful. How many OEMs take a lan78xx chip, created a USB
dongle and shipped it using USB enumeration data:
LAN78XX_USB_VENDOR_ID:LAN7800_USB_PRODUCT_ID. How many motherboards
have a r8169 PCIe device using realteks PCI enumeration data? There is
no useful source of information in devices like this. But what we do
know is that the PHY can control X LED output pins, and we know what
patterns it can blink those LED pins. So we export them, and let the
user figure it out. This is the general case.

If we have DT, or ACPI, or some other source, we can then refine this
representation. If we have LED information, but a specific LED is
missing from DT, don't export it. If the colour is available, use that
in the name. If the default mode information is available, configure
it that way, etc.

Now, it could be we don't start with this, we just export those that
do have DT. But i will want to ensure that the API/ABI we define is
generic enough to support this. We need to start somewhere, get some
basic support merged, and then do incremental improvements.

	  Andrew

Re: lets settle the LED `function` property regarding the netdev trigger

[Jacek Anaszewski]

Hi Marek,

On 10/4/21 5:08 PM, Marek Behún wrote:
> On Mon, 4 Oct 2021 16:50:09 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
>>> Hello Andrew,
>>>
>>> I am aware of this, and in fact am working on a proposal for an
>>> extension of netdev LED extension, to support the different link
>>> modes. (And also to support for multi-color LEDs.)
>>>
>>> But I am not entirely sure whether these different link modes should be
>>> also definable via device-tree. Are there devices with ethernet LEDs
>>> dedicated for a specific speed? (i.e. the manufacturer says in the
>>> documentation of the device, or perhaps on the device's case, that this
>>> LED shows 100M/1000M link, and that other LED is shows 10M link?)
>>> If so, that this should be specified in the devicetree, IMO. But are
>>> such devices common?
>>
>> I have a dumb 5 port switch next to me. One port is running at 1G. Its
>> left green LED is on and blinks with traffic. Another port of the
>> switch is running at 100Mbps and its right orange LED is on, blinks
>> for traffic. And there is text on the case saying 10/100 orange, 1G
>> green.
>>
>> I think this is pretty common. You generally do want to know if 10/100
>> is being used, it can indicate problems. Same for a 10G port running
>> at 1G, etc.
> 
> OK then. I will work no a proposal for device tree bindings for this.
> 
>>> And what about multi-color LEDs? There are ethernet ports where one LED
>>> is red-green, and so can generate red, green, and yellow color. Should
>>> device tree also define which color indicates which mode?
>>
>> There are two different ways this can be implemented. There can be two
>> independent LEDs within the same package. So you can generate three
>> colours. Or there can be two cross connected LEDs within the
>> package. Apply +ve you get one colour, apply -ve you get a different
>> colour. Since you cannot apply both -ve and +ve at the same time, you
>> cannot get both colours at once.
>>
>> If you have two independent LEDs, I would define two LEDs in DT.
> 
> No, we have multicolor LED API which is meant for exactly this
> situation: a multicolor LED.

Multicolor LED framework is especially useful for the arrangements
where we want to have a possibility of controlling mixed LED color
in a wide range.
In the discussed case it seems that having two separate LED class
devices will be sufficient. Unless the LEDs have 255 or so possible
brightness levels each and they can produce meaningful mixed color
per some device state.

> (I am talking about something like the KJ2518D-262 from
>   http://www.rego.com.tw/product_detail.php?prdt_id=258
>   which has Green/Orange on left and Yellow on right side.
>   The left Green/Orange LED has 3 pins, and so it can mix the colors into
>   yellow.)
> 
>> Things get tricky for the two dependency LEDs. Does the LED core have
>> support for such LEDs?
> 
> Unfortunately not yet. The multicolor API supports LEDs where the
> sub-leds are independent.

What do you mean by dependency here? You can write LED multicolor class
driver that will aggregate whatever LEDs you want, provided that it will
know how to control them. However, the target use case was RGB LED
controllers.

>> This is where we need to strike a balance between too simple and too
>> complex. Implement most of the common features, but don't support
>> exotic stuff, like two dependency LEDs?
> 
> I think the best solution here would be a subclass "enumcolor" (or
> different name), where you can choose between several pre-defined colors.
> In sysfs you could then do
>    echo 1 >brightness
>    echo green >color
>    echo yellow >color
> 
> There already are other people who need to register such LEDs.
> 
> Marek
> 

-- 
Best regards,
Jacek Anaszewski

Re: lets settle the LED `function` property regarding the netdev trigger

[Andrew Lunn]

> > > There are two different ways this can be implemented. There can be two
> > > independent LEDs within the same package. So you can generate three
> > > colours. Or there can be two cross connected LEDs within the
> > > package. Apply +ve you get one colour, apply -ve you get a different
> > > colour. Since you cannot apply both -ve and +ve at the same time, you
> > > cannot get both colours at once.
> > > 
> > > If you have two independent LEDs, I would define two LEDs in DT.
> > 
> > No, we have multicolor LED API which is meant for exactly this
> > situation: a multicolor LED.

> What do you mean by dependency here?

https://www.youtube.com/watch?v=5M9p25OfKdg

There are two different ways you can two LEDs in one package.

Some Ethernet PHY RJ45 connector housings have bi-colour LEDs. Some
have tri-colour LEDs, and some have mono-colour LEDs.

      Andrew

Re: lets settle the LED `function` property regarding the netdev trigger

[Marek Behún]

Hello Jacek,

On Tue, 5 Oct 2021 21:58:13 +0200
Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:

> Hi Marek,
> 
> On 10/4/21 5:08 PM, Marek Behún wrote:
> > On Mon, 4 Oct 2021 16:50:09 +0200
> > Andrew Lunn <andrew@lunn.ch> wrote:
> >   
> >>> Hello Andrew,
> >>>
> >>> I am aware of this, and in fact am working on a proposal for an
> >>> extension of netdev LED extension, to support the different link
> >>> modes. (And also to support for multi-color LEDs.)
> >>>
> >>> But I am not entirely sure whether these different link modes should be
> >>> also definable via device-tree. Are there devices with ethernet LEDs
> >>> dedicated for a specific speed? (i.e. the manufacturer says in the
> >>> documentation of the device, or perhaps on the device's case, that this
> >>> LED shows 100M/1000M link, and that other LED is shows 10M link?)
> >>> If so, that this should be specified in the devicetree, IMO. But are
> >>> such devices common?  
> >>
> >> I have a dumb 5 port switch next to me. One port is running at 1G. Its
> >> left green LED is on and blinks with traffic. Another port of the
> >> switch is running at 100Mbps and its right orange LED is on, blinks
> >> for traffic. And there is text on the case saying 10/100 orange, 1G
> >> green.
> >>
> >> I think this is pretty common. You generally do want to know if 10/100
> >> is being used, it can indicate problems. Same for a 10G port running
> >> at 1G, etc.  
> > 
> > OK then. I will work no a proposal for device tree bindings for this.
> >   
> >>> And what about multi-color LEDs? There are ethernet ports where one LED
> >>> is red-green, and so can generate red, green, and yellow color. Should
> >>> device tree also define which color indicates which mode?  
> >>
> >> There are two different ways this can be implemented. There can be two
> >> independent LEDs within the same package. So you can generate three
> >> colours. Or there can be two cross connected LEDs within the
> >> package. Apply +ve you get one colour, apply -ve you get a different
> >> colour. Since you cannot apply both -ve and +ve at the same time, you
> >> cannot get both colours at once.
> >>
> >> If you have two independent LEDs, I would define two LEDs in DT.  
> > 
> > No, we have multicolor LED API which is meant for exactly this
> > situation: a multicolor LED.  
> 
> Multicolor LED framework is especially useful for the arrangements
> where we want to have a possibility of controlling mixed LED color
> in a wide range.
> In the discussed case it seems that having two separate LED class
> devices will be sufficient. Unless the LEDs have 255 or so possible
> brightness levels each and they can produce meaningful mixed color
> per some device state.

In the discussed case (ethernet PHY LEDs) - it is sometimes possible to
have multiple brightness levels per color channel. For example some
Marvell PHYs allow to set 8 levels of brightness for Dual Mode LEDs.
Dual Mode is what Marvell calls when the PHY allows to pair two
LED pins to control one dual-color LED (green-red, for example) into
one.

Moreover for this Dual Mode case they also allow for HW control of
this dual LED, which, when enabled, does something like this, in HW:
  1g link	green
  100m link	yellow
  10m link	red
  no link	off

Note that actual colors depend on the LEDs themselves. The PHY
documentation does not talk about the color, only about which pin is
on/off. The thing is that if we want to somehow set this mode for the
LED, it should be represented as one LED class device.

I want to extend the netdev trigger to support such configuration,
so that when you have multicolor LED, you will be able to say which
color should be set for which link mode.

> > (I am talking about something like the KJ2518D-262 from
> >   http://www.rego.com.tw/product_detail.php?prdt_id=258
> >   which has Green/Orange on left and Yellow on right side.
> >   The left Green/Orange LED has 3 pins, and so it can mix the colors into
> >   yellow.)
> >   
> >> Things get tricky for the two dependency LEDs. Does the LED core have
> >> support for such LEDs?  
> > 
> > Unfortunately not yet. The multicolor API supports LEDs where the
> > sub-leds are independent.  
> 
> What do you mean by dependency here? You can write LED multicolor class
> driver that will aggregate whatever LEDs you want, provided that it will
> know how to control them. However, the target use case was RGB LED
> controllers.

Andrew explain in another reply, basically LEDs where you can choose
between, for example: OFF, green, yellow (but not both green and
yellow, because the wiring does not allow this).

The current MC framework does not work with this - unless we make it
return -EOPNOTSUPP when user does
  echo 1 1 >multi_intensity
(so that only "0 0", "0 1" and "1 0" are allowed).

Also registering this green-yellow LED as two LED classdevs is
insufficient, since setting brightness to 1 on both won't work. Only
one can be enabled.

I think the better solution here would be to have another subclass,
where you can set brightness, and color from a list of available colors.

Marek

Re: lets settle the LED `function` property regarding the netdev trigger

[Andrew Lunn]

> In the discussed case (ethernet PHY LEDs) - it is sometimes possible to
> have multiple brightness levels per color channel. For example some
> Marvell PHYs allow to set 8 levels of brightness for Dual Mode LEDs.
> Dual Mode is what Marvell calls when the PHY allows to pair two
> LED pins to control one dual-color LED (green-red, for example) into
> one.
> 
> Moreover for this Dual Mode case they also allow for HW control of
> this dual LED, which, when enabled, does something like this, in HW:
>   1g link	green
>   100m link	yellow
>   10m link	red
>   no link	off
> 
> Note that actual colors depend on the LEDs themselves. The PHY
> documentation does not talk about the color, only about which pin is
> on/off. The thing is that if we want to somehow set this mode for the
> LED, it should be represented as one LED class device.
> 
> I want to extend the netdev trigger to support such configuration,
> so that when you have multicolor LED, you will be able to say which
> color should be set for which link mode.

This is getting into the exotic level i don't think we need to
support. How many PHYs have you seen that support something like this?

I suggest we start with simple independent LEDs. That gives enough to
support the majority of use cases people actually need. And is enough
to unblock people who i keep NACKing patches and tell them to wait for
this work to get merged.

     Andrew

Re: lets settle the LED `function` property regarding the netdev trigger

[Marek Behún]

On Tue, 5 Oct 2021 23:01:18 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > In the discussed case (ethernet PHY LEDs) - it is sometimes possible to
> > have multiple brightness levels per color channel. For example some
> > Marvell PHYs allow to set 8 levels of brightness for Dual Mode LEDs.
> > Dual Mode is what Marvell calls when the PHY allows to pair two
> > LED pins to control one dual-color LED (green-red, for example) into
> > one.
> > 
> > Moreover for this Dual Mode case they also allow for HW control of
> > this dual LED, which, when enabled, does something like this, in HW:
> >   1g link	green
> >   100m link	yellow
> >   10m link	red
> >   no link	off
> > 
> > Note that actual colors depend on the LEDs themselves. The PHY
> > documentation does not talk about the color, only about which pin is
> > on/off. The thing is that if we want to somehow set this mode for the
> > LED, it should be represented as one LED class device.
> > 
> > I want to extend the netdev trigger to support such configuration,
> > so that when you have multicolor LED, you will be able to say which
> > color should be set for which link mode.  
> 
> This is getting into the exotic level i don't think we need to
> support. How many PHYs have you seen that support something like this?

This isn't about whether there are PHYs which support this in HW.
The extension to netdev trigger will be able to do this in SW.

For example the Turris Omnia has 12 RGB LEDs on the front panel, of
which 6 are dedicated to ethernet ports (and there are no LEDs on
ethernet ports themselves). It would make sense to be able to have
netdev trigger (or it's extension) show link mode by color (for example
green on 1g, yellow on 100g, orange on 10g).

Anyway when you have a green-yellow LED on an ethernet port wired in
such a way than it can only be off, green or yellow, but not both green
and yellow, I don't think we should register these as 2 LED class
devices.

> I suggest we start with simple independent LEDs. That gives enough to
> support the majority of use cases people actually need. And is enough
> to unblock people who i keep NACKing patches and tell them to wait for
> this work to get merged.

Of course, and I plan to do so. Those netdev trigger extensions and
multi-color function definitions are for later :)

We got side tracked in this discussion, sorry about that.

In this thread I just wanted to settle the LED function property for
LEDs indicating network ports.

So would you, Andrew, agree with:
- extending function property to be array of strings instead of only
  one string, so that we can do
    function = "link", "activity";
- having separate functions for different link modes
    function = "link1000", "link100";
  or should this insted be in another property
    function = "link";
    link-modes = <1000 100>;
  ?

Marek

Re: lets settle the LED `function` property regarding the netdev trigger

[Andrew Lunn]

> > I suggest we start with simple independent LEDs. That gives enough to
> > support the majority of use cases people actually need. And is enough
> > to unblock people who i keep NACKing patches and tell them to wait for
> > this work to get merged.
> 
> Of course, and I plan to do so. Those netdev trigger extensions and
> multi-color function definitions are for later :)

Great.
 
> We got side tracked in this discussion, sorry about that.
> 
> In this thread I just wanted to settle the LED function property for
> LEDs indicating network ports.
> 
> So would you, Andrew, agree with:
> - extending function property to be array of strings instead of only
>   one string, so that we can do
>     function = "link", "activity";

I agree with having a list, and we use the combination. If the
combination is not possible by the hardware, then -EINVAL, or
-EOPNOTSUPP.

> - having separate functions for different link modes
>     function = "link1000", "link100";

I would suggest this, so you can use 

function = "link1000", "link100", "activity"

What could be interesting is how you do this in sysfs?  How do you
enumerate what the hardware can do? How do you select what you want?

Do you need to do

echo "link1000 link100 activity" > /sys/class/net/eth0/phy/led/function

And we can have something like

cat /sys/class/net/eth0/phy/led/function
activity
link10 activity
link100 activity
link1000 activity
[link100 link1000 activity]
link10
link100
link1000

each line being a combination the hardware supports, and the line in
[] is the currently select function.

   Andrew

Re: lets settle the LED `function` property regarding the netdev trigger

[Marek Behún]

On Wed, 6 Oct 2021 00:06:58 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > > I suggest we start with simple independent LEDs. That gives enough to
> > > support the majority of use cases people actually need. And is enough
> > > to unblock people who i keep NACKing patches and tell them to wait for
> > > this work to get merged.  
> > 
> > Of course, and I plan to do so. Those netdev trigger extensions and
> > multi-color function definitions are for later :)  
> 
> Great.
>  
> > We got side tracked in this discussion, sorry about that.
> > 
> > In this thread I just wanted to settle the LED function property for
> > LEDs indicating network ports.
> > 
> > So would you, Andrew, agree with:
> > - extending function property to be array of strings instead of only
> >   one string, so that we can do
> >     function = "link", "activity";  
> 
> I agree with having a list, and we use the combination. If the
> combination is not possible by the hardware, then -EINVAL, or
> -EOPNOTSUPP.
> 
> > - having separate functions for different link modes
> >     function = "link1000", "link100";  
> 
> I would suggest this, so you can use 
> 
> function = "link1000", "link100", "activity"

The problem here is that LED core uses function to compose LED name:
  devicename:color:function
Should we use the first function? Then this LED will be named:
  ethphy42:green:link1000
but it also indicates link100...

> What could be interesting is how you do this in sysfs?  How do you
> enumerate what the hardware can do? How do you select what you want?

This is again sidetrack from the original discussion, which was only
meant to discuss DT, but okay :)

> Do you need to do
> 
> echo "link1000 link100 activity" > /sys/class/net/eth0/phy/led/function
> 
> And we can have something like
> 
> cat /sys/class/net/eth0/phy/led/function
> activity
> link10 activity
> link100 activity
> link1000 activity
> [link100 link1000 activity]
> link10
> link100
> link1000

No, my current ideas about the netdev trigger extension are as follows
(not yet complete):

$ cd /sys/.../<LED>
$ echo netdev >trigger	# To enable netdev trigger
$ echo eth0 >device_name
$ echo 1 >ext		# To enable extended netdev trigger.
			# This will create directory modes if there is
			# a PHY attached to the interface  
$ ls modes/		
1000baseT_Full 100BaseT_Full 100BaseT_Half 10BaseT_Full 10BaseT_Half

$ cd modes/1000baseT_Full
$ ls
brightness link rx tx interval

So basically if you enable the extended netdev trigger, you will get
all the standard netdev settings for each PHY mode. (With a little
change to support blinking on link.)

With this you can set the LED:
  ON when linked and speed=1000m or 100m, blink on activity
or
  blink with 50ms interval when speed=1000m
  blink with 100ms interval when speed=100m
  blink with 200ms interval when speed=10m

(Note that these don't need to be supported by PHY. We are talking
 about SW control. If the PHY supports some of these in HW, then the
 trigger can be offloaded.)

Marek

Re: lets settle the LED `function` property regarding the netdev trigger

[Andrew Lunn]

> > I agree with having a list, and we use the combination. If the
> > combination is not possible by the hardware, then -EINVAL, or
> > -EOPNOTSUPP.
> > 
> > > - having separate functions for different link modes
> > >     function = "link1000", "link100";  
> > 
> > I would suggest this, so you can use 
> > 
> > function = "link1000", "link100", "activity"
> 
> The problem here is that LED core uses function to compose LED name:
>   devicename:color:function
> Should we use the first function? Then this LED will be named:
>   ethphy42:green:link1000
> but it also indicates link100...

This makes no sense. Using function makes no sense, when the whole
point of using the LED framework is we have a uniform way of
setting/changing the function at run time.

An LED called ethphy42:green:link1000 which is actually showing
activity makes no sense.

ethphy42:green:state would be a better name.  The function of the LED
is to give you some idea of what the state of the PHY is. What state
it actually indicates is up to the user.

> > What could be interesting is how you do this in sysfs?  How do you
> > enumerate what the hardware can do? How do you select what you want?
> 
> This is again sidetrack from the original discussion, which was only
> meant to discuss DT, but okay :)
> 
> > Do you need to do
> > 
> > echo "link1000 link100 activity" > /sys/class/net/eth0/phy/led/function
> > 
> > And we can have something like
> > 
> > cat /sys/class/net/eth0/phy/led/function
> > activity
> > link10 activity
> > link100 activity
> > link1000 activity
> > [link100 link1000 activity]
> > link10
> > link100
> > link1000
> 
> No, my current ideas about the netdev trigger extension are as follows
> (not yet complete):
> 
> $ cd /sys/.../<LED>
> $ echo netdev >trigger	# To enable netdev trigger
> $ echo eth0 >device_name
> $ echo 1 >ext		# To enable extended netdev trigger.
> 			# This will create directory modes if there is
> 			# a PHY attached to the interface  
> $ ls modes/		
> 1000baseT_Full 100BaseT_Full 100BaseT_Half 10BaseT_Full 10BaseT_Half
>
> $ cd modes/1000baseT_Full
> $ ls
> brightness link rx tx interval
> 
> So basically if you enable the extended netdev trigger, you will get
> all the standard netdev settings for each PHY mode. (With a little
> change to support blinking on link.)
> 
> With this you can set the LED:
>   ON when linked and speed=1000m or 100m, blink on activity
> or
>   blink with 50ms interval when speed=1000m
>   blink with 100ms interval when speed=100m
>   blink with 200ms interval when speed=10m
> 
> (Note that these don't need to be supported by PHY. We are talking
>  about SW control. If the PHY supports some of these in HW, then the
>  trigger can be offloaded.)

I see a number of problems with this

1) Not all PHYs support software control of the LEDs. i.e. software
on/off. But they do support different blink modes. Just looking at the
data sheets i have lying around like this:

LAN8740 
KSZ8041

and i'm sure there are more. So these PHYs LEDs will always be in
offloaded mode, you cannot do software blinking. But they do support
multiple blinking modes, so we want to be able to control that.

2) Marvell PHY i have the datasheet open for at the moment has no way
to indicate duplex. So you cannot actually offload 100baseT_Full. You
need to user to configure the same blink mode for both 100baseT_Full
and then 100baseT_Half, so duplex is irrelevant, then you can offload
it, which is not very obvious.

3) phylib does not actually tell you what link mode the PHY is
operating in. All you get is the resolved speed and duplex. And
mapping speed an duplex back to a link mode is not obvious. Take for
example 100baseT_Full & 100baseT1_Full. Both are 100Mbps, both full
duplex. Currently there is no PHY which actually implements both, so
currently you can work it out, but in general, you cannot. But this is
very true for higher speeds, where the MAC is providing the PHY LED
control, but ideally we want the same /sysfs interface:

        ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT = 23,
        ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT = 24,
        ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT = 25,
        ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT = 26,

Are you suggesting 4 different directories for the same speed?

I think you need duplex, KR4/CR4/SR4/LR4, T1/T2 as separate
attributes, which the LED might support, but are not required.

4) Software blinking can add quite a lot of overhead. Getting the
counters from the device can be expensive, particularly for Ethernet
switches on slow busses. If anybody sets the interval to 5ms, they
could saturate the MDIO bus. And blinking the LEDs is not for free.
So either we need to indicate if software or hardware is used, or we
should limit the choices to those which the hardware can actually do,
so we guarantee offload.

   Andrew

Re: lets settle the LED `function` property regarding the netdev trigger

[Marek Behún]

On Wed, 6 Oct 2021 14:57:13 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > > I agree with having a list, and we use the combination. If the
> > > combination is not possible by the hardware, then -EINVAL, or
> > > -EOPNOTSUPP.
> > >   
> > > > - having separate functions for different link modes
> > > >     function = "link1000", "link100";    
> > > 
> > > I would suggest this, so you can use 
> > > 
> > > function = "link1000", "link100", "activity"  
> > 
> > The problem here is that LED core uses function to compose LED name:
> >   devicename:color:function
> > Should we use the first function? Then this LED will be named:
> >   ethphy42:green:link1000
> > but it also indicates link100...  
> 
> This makes no sense. Using function makes no sense, when the whole
> point of using the LED framework is we have a uniform way of
> setting/changing the function at run time.
>
> An LED called ethphy42:green:link1000 which is actually showing
> activity makes no sense.
>
> ethphy42:green:state would be a better name.  The function of the LED
> is to give you some idea of what the state of the PHY is. What state
> it actually indicates is up to the user.

The LED device name is supposed to reflect what the LED was dedicated
for by vendor. So if on the device chassis next to the LED there is an
icon or text indicating that the LED is supposed to show link, then the
function part of the LED name should be "link", no matter what the user
does with the LED during runtime.

In many cases the icon/text by the LED on the chassis says only something
like "wan" or "lanN". On Turris Omnia for example this is so, and the LED
is supposed to show link and blink on activity.

For LEDs on ethernet ports usually the port is dedicated (i.e. it is a
wan port or a lanN port), and the LEDs are also dedicated in some way
(for example green for link1000, blink on activity, yellow for link100).

Currently device tree binding for LEDs allows setting function and
function-enumerator, and there are only macros
  LED_FUNCTION_WAN	(expands to "wan")
  LED_FUNCTION_LAN	(expands to "lan")
So the device tree can specify:
  color = <LED_COLOR_ID_GREEN>;
  function = LED_FUNCTION_WAN;
and the LED will be called
  <devicename>:green:wan (with no devicename just green:wan)
or
  color = <LED_COLOR_ID_GREEN>;
  function = LED_FUNCTION_LAN;
  function-enumerator = <2>;
and the LED will be called
  <devicename>:green:lan-2

We need to somehow keep the function part of the LED name consistent
with what the LED was dedicated for by vendor, but also to be able to
set more specific trigger settings (link1000, link100, link10,
activity).

Maybe it would make sense to keep the first string in the function
property "lan" / "wan", and the other strings to specify more specific
functions?

> > No, my current ideas about the netdev trigger extension are as follows
> > (not yet complete):
> > 
> > $ cd /sys/.../<LED>
> > $ echo netdev >trigger	# To enable netdev trigger
> > $ echo eth0 >device_name
> > $ echo 1 >ext		# To enable extended netdev trigger.
> > 			# This will create directory modes if there is
> > 			# a PHY attached to the interface  
> > $ ls modes/		
> > 1000baseT_Full 100BaseT_Full 100BaseT_Half 10BaseT_Full 10BaseT_Half
> >
> > $ cd modes/1000baseT_Full
> > $ ls
> > brightness link rx tx interval
> > 
> > So basically if you enable the extended netdev trigger, you will get
> > all the standard netdev settings for each PHY mode. (With a little
> > change to support blinking on link.)
> > 
> > With this you can set the LED:
> >   ON when linked and speed=1000m or 100m, blink on activity
> > or
> >   blink with 50ms interval when speed=1000m
> >   blink with 100ms interval when speed=100m
> >   blink with 200ms interval when speed=10m
> > 
> > (Note that these don't need to be supported by PHY. We are talking
> >  about SW control. If the PHY supports some of these in HW, then the
> >  trigger can be offloaded.)  
> 
> I see a number of problems with this
> 
> 1) Not all PHYs support software control of the LEDs. i.e. software
> on/off. But they do support different blink modes. Just looking at the
> data sheets i have lying around like this:
> 
> LAN8740 
> KSZ8041
> 
> and i'm sure there are more. So these PHYs LEDs will always be in
> offloaded mode, you cannot do software blinking. But they do support
> multiple blinking modes, so we want to be able to control that.

I am aware of such LEDs. Currently the LED subsystem does not support
such LEDs. I wanted first to extend the LED API to support offloading,
and wanted to look at LEDs that only can be HW controlled later.

> 2) Marvell PHY i have the datasheet open for at the moment has no way
> to indicate duplex. So you cannot actually offload 100baseT_Full. You
> need to user to configure the same blink mode for both 100baseT_Full
> and then 100baseT_Half, so duplex is irrelevant, then you can offload
> it, which is not very obvious.

This was just a proposal, I have not written code for this yet. We can
do this by speed only, if it makes more sense.

> 3) phylib does not actually tell you what link mode the PHY is
> operating in. All you get is the resolved speed and duplex. And
> mapping speed an duplex back to a link mode is not obvious. Take for
> example 100baseT_Full & 100baseT1_Full. Both are 100Mbps, both full
> duplex. Currently there is no PHY which actually implements both, so
> currently you can work it out, but in general, you cannot. But this is
> very true for higher speeds, where the MAC is providing the PHY LED
> control, but ideally we want the same /sysfs interface:
> 
>         ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT = 23,
>         ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT = 24,
>         ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT = 25,
>         ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT = 26,
> 
> Are you suggesting 4 different directories for the same speed?
> 
> I think you need duplex, KR4/CR4/SR4/LR4, T1/T2 as separate
> attributes, which the LED might support, but are not required.

OK, it would make more sense to do this by speed only at first. And
later if someone needs to do it more specific, it can be extended.

$ cd /sys/class/leds/<LED>
$ echo netdev >trigger
$ echo speed >ext
$ ls modes
1000 100 10

Or maybe the subdirectories can be added by request:

$ cd /sys/class/leds/<LED>
$ echo netdev >trigger
$ cat ext
[none] speed link_mode
$ echo link_mode >ext
$ ls modes
add  delete
$ echo 40000baseKR4_Full >modes/add
$ ls modes
40000baseKR4_Full  add  delete

> 4) Software blinking can add quite a lot of overhead. Getting the
> counters from the device can be expensive, particularly for Ethernet
> switches on slow busses. If anybody sets the interval to 5ms, they
> could saturate the MDIO bus. And blinking the LEDs is not for free.

This is how the netdev trigger works currently. I just wanted first to
extend it to support in SW things that are offloadable to some HW.

You are right that MDIO bus can be slow and saturated by SW blinking,
that is why we need offloading.

Also currently netdev trigger blink on activity does not work for DSA
switch ports (at least not for mv88e6xxx), unless the packet goes to
CPU also, since the trigger looks at CPU interface stats...

> So either we need to indicate if software or hardware is used, or we
> should limit the choices to those which the hardware can actually do,
> so we guarantee offload.

I think we should indicate whether the trigger is offloaded and let the
user control the LED also in SW. An utility (ledtool) can be written
which could then list HW offloadable modes and let the user choose only
between those.

Marek