mbox series

[leds,v1,0/5] Add support for offloading netdev trigger to HW

Message ID 20210526180020.13557-1-kabel@kernel.org
Headers show
Series Add support for offloading netdev trigger to HW | expand

Message

Marek Behún May 26, 2021, 6 p.m. UTC
Hello,

I am sending the first non-RFC version of the series adding support
for offloading LED triggers to HW, with the netdev trigger being the
first user.

I have addressed the issues with the RFC version from October.

This version only implements the offloading API and adds support for
offloading to the netdev trigger. In the RFC I also added one user of
this API (Marvell ethernet PHY driver). I plan to continue the work on
those patches once the offloading API is finally merged.

Also, there is now other possible user for this API: leds-nuc driver
by Mauro.

Changes since RFC:
- split the patch adding HW offloading support to netdev trigger into
  several separate patches (suggested by Pavel):
  1. move trigger data structure to include/linux/ledtrig.h
  2. support HW offloading
  3. change spinlock to mutex
- fixed bug where the .offloaded variable was not set to false when
  offloading was disabled (suggested by Pavel)
- removed the code saving one call to set_baseline_state() on the
  NETDEV_CHANGE event. It is not needed, the trigger_offload() method
  can handle this situation on its own (suggested by Pavel)
- documentation now explicitly says that when offloading is being
  disabled, the function must return 0 (no error) (suggested by Pavel)

Marek Behún (5):
  leds: trigger: netdev: don't explicitly zero kzalloced data
  leds: trigger: add API for HW offloading of triggers
  leds: trigger: netdev: move trigger data structure to global include
    dir
  leds: trigger: netdev: support HW offloading
  leds: trigger: netdev: change spinlock to mutex

 Documentation/leds/leds-class.rst     | 22 +++++++++++++
 drivers/leds/led-triggers.c           |  1 +
 drivers/leds/trigger/ledtrig-netdev.c | 47 ++++++++-------------------
 include/linux/leds.h                  | 29 +++++++++++++++++
 include/linux/ledtrig.h               | 40 +++++++++++++++++++++++
 5 files changed, 105 insertions(+), 34 deletions(-)
 create mode 100644 include/linux/ledtrig.h

Comments

Andrew Lunn May 27, 2021, 4:38 p.m. UTC | #1
On Wed, May 26, 2021 at 08:00:16PM +0200, Marek Behún wrote:
> The trigger_data struct is allocated with kzalloc, so we do not need to
> explicitly set members to zero.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> Acked-by: Pavel Machek <pavel@ucw.cz>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew
Andrew Lunn May 27, 2021, 4:48 p.m. UTC | #2
On Wed, May 26, 2021 at 08:00:18PM +0200, Marek Behún wrote:
> In preparation for HW offloading of netdev trigger, move struct
> led_trigger_data into global include directory, into file
> linux/ledtrig.h, so that drivers wanting to offload the trigger can see
> the requested settings.
> 
> Signed-off-by: Marek Behún <kabel@kernel.org>
> ---
>  drivers/leds/trigger/ledtrig-netdev.c | 23 +---------------
>  include/linux/ledtrig.h               | 38 +++++++++++++++++++++++++++

I'm wondering how this is going to scale, if we have a lot of triggers
which can be offloaded. Rather than try to pack them all into
one header, would it make more sense to add

include/linux/led/ledtrig-netdev.h

	Andrew
Marek Behún May 28, 2021, 6:28 a.m. UTC | #3
On Thu, 27 May 2021 18:48:21 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Wed, May 26, 2021 at 08:00:18PM +0200, Marek Behún wrote:
> > In preparation for HW offloading of netdev trigger, move struct
> > led_trigger_data into global include directory, into file
> > linux/ledtrig.h, so that drivers wanting to offload the trigger can
> > see the requested settings.
> > 
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/leds/trigger/ledtrig-netdev.c | 23 +---------------
> >  include/linux/ledtrig.h               | 38
> > +++++++++++++++++++++++++++  
> 
> I'm wondering how this is going to scale, if we have a lot of triggers
> which can be offloaded. Rather than try to pack them all into
> one header, would it make more sense to add
> 
> include/linux/led/ledtrig-netdev.h
> 
> 	Andrew

Hmm, I guess you are right. Also when looking at a LED controller
driver we could immediately see which triggers this driver can offload,
when looking at headers included.