[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <59d08292-6f38-4784-b12b-520fd24600a7@linux.dev>
Date: Mon, 17 Nov 2025 17:39:13 +0000
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
Andrew Lunn <andrew+netdev@...n.ch>, Jakub Kicinski <kuba@...nel.org>
Cc: Florian Fainelli <florian.fainelli@...adcom.com>,
Heiner Kallweit <hkallweit1@...il.com>, "David S. Miller"
<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, Andrei Botila <andrei.botila@....nxp.com>,
Richard Cochran <richardcochran@...il.com>, Andrew Lunn <andrew@...n.ch>,
Simon Horman <horms@...nel.org>, Vladimir Oltean <vladimir.oltean@....com>,
Jacob Keller <jacob.e.keller@...el.com>,
Kory Maincent <kory.maincent@...tlin.com>,
bcm-kernel-feedback-list@...adcom.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/9] phy: add hwtstamp_get callback to phy
drivers
On 13/11/2025 12:24, Russell King (Oracle) wrote:
> On Thu, Nov 13, 2025 at 12:12:44PM +0000, Vadim Fedorenko wrote:
>> On 13/11/2025 12:02, Russell King (Oracle) wrote:
>>> On Thu, Nov 13, 2025 at 11:32:00AM +0000, Vadim Fedorenko wrote:
>>>> PHY devices had lack of hwtstamp_get callback even though most of them
>>>> are tracking configuration info. Introduce new call back to
>>>> mii_timestamper.
>>>>
>>>> Signed-off-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
>>>
>>> As part of my Marvell PTP work, I have a similar patch, but it's
>>> way simpler. Is this not sufficient?
>>>
>>> __phy_hwtstamp_get() is called via phylib_stubs struct and
>>> phy_hwtstamp_get(), dev_get_hwtstamp_phylib(), dev_get_hwtstamp(),
>>> and dev_ifsioc().
>>>
>>> Using the phylib ioctl handler means we're implementing a path that
>>> is already marked as legacy - see dev_get_hwtstamp():
>>>
>>> if (!ops->ndo_hwtstamp_get)
>>> return dev_eth_ioctl(dev, ifr, SIOCGHWTSTAMP); /* legacy */
>>>
>>> So, I think the below would be the preferred implementation.
>>
>> You mean do not add SIOCGHWTSTAMP case in phy_mii_ioctl() as we should
>> never reach this legacy option?
>
> We _can_ reach phy_mii_ioctl() for SIOCGHWTSTAMP where drivers do not
> provide the ndo_hwtstamp_get() method. However, as this is legacy code,
> the question is: should we add it?
>
>> Technically, some drivers are (yet) not
>> converted to ndo_hwtstamp callbacks and this part can potentially work
>> for bnx2x driver, until the other series lands.
>
> Right, but providing new features to legacy paths gives less reason for
> people to stop using the legacy paths.
>
>> I was planning to remove SIOCSHWTSTAMP/SIOCGHWTSTAMP dev_eth_ioctl calls
>> later once everything has landed and we have tests confirming that ioctl
>> and netlink interfaces work exactly the same way.
>
> However, implementations that do populate non-legacy ndo_hwtstamp_get()
> won't work correctly with your conversion, since we'll fall through to
> the path which calls __phy_hwtstamp_get() which won't do anything.
>
> So I disagree with your patch - it only adds support for legacy net
> drivers to get the hwtstamp settings from the PHY. Non-legacy won't be
> supported.
>
> At minimum, we should be adding support for non-legacy, and _possibly_
> legacy.
>
> Let's wait for others to comment on my point about adding this for the
> legacy drivers/code path.
As there was no conversation for a couple of days, and Andrew expressed
the same thought about not going to implement SIOCGHWTSTAMP, I'm going
to publish new version with this part removed.
Powered by blists - more mailing lists