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

Message-ID: <20250923071924.mv6ytwtifuu5limg@DEN-DL-M31836.microchip.com>
Date: Tue, 23 Sep 2025 09:19:24 +0200
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: Jakub Kicinski <kuba@...nel.org>, <andrew@...n.ch>,
	<hkallweit1@...il.com>, <linux@...linux.org.uk>, <davem@...emloft.net>,
	<edumazet@...gle.com>, <pabeni@...hat.com>, <richardcochran@...il.com>,
	<vadim.fedorenko@...ux.dev>, <rmk+kernel@...linux.org.uk>,
	<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

The 09/22/2025 16:28, Vladimir Oltean 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;
> >       }
> > }
> 
> Personally, I think you are making the code harder to understand what
> PHYs the test is referring to, and why it exists in the first place.
> 
> Ideally this test would have not existed. Instead of the open-coded
> phy_id and phy_id_mask fields from the struct phy_driver array entries,
> one could have used PHY_ID_MATCH_MODEL() for those entries where the
> bits 3:0 of the PHY ID do not matter, and PHY_ID_MATCH_EXACT() where
> they do. Instead of failing the probe, just not match the device with
> this driver and let the system handle it some other way (Generic PHY).

Yes, I can see your point. That would be a nicer fix.

> 
> I'm not sure if this is intended or not, but the combined effect of:
> - commit a5afc1678044 ("net: phy: mscc: add support for VSC8584 PHY")
> - commit 75a1ccfe6c72 ("mscc.c: Add support for additional VSC PHYs")
> 
> is that for VSC856X, VSC8575, VSC8582, VSC8584, the driver will only
> probe on Rev B silicon, and fail otherwise. Initially, the revision test
> was only there for VSC8584, and it transferred to the others by virtue
> of reusing the same vsc8584_probe() function. I don't see signs that
> this was 100% intentional. I say this because when probing e.g. on
> VSC8575 revA, the kernel will print "Only VSC8584 revB is supported."
> which looks more like an error than someone's actual intention.
> 
> By excluding VSC8574 and VSC8572 from the above revision test, it feels
> like a double workaround rather than using the conventional PHY ID match
> helpers as intended.
> 
> As a Microchip employee, maybe you have access to some info regarding
> whether the limitations mentioned by Quentin Schulz for VSC8584 revA
> are valid for all the other PHYs for which they are currently imposed.
> What makes VSC8574/VSC8572 unlike the others in this regard?

Let me start by asking my colleagues and figure out which revisions were
produced for which PHYs.
Thanks for the advice.

> 
> It looks like the review comments to clean things up are getting bigger.
> I'm not sure this is all adequate for 'net' any longer.
> On the other hand, you said PTP never worked for VSC8574/VSC8572,
> without any crash, it was just not enabled. Maybe this can all be
> reconsidered as new functionality for net-next, and there we have more
> space for shuffling things around?

-- 
/Horatiu