| Message ID | 20240227093945.21525-3-bastien.curutchet@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: phy: Add TI's DP83640 device tree binding | expand |
On Tue, Feb 27, 2024 at 10:39:41AM +0100, Bastien Curutchet wrote: > Collisions on link does not fit into one of the existing netdev triggers. > > Add TRIGGER_NETDEV_COLLISION in the enum led_trigger_netdev_modes. > Add its definition in Documentation. > Add its handling in ledtrig-netdev, it can only be supported by hardware > so no software fallback is implemented. How useful is collision? How did you test this? How did you cause collisions to see if the LED actually worked? As far as i can see, this is just a normal 100Base-T PHY. Everybody uses that point-to-point nowadays. If it was an 100Base-T1, with a shared medium, good old CSMA/CD then collision might actually be useful. I also disagree with not having software fallback: ip -s link show eth0 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000 link/ether 80:ee:73:83:60:27 brd ff:ff:ff:ff:ff:ff RX: bytes packets errors dropped missed mcast 4382213540983 2947876747 0 0 0 154890 TX: bytes packets errors dropped carrier collsns 18742773651 197507119 0 0 0 0 collsns = 0. The information is there in a standard format. However, when did you last see it not 0? Andrew
On Tue, Feb 27, 2024 at 05:03:14PM +0100, Andrew Lunn wrote: > On Tue, Feb 27, 2024 at 10:39:41AM +0100, Bastien Curutchet wrote: > > Collisions on link does not fit into one of the existing netdev triggers. > > > > Add TRIGGER_NETDEV_COLLISION in the enum led_trigger_netdev_modes. > > Add its definition in Documentation. > > Add its handling in ledtrig-netdev, it can only be supported by hardware > > so no software fallback is implemented. > > How useful is collision? How did you test this? How did you cause > collisions to see if the LED actually worked? > > As far as i can see, this is just a normal 100Base-T PHY. Everybody > uses that point-to-point nowadays. That's largely irrelevant when it comes to collisions. If the link has negotiated half-duplex mode (which we still support) then even on twisted pair, there can be collisions, even though TX and RX are using separate pairs. It's a quirk of 802.3 that this is the case.
Hi Andrew, On 2/27/24 17:03, Andrew Lunn wrote: > On Tue, Feb 27, 2024 at 10:39:41AM +0100, Bastien Curutchet wrote: >> Collisions on link does not fit into one of the existing netdev triggers. >> >> Add TRIGGER_NETDEV_COLLISION in the enum led_trigger_netdev_modes. >> Add its definition in Documentation. >> Add its handling in ledtrig-netdev, it can only be supported by hardware >> so no software fallback is implemented. > How useful is collision? How did you test this? How did you cause > collisions to see if the LED actually worked? Indeed I am not able to generate collision on my setup so I did not test this collision part. My use case is that the hardware strap configuration that selects the LED output mode can not be trusted so I have to force configuration with software. I added this collision part because I wanted to cover all the LED configuration modes offered by the PHY. > As far as i can see, this is just a normal 100Base-T PHY. Everybody > uses that point-to-point nowadays. If it was an 100Base-T1, with a > shared medium, good old CSMA/CD then collision might actually be > useful. > > I also disagree with not having software fallback: > > ip -s link show eth0 > 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000 > link/ether 80:ee:73:83:60:27 brd ff:ff:ff:ff:ff:ff > RX: bytes packets errors dropped missed mcast > 4382213540983 2947876747 0 0 0 154890 > TX: bytes packets errors dropped carrier collsns > 18742773651 197507119 0 0 0 0 > > collsns = 0. The information is there in a standard format. However, > when did you last see it not 0? Ok, I could add the software callback but I will not be able to test it ... Best regards, Bastien
> > How useful is collision? How did you test this? How did you cause > > collisions to see if the LED actually worked? > Indeed I am not able to generate collision on my setup so I did not test > this > collision part. > My use case is that the hardware strap configuration that selects the LED > output mode > can not be trusted so I have to force configuration with software. I added > this collision > part because I wanted to cover all the LED configuration modes offered by > the PHY. There are a few things i want to avoid here: 1) Vendor SDK mentality. The hardware can do this, lets add a knob to make use of it. We end up with 100 of configuration knobs which nobody ever uses. Do you actually have a board where the strapping is wrong? Are you going to submit a .dts file making use of this option? 2) LEDs are the wild west, because it is not part of 802.3. Every vendor does it differently, and has their own special blinking patterns. My preference is to keep it simple to what people actually use. You cannot actually generate a collision, the developer who wants to add support for collision. I have to ask, is collision actually useful? > > As far as i can see, this is just a normal 100Base-T PHY. Everybody > > uses that point-to-point nowadays. If it was an 100Base-T1, with a > > shared medium, good old CSMA/CD then collision might actually be > > useful. > > > > I also disagree with not having software fallback: > > > > ip -s link show eth0 > > 2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000 > > link/ether 80:ee:73:83:60:27 brd ff:ff:ff:ff:ff:ff > > RX: bytes packets errors dropped missed mcast > > 4382213540983 2947876747 0 0 0 154890 > > TX: bytes packets errors dropped carrier collsns > > 18742773651 197507119 0 0 0 0 > > > > collsns = 0. The information is there in a standard format. However, > > when did you last see it not 0? > > Ok, I could add the software callback but I will not be able to test it ... My personal experience is, anything not tested is broken... Think about what Russell actually said. That should give you a clue how to cause collisions. If not, go study history books about CSMA/CD. Andrew
On Thu, Feb 29, 2024 at 04:17:47PM +0100, Andrew Lunn wrote: > 2) LEDs are the wild west, because it is not part of 802.3. Every > vendor does it differently, and has their own special blinking > patterns. My preference is to keep it simple to what people actually > use. You cannot actually generate a collision, the developer who wants > to add support for collision. I have to ask, is collision actually > useful? I'm not sure when you say "You cannot actually generate a collision" whether you're referring to the link or the LEDs here? Obviously, we can't detect a collision unless the hardware tells us that it happened.
diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-netdev b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev index a6c307c4befa..fbb2bc1d6108 100644 --- a/Documentation/ABI/testing/sysfs-class-led-trigger-netdev +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-netdev @@ -178,3 +178,14 @@ Description: If set to 1, the LED's normal state reflects the link full duplex state of the named network device. Setting this value also immediately changes the LED state. + +What: /sys/class/leds/<led>/collision +Date: Feb 2024 +KernelVersion: 6.8 +Contact: linux-leds@vger.kernel.org +Description: + Signal collision of the named network device. + + If set to 0 (default), the LED's normal state is off. + + If set to 1, the LED's normal state reflects collisions. diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c index 8e5475819590..5c17b8e27d5c 100644 --- a/drivers/leds/trigger/ledtrig-netdev.c +++ b/drivers/leds/trigger/ledtrig-netdev.c @@ -318,6 +318,7 @@ static ssize_t netdev_led_attr_show(struct device *dev, char *buf, case TRIGGER_NETDEV_FULL_DUPLEX: case TRIGGER_NETDEV_TX: case TRIGGER_NETDEV_RX: + case TRIGGER_NETDEV_COLLISION: bit = attr; break; default: @@ -352,6 +353,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf, case TRIGGER_NETDEV_FULL_DUPLEX: case TRIGGER_NETDEV_TX: case TRIGGER_NETDEV_RX: + case TRIGGER_NETDEV_COLLISION: bit = attr; break; default: @@ -410,6 +412,7 @@ DEFINE_NETDEV_TRIGGER(half_duplex, TRIGGER_NETDEV_HALF_DUPLEX); DEFINE_NETDEV_TRIGGER(full_duplex, TRIGGER_NETDEV_FULL_DUPLEX); DEFINE_NETDEV_TRIGGER(tx, TRIGGER_NETDEV_TX); DEFINE_NETDEV_TRIGGER(rx, TRIGGER_NETDEV_RX); +DEFINE_NETDEV_TRIGGER(collision, TRIGGER_NETDEV_COLLISION); static ssize_t interval_show(struct device *dev, struct device_attribute *attr, char *buf) @@ -473,6 +476,7 @@ static struct attribute *netdev_trig_attrs[] = { &dev_attr_tx.attr, &dev_attr_interval.attr, &dev_attr_offloaded.attr, + &dev_attr_collision.attr, NULL }; ATTRIBUTE_GROUPS(netdev_trig); diff --git a/include/linux/leds.h b/include/linux/leds.h index 4754b02d3a2c..8864d6ce8185 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -578,6 +578,7 @@ enum led_trigger_netdev_modes { TRIGGER_NETDEV_FULL_DUPLEX, TRIGGER_NETDEV_TX, TRIGGER_NETDEV_RX, + TRIGGER_NETDEV_COLLISION, /* Keep last */ __TRIGGER_NETDEV_MAX,
Collisions on link does not fit into one of the existing netdev triggers. Add TRIGGER_NETDEV_COLLISION in the enum led_trigger_netdev_modes. Add its definition in Documentation. Add its handling in ledtrig-netdev, it can only be supported by hardware so no software fallback is implemented. Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com> --- .../ABI/testing/sysfs-class-led-trigger-netdev | 11 +++++++++++ drivers/leds/trigger/ledtrig-netdev.c | 4 ++++ include/linux/leds.h | 1 + 3 files changed, 16 insertions(+)