netdev - Re: [PATCH net-next v2 6/7] dsa: mv88e6xxx: Create port/netdev LEDs

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] [day] [month] [year] [list]

Date: Mon, 1 Apr 2024 14:43:31 +0200
From: Andrew Lunn <andrew@...n.ch>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Florian Fainelli <f.fainelli@...il.com>,
	Vladimir Oltean <olteanv@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Gregory Clement <gregory.clement@...tlin.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 6/7] dsa: mv88e6xxx: Create port/netdev LEDs

On Sun, Mar 31, 2024 at 04:55:44PM +0100, Russell King (Oracle) wrote:
> On Sat, Mar 30, 2024 at 01:32:03PM -0500, Andrew Lunn wrote:
> 
> > +static int mv88e6xxx_led_brightness_set(struct net_device *ndev,
> > +					u8 led, enum led_brightness value)
> > +{
> > +	struct dsa_switch *ds = dsa_user_to_ds(ndev);
> > +	struct mv88e6xxx_chip *chip = ds->priv;
> > +	int port = dsa_user_to_index(ndev);
> 
> This breaks the model that the DSA layer contains shims to translate
> stuff to a dsa_switch pointer and port index. That's not a complaint.
> I think it's the right way forward, because the shim later feels like
> it makes maintenance needlessly more complex.

It was something Vladimir requested after reviewing a patchset trying
to reuse parts of a DSA driver in a pure switchdev driver. He wanted
more generic building blocks which could be put together in different
ways. It does result in more boilerplate code in each callback, but
the helpers i added keep it down.

> I have been thinking whether to do the same for the various phylink
> functions - having DSA drivers provide the phylink_mac_ops themselves
> where they implement the phylink ops, and convert from the "config"
> to dsa_switch+port or whatever is suitable for them where necessary.

If it helps, do it. But i would suggest adding helpers to try to keep
the boilerplate down. That is mostly what the wrappers in the DSA core
do, centralise the boilerplate so we only have one copy.

    Andrew