* Re: [PATCH net] net: ethtool: let tsconfig reach a PHY-only timestamp provider
2026-09-22 7:08 [PATCH net] net: ethtool: let tsconfig reach a PHY-only timestamp provider Nicolai Buchwitz
@ 2026-09-22 8:05 ` Nicolai Buchwitz
2026-09-23 7:58 ` Kory Maincent
2026-09-23 10:10 ` netdev-bot+sashiko
2 siblings, 0 replies; 5+ messages in thread
From: Nicolai Buchwitz @ 2026-09-22 8:05 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Andrew Lunn, Kory Maincent, vadim.fedorenko
Cc: James Clark, netdev, linux-kernel
+TO vadim.fedorenko@linux.dev
(get_maintainers seems to missed Vadim, sorry)
On 22.9.2026 09:08, Nicolai Buchwitz wrote:
> TSCONFIG_GET and TSCONFIG_SET reject a device that implements neither
> hwtstamp NDO, even when its PHY can serve the request. The ioctls they
> meant to replace handle it, so the two interfaces disagree on the same
> hardware and user space has to pick one.
>
> Drop both checks and let the phylib helpers decide, like the ioctls do.
>
> As this makes a netdev provider reachable without ndo_hwtstamp_get,
> test
> for the callback before calling it.
>
> Fixes: 6e9e2eed4f39 ("net: ethtool: Add support for tsconfig command to
> get/set hwtstamp config")
> Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
> ---
> Follow-up to 31995571219c ("net: don't require the hwtstamp NDOs when a
> PHY provides timestamping"), which fixed the ioctl side:
>
> https://lore.kernel.org/netdev/20260918095540.34286-1-nb@tipi-net.de/
>
> Tested on a Raspberry Pi CM4 with BCM54213PE.
>
> net/core/dev_ioctl.c | 3 ++-
> net/ethtool/tsconfig.c | 15 ---------------
> 2 files changed, 2 insertions(+), 16 deletions(-)
>
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 164643140a52..f6029f60c1dc 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -267,7 +267,8 @@ int dev_get_hwtstamp_phylib(struct net_device *dev,
> hwprov->phydev)
> return phy_hwtstamp_get(hwprov->phydev, cfg);
>
> - if (hwprov->source == HWTSTAMP_SOURCE_NETDEV)
> + if (hwprov->source == HWTSTAMP_SOURCE_NETDEV &&
> + dev->netdev_ops->ndo_hwtstamp_get)
> return dev->netdev_ops->ndo_hwtstamp_get(dev, cfg);
>
> return -EOPNOTSUPP;
> diff --git a/net/ethtool/tsconfig.c b/net/ethtool/tsconfig.c
> index 6be3aa5d4bc1..8c99b2dc26d0 100644
> --- a/net/ethtool/tsconfig.c
> +++ b/net/ethtool/tsconfig.c
> @@ -42,9 +42,6 @@ static int tsconfig_prepare_data(const struct
> ethnl_req_info *req_base,
> struct kernel_hwtstamp_config cfg = {};
> int ret;
>
> - if (!dev->netdev_ops->ndo_hwtstamp_get)
> - return -EOPNOTSUPP;
> -
> ret = ethnl_ops_begin(dev);
> if (ret < 0)
> return ret;
> @@ -248,17 +245,6 @@ static int tsconfig_send_reply(struct net_device
> *dev, struct genl_info *info)
> return ret;
> }
>
> -static int ethnl_set_tsconfig_validate(struct ethnl_req_info
> *req_base,
> - struct genl_info *info)
> -{
> - const struct net_device_ops *ops = req_base->dev->netdev_ops;
> -
> - if (!ops->ndo_hwtstamp_set || !ops->ndo_hwtstamp_get)
> - return -EOPNOTSUPP;
> -
> - return 1;
> -}
> -
> static struct hwtstamp_provider *
> tsconfig_set_hwprov_from_desc(struct net_device *dev,
> struct genl_info *info,
> @@ -459,6 +445,5 @@ const struct ethnl_request_ops
> ethnl_tsconfig_request_ops = {
> .reply_size = tsconfig_reply_size,
> .fill_reply = tsconfig_fill_reply,
>
> - .set_validate = ethnl_set_tsconfig_validate,
> .set = ethnl_set_tsconfig,
> };
>
> base-commit: f0b88fade64c6fe52e15b246097d10bb115d8af3
Regards Nicolai
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net] net: ethtool: let tsconfig reach a PHY-only timestamp provider
2026-09-22 7:08 [PATCH net] net: ethtool: let tsconfig reach a PHY-only timestamp provider Nicolai Buchwitz
2026-09-22 8:05 ` Nicolai Buchwitz
2026-09-23 7:58 ` Kory Maincent
@ 2026-09-23 10:10 ` netdev-bot+sashiko
2026-09-23 10:24 ` Nicolai Buchwitz
2 siblings, 1 reply; 5+ messages in thread
From: netdev-bot+sashiko @ 2026-09-23 10:10 UTC (permalink / raw)
To: nb
Cc: davem, edumazet, kuba, pabeni, horms, andrew, kory.maincent, jjc,
netdev, linux-kernel
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 2 (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).
- [High] TSCONFIG_SET now reports success (bare ACK, ret 0) with no
ETHTOOL_MSG_TSCONFIG_SET_REPLY on devices that have no hardware…
- [Medium] With ethnl_set_tsconfig_validate() gone, ethnl_set_tsconfig()
is reachable on netdevs that implement neither ndo_hwtstamp_get nor…
- [Medium] net: ethtool: TSCONFIG_SET provider switch fails on devices
lacking ndo_hwtstamp_set
Pre-existing issues:
- [Medium] A struct hwtstamp_provider installed into dev->hwprov by
TSCONFIG_SET is never freed when the netdevice is unregistered/freed…
--
net: ethtool: let tsconfig reach a PHY-only timestamp provider
This drops the ndo_hwtstamp_get check in tsconfig_prepare_data() and the
whole ethnl_set_tsconfig_validate() gate so that TSCONFIG_GET/SET can be
served by a PHY on a MAC that implements no hwtstamp NDO. A guard is added
in dev_get_hwtstamp_phylib() before calling ndo_hwtstamp_get.
Related discussion for the ioctl side:
https://lore.kernel.org/netdev/20260918095540.34286-1-nb@tipi-net.de/
> diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
> index 164643140a523..f6029f60c1dc9 100644
> --- a/net/core/dev_ioctl.c
> +++ b/net/core/dev_ioctl.c
> @@ -267,7 +267,8 @@ int dev_get_hwtstamp_phylib(struct net_device *dev,
> hwprov->phydev)
> return phy_hwtstamp_get(hwprov->phydev, cfg);
>
> - if (hwprov->source == HWTSTAMP_SOURCE_NETDEV)
> + if (hwprov->source == HWTSTAMP_SOURCE_NETDEV &&
> + dev->netdev_ops->ndo_hwtstamp_get)
> return dev->netdev_ops->ndo_hwtstamp_get(dev, cfg);
>
> return -EOPNOTSUPP;
[Severity: Medium]
This guard makes the read side safe, but can the install side now latch a
HWTSTAMP_SOURCE_NETDEV provider on a device that has no hwtstamp NDOs at
all, with no way back?
With ethnl_set_tsconfig_validate() gone, ethnl_set_tsconfig() runs on a MAC
like bcmgenet (no ndo_hwtstamp_*, .get_ts_info = ethtool_op_get_ts_info)
that has a default-timestamp PHY, e.g. BCM54213PE. Two requests:
1) TSCONFIG_SET with provider {index = PHY phc index, qualifier = precise}.
dev->hwprov is still NULL and phy_is_default_hwtstamp(dev->phydev) is
true, so dev_set_hwtstamp_phylib(&zero_config) succeeds through
phy_hwtstamp_set() and the PHYLIB provider is installed.
2) TSCONFIG_SET with provider {index = 0xFFFFFFFF, qualifier = precise}.
ETHTOOL_A_TS_HWTSTAMP_PROVIDER_INDEX is a plain NLA_U32 stored into the
int hwtstamp_provider_desc.index, so the request index becomes -1. In
ethtool_net_get_ts_info_by_phc():
if (info->phc_index == hwprov_desc->index &&
net_support_hwtstamp_qualifier(dev, hwprov_desc->qualifier))
return 0;
ethtool_init_tsinfo() presets phc_index = -1 and ethtool_op_get_ts_info()
leaves it at -1, and net_support_hwtstamp_qualifier() returns true for
PRECISE when the driver declares no qualifier mask, so
tsconfig_set_hwprov_from_desc() picks source = HWTSTAMP_SOURCE_NETDEV
with phydev == NULL.
In ethnl_set_tsconfig() the hwprov_mod block then only disables the old
provider before committing the new one:
ret = dev_set_hwtstamp_phylib(dev, &zero_config, info->extack);
if (ret < 0)
goto err_free_hwprov;
/* Change the selected hwtstamp source */
__hwprov = rcu_replace_pointer(dev->hwprov, hwprov,
netdev_is_locked_ops_compat(dev));
Nothing checks that the new NETDEV provider can be programmed, and there is
no rollback. After that, this hunk's guard returns -EOPNOTSUPP for every
GET, and dev_set_hwtstamp_phylib() takes the phy_ts = false path and stops
at its "if (!ops->ndo_hwtstamp_set)" check, so SIOCGHWTSTAMP/SIOCSHWTSTAMP
fail too.
Is recovery possible through the same interface? A later TSCONFIG_SET
re-selecting the PHY provider enters the hwprov_mod block and fails on its
first statement, dev_set_hwtstamp_phylib(&zero_config) evaluated against the
current NETDEV provider, jumping to err_free_hwprov before ever reaching
rcu_replace_pointer(). phy_detach() only clears dev->hwprov when
hwprov->phydev == phydev, which is NULL here, so ifdown or PHY disconnect
does not clear it either.
> diff --git a/net/ethtool/tsconfig.c b/net/ethtool/tsconfig.c
> index 6be3aa5d4bc1e..8c99b2dc26d03 100644
> --- a/net/ethtool/tsconfig.c
> +++ b/net/ethtool/tsconfig.c
> @@ -42,9 +42,6 @@ static int tsconfig_prepare_data(const struct ethnl_req_info *req_base,
> struct kernel_hwtstamp_config cfg = {};
> int ret;
>
> - if (!dev->netdev_ops->ndo_hwtstamp_get)
> - return -EOPNOTSUPP;
> -
> ret = ethnl_ops_begin(dev);
> if (ret < 0)
> return ret;
> @@ -248,17 +245,6 @@ static int tsconfig_send_reply(struct net_device *dev, struct genl_info *info)
> return ret;
> }
>
> -static int ethnl_set_tsconfig_validate(struct ethnl_req_info *req_base,
> - struct genl_info *info)
> -{
> - const struct net_device_ops *ops = req_base->dev->netdev_ops;
> -
> - if (!ops->ndo_hwtstamp_set || !ops->ndo_hwtstamp_get)
> - return -EOPNOTSUPP;
> -
> - return 1;
> -}
> -
[Severity: Medium]
With ethnl_set_tsconfig_validate() gone, a TSCONFIG_SET that selects
the PHY provider now reaches ethnl_set_tsconfig() on a MAC without
hwtstamp NDOs. Does the provider switch itself ever succeed there
unless the PHY happens to be the default timestamper?
Take this setup:
- a MAC with no ndo_hwtstamp_get/set
- a PHY that implements timestamping but has
phydev->default_timestamp cleared
- dev->hwprov still NULL
A TSCONFIG_SET naming the PHY's phc index makes
tsconfig_set_hwprov_from_desc() return a HWTSTAMP_SOURCE_PHYLIB
provider. hwprov_mod is then true, and the handler first tries to turn
the current provider off:
ret = dev_set_hwtstamp_phylib(dev, &zero_config, info->extack);
if (ret < 0)
goto err_free_hwprov;
In dev_set_hwtstamp_phylib(), hwprov is NULL and
phy_is_default_hwtstamp(dev->phydev) is false, so phy_ts is false and
the function ends up at:
if (!ops->ndo_hwtstamp_set)
return -EOPNOTSUPP;
The request then fails with -EOPNOTSUPP before rcu_replace_pointer() is
reached, so the PHY provider is never installed. The provider being
disabled here is an implicit netdev source that cannot have been
enabled in the first place.
The GET side does not improve for this hardware either.
dev_get_hwtstamp_phylib() still falls through to the !ndo_hwtstamp_get
check and returns -EOPNOTSUPP. The ioctls cannot select a non-default
PHY either, so TSCONFIG_SET is the only way to reach it, and that path
still refuses.
Should the disable step tolerate -EOPNOTSUPP when the outgoing provider
is the implicit netdev one and ndo_hwtstamp_set is absent? That would
match how the !hwprov_mod branch already ignores -EOPNOTSUPP from
dev_get_hwtstamp_phylib(). Or is only the default-timestamp PHY case
meant to be covered, in which case could the commit message say so?
[Severity: High]
Does removing this gate turn TSCONFIG_SET into a silent success on devices
that have no timestamp provider at all?
On a netdev with neither ndo_hwtstamp_get/set nor a default-timestamp PHY
(loopback, veth, tun, bcmgenet without a PTP PHY), a TSCONFIG_SET carrying
only the header, or asking for tx-type off / rx-filter none:
- no ETHTOOL_A_TSCONFIG_HWTSTAMP_PROVIDER attribute, so hwprov_mod stays
false and ethnl_set_tsconfig() calls:
if (!hwprov_mod) {
ret = dev_get_hwtstamp_phylib(dev, &hwtst_config);
if (ret < 0 && ret != -EOPNOTSUPP)
goto err_free_hwprov;
}
dev_get_hwtstamp_phylib() returns -EOPNOTSUPP here (hwprov NULL,
phy_is_default_hwtstamp(NULL) false, no ndo_hwtstamp_get) and that is
deliberately swallowed, leaving hwtst_config all zero.
- ethnl_update_bitset32() reports no change, so config_mod stays false and
the "if (config_mod)" dev_set_hwtstamp_phylib() call is skipped.
net_hwtstamp_validate({0,0,0}) returns 0 because HWTSTAMP_TX_OFF and
HWTSTAMP_FILTER_NONE are both valid.
- the tail then swallows the error from the reply path as well:
ret = tsconfig_send_reply(dev, info);
if (ret && ret != -EOPNOTSUPP) {
NL_SET_ERR_MSG(info->extack,
"error while reading the new configuration set");
return ret;
}
/* tsconfig has no notification */
return 0;
tsconfig_send_reply() -> tsconfig_prepare_data() ->
dev_get_hwtstamp_phylib() is the -EOPNOTSUPP that gets filtered out, so
the handler returns 0.
Two consequences follow. First, the user space contradiction the commit
message sets out to remove seems to get worse rather than better:
TSCONFIG_GET on the very same device still fails with -EOPNOTSUPP from
tsconfig_prepare_data(), and SIOCSHWTSTAMP still fails in
dev_set_hwtstamp_phylib() at "if (!ops->ndo_hwtstamp_set)", while
TSCONFIG_SET now reports success.
Second, a successful do operation completes with a bare ACK and no
ETHTOOL_MSG_TSCONFIG_SET_REPLY, although the spec declares a full reply:
Documentation/netlink/specs/ethtool.yaml, tsconfig-set:
do:
request:
attributes: *tsconfig
reply:
attributes: *tsconfig
That is the case commit e8ab231782e9 ("net: ethtool: tsconfig: set command
must provide a reply") closed.
The commit message says "Drop both checks and let the phylib helpers
decide, like the ioctls do." On the SET path, is the phylib helper
consulted at all when config_mod is false? Would narrowing the gate to
reject only when neither a PHY nor an NDO provider can serve the device, or
building the reply from the configuration SET already has, keep both the
error and the reply semantics intact?
Could the change in SET error and reply semantics also be mentioned in the
commit message?
> static struct hwtstamp_provider *
> tsconfig_set_hwprov_from_desc(struct net_device *dev,
> struct genl_info *info,
> @@ -459,6 +445,5 @@ const struct ethnl_request_ops ethnl_tsconfig_request_ops = {
> .reply_size = tsconfig_reply_size,
> .fill_reply = tsconfig_fill_reply,
>
> - .set_validate = ethnl_set_tsconfig_validate,
> .set = ethnl_set_tsconfig,
> };
[Severity: Medium]
This isn't a bug introduced by this patch, but the dev->hwprov lifetime
looks unbalanced and dropping .set_validate widens the set of devices that
can reach the install path.
tsconfig_set_hwprov_from_desc() kzalloc's the provider and
ethnl_set_tsconfig() publishes it, freeing only the one it replaced:
__hwprov = rcu_replace_pointer(dev->hwprov, hwprov,
netdev_is_locked_ops_compat(dev));
if (__hwprov)
kfree_rcu(__hwprov, rcu_head);
The only teardown-time free is in phy_detach():
hwprov = rtnl_dereference(dev->hwprov);
/* Disable timestamp if it is the one selected */
if (hwprov && hwprov->phydev == phydev) {
rcu_assign_pointer(dev->hwprov, NULL);
kfree_rcu(hwprov, rcu_head);
}
A HWTSTAMP_SOURCE_NETDEV provider has phydev == NULL, so that branch never
matches, and neither unregister_netdevice() nor free_netdev() releases
dev->hwprov. Does the allocation then outlive the struct net_device when an
interface with a netdev-source provider selected is unbound or its netns
goes away?
With the ndo_hwtstamp_get/set gate gone, provider selection now depends only
on ethtool_ops->get_ts_info() plus net_support_hwtstamp_qualifier(), so more
devices can install such a provider.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922070813.1458713-1-nb%40tipi-net.de
^ permalink raw reply [flat|nested] 5+ messages in thread