| Message ID | 20200728150530.28827-1-marek.behun@nic.cz |
|---|---|
| Headers | show |
| Series | Add support for LEDs on Marvell PHYs | expand |
On Tue, 28 Jul 2020 17:05:28 +0200 Marek Behún wrote:
> this is v4 of my RFC adding support for LEDs connected to Marvell PHYs.
FWIW a heads up for when you post a non-RFC version - neither patch
builds on allmodconfig right now.
Hi! > this is v4 of my RFC adding support for LEDs connected to Marvell PHYs. > > Please note that if you want to test this, you still need to first apply > the patch adding the LED private triggers support from Pavel's tree. > https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=93690cdf3060c61dfce813121d0bfc055e7fa30d > > What I still don't like about this is that the LEDs created by the code > don't properly support device names. LEDs should have name in format > "device:color:function", for example "eth0:green:activity". > > The code currently looks for attached netdev for a given PHY, but > at the time this happens there is no netdev attached, so the LEDs gets > names without the device part (ie ":green:activity"). > > This can be addressed in next version by renaming the LED when a netdev > is attached to the PHY, but first a API for LED device renaming needs to > be proposed. I am going to try to do that. This would also solve the > same problem when userspace renames an interface. > > And no, I don't want phydev name there. Ummm. Can we get little more explanation on that? I fear that LED device renaming will be tricky and phydev would work around that nicely. Best regards, Pavel
> > And no, I don't want phydev name there. > > Ummm. Can we get little more explanation on that? I fear that LED > device renaming will be tricky and phydev would work around that > nicely. Hi Pavel The phydev name is not particularly nice: !mdio-mux!mdio@1!switch@0!mdio:00 !mdio-mux!mdio@1!switch@0!mdio:01 !mdio-mux!mdio@1!switch@0!mdio:02 !mdio-mux!mdio@2!switch@0!mdio:00 !mdio-mux!mdio@2!switch@0!mdio:01 !mdio-mux!mdio@2!switch@0!mdio:02 !mdio-mux!mdio@4!switch@0!mdio:00 !mdio-mux!mdio@4!switch@0!mdio:01 !mdio-mux!mdio@4!switch@0!mdio:02 400d0000.ethernet-1:00 400d0000.ethernet-1:01 fixed-0:00 The interface name are: 1: lo: 2: eth0: 3: eth1: 4: lan0@eth1: 5: lan1@eth1: 6: lan2@eth1: 7: lan3@eth1: 8: lan4@eth1: 9: lan5@eth1: 10: lan6@eth1: 11: lan7@eth1: 12: lan8@eth1: 13: optical3@eth1: 14: optical4@eth1: You could make a good guess at matching to two together, but it is error prone. Phys are low level things which the user is not really involved in. They interact with interface names. ethtool, ip, etc, all use interface names. In fact, i don't know of any tool which uses phydev names. Andrew
On Tue, 2020-07-28 at 17:05 +0200, Marek Behún wrote: > Hi, > > this is v4 of my RFC adding support for LEDs connected to Marvell > PHYs. > > Please note that if you want to test this, you still need to first > apply > the patch adding the LED private triggers support from Pavel's tree. > https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=93690cdf3060c61dfce813121d0bfc055e7fa30d > > What I still don't like about this is that the LEDs created by the > code > don't properly support device names. LEDs should have name in format > "device:color:function", for example "eth0:green:activity". > > The code currently looks for attached netdev for a given PHY, but > at the time this happens there is no netdev attached, so the LEDs > gets > names without the device part (ie ":green:activity"). > > This can be addressed in next version by renaming the LED when a > netdev > is attached to the PHY, but first a API for LED device renaming needs > to > be proposed. I am going to try to do that. This would also solve the > same problem when userspace renames an interface. > > And no, I don't want phydev name there. Hello Marek, thanks for your patches - Andrew suggested me to have a look at them as I'm currently trying to add LED trigger support to the TI DP83867 PHY. Is there already a plan to add support for polarity and similiar settings, at least to the generic part of your changes? In the TI DP83867, there are 2 separate settings for each LED: - Trigger event - Polarity or override (active-high/active-low/force-high/force-low - the latter two would be used for led_brightness_set) - (There is also a 3rd register that defines the blink frequency, but as it allows only a single setting for all LEDs, I would ignore it for now) At least the per-LED polarity setting would be essential to have for this feature to be useful for our TQ-Systems mainboards with TI PHYs. Kind regards, Matthias > > Changes since v3: > - addressed some of Andrew's suggestions > - phy_hw_led_mode.c renamed to phy_led.c > - the DT reading code is now also generic, moved to phy_led.c and > called > from phy_probe > - the function registering the phydev-hw-mode trigger is now called > from > phy_device.c function phy_init before registering genphy drivers > - PHY LED functionality now depends on CONFIG_LEDS_TRIGGERS > > Changes since v2: > - to share code with other drivers which may want to also offer PHY > HW > control of LEDs some of the code was refactored and now resides in > phy_hw_led_mode.c. This code is compiled in when config option > LED_TRIGGER_PHY_HW is enabled. Drivers wanting to offer PHY HW > control > of LEDs should depend on this option. > - the "hw-control" trigger is renamed to "phydev-hw-mode" and is > registered by the code in phy_hw_led_mode.c > - the "hw_control" sysfs file is renamed to "hw_mode" > - struct phy_driver is extended by three methods to support PHY HW > LED > control > - I renamed the various HW control modes offeret by Marvell PHYs to > conform to other Linux mode names, for example the > "1000/100/10/else" > mode was renamed to "1Gbps/100Mbps/10Mbps", or "recv/else" was > renamed > to "rx" (this is the name of the mode in netdev trigger). > > Marek > > > Marek Behún (2): > net: phy: add API for LEDs controlled by PHY HW > net: phy: marvell: add support for PHY LEDs via LED class > > drivers/net/phy/Kconfig | 4 + > drivers/net/phy/Makefile | 1 + > drivers/net/phy/marvell.c | 287 > +++++++++++++++++++++++++++++++++++ > drivers/net/phy/phy_device.c | 25 ++- > drivers/net/phy/phy_led.c | 176 +++++++++++++++++++++ > include/linux/phy.h | 51 +++++++ > 6 files changed, 537 insertions(+), 7 deletions(-) > create mode 100644 drivers/net/phy/phy_led.c >
Hi! > > > And no, I don't want phydev name there. > > > > Ummm. Can we get little more explanation on that? I fear that LED > > device renaming will be tricky and phydev would work around that > > nicely. > > Hi Pavel > > The phydev name is not particularly nice: > > !mdio-mux!mdio@1!switch@0!mdio:00 > !mdio-mux!mdio@1!switch@0!mdio:01 > !mdio-mux!mdio@1!switch@0!mdio:02 > !mdio-mux!mdio@2!switch@0!mdio:00 > !mdio-mux!mdio@2!switch@0!mdio:01 > !mdio-mux!mdio@2!switch@0!mdio:02 > !mdio-mux!mdio@4!switch@0!mdio:00 > !mdio-mux!mdio@4!switch@0!mdio:01 > !mdio-mux!mdio@4!switch@0!mdio:02 > 400d0000.ethernet-1:00 > 400d0000.ethernet-1:01 > fixed-0:00 Not nice, I see. In particular, it contains ":"... which would be a problem. > The interface name are: > > 1: lo: > 2: eth0: > 3: eth1: > 4: lan0@eth1: > 5: lan1@eth1: > 6: lan2@eth1: > 7: lan3@eth1: > 8: lan4@eth1: > 9: lan5@eth1: > 10: lan6@eth1: > 11: lan7@eth1: > 12: lan8@eth1: > 13: optical3@eth1: > 14: optical4@eth1: OTOH... renaming LEDs when interface is renamed... sounds like a disaster, too. > You could make a good guess at matching to two together, but it is > error prone. Phys are low level things which the user is not really > involved in. They interact with interface names. ethtool, ip, etc, all > use interface names. In fact, i don't know of any tool which uses > phydev names. So... proposal: Users should not be dealing with sysfs interface directly, anyway. We should have a tool for that. It can live in kernel/tools somewhere, I guess. Would we name leds phy0:... (with simple incrementing number), and expose either interface name or phydev name as a attribute? So user could do cat /sys/class/leds/phy14:green:foobar/netdev lan5@eth1: and we'd have tool hiding that complexity... Best regards, Pavel
On Sun, Aug 30, 2020 at 12:43:51AM +0200, Pavel Machek wrote: > Hi! > > > > > And no, I don't want phydev name there. > > > > > > Ummm. Can we get little more explanation on that? I fear that LED > > > device renaming will be tricky and phydev would work around that > > > nicely. > > > > Hi Pavel > > > > The phydev name is not particularly nice: > > > > !mdio-mux!mdio@1!switch@0!mdio:00 > > !mdio-mux!mdio@1!switch@0!mdio:01 > > !mdio-mux!mdio@1!switch@0!mdio:02 > > !mdio-mux!mdio@2!switch@0!mdio:00 > > !mdio-mux!mdio@2!switch@0!mdio:01 > > !mdio-mux!mdio@2!switch@0!mdio:02 > > !mdio-mux!mdio@4!switch@0!mdio:00 > > !mdio-mux!mdio@4!switch@0!mdio:01 > > !mdio-mux!mdio@4!switch@0!mdio:02 > > 400d0000.ethernet-1:00 > > 400d0000.ethernet-1:01 > > fixed-0:00 > > Not nice, I see. In particular, it contains ":"... which would be a > problem. > > > The interface name are: > > > > 1: lo: > > 2: eth0: > > 3: eth1: > > 4: lan0@eth1: > > 5: lan1@eth1: > > 6: lan2@eth1: > > 7: lan3@eth1: > > 8: lan4@eth1: > > 9: lan5@eth1: > > 10: lan6@eth1: > > 11: lan7@eth1: > > 12: lan8@eth1: > > 13: optical3@eth1: > > 14: optical4@eth1: > > OTOH... renaming LEDs when interface is renamed... sounds like a > disaster, too. I don't think it is. The stack has all the needed support. There is a notification before the rename, and another notification after the rename. Things like bonding, combing two interfaces into one and load balancing, etc. hook these notifiers. There is plenty of examples to follow. What i don't know about is the lifetime of files under /sys/class/led, does the destroying of an LED block while one of the files is open?. > > You could make a good guess at matching to two together, but it is > > error prone. Phys are low level things which the user is not really > > involved in. They interact with interface names. ethtool, ip, etc, all > > use interface names. In fact, i don't know of any tool which uses > > phydev names. > > So... proposal: > > Users should not be dealing with sysfs interface directly, anyway. We > should have a tool for that. It can live in kernel/tools somewhere, I > guess. We already have one, ethtool(1). > > Would we name leds phy0:... (with simple incrementing number), and > expose either interface name or phydev name as a attribute? > > So user could do > > cat /sys/class/leds/phy14:green:foobar/netdev > lan5@eth1: Which is the wrong way around. ethtool will be passed the interface name and an PHY descriptor of some sort, and it has to go search through all the LEDs to find the one with this attribute. I would be much more likely to add a sysfs link from /sys/class/net/lan5/phy:left:green to /sys/class/leds/phy14:left:green. Andrew
> > > You could make a good guess at matching to two together, but it is > > > error prone. Phys are low level things which the user is not really > > > involved in. They interact with interface names. ethtool, ip, etc, all > > > use interface names. In fact, i don't know of any tool which uses > > > phydev names. > > > > So... proposal: > > > > Users should not be dealing with sysfs interface directly, anyway. We > > should have a tool for that. It can live in kernel/tools somewhere, I > > guess. > > We already have one, ethtool(1). > > > > > Would we name leds phy0:... (with simple incrementing number), and > > expose either interface name or phydev name as a attribute? > > > > So user could do > > > > cat /sys/class/leds/phy14:green:foobar/netdev > > lan5@eth1: I forgot about network name spaces. There can be multiple interfaces with the name eth0, each in its own network namespace. For your proposal to work, /sys/class/leds/phy14:green:foobar needs to be in the network namespace, so it is only visible to other processes in the same name space, and lan5@eth1 is then unique to that namespace. > Which is the wrong way around. ethtool will be passed the interface > name and an PHY descriptor of some sort, and it has to go search > through all the LEDs to find the one with this attribute. I would be > much more likely to add a sysfs link from > /sys/class/net/lan5/phy:left:green to > /sys/class/leds/phy14:left:green. I need to test a bit, but i think this works. Everything under /sys/class/net is network namespace aware. You only see /sys/class/net/lan5 if there is a lan5 is in the current name space, and you see the current namespaces version of lan5.. A sysfs symlink out of namespace to /sys/class/led should work, assuming /sys/class/led is namespace unaware, and phy14 is unique across all network name spaces. But you cannot have a link in the opposite direction from /sys/class/led/phy14 to /sys/class/net/lan5, since it has no idea which lan5 to symlink to. Andrew
Hi! > > > The phydev name is not particularly nice: > > > > > > !mdio-mux!mdio@1!switch@0!mdio:00 ... > > > 400d0000.ethernet-1:00 > > > 400d0000.ethernet-1:01 > > > fixed-0:00 > > > > Not nice, I see. In particular, it contains ":"... which would be a > > problem. > > > > > The interface name are: > > > > > > 1: lo: > > > 2: eth0: > > > 3: eth1: ... > > > 13: optical3@eth1: > > > 14: optical4@eth1: > > > > OTOH... renaming LEDs when interface is renamed... sounds like a > > disaster, too. > > I don't think it is. The stack has all the needed support. There is a > notification before the rename, and another notification after the > rename. Things like bonding, combing two interfaces into one and load > balancing, etc. hook these notifiers. There is plenty of examples to > follow. What i don't know about is the lifetime of files under > /sys/class/led, does the destroying of an LED block while one of the > files is open?. Well, there may be no problems on the networking side, but I'd prefer not to make LED side more complex. Files could be open, and userland could have assumptions about LEDs not changing names... > > > You could make a good guess at matching to two together, but it is > > > error prone. Phys are low level things which the user is not really > > > involved in. They interact with interface names. ethtool, ip, etc, all > > > use interface names. In fact, i don't know of any tool which uses > > > phydev names. > > > > So... proposal: > > > > Users should not be dealing with sysfs interface directly, anyway. We > > should have a tool for that. It can live in kernel/tools somewhere, I > > guess. > > We already have one, ethtool(1). Well... ethtool is for networking, we'll want to have a ledtool, too :-). > > Would we name leds phy0:... (with simple incrementing number), and > > expose either interface name or phydev name as a attribute? > > > > So user could do > > > > cat /sys/class/leds/phy14:green:foobar/netdev > > lan5@eth1: > > Which is the wrong way around. ethtool will be passed the interface > name and an PHY descriptor of some sort, and it has to go search > through all the LEDs to find the one with this attribute. I would be > much more likely to add a sysfs link from > /sys/class/net/lan5/phy:left:green to > /sys/class/leds/phy14:left:green. Okay, that might be even better, as it provides links in the more useful direction. Best regards, Pavel
On Tue, 25 Aug 2020 10:13:59 +0200 Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote: > On Tue, 2020-07-28 at 17:05 +0200, Marek Behún wrote: > > Hi, > > > > this is v4 of my RFC adding support for LEDs connected to Marvell > > PHYs. > > > > Please note that if you want to test this, you still need to first > > apply > > the patch adding the LED private triggers support from Pavel's tree. > > > https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=93690cdf3060c61dfce813121d0bfc055e7fa30d > > > > What I still don't like about this is that the LEDs created by the > > code > > don't properly support device names. LEDs should have name in format > > "device:color:function", for example "eth0:green:activity". > > > > The code currently looks for attached netdev for a given PHY, but > > at the time this happens there is no netdev attached, so the LEDs > > gets > > names without the device part (ie ":green:activity"). > > > > This can be addressed in next version by renaming the LED when a > > netdev > > is attached to the PHY, but first a API for LED device renaming needs > > to > > be proposed. I am going to try to do that. This would also solve the > > same problem when userspace renames an interface. > > > > And no, I don't want phydev name there. > > > Hello Marek, > > thanks for your patches - Andrew suggested me to have a look at them as > I'm currently trying to add LED trigger support to the TI DP83867 PHY. > > Is there already a plan to add support for polarity and similiar > settings, at least to the generic part of your changes? > Hello Matthias, sorry for answering with delay, I somehow overlooked your email. Yes, I plan to add some basic platform data properties (like polarity) in the generic part. > In the TI DP83867, there are 2 separate settings for each LED: > > - Trigger event > - Polarity or override (active-high/active-low/force-high/force-low - > the latter two would be used for led_brightness_set) > - (There is also a 3rd register that defines the blink frequency, but > as it allows only a single setting for all LEDs, I would ignore it for > now) I think that blink frequency should be in the generic part as well. > > At least the per-LED polarity setting would be essential to have for > this feature to be useful for our TQ-Systems mainboards with TI PHYs. > I will try to send new version next week (starting 7th September). Marek > > Kind regards, > Matthias > > > > > > > Changes since v3: > > - addressed some of Andrew's suggestions > > - phy_hw_led_mode.c renamed to phy_led.c > > - the DT reading code is now also generic, moved to phy_led.c and > > called > > from phy_probe > > - the function registering the phydev-hw-mode trigger is now called > > from > > phy_device.c function phy_init before registering genphy drivers > > - PHY LED functionality now depends on CONFIG_LEDS_TRIGGERS > > > > Changes since v2: > > - to share code with other drivers which may want to also offer PHY > > HW > > control of LEDs some of the code was refactored and now resides in > > phy_hw_led_mode.c. This code is compiled in when config option > > LED_TRIGGER_PHY_HW is enabled. Drivers wanting to offer PHY HW > > control > > of LEDs should depend on this option. > > - the "hw-control" trigger is renamed to "phydev-hw-mode" and is > > registered by the code in phy_hw_led_mode.c > > - the "hw_control" sysfs file is renamed to "hw_mode" > > - struct phy_driver is extended by three methods to support PHY HW > > LED > > control > > - I renamed the various HW control modes offeret by Marvell PHYs to > > conform to other Linux mode names, for example the > > "1000/100/10/else" > > mode was renamed to "1Gbps/100Mbps/10Mbps", or "recv/else" was > > renamed > > to "rx" (this is the name of the mode in netdev trigger). > > > > Marek > > > > > > Marek Behún (2): > > net: phy: add API for LEDs controlled by PHY HW > > net: phy: marvell: add support for PHY LEDs via LED class > > > > drivers/net/phy/Kconfig | 4 + > > drivers/net/phy/Makefile | 1 + > > drivers/net/phy/marvell.c | 287 > > +++++++++++++++++++++++++++++++++++ > > drivers/net/phy/phy_device.c | 25 ++- > > drivers/net/phy/phy_led.c | 176 +++++++++++++++++++++ > > include/linux/phy.h | 51 +++++++ > > 6 files changed, 537 insertions(+), 7 deletions(-) > > create mode 100644 drivers/net/phy/phy_led.c > > >