netdev - Re: [PATCH net-next v3 0/4] Support offload LED blinking to PHY.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZNO6hfjrJthoIUi9@makrotopia.org>
Date: Wed, 9 Aug 2023 17:10:45 +0100
From: Daniel Golle <daniel@...rotopia.org>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev <netdev@...r.kernel.org>, Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <rmk+kernel@...linux.org.uk>,
	Simon Horman <simon.horman@...igine.com>,
	Christian Marangi <ansuelsmth@...il.com>
Subject: Re: [PATCH net-next v3 0/4] Support offload LED blinking to PHY.

Hi Andrew,

On Tue, Aug 08, 2023 at 11:04:32PM +0200, Andrew Lunn wrote:
> Allow offloading of the LED trigger netdev to PHY drivers and
> implement it for the Marvell PHY driver. Additionally, correct the
> handling of when the initial state of the LED cannot be represented by
> the trigger, and so an error is returned. As with ledtrig-timer,
> disable offload when the trigger is deactivate, or replaced by another
> trigger.

I've tested the series and changed my driver accordingly, now
deactivation of the offloaded tasks works fine when changing
the trigger.

Overall I believe this is good to go, however, what remains
unresolved is the chicken-egg when assigning the 'netdev' trigger
using linux,default-trigger in device tree: In this case selection of
the netdev trigger happens on creation of the PHY instance which is
*before* the creation of the netdev the PHY is going to be attached to.
Hence 'netdev' gets activated *without* any hardware offloading.

And while reading the current hardware state (as left behind by IC
defaults or by the bootloader) works fine, by default the LEDs would
show trigger 'none' in Linux right after boot (despite e.g. link and
traffic indications are active by default -- which is would I tried to
express by using linux,default-trigger...)

Tested-by: Daniel Golle <daniel@...rotopia.org>

> 
> Since v2:
> Add support for link speeds, not just link
> Add missing checks for return values
> Add patch disabling offload when driver is deactivated
> 
> Since v1:
> 
> Add true kerneldoc for the new entries in struct phy_driver
> Add received Reviewed-by: tags
> 
> Since v0:
> 
> Make comments in struct phy_driver look more like kerneldoc
> Add cover letter
> 
> Andrew Lunn (4):
>   led: trig: netdev: Fix requesting offload device
>   net: phy: phy_device: Call into the PHY driver to set LED offload
>   net: phy: marvell: Add support for offloading LED blinking
>   leds: trig-netdev: Disable offload on deactivation of trigger
> 
>  drivers/leds/trigger/ledtrig-netdev.c |  10 +-
>  drivers/net/phy/marvell.c             | 281 ++++++++++++++++++++++++++
>  drivers/net/phy/phy_device.c          |  68 +++++++
>  include/linux/phy.h                   |  33 +++
>  4 files changed, 389 insertions(+), 3 deletions(-)
> 
> -- 
> 2.40.1
>