[PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Josua Mayer]

Dear Maintainers,

I am working on a new device based on the LX2160A platform, that exposes
16 sfp connectors, each with its own led controlled by gpios intended
to show link status.
This patch intends to illustrate in code what we want to achieve,
so that I can better ask you all:
How can this work with the generic led framework, and how was it meant to work?

The paragraphs below are a discussion of paths I have explored without success.
Please let me know if you have any opinions and ideas.

Describing in device-tree that an led node belongs to a particular network
device (dpmac) however seems impossible. Instead the standard appears to work
through triggers, where in device-tree one only annotates that the led should
show a trigger "netdev" or "phy". Both of these make no sense when multiple
network interfaces exist - raising the first question:
How can device-tree indicate that an individual led should show the events of
a particular network interface?

We have found that there is a way in sysfs to echo the name of the network
device to the api of the led driver, and it will start showing link status.
However this has to be done at runtime by the user.
But network interface names are unstable. They depend on probe order and
can be changed at will. Further they can be moved to different namespaces,
which will allow e.g. two instances of "eth0" to coexist.
On the Layerscape platform in particular these devices are created dynamically
by the networkign coprocessor, which supports complex functions such as
creating one network interface that spans multiple ports.
It seems to me that the netdev trigger therefore can not properly reflect
the relation between an LED (which is hard-wired to an sfp cage), and the
link state reported by e.g. a phy inside an sfp module.

There exists also a phy trigger for leds.
When invoking the phy_attach or phy_connect functions from the generic phy
framework to connect an instance of net_device with an instance of phy_device,
a trigger providing the link and speed events is registered.
You may notice that again leds are tied to existence of a particular logical
network interface, which may or may not exist, and may span multiple
physical interfaces in case of layerscape.
This is a dead end though, simply because the dpaa2 driver does not even use
objects of phy_device, so this registering of triggers never happens.

In addition the dpmac nodes in device-tree don't really have a phy modeled.
One end are the serdes which are managed by the networking coprocessor.
The other end has removable sfp modules, which may or may not contain a phy.

The serdes are modeled as phy in device-tree though, perhaps the dpaa2 driver
could be extended to instantiate phy_device objects for the serdes?
However I feel like this would especially not solve when mutliple physical
ports are used as one logical interface.

It seems to me that there should be a way to explicitly link gpio-driven LEDs to
either specific phy nodes, or specific sfp connectors - and have them receive
link events from the respective phy, fully independent even from whether there
is a logical network interface.

If you got here, thank you very much for reading!
Ay comments?

sincerely
Josua Mayer

Signed-off-by: Josua Mayer <josua@solid-run.com>
---
 .../devicetree/bindings/net/sff,sfp.txt       |  4 +++
 drivers/net/phy/sfp.c                         | 36 +++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/sff,sfp.txt b/Documentation/devicetree/bindings/net/sff,sfp.txt
index 832139919f20..d46df1300d28 100644
--- a/Documentation/devicetree/bindings/net/sff,sfp.txt
+++ b/Documentation/devicetree/bindings/net/sff,sfp.txt
@@ -37,6 +37,10 @@ Optional Properties:
   Specifies the maximum power consumption allowable by a module in the
   slot, in milli-Watts.  Presently, modules can be up to 1W, 1.5W or 2W.
 
+- link-status-led:
+    description: An LED node for showing link status.
+    $ref: /schemas/types.yaml#/definitions/phandle
+
 Example #1: Direct serdes to SFP connection
 
 sfp_eth3: sfp-eth3 {
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 2fff62695455..0f18e77b8b68 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -7,6 +7,7 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/jiffies.h>
+#include <linux/leds.h>
 #include <linux/mdio/mdio-i2c.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -258,6 +259,7 @@ struct sfp {
 	char *hwmon_name;
 #endif
 
+	struct led_classdev *link_status_led;
 };
 
 static bool sff_module_supported(const struct sfp_eeprom_id *id)
@@ -1490,6 +1492,8 @@ static int sfp_sm_probe_phy(struct sfp *sfp, bool is_c45)
 
 static void sfp_sm_link_up(struct sfp *sfp)
 {
+	if (sfp->link_status_led)
+		led_set_brightness(sfp->link_status_led, sfp->link_status_led->max_brightness);
 	sfp_link_up(sfp->sfp_bus);
 	sfp_sm_next(sfp, SFP_S_LINK_UP, 0);
 }
@@ -1497,6 +1501,8 @@ static void sfp_sm_link_up(struct sfp *sfp)
 static void sfp_sm_link_down(struct sfp *sfp)
 {
 	sfp_link_down(sfp->sfp_bus);
+	if (sfp->link_status_led)
+		led_set_brightness(sfp->link_status_led, LED_OFF);
 }
 
 static void sfp_sm_link_check_los(struct sfp *sfp)
@@ -2425,6 +2429,23 @@ static int sfp_probe(struct platform_device *pdev)
 
 		i2c = of_find_i2c_adapter_by_node(np);
 		of_node_put(np);
+
+		np = of_parse_phandle(node, "link-status-led", 0);
+		sfp->link_status_led = of_led_get_hack(np);
+		of_node_put(np);
+
+		if (IS_ERR(sfp->link_status_led)) {
+			switch (PTR_ERR(sfp->link_status_led)) {
+			case -ENODEV:
+				sfp->link_status_led = NULL;
+				break;
+			default:
+				dev_err(sfp->dev, "failed to get link-statusled from 'link-status-led' property: %pe\n", sfp->link_status_led);
+				fallthrough;
+			case -EPROBE_DEFER:
+				return PTR_ERR(sfp->link_status_led);
+			};
+		}
 	} else if (has_acpi_companion(&pdev->dev)) {
 		struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
 		struct fwnode_handle *fw = acpi_fwnode_handle(adev);
@@ -2453,6 +2476,14 @@ static int sfp_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	if (sfp->link_status_led) {
+		/* remove from sysfs to avoid userspce control */
+		led_sysfs_disable(sfp->link_status_led);
+
+		/* turn off initially */
+		led_set_brightness(sfp->link_status_led, LED_OFF);
+	}
+
 	for (i = 0; i < GPIO_MAX; i++)
 		if (sff->gpios & BIT(i)) {
 			sfp->gpio[i] = devm_gpiod_get_optional(sfp->dev,
@@ -2545,6 +2576,11 @@ static int sfp_remove(struct platform_device *pdev)
 {
 	struct sfp *sfp = platform_get_drvdata(pdev);
 
+	if (sfp->link_status_led) {
+		/* re-enable sysfs interface */
+		led_sysfs_enable(sfp->link_status_led);
+	}
+
 	sfp_unregister_socket(sfp->sfp_bus);
 
 	rtnl_lock();
-- 
2.35.3

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Andrew Lunn]

On Mon, May 09, 2022 at 03:29:38PM +0300, Josua Mayer wrote:
> Dear Maintainers,
> 
> I am working on a new device based on the LX2160A platform, that exposes
> 16 sfp connectors, each with its own led controlled by gpios intended
> to show link status.

Can you define link status? It is a messy concept with SFPs. Is it
!LOS? I guess not, because you would not of used a GPIO, just hard
wired it. Does it mean the SERDES has sync? Does it reflect the netdev
carrier status?

> We have found that there is a way in sysfs to echo the name of the network
> device to the api of the led driver, and it will start showing link status.
> However this has to be done at runtime by the user.

Please take a look at the patches Ansuel Smith submitted last week,
maybe the week before last.

> On the Layerscape platform in particular these devices are created dynamically
> by the networkign coprocessor, which supports complex functions such as
> creating one network interface that spans multiple ports.

The linux model is that each MAC has a netdev, hence a name. If you
need to span multiple ports, you then add a bridge and add the MACs to
the bridge. So there should not be an issue here.

> It seems to me that the netdev trigger therefore can not properly reflect
> the relation between an LED (which is hard-wired to an sfp cage), and the
> link state reported by e.g. a phy inside an sfp module.

The netdev carrier will correctly reflect this.

> You may notice that again leds are tied to existence of a particular logical
> network interface, which may or may not exist, and may span multiple
> physical interfaces in case of layerscape.

As far as i'm aware, the in kernel code always has a netdev for each
MAC. Are you talking about the vendor stack?

	Andrew

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Russell King (Oracle)]

On Mon, May 09, 2022 at 03:29:38PM +0300, Josua Mayer wrote:
> Dear Maintainers,
> 
> I am working on a new device based on the LX2160A platform, that exposes
> 16 sfp connectors, each with its own led controlled by gpios intended
> to show link status.
> This patch intends to illustrate in code what we want to achieve,
> so that I can better ask you all:
> How can this work with the generic led framework, and how was it meant to work?
> 
> The paragraphs below are a discussion of paths I have explored without success.
> Please let me know if you have any opinions and ideas.
> 
> Describing in device-tree that an led node belongs to a particular network
> device (dpmac) however seems impossible. Instead the standard appears to work
> through triggers, where in device-tree one only annotates that the led should
> show a trigger "netdev" or "phy". Both of these make no sense when multiple
> network interfaces exist - raising the first question:
> How can device-tree indicate that an individual led should show the events of
> a particular network interface?
> 
> We have found that there is a way in sysfs to echo the name of the network
> device to the api of the led driver, and it will start showing link status.
> However this has to be done at runtime by the user.
> But network interface names are unstable. They depend on probe order and
> can be changed at will. Further they can be moved to different namespaces,
> which will allow e.g. two instances of "eth0" to coexist.
> On the Layerscape platform in particular these devices are created dynamically
> by the networkign coprocessor, which supports complex functions such as
> creating one network interface that spans multiple ports.
> It seems to me that the netdev trigger therefore can not properly reflect
> the relation between an LED (which is hard-wired to an sfp cage), and the
> link state reported by e.g. a phy inside an sfp module.
> 
> There exists also a phy trigger for leds.
> When invoking the phy_attach or phy_connect functions from the generic phy
> framework to connect an instance of net_device with an instance of phy_device,
> a trigger providing the link and speed events is registered.
> You may notice that again leds are tied to existence of a particular logical
> network interface, which may or may not exist, and may span multiple
> physical interfaces in case of layerscape.
> This is a dead end though, simply because the dpaa2 driver does not even use
> objects of phy_device, so this registering of triggers never happens.
> 
> In addition the dpmac nodes in device-tree don't really have a phy modeled.
> One end are the serdes which are managed by the networking coprocessor.
> The other end has removable sfp modules, which may or may not contain a phy.
> 
> The serdes are modeled as phy in device-tree though, perhaps the dpaa2 driver
> could be extended to instantiate phy_device objects for the serdes?
> However I feel like this would especially not solve when mutliple physical
> ports are used as one logical interface.
> 
> It seems to me that there should be a way to explicitly link gpio-driven LEDs to
> either specific phy nodes, or specific sfp connectors - and have them receive
> link events from the respective phy, fully independent even from whether there
> is a logical network interface.
> 
> If you got here, thank you very much for reading!
> Ay comments?

You really don't need any of this.

We already have the "netdev" trigger - you just need to assign the LED
to the appropriate netdev link.

I do this on the SolidSense platform with the two LEDs when using it as
my internet gateway on the boat - one LED gives wlan status, the other
LED gives wwan status, both of them green for link and red for tx/rx
activity.

Exactly the same can be done with SFPs if the net device is exclusive
to the SFP socket. Doing it this way also tells you that the netdev
link is up, not just that a SFP has decided to deassert the RXLOS
signals, which on some SFPs is tied inactive.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Josua Mayer]

Am 09.05.22 um 15:49 schrieb Andrew Lunn:
> On Mon, May 09, 2022 at 03:29:38PM +0300, Josua Mayer wrote:
>> Dear Maintainers,
>>
>> I am working on a new device based on the LX2160A platform, that exposes
>> 16 sfp connectors, each with its own led controlled by gpios intended
>> to show link status.
> Can you define link status?
I am still struggling with the lower levels of networking terminology 
... so I was considering
when ethtool would report "Link detected: yes".
>   It is a messy concept with SFPs. Is it
> !LOS? I guess not, because you would not of used a GPIO, just hard
> wired it.
I believe the intention was to decide later what information to visualize.
In this iteration there is one LED per sfp connector, with one colour.
But it is conceivable to in the future add more, and use them to 
indicate e.g. the negotiated speed (10/100/1000/10000).
> Does it mean the SERDES has sync? Does it reflect the netdev
> carrier status?
>
>> We have found that there is a way in sysfs to echo the name of the network
>> device to the api of the led driver, and it will start showing link status.
>> However this has to be done at runtime by the user.
> Please take a look at the patches Ansuel Smith submitted last week,
> maybe the week before last.

Found them. Those are a great pointer - I did not notice the 
trigger-sources property
while looking at led documentation and bindings,
but Documentation/devicetree/bindings/leds/common.yaml does have it.

So what his patch-set proposes covers a large part of my question here, 
thanks!

>> On the Layerscape platform in particular these devices are created dynamically
>> by the networkign coprocessor, which supports complex functions such as
>> creating one network interface that spans multiple ports.
> The linux model is that each MAC has a netdev, hence a name. If you
> need to span multiple ports, you then add a bridge and add the MACs to
> the bridge. So there should not be an issue here.
Okay. That will do for the immediate use-case.
>> It seems to me that the netdev trigger therefore can not properly reflect
>> the relation between an LED (which is hard-wired to an sfp cage), and the
>> link state reported by e.g. a phy inside an sfp module.
> The netdev carrier will correctly reflect this.
Once the netdev (eth[0-9]+) has been created, all relations are known, yes.
And because you mentioned above that each MAC has a netdev, it will do.
E.g. while whatever we plug into the sfp connector changes, the MAC 
stays where it is - connected to a particular cage.
>
>> You may notice that again leds are tied to existence of a particular logical
>> network interface, which may or may not exist, and may span multiple
>> physical interfaces in case of layerscape.
> As far as i'm aware, the in kernel code always has a netdev for each
> MAC. Are you talking about the vendor stack?
The coprocessor can be configured both at boot-time and runtime.
During runtime there is a vendor tool "restool" which can create and 
destroy network interfaces, which the dpaa2 driver knows to discover and 
bind to.

Your statement holds, quoting from the dpaa2 driver documentation:
"... exposes each switch port as a network interface, which can be 
included in a bridge or used as a standalone interface"

I made a false assumption here, Thank you!

> 	Andrew
sincerely
Josua Mayer

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Josua Mayer]

Hi Russell,

Am 09.05.22 um 18:54 schrieb Russell King (Oracle):
> On Mon, May 09, 2022 at 03:29:38PM +0300, Josua Mayer wrote:
>> Dear Maintainers,
>>
>> I am working on a new device based on the LX2160A platform, that exposes
>> 16 sfp connectors, each with its own led controlled by gpios intended
>> to show link status.
>> This patch intends to illustrate in code what we want to achieve,
>> so that I can better ask you all:
>> How can this work with the generic led framework, and how was it meant to work?
>>
>> The paragraphs below are a discussion of paths I have explored without success.
>> Please let me know if you have any opinions and ideas.
>>
>> Describing in device-tree that an led node belongs to a particular network
>> device (dpmac) however seems impossible. Instead the standard appears to work
>> through triggers, where in device-tree one only annotates that the led should
>> show a trigger "netdev" or "phy". Both of these make no sense when multiple
>> network interfaces exist - raising the first question:
>> How can device-tree indicate that an individual led should show the events of
>> a particular network interface?
>>
>> We have found that there is a way in sysfs to echo the name of the network
>> device to the api of the led driver, and it will start showing link status.
>> However this has to be done at runtime by the user.
>> But network interface names are unstable. They depend on probe order and
>> can be changed at will. Further they can be moved to different namespaces,
>> which will allow e.g. two instances of "eth0" to coexist.
>> On the Layerscape platform in particular these devices are created dynamically
>> by the networkign coprocessor, which supports complex functions such as
>> creating one network interface that spans multiple ports.
>> It seems to me that the netdev trigger therefore can not properly reflect
>> the relation between an LED (which is hard-wired to an sfp cage), and the
>> link state reported by e.g. a phy inside an sfp module.
>>
>> There exists also a phy trigger for leds.
>> When invoking the phy_attach or phy_connect functions from the generic phy
>> framework to connect an instance of net_device with an instance of phy_device,
>> a trigger providing the link and speed events is registered.
>> You may notice that again leds are tied to existence of a particular logical
>> network interface, which may or may not exist, and may span multiple
>> physical interfaces in case of layerscape.
>> This is a dead end though, simply because the dpaa2 driver does not even use
>> objects of phy_device, so this registering of triggers never happens.
>>
>> In addition the dpmac nodes in device-tree don't really have a phy modeled.
>> One end are the serdes which are managed by the networking coprocessor.
>> The other end has removable sfp modules, which may or may not contain a phy.
>>
>> The serdes are modeled as phy in device-tree though, perhaps the dpaa2 driver
>> could be extended to instantiate phy_device objects for the serdes?
>> However I feel like this would especially not solve when mutliple physical
>> ports are used as one logical interface.
>>
>> It seems to me that there should be a way to explicitly link gpio-driven LEDs to
>> either specific phy nodes, or specific sfp connectors - and have them receive
>> link events from the respective phy, fully independent even from whether there
>> is a logical network interface.
>>
>> If you got here, thank you very much for reading!
>> Ay comments?
> You really don't need any of this.
>
> We already have the "netdev" trigger - you just need to assign the LED
> to the appropriate netdev link.
Yes, that is what we are doing at runtime now:
echo eth12 > /sys/class/leds/led_c1_at/device_name

I feared though (without testing) that this might break when interfaces 
are moved between namespaces, or renamed.
And it seemed wrong requiring the user to do explicitly make this 
assignment, when the purpose is very much fixed once you put a case 
around it and add some labels.

> I do this on the SolidSense platform with the two LEDs when using it as
> my internet gateway on the boat - one LED gives wlan status, the other
> LED gives wwan status, both of them green for link and red for tx/rx
> activity.
Ah. And do you put the assignment of the LEDs into an init script?
> Exactly the same can be done with SFPs if the net device is exclusive
> to the SFP socket.
One issue is that the interfaces don't have stable names. It purely 
depends on probe order,
which is controlled by sending commands to the networking coprocessor.

We actually get asked this question sometimes how to have stable device 
names, and so far the answer has been systemd services with explicit 
sleep to force the order.
But this is a different topic.

> Doing it this way also tells you that the netdev
> link is up, not just that a SFP has decided to deassert the RXLOS
> signals, which on some SFPs is tied inactive.
Good point, I have already noticed inconsistencies in this regard.
The information provided by the netdev trigger does seem superior.

We will try to adopt a solution based on what Andrew pointed me to, as 
that one appears to be a good way to describe this relationship between 
LEDs and MACs (by extension eth[0-9]+ interfaces).

> Thanks.

Thank you for your comments!

sincerely
Josua Mayer

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Andrew Lunn]

> > As far as i'm aware, the in kernel code always has a netdev for each
> > MAC. Are you talking about the vendor stack?
> The coprocessor can be configured both at boot-time and runtime.
> During runtime there is a vendor tool "restool" which can create and destroy
> network interfaces, which the dpaa2 driver knows to discover and bind to.

There should not be any need to use a vendor tool for mainline. In
fact, that is strongly discouraged, since it leads to fragmentation,
each device doing its own thing, the user needing to read each vendors
user manual, rather than it just being a standard Unix box with
interfaces.

What should happen is that all the front panel interfaces exist from
boot. If you want to add them to a bridge, for example, you do so in
the normal linux way, create a bridge and add an interface to the
bridge. The kernel driver can then talk to the coprocessor and magic
up a dpaa2 bridge object, and add the port to the bridge, but as far
as the user is concerned, it should all be the usual iproute2
commands, nothing more.

	  Andrew

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Russell King (Oracle)]

On Tue, May 10, 2022 at 11:56:06AM +0300, Josua Mayer wrote:
> 
> Am 09.05.22 um 15:49 schrieb Andrew Lunn:
> > On Mon, May 09, 2022 at 03:29:38PM +0300, Josua Mayer wrote:
> > > Dear Maintainers,
> > > 
> > > I am working on a new device based on the LX2160A platform, that exposes
> > > 16 sfp connectors, each with its own led controlled by gpios intended
> > > to show link status.
> > Can you define link status?
> I am still struggling with the lower levels of networking terminology ... so
> I was considering when ethtool would report "Link detected: yes".

That is what the netdev LED trigger uses for "link".

> >   It is a messy concept with SFPs. Is it
> > !LOS? I guess not, because you would not of used a GPIO, just hard
> > wired it.
> I believe the intention was to decide later what information to visualize.
> In this iteration there is one LED per sfp connector, with one colour.
> But it is conceivable to in the future add more, and use them to indicate
> e.g. the negotiated speed (10/100/1000/10000).

Doing this at the SFP layer won't get you that - the SFP layer doesn't
know what speed has actually been decided upon.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Russell King (Oracle)]

On Tue, May 10, 2022 at 12:44:41PM +0300, Josua Mayer wrote:
> Hi Russell,
> 
> Am 09.05.22 um 18:54 schrieb Russell King (Oracle):
> > On Mon, May 09, 2022 at 03:29:38PM +0300, Josua Mayer wrote:
> > I do this on the SolidSense platform with the two LEDs when using it as
> > my internet gateway on the boat - one LED gives wlan status, the other
> > LED gives wwan status, both of them green for link and red for tx/rx
> > activity.
> Ah. And do you put the assignment of the LEDs into an init script?

Yes, I do it from a systemd unit that runs a "platform-leds-init"
script as the easiest way on the SolidSense - since I want one for
wwan0 and the other for wlan0, there is no chance of the names
being swapped.

However, I'm quite sure it's possible to have udev and systemd scripts
that do what is necessary, or poke in sysfs.

I can't remotely power up my Clearfog-CX to see exactly how to do it,
but I am quite sure one can have a script in udev that notices a new
netdev coming along, works out which it is (by looking at
/sys/class/net/$name/device, or more probably some udev variable
that gives you that) and decides which LED should be configured.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Russell King (Oracle)]

On Tue, May 10, 2022 at 02:13:42PM +0200, Andrew Lunn wrote:
> > > As far as i'm aware, the in kernel code always has a netdev for each
> > > MAC. Are you talking about the vendor stack?
> > The coprocessor can be configured both at boot-time and runtime.
> > During runtime there is a vendor tool "restool" which can create and destroy
> > network interfaces, which the dpaa2 driver knows to discover and bind to.
> 
> There should not be any need to use a vendor tool for mainline. In
> fact, that is strongly discouraged, since it leads to fragmentation,
> each device doing its own thing, the user needing to read each vendors
> user manual, rather than it just being a standard Unix box with
> interfaces.

You're missing the bigger picture.

There are two ways to setup the networking on LX2160A - one is via
DT-like files that are processed by the network firmware, which tells
it what you want to do with each individual network connection.

Then there is a userspace tool that talks to the LX2160A network
firmware and requests it to configure the network layer - e.g. create
a network interface to connect to a network connection, or whatever.

I believe that when using DPDK, one does not want the network
connections to be associated with Linux network interfaces - but
don't quote me on that.

The tool has nothing to do with LEDs. It's all about talking to the
networking firmware on the chip.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Ioana Ciornei]

On Tue, May 10, 2022 at 12:44:41PM +0300, Josua Mayer wrote:

> One issue is that the interfaces don't have stable names. It purely depends
> on probe order,
> which is controlled by sending commands to the networking coprocessor.
> 
> We actually get asked this question sometimes how to have stable device
> names, and so far the answer has been systemd services with explicit sleep
> to force the order.
> But this is a different topic.
> 

Stable names can be achieved using some udev rules based on the OF node.
For example, I am using the following rules on a Clearfog CX LX2:

[root@clearfog-cx-lx2 ~] # cat /etc/udev/rules.d/70-persistent-net.rules
SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@7", NAME="eth7"
SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@8", NAME="eth8"
SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@9", NAME="eth9"
SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@a", NAME="eth10"
SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@11", NAME="eth17"

Ioana

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Russell King (Oracle)]

On Wed, May 11, 2022 at 01:22:22PM +0000, Ioana Ciornei wrote:
> On Tue, May 10, 2022 at 12:44:41PM +0300, Josua Mayer wrote:
> 
> > One issue is that the interfaces don't have stable names. It purely depends
> > on probe order,
> > which is controlled by sending commands to the networking coprocessor.
> > 
> > We actually get asked this question sometimes how to have stable device
> > names, and so far the answer has been systemd services with explicit sleep
> > to force the order.
> > But this is a different topic.
> > 
> 
> Stable names can be achieved using some udev rules based on the OF node.
> For example, I am using the following rules on a Clearfog CX LX2:
> 
> [root@clearfog-cx-lx2 ~] # cat /etc/udev/rules.d/70-persistent-net.rules
> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@7", NAME="eth7"
> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@8", NAME="eth8"
> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@9", NAME="eth9"
> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@a", NAME="eth10"
> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@11", NAME="eth17"

Or by using systemd - for example, on the Armada 38x Clearfog platform,
I use:

/etc/systemd/network/01-ded.link:
[Match]
Path=platform-f1070000.ethernet
[Link]
MACAddressPolicy=none
Name=eno0

/etc/systemd/network/02-sw.link:
[Match]
Path=platform-f1030000.ethernet
[Link]
MACAddressPolicy=none
Name=eno1

/etc/systemd/network/03-sfp.link:
[Match]
Path=platform-f1034000.ethernet
[Link]
MACAddressPolicy=none
Name=eno2

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Ioana Ciornei]

On Wed, May 11, 2022 at 11:26:12AM +0100, Russell King (Oracle) wrote:
> On Tue, May 10, 2022 at 02:13:42PM +0200, Andrew Lunn wrote:
> > > > As far as i'm aware, the in kernel code always has a netdev for each
> > > > MAC. Are you talking about the vendor stack?
> > > The coprocessor can be configured both at boot-time and runtime.
> > > During runtime there is a vendor tool "restool" which can create and destroy
> > > network interfaces, which the dpaa2 driver knows to discover and bind to.
> > 
> > There should not be any need to use a vendor tool for mainline. In
> > fact, that is strongly discouraged, since it leads to fragmentation,
> > each device doing its own thing, the user needing to read each vendors
> > user manual, rather than it just being a standard Unix box with
> > interfaces.
> 
> You're missing the bigger picture.
> 
> There are two ways to setup the networking on LX2160A - one is via
> DT-like files that are processed by the network firmware, which tells
> it what you want to do with each individual network connection.
> 
> Then there is a userspace tool that talks to the LX2160A network
> firmware and requests it to configure the network layer - e.g. create
> a network interface to connect to a network connection, or whatever.

Yes, that is correct.
Beside that, the userspace tool is not mandatory by any means. You can
do the necessary setup at boot time.

> 
> I believe that when using DPDK, one does not want the network
> connections to be associated with Linux network interfaces - but
> don't quote me on that.

Hmm, not exactly true.

Yes, when using DPDK there won't be a typical network interface exported
by the dpaa2-eth driver present upstream. But there is a downstream only
driver will which take care of the phy, pcs, MAC part with phylink's
help so that DPDK can only concentrate on the fast path.

We did this so that we can leverage the phylink integration with DPDK
also, but we are not pushing it to upstream since it's not really clean
to have a netdev just for configuration purposes.

Ioana

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Ioana Ciornei]

On Tue, May 10, 2022 at 11:56:06AM +0300, Josua Mayer wrote:
> 

Hi Josua,

> > > On the Layerscape platform in particular these devices are created dynamically
> > > by the networkign coprocessor, which supports complex functions such as
> > > creating one network interface that spans multiple ports.

Are you sure that by multiple ports you do not mean multiple SerDes
lanes? Otherwise, I do not understand what you are referring to.

The dpaa2-eth driver will register one netdev for each DPNI object
(network interface) and will control/configure one DPMAC (MAC).
There is a 1-1 relationship between these two.

Ioana

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Josua Mayer]

Hi Ioana,

Am 11.05.22 um 18:48 schrieb Ioana Ciornei:
> On Tue, May 10, 2022 at 11:56:06AM +0300, Josua Mayer wrote:
>>
> 
> Hi Josua,
> 
>>>> On the Layerscape platform in particular these devices are created dynamically
>>>> by the networkign coprocessor, which supports complex functions such as
>>>> creating one network interface that spans multiple ports.
> 
> Are you sure that by multiple ports you do not mean multiple SerDes
> lanes? Otherwise, I do not understand what you are referring to.
It is very likely that I meant serdes lanes.
However the concept of my question only makes sense when their 
assignment to netdevs remains fixed.
> 
> The dpaa2-eth driver will register one netdev for each DPNI object
> (network interface) and will control/configure one DPMAC (MAC).
> There is a 1-1 relationship between these two.
That is good, so the dpmac nodes in device-tree are good objects for 
linking LEDs to.
> 
> Ioana

Thank you for the clarification!

sincerely
Josua Mayer

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Josua Mayer]

Hi Russell,

Thank you for the examples below!
Stable names do help in some cases, but not in all.

I did some testing the other day with renaming network interfaces 
through the ip command:
ip link set dev eth8 down
ip link set dev eth12 down
ip link set eth12 name eth20
ip link set eth8 name eth12
ip link set eth20 name eth8
ip link set eth8 up
ip link set dev eth12 up

Swapping interface names like this seems a perfectly legal thing for 
userspace to do. I personally would in such case expect the previous LED 
assignment to move along with the name change, however this does not happen.
Instead after the interface rename, the LEDs are effectively swapped.

Further the netdev trigger implementation seems incorrect when you add 
network namespaces.
Two namespaces can each contain a device named eth0, but the netdev 
trigger does not look at the namespace id when matching events to 
triggers, just the name.

Is it intended for userspace to track interface renames and reassign LEDs?
Or should the trigger driver watch for name changes and adapt accordingly?

Finally I also noticed that the netdev trigger by default does not 
propagate any information to the LED.
All properties - link, rx, tx are 0.
Attempts at setting this by default through udev were widely unsuccessful:
E.g. before setting the trigger property to netdev, the device_name or 
link properties do not exist.
Therefore a rule that sets trigger and link and device at the same time 
does not function:
SUBSYSTEM=="leds", ACTION=="add|change", 
ENV{OF_FULLNAME}=="/leds/led_c1_at", ATTR{trigger}="netdev", 
ATTR{link}="1", ATTR{device_name}="eth0"

It appears necessary to use 2 rules, one that selects netdev, another 
one that chooses what property to show, e.g. link;
and finally some rule that tracks the netdev name and updates 
device_name property accordingly.
All while watching out for infinite loops because the property changes 
appear to trigger more change events, and e.g. setting trigger to netdev 
again causes another change event and resets the properties ...

I get the impression that this is very complex and might be described 
much better in device-tree, at least when a vendor makes explicit 
decisions to the purpose of each led.

Thee has been a recent patchset floating this list by Rafał Miłecki,
which I very much liked:
[PATCH RESEND PoC] leds: trigger: netdev: support DT "trigger-sources" 
property

It does allow declaring the relation from dpmac to led in dts as I would 
have expected.

In addition I believe there should be a way in dts to also set a default 
for what information to show, e.g.
default-function = "link";

And finally dynamic tracking of the interface name.

I would be willing to work on the last two ideas, if this is an 
acceptable approach.

Am 11.05.22 um 16:39 schrieb Russell King (Oracle):
> On Wed, May 11, 2022 at 01:22:22PM +0000, Ioana Ciornei wrote:
>> On Tue, May 10, 2022 at 12:44:41PM +0300, Josua Mayer wrote:
>>
>>> One issue is that the interfaces don't have stable names. It purely depends
>>> on probe order,
>>> which is controlled by sending commands to the networking coprocessor.
>>>
>>> We actually get asked this question sometimes how to have stable device
>>> names, and so far the answer has been systemd services with explicit sleep
>>> to force the order.
>>> But this is a different topic.
>>>
>>
>> Stable names can be achieved using some udev rules based on the OF node.
>> For example, I am using the following rules on a Clearfog CX LX2:
>>
>> [root@clearfog-cx-lx2 ~] # cat /etc/udev/rules.d/70-persistent-net.rules
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@7", NAME="eth7"
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@8", NAME="eth8"
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@9", NAME="eth9"
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@a", NAME="eth10"
>> SUBSYSTEM=="net", ACTION=="add", DRIVERS=="fsl_dpaa2_eth", ENV{OF_FULLNAME}=="/soc/fsl-mc@80c000000/dpmacs/ethernet@11", NAME="eth17"
Yes this seems to work okay.
I feel that it is wrong for userspace to recognize particular dts nodes, 
but it *does* work.

> 
> Or by using systemd - for example, on the Armada 38x Clearfog platform,
> I use:
> 
> /etc/systemd/network/01-ded.link:
> [Match]
> Path=platform-f1070000.ethernet
> [Link]
> MACAddressPolicy=none
> Name=eno0
> 
> /etc/systemd/network/02-sw.link:
> [Match]
> Path=platform-f1030000.ethernet
> [Link]
> MACAddressPolicy=none
> Name=eno1
> 
> /etc/systemd/network/03-sfp.link:
> [Match]
> Path=platform-f1034000.ethernet
> [Link]
> MACAddressPolicy=none
> Name=eno2
> 

sincerely
Josua Mayer

Re: [PATCH RFC] net: sfp: support assigning status LEDs to SFP connectors

[Russell King (Oracle)]

On Wed, Jun 01, 2022 at 01:18:12PM +0300, Josua Mayer wrote:
> Hi Russell,
> 
> Thank you for the examples below!
> Stable names do help in some cases, but not in all.
> 
> I did some testing the other day with renaming network interfaces through
> the ip command:
> ip link set dev eth8 down
> ip link set dev eth12 down
> ip link set eth12 name eth20
> ip link set eth8 name eth12
> ip link set eth20 name eth8
> ip link set eth8 up
> ip link set dev eth12 up
> 
> Swapping interface names like this seems a perfectly legal thing for
> userspace to do. I personally would in such case expect the previous LED
> assignment to move along with the name change, however this does not happen.
> Instead after the interface rename, the LEDs are effectively swapped.

Someone may also take the view that this is exactly what should happen.
The requested configuration was to bind the trigger to a network
interface called "eth8" and if that interface goes away (through
whatever means) and then something else comes back as "eth8" then
"eth8" should be used.

The same is true of netfilter rules. If you specify a rule that
matches an interface called "eth8" then you get that match on that
name irrespective of network interface renames. The rule doesn't get
updated because you've changed the interface name. So in that regard,
the netdev LED trigger is operating just the same way as other parts
of the network subsystem.

> Further the netdev trigger implementation seems incorrect when you add
> network namespaces.
> Two namespaces can each contain a device named eth0, but the netdev trigger
> does not look at the namespace id when matching events to triggers, just the
> name.

That sounds like a bug - the netdev trigger will select the last
matching interface that it hears about when a netdev is registered or
its name is changed to something that matches what it's looking for.
So if you have multiple namespaces with identical names, the last
namespace with a matching interface name gets to control the LED.

However, I suspect the LED trigger folk may say "don't do that".

> Is it intended for userspace to track interface renames and reassign LEDs?
> Or should the trigger driver watch for name changes and adapt accordingly?
> 
> Finally I also noticed that the netdev trigger by default does not propagate
> any information to the LED.
> All properties - link, rx, tx are 0.
> Attempts at setting this by default through udev were widely unsuccessful:
> E.g. before setting the trigger property to netdev, the device_name or link
> properties do not exist.

The trigger specific properties will not exist until such time that
the trigger has been assigned - because each trigger is an entirely
separate chunk of code which might even be in a loadable module, and
each trigger driver is responsible for creating the properties that
it needs. I'm guessing this was part of the design of the LED
subsystem from the start, and it does make total sense.

> Therefore a rule that sets trigger and link and device at the same time does
> not function:
> SUBSYSTEM=="leds", ACTION=="add|change",
> ENV{OF_FULLNAME}=="/leds/led_c1_at", ATTR{trigger}="netdev", ATTR{link}="1",
> ATTR{device_name}="eth0"

I'm guessing that's because udev effectively sorts or randomises
the attributes, and gives no guarantees what order the attributes
are written. Sounds like we need udev people to comment on that
point.

> It appears necessary to use 2 rules, one that selects netdev, another one
> that chooses what property to show, e.g. link;
> and finally some rule that tracks the netdev name and updates device_name
> property accordingly.
> All while watching out for infinite loops because the property changes
> appear to trigger more change events, and e.g. setting trigger to netdev
> again causes another change event and resets the properties ...
> 
> I get the impression that this is very complex and might be described much
> better in device-tree, at least when a vendor makes explicit decisions to
> the purpose of each led.
> 
> Thee has been a recent patchset floating this list by Rafał Miłecki,
> which I very much liked:
> [PATCH RESEND PoC] leds: trigger: netdev: support DT "trigger-sources"
> property
> 
> It does allow declaring the relation from dpmac to led in dts as I would
> have expected.
> 
> In addition I believe there should be a way in dts to also set a default for
> what information to show, e.g.
> default-function = "link";
> 
> And finally dynamic tracking of the interface name.
> 
> I would be willing to work on the last two ideas, if this is an acceptable
> approach.

I think this is all something to bring up with those who look after
the netdev trigger.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!