netdev - Re: [PATCH RFC net-next 0/8] DSA LED infrastructure, mv88e6xxx and QCA8K

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]

Date: Wed, 29 Nov 2023 14:38:19 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev <netdev@...r.kernel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Christian Marangi <ansuelsmth@...il.com>,
	Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH RFC net-next 0/8] DSA LED infrastructure, mv88e6xxx and
 QCA8K

Hi Andrew,

On Wed, Nov 29, 2023 at 12:21:27AM +0100, Andrew Lunn wrote:
> This patchset extends the DSA core to add support for port LEDs being
> controlled via sys/class/leds, and offloading blinking via
> ledtrig-netdev. The core parses the device tree binding, and registers
> LEDs. The DSA switch ops structure is extended with the needed
> functions.
> 
> The mv88e6xxx support is partially added. Support for setting the
> brightness and blinking is provided, but offloading of blinking is not
> yet available. To demonstrate this, the wrt1900ac device tree is
> extended with LEDs.
> 
> The existing QCA8K code is refactored to make use of this shared code.
> 
> RFC:
> 
> Linus, can you rework your code into this for offloading blinking ?
> And test with ports 5 & 6.
> 
> Christian: Please test QCA8K. I would not be surprised if there is an
> off-by-one.
> 
> This code can also be found in
> 
> https://github.com/lunn/ v6.7-rc2-net-next-mv88e6xxx-leds

I am disappointed to see the dsa_switch_ops API polluted with odds and
ends which have nothing to do with Ethernet-connected Ethernet switches
(DSA's focus).

Looking at the code, I don't see why dsa_port_leds_setup() cannot be
rebranded as library code usable by any netdev driver and which bypasses DSA.
Individual DSA switch drivers could call it directly while providing
their struct device for the port, and a smaller ops structure for the
cdev. But more importantly, other non-DSA drivers could do the same.

I think it comes as no surprise that driver authors prefer using the DSA
API as their first choice even for technically non-DSA switches, seeing
how we tend to cram all sorts of unrelated stuff into the monolithic
struct dsa_switch_ops, and how that makes the API attractive. But then
we push them away from DSA for valid reasons, and they end up copying
its support code word for word.

Maybe this sounds a bit harsh, but NACK from me for the approach.