netdev - 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: Tue, 16 Jan 2024 14:24:55 -0800
From: Tim Menninger <tmenninger@...estorage.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: f.fainelli@...il.com, olteanv@...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 Tue, Jan 16, 2024 at 11:59 AM Andrew Lunn <andrew@...n.ch> wrote:
>
> On Tue, Jan 16, 2024 at 07:35:42PM +0000, Tim Menninger wrote:
> > Set the read_c45 callback in the mii_bus struct in mv88e6xxx only if there
> > is a non-NULL phy_read_c45 callback on the chip mv88e6xxx_ops. Similarly
> > for write_c45 and phy_write_c45.
> >
> > In commit 743a19e38d02 ("net: dsa: mv88e6xxx: Separate C22 and C45 transactions")
> > the MDIO bus driver split its API to separate C22 and C45 transfers.
> >
> > In commit 1a136ca2e089 ("net: mdio: scan bus based on bus capabilities for C22 and C45")
> > we do a C45 mdio bus scan based on existence of the read_c45 callback
> > rather than checking MDIO bus capabilities then in
> > commit da099a7fb13d ("net: phy: Remove probe_capabilities") we remove the
> > probe_capabilities from the mii_bus struct.
> >
> > The combination of the above results in a scenario (e.g. mv88e6185)
> > where we take a non-NULL read_c45 callback on the mii_bus struct to mean
> > we can perform a C45 read and proceed with a C45 MDIO bus scan. The scan
> > encounters a NULL phy_read_c45 callback in the mv88e6xxx_ops which implies
> > we can NOT perform a C45 read and fails with EOPNOTSUPP. The read_c45
> > callback should be NULL if phy_read_c45 is NULL, and similarly for
> > write_c45 and phy_write_c45.
>
> Hi Tim
>
> What does phylib do with the return of -EOPNOTSUPP? I've not tested
> it, but i would expect it just keeps going with the scan? It treats it
> as if there is no device there? And since it never accesses the
> hardware, this should be fast?
>
> Or is my assumption wrong? Do you see the EPOPNOTSUPP getting reported
> back to user space, and the probe failing?
>
>      Andrew
>

Hi Andrew,

It bubbles up as EIO (the translation happens in get_phy_c45_ids when
get_phy_c45_devs_in_pkg fails) and ultimately causes the probe to fail.

The EIO causes the scan to stop and fail immediately - the way I read
mdiobus_scan_bus_c45, only ENODEV is permissible.

>From logs:
$ dmesg | grep mv88e6
[   12.951149] mv88e6085 ixgbe-mdio-0000:05:00.0:00: switch 0x1a70
detected: Marvell 88E6185, revision 2
[   13.272812] mv88e6085 ixgbe-mdio-0000:05:00.0:00: Cannot register
MDIO bus (-5)
[   13.401140] mv88e6085: probe of ixgbe-mdio-0000:05:00.0:00 failed
with error -5
[   13.413105] mv88e6085 ixgbe-mdio-0000:05:00.1:00: switch 0x1a70
detected: Marvell 88E6185, revision 2
[   13.730227] mv88e6085 ixgbe-mdio-0000:05:00.1:00: Cannot register
MDIO bus (-5)
[   13.858336] mv88e6085: probe of ixgbe-mdio-0000:05:00.1:00 failed
with error -5

Tim