| Message ID | 20230617115355.22868-1-ansuelsmth@gmail.com |
|---|---|
| Headers | show |
| Series | leds: trigger: netdev: add additional modes | expand |
On Sat, Jun 17, 2023 at 01:53:55PM +0200, Christian Marangi wrote: > Expose hw_control status via sysfs for the netdev trigger to give > userspace better understanding of the current state of the trigger and > the LED. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Sat, 17 Jun 2023, Christian Marangi wrote: > This is a continue of [1]. It was decided to take a more gradual > approach to implement LEDs support for switch and phy starting with > basic support and then implementing the hw control part when we have all > the prereq done. > > This should be the final part for the netdev trigger. > I added net-next tag and added netdev mailing list since I was informed > that this should be merged with netdev branch. > > We collect some info around and we found a good set of modes that are > common in almost all the PHY and Switch. > > These modes are: > - Modes for dedicated link speed(10, 100, 1000 mbps). Additional mode > can be added later following this example. > - Modes for half and full duplex. > > The original idea was to add hw control only modes. > While the concept makes sense in practice it would results in lots of > additional code and extra check to make sure we are setting correct modes. > > With the suggestion from Andrew it was pointed out that using the ethtool > APIs we can actually get the current link speed and duplex and this > effectively removed the problem of having hw control only modes since we > can fallback to software. > > Since these modes are supported by software, we can skip providing an > user for this in the LED driver to support hw control for these new modes > (that will come right after this is merged) and prevent this to be another > multi subsystem series. > > For link speed and duplex we use ethtool APIs. > > To call ethtool APIs, rtnl lock is needed but this can be skipped on > handling netdev events as the lock is already held. > > [1] https://lore.kernel.org/lkml/20230216013230.22978-1-ansuelsmth@gmail.com/ > > Changes v4: > - Add net-next tag > - Add additional patch to expose hw_control via sysfs > - CC netdev mailing list > Changes v3: > - Add Andrew review tag > - Use SPEED_UNKNOWN to init link_speed > - Fix using HALF_DUPLEX as duplex init and use DUPLEX_UNKNOWN instead > Changes v2: > - Drop ACTIVITY patch as it can be handled internally in the LED driver > - Reduce duplicate code and move the link state to a dedicated helper > > Christian Marangi (3): > leds: trigger: netdev: add additional specific link speed mode > leds: trigger: netdev: add additional specific link duplex mode > leds: trigger: netdev: expose hw_control status via sysfs > > drivers/leds/trigger/ledtrig-netdev.c | 114 +++++++++++++++++++++++--- > include/linux/leds.h | 5 ++ > 2 files changed, 109 insertions(+), 10 deletions(-) Seeing as we're on -rc7 already, any reason why we shouldn't hold off and simply apply these against LEDs once v6.5 is released?
> Seeing as we're on -rc7 already, any reason why we shouldn't hold off > and simply apply these against LEDs once v6.5 is released? Each subsystem has its own policies. netdev tends to accept patches right up until the merge window opens, sometimes even a couple of days into the merge window for low risk changes. Maybe this is because netdev is fast moving, two weeks of not merging results in a big backlog of patches, making it a bumpy restart once merging is started again. And is some of those late patches breaks something, there is still 7 weeks to fix it. Since this is cross subsystems i would expect both subsystems Maintainers to agree to a merge or not. If you want to be more conservative than netdev, wait until after the next merge window, please say so. If you do decided to wait, you are going to need to create another stable branch to pull into netdev. I know it is not a huge overhead, but it is still work, coordination etc. Andrew
On Sat, Jun 17, 2023 at 01:53:52PM +0200, Christian Marangi wrote: > This is a continue of [1]. It was decided to take a more gradual > approach to implement LEDs support for switch and phy starting with > basic support and then implementing the hw control part when we have all > the prereq done. > > This should be the final part for the netdev trigger. > I added net-next tag and added netdev mailing list since I was informed > that this should be merged with netdev branch. > > We collect some info around and we found a good set of modes that are > common in almost all the PHY and Switch. > > These modes are: > - Modes for dedicated link speed(10, 100, 1000 mbps). Additional mode > can be added later following this example. > - Modes for half and full duplex. > > The original idea was to add hw control only modes. > While the concept makes sense in practice it would results in lots of > additional code and extra check to make sure we are setting correct modes. > > With the suggestion from Andrew it was pointed out that using the ethtool > APIs we can actually get the current link speed and duplex and this > effectively removed the problem of having hw control only modes since we > can fallback to software. > > Since these modes are supported by software, we can skip providing an > user for this in the LED driver to support hw control for these new modes > (that will come right after this is merged) and prevent this to be another > multi subsystem series. > > For link speed and duplex we use ethtool APIs. > > To call ethtool APIs, rtnl lock is needed but this can be skipped on > handling netdev events as the lock is already held. > > [1] https://lore.kernel.org/lkml/20230216013230.22978-1-ansuelsmth@gmail.com/ Hi Christian, I am sorry if I am missing something obvious here, but this series does not appear to apply on top of net-next. Please consider rebasing and reposting. As you probably know, you can include the reviewed-by tags provided by Andrew for this posting, unless there are substantial changes.
On Mon, Jun 19, 2023 at 10:27:05PM +0200, Simon Horman wrote: > On Sat, Jun 17, 2023 at 01:53:52PM +0200, Christian Marangi wrote: > > This is a continue of [1]. It was decided to take a more gradual > > approach to implement LEDs support for switch and phy starting with > > basic support and then implementing the hw control part when we have all > > the prereq done. > > > > This should be the final part for the netdev trigger. > > I added net-next tag and added netdev mailing list since I was informed > > that this should be merged with netdev branch. > > > > We collect some info around and we found a good set of modes that are > > common in almost all the PHY and Switch. > > > > These modes are: > > - Modes for dedicated link speed(10, 100, 1000 mbps). Additional mode > > can be added later following this example. > > - Modes for half and full duplex. > > > > The original idea was to add hw control only modes. > > While the concept makes sense in practice it would results in lots of > > additional code and extra check to make sure we are setting correct modes. > > > > With the suggestion from Andrew it was pointed out that using the ethtool > > APIs we can actually get the current link speed and duplex and this > > effectively removed the problem of having hw control only modes since we > > can fallback to software. > > > > Since these modes are supported by software, we can skip providing an > > user for this in the LED driver to support hw control for these new modes > > (that will come right after this is merged) and prevent this to be another > > multi subsystem series. > > > > For link speed and duplex we use ethtool APIs. > > > > To call ethtool APIs, rtnl lock is needed but this can be skipped on > > handling netdev events as the lock is already held. > > > > [1] https://lore.kernel.org/lkml/20230216013230.22978-1-ansuelsmth@gmail.com/ > > Hi Christian, > > I am sorry if I am missing something obvious here, > but this series does not appear to apply on top of net-next. > > Please consider rebasing and reposting. > > As you probably know, you can include the reviewed-by tags > provided by Andrew for this posting, unless there are > substantial changes. > > -- > pw-bot: changes-requested > Hi, sorry for the mistake. I just sent v5 and added the additional Review-by tag.
> > If you do decided to wait, you are going to need to create another > > stable branch to pull into netdev. I know it is not a huge overhead, > > but it is still work, coordination etc. > > Can you clarify you last point for me please? This patchset extends the conditions on which the trigger blinks the LED. It adds a couple more values to enum led_trigger_netdev_modes in include/linux/leds.h. Once it gets merged, i will have a followup patch extending the Marvell PHY driver to make us of them. It will need these additional enum values. I also expect other PHY drivers to gain support for them. Probably the dp83867.c driver since Alexander Stein already has a patch merged adding support for what the current API supports. If we merge this patchset now via netdev, -rc1 should have everything we need for this continuing development work. If we wait to merge these patches until -rc1, only the LED tree has the needed patches, so these network drivers will need a stable branch we can pull into netdev. Both ways work, we can do either. But it is probably easier for everybody to merge now via netdev. Andrew
On Tue, 20 Jun 2023, Andrew Lunn wrote: > > > If you do decided to wait, you are going to need to create another > > > stable branch to pull into netdev. I know it is not a huge overhead, > > > but it is still work, coordination etc. > > > > Can you clarify you last point for me please? > > This patchset extends the conditions on which the trigger blinks the > LED. It adds a couple more values to enum led_trigger_netdev_modes in > include/linux/leds.h. Once it gets merged, i will have a followup > patch extending the Marvell PHY driver to make us of them. It will > need these additional enum values. I also expect other PHY drivers to > gain support for them. Probably the dp83867.c driver since Alexander > Stein already has a patch merged adding support for what the current > API supports. > > If we merge this patchset now via netdev, -rc1 should have everything > we need for this continuing development work. If we wait to merge > these patches until -rc1, only the LED tree has the needed patches, so > these network drivers will need a stable branch we can pull into > netdev. > > Both ways work, we can do either. But it is probably easier for > everybody to merge now via netdev. Got it, thanks.