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: <20250922123301.y7qjguatajhci67o@DEN-DL-M31836.microchip.com>
Date: Mon, 22 Sep 2025 14:33:01 +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 15:15, Vladimir Oltean wrote:

Hi,

> 
> On Thu, Sep 18, 2025 at 04:09:42PM -0700, Jakub Kicinski wrote:
> > On Wed, 17 Sep 2025 13:33:16 +0200 Horatiu Vultur wrote:
> > > When trying to enable PTP on vsc8574 and vsc8572 it is not working even
> > > if the function vsc8584_ptp_init it says that it has support for PHY
> > > timestamping. It is not working because there is no PTP device.
> > > So, to fix this make sure to create a PTP device also for this PHYs as
> > > they have the same PTP IP as the other vsc PHYs.
> >
> > May be useful to proof read your commit message, or run it thru
> > a grammar checker. Copy & paste into a Google Doc would be enough..
> 
> I agree, and I did not understand the problem from the commit message.
> 
> I would suggest something like below (maybe not identical).
> 
> The PTP initialization is two-step: first we have vsc8584_ptp_probe_once() /
> vsc8584_ptp_probe() at probe() time, then we have vsc8584_ptp_init() at
> config_init() time.
> 
> For VSC8574 and VSC8572, the PTP initialization is incomplete. We are
> making the second step without having previously made the first one.
> This means, for example, that ptp_clock_register() is never called.
> 
> Nothing crashes as a result of this, but it is unexpected that some PHY
> generations have PTP functionality exposed by the driver and some do
> not, even though they share the same PTP clock IP.

I agree, I need to be more carefull and more clear in the commit
messages. Thanks for the great example.

> 
> > Regarding the patch the new function looks like a spitting image
> > of vsc8584_probe(), minus the revision check.
> > --
> > pw-bot: cr
> 
> Also, even without this patch, vsc8574_probe() and vsc8584_probe() are
> structurally very similar and could use some consolidation.
> 
> Would it make sense to create a static int vsc8531_probe_common(struct
> phy_device *phydev, bool ptp) and call it from multiple wrappers? The
> VSC8584_REVB check can go in the vsc8584_probe() wrapper. The "size_t
> priv_size" argument of devm_phy_package_join() can be set based on the
> "bool ptp" argument, because struct vsc85xx_shared_private is used only
> in PTP code.
> 
> You can make a preparatory change in 'net' patch sets, even without
> a Fixes: tag, if you clearly explain what it's for.

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;
	}
}
---


-- 
/Horatiu