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

Message-ID: <20240409165230.eznwc4opf3mq7qkl@skbuf>
Date: Tue, 9 Apr 2024 19:52:30 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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 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.