linux-kernel - Re: [PATCH net v2] phy: mscc: Fix PTP for vsc8574 and VSC8572

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: <20250926095203.dqg2vkjr5tdwri7w@skbuf>
Date: Fri, 26 Sep 2025 12:52:03 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Horatiu Vultur <horatiu.vultur@...rochip.com>,
	Jakub Kicinski <kuba@...nel.org>, andrew@...n.ch,
	hkallweit1@...il.com, davem@...emloft.net, edumazet@...gle.com,
	pabeni@...hat.com, richardcochran@...il.com,
	vadim.fedorenko@...ux.dev, christophe.jaillet@...adoo.fr,
	rosenp@...il.com, steen.hegelund@...rochip.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v2] phy: mscc: Fix PTP for vsc8574 and VSC8572

On Fri, Sep 26, 2025 at 10:46:47AM +0100, Russell King (Oracle) wrote:
> On Mon, Sep 22, 2025 at 02:33:01PM +0200, Horatiu Vultur wrote:
> > Thanks for the advice.
> > What about to make the PHY_ID_VSC8572 and PHY_ID_VSC8574 to use
> > vsc8584_probe() and then in this function just have this check:
> > 
> > ---
> > if ((phydev->phy_id & 0xfffffff0) != PHY_ID_VSC8572 &&
> >     (phydev->phy_id & 0xfffffff0) != PHY_ID_VSC8574) {
> > 	if ((phydev->phy_id & MSCC_DEV_REV_MASK) != VSC8584_REVB) {
> > 		dev_err(&phydev->mdio.dev, "Only VSC8584 revB is supported.\n");
> > 		return -ENOTSUPP;
> > 	}
> > }
> 
> Please, no, not like this. Have a look how the driver already compares
> PHY IDs in the rest of the code.
> 
> When a PHY driver is matched, the PHY ID is compared using the
> .phy_id and .phy_id_mask members of the phy_driver structure.
> 
> The .phy_id is normally stuff like PHY_ID_VSC8572 and PHY_ID_VSC8574.
> 
> When the driver is probed, phydev->drv is set to point at the
> appropriate phy_driver structure. Thus, the tests can be simplified
> to merely looking at phydev->drv->phy_id:
> 
> 	if (phydev->drv->phy_id != PHY_ID_VSC8572 &&
> 	    phydev->drv->phy_id != PHY_ID_VSC8574 &&
> 	    (phydev->phy_id & MSCC_DEV_REV_MASK) != VSC8584_REVB) {
> ...
> 
> Alternatively, please look at the phy_id*() and phydev_id_compare()
> families of functions.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

The phydev->phy_id comparisons are problematic with clause 45 PHYs where
this field is zero, but with clause 22 they should technically work.
It's good to know coming from a phylib maintainer that it's preferable
to use phydev->drv->phy_id everywhere (I also wanted to comment on this
aspect, but because technically nothing is broken, I didn't).