linux-kernel - Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with 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]

Date: Mon, 22 Jan 2024 17:12:51 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Tim Menninger <tmenninger@...estorage.com>, f.fainelli@...il.com,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: dsa: mv88e6xxx: Make *_c45 callbacks agree with
 phy_*_c45 callbacks

On Mon, Jan 22, 2024 at 03:30:20PM +0100, Andrew Lunn wrote:
> On Mon, Jan 22, 2024 at 02:33:49PM +0200, Vladimir Oltean wrote:
> > On Tue, Jan 16, 2024 at 05:51:13PM -0800, Tim Menninger wrote:
> > > My impression is still that the read_c45 function should agree with the
> > > phy_read_c45 function, but that isn't a hill I care to die on if you still
> > > think otherwise. Thoughts?
> > 
> > FWIW, Tim's approach is consistent with what drivers/net/mdio/mdio-mux.c does.
> > 
> > 		if (parent_bus->read)
> > 			cb->mii_bus->read = mdio_mux_read;
> > 		if (parent_bus->write)
> > 			cb->mii_bus->write = mdio_mux_write;
> > 		if (parent_bus->read_c45)
> > 			cb->mii_bus->read_c45 = mdio_mux_read_c45;
> > 		if (parent_bus->write_c45)
> > 			cb->mii_bus->write_c45 = mdio_mux_write_c45;
> > 
> > My only objection to his patch (apart from the commit message which
> > should indeed be more detailed) is that I would have preferred the same
> > "if" syntax rather than the use of a ternary operator with NULL.
> 
> I agree it could be fixed this way. But what i don't like about the
> current code is how C22 and C45 do different things with error
> codes. Since the current code is trying to use an error code, i would
> prefer to fix that error code handling, rather than swap to a
> different way to indicate its not supported.
> 
> 	  Andrew

You did write in commit da099a7fb13d ("net: phy: Remove probe_capabilities")
that the MDIO bus API is now this: "Deciding if to probe of PHYs using
C45 is now determine by if the bus provides the C45 read method."

Do you not agree that Tim's approach is the more straightforward
solution overall to skip C45 PHY probing, given this API, both code wise
and runtime wise? Are there downsides to it?

I have no objection to the C22 vs C45 error code handling inconsistency.
It can be improved, sure. But it also does not matter here, if we agree
that this problem can be sorted out in a more straightforward way with
no negative consequences.

I sort of don't understand the desire to have the smallest patch in
terms of lines of code, when the end result will end up being suboptimal
compared to something with just a little more lines (1 vs 4).