Re: [PATCH net] net: don't require the hwtstamp NDOs when a PHY provides timestamping

[Nicolai Buchwitz]

Hi (S|Cl)ashiko

On 21.9.2026 11:57, netdev-bot+sashiko@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let 
> patchwork
> know by including "pw-bot: cr" as a separate line at the end of your 
> reply
> (one such reply per series is enough).
>
> - [Medium] Incomplete fix: the ethtool-netlink tsconfig path still
>   rejects PHY-only devices, so the two documented-equivalent uAPIs now…
>
> --
>
> [...]

> [Severity: Medium]
> With the checks relocated here, the ioctl entry points now reach
> phy_hwtstamp_get()/phy_hwtstamp_set() for a device whose MAC implements
> neither NDO but whose phydev satisfies phy_is_default_hwtstamp().  The
> ethtool netlink peers of those ioctls keep the old unconditional gates,
> and they run before these helpers are ever called.
>
> net/ethtool/tsconfig.c:tsconfig_prepare_data() {
> 	...
> 	if (!dev->netdev_ops->ndo_hwtstamp_get)
> 		return -EOPNOTSUPP;
>
> 	ret = ethnl_ops_begin(dev);
> 	...
> 	ret = dev_get_hwtstamp_phylib(dev, &cfg);
> 	...
> }
>
> net/ethtool/tsconfig.c:ethnl_set_tsconfig_validate() {
> 	const struct net_device_ops *ops = req_base->dev->netdev_ops;
>
> 	if (!ops->ndo_hwtstamp_set || !ops->ndo_hwtstamp_get)
> 		return -EOPNOTSUPP;
>
> 	return 1;
> }
>
> Does this leave the two interfaces disagreeing on the same hardware?  
> On

Yes, but they already disagreed before this patch (EOPNOTSUPP).

The ioctls are the regression and AFAIU TSCONFIG_GET/SET never worked
here. Both gates are part of 6e9e2eed4f39 (~ a year before 
5062245a5a7f).

> the bcmgenet plus BCM54213PE setup named in the changelog,
> SIOCGHWTSTAMP/SIOCSHWTSTAMP would succeed after this patch, while
> ETHTOOL_MSG_TSCONFIG_GET and ETHTOOL_MSG_TSCONFIG_SET would still 
> return
> -EOPNOTSUPP, even though Documentation/networking/ethtool-netlink.rst
> lists those netlink operations as the replacements for the two ioctls 
> in
> its ioctl-to-netlink mapping table.
>
> The note about this ("tsconfig keeps its own copy of the check ... IMHO
> this is an extra patch for net-next") sits below the --- separator, so 
> it
> does not end up in git history.  Could it be moved into the commit 
> message
> so that maintainers and stable backporters can see that PHY-only
> timestamping devices are only fixed on the ioctl path?

That would suggest the regression is half fixed. It isn't, the tsconfig
gap is a separate and older bug. I placed it deliberately below ---.

> One detail for whoever narrows the tsconfig gates later: the fix is not 
> a
> mechanical copy of this one.  The HWTSTAMP_SOURCE_NETDEV branch in
> dev_get_hwtstamp_phylib() still calls the callback with no NULL test:
>
> net/core/dev_ioctl.c:dev_get_hwtstamp_phylib() {
> 	...
> 	if (hwprov->source == HWTSTAMP_SOURCE_NETDEV)
> 		return dev->netdev_ops->ndo_hwtstamp_get(dev, cfg);
> 	...
> }
>
> dev->hwprov is only ever assigned in ethnl_set_tsconfig(), which is
> reached through ethnl_set_tsconfig_validate(), so that branch is safe
> today precisely because of the check under discussion.  Should the two
> sites be changed together?

No, IMHO not here. It would change change behaviour that never worked.
Thus I would suggest fixing this with a follow-up to net-next.

> [ ... ]

Thanks,
Nicolai