netdev - 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250922132846.jkch266gd2p6k4w5@skbuf>
Date: Mon, 22 Sep 2025 16:28:46 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Horatiu Vultur <horatiu.vultur@...rochip.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

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).

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?

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?