netdev - Re: [PATCH net-next 2/3] net: dsa: allow DSA switch drivers to provide their own phylink mac ops

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]

Message-ID: <ZhWHVc/bhaC/rYq7@shell.armlinux.org.uk>
Date: Tue, 9 Apr 2024 19:22:13 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 2/3] net: dsa: allow DSA switch drivers to
 provide their own phylink mac ops

On Tue, Apr 09, 2024 at 07:52:30PM +0300, Vladimir Oltean wrote:
> On Tue, Apr 09, 2024 at 05:29:23PM +0100, Russell King (Oracle) wrote:
> > This changes the logic - it allows driver authors to provide the
> > MAC operations, omit the mac_link_down() op _and_ an
> > ops->phylink_mac_link_down() function. This could lead to buggy
> > drivers since this will only happen in this path and none of the
> > others.
> > 
> > I want this to be an "either provide phylink_mac_ops, and thus
> > none of the phylink_mac_* ops in dsa_switch_ops will be called" or
> > "don't provide phylink_mac_ops and the phylink_mac_* ops in
> > dsa_switch_ops will be called". It's then completely clear cut
> > that it's one or the other, whereas the code above makes it
> > unclear.
> 
> If you want for the API transition to be self-documenting and clear,
> it would be good to do that validation separately and more comprehensively
> rather than just a fall-through for one single operation here.
> 
> If phylink_mac_link_ops is provided, the following ds->ops methods are
> obsoleted and can't be provided at the same time (fail probing otherwise):
> 
> - phylink_mac_select_pcs()
> - phylink_mac_prepare()
> - phylink_mac_config()
> - phylink_mac_finish()
> - phylink_mac_link_down()
> - phylink_mac_link_up()
> 
> Hopefully it makes it more clear that the following are _not_ obsoleted
> by the dedicated phylink mac_ops:
> 
> - phylink_get_caps()
> - phylink_fixed_state()
> 
> Then (after this validation), the simplified
> "if (ops && ops->mac_link_down) else (ds->ops->phylink_mac_link_down)"
> would be equivalent, because we've errored out on the case which has a
> mix of old and new API.

Okay. However, I can't predict when I'll get around to doing these
changes as my time is very limited over this week and next week - so
may be right before the merge window is due.

Maybe we should've done that for the ops->adjust_link vs
ops->phylink_mac_link_down/ops->phylink_mac_link_up as well.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!