[net-next,v4,0/3] leds: trigger: netdev: add additional modes

  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(-)
  

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.
  

On Mon, 19 Jun 2023, Andrew Lunn wrote:

> > 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.

Can you clarify you last point for me please?
  

> > 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.