[PATCH net v2 1/2] net: ethtool: don't match a hwtstamp provider on a device without a PHC

[PATCH net v2 1/2] net: ethtool: don't match a hwtstamp provider on a device without a PHC

[Nicolai Buchwitz]

A device without a PHC reports phc_index -1. A request for index
0xFFFFFFFF is stored in an int and becomes -1 too, so the two match and
a netdev provider is selected on a device that has none.

Fix this and only match a real PHC index.

Fixes: b9e3f7dc9ed9 ("net: ethtool: tsinfo: Enhance tsinfo to support several hwtstamp by net topology")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 net/ethtool/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ethtool/common.c b/net/ethtool/common.c
index 23db40618fed..23e8282563cc 100644
--- a/net/ethtool/common.c
+++ b/net/ethtool/common.c
@@ -980,7 +980,7 @@ int ethtool_net_get_ts_info_by_phc(struct net_device *dev,
 	if (err)
 		return err;
 
-	if (info->phc_index == hwprov_desc->index &&
+	if (info->phc_index >= 0 && info->phc_index == hwprov_desc->index &&
 	    net_support_hwtstamp_qualifier(dev, hwprov_desc->qualifier))
 		return 0;
 
-- 
2.53.0

[PATCH net v2 2/2] net: ethtool: let tsconfig reach a PHY-only timestamp provider

[Nicolai Buchwitz]

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 the check on the get side, dev_get_hwtstamp_phylib() already fails
when there is no provider. On the set side also accept the default
timestamping PHY and move the check into ethnl_set_tsconfig() as the
validate callback runs without rtnl.

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>
---
 net/core/dev_ioctl.c   |  3 ++-
 net/ethtool/tsconfig.c | 20 +++++---------------
 2 files changed, 7 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..2b1cdd9b5a23 100644
--- a/net/ethtool/tsconfig.c
+++ b/net/ethtool/tsconfig.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 
 #include <linux/net_tstamp.h>
+#include <linux/phy.h>
 #include <linux/ptp_clock_kernel.h>
 #include <net/netdev_lock.h>
 
@@ -42,9 +43,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 +246,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,
@@ -313,6 +300,10 @@ static int ethnl_set_tsconfig(struct ethnl_req_info *req_base,
 	if (!netif_device_present(dev))
 		return -ENODEV;
 
+	if (!dev->netdev_ops->ndo_hwtstamp_set &&
+	    !phy_is_default_hwtstamp(dev->phydev))
+		return -EOPNOTSUPP;
+
 	if (tb[ETHTOOL_A_TSCONFIG_HWTSTAMP_PROVIDER]) {
 		struct hwtstamp_provider_desc __hwprov_desc = {.index = -1};
 		struct hwtstamp_provider *__hwprov;
@@ -459,6 +450,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,
 };
-- 
2.53.0

Re: [PATCH net v2 1/2] net: ethtool: don't match a hwtstamp provider on a device without a PHC

[Nicolai Buchwitz]

+CC Maxime & Bjorn

On 23.9.2026 14:16, Nicolai Buchwitz wrote:
> A device without a PHC reports phc_index -1. A request for index
> 0xFFFFFFFF is stored in an int and becomes -1 too, so the two match and
> a netdev provider is selected on a device that has none.
> 
> Fix this and only match a real PHC index.
> 
> Fixes: b9e3f7dc9ed9 ("net: ethtool: tsinfo: Enhance tsinfo to support 
> several hwtstamp by net topology")
> Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
> ---
>  net/ethtool/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 23db40618fed..23e8282563cc 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -980,7 +980,7 @@ int ethtool_net_get_ts_info_by_phc(struct 
> net_device *dev,
>  	if (err)
>  		return err;
> 
> -	if (info->phc_index == hwprov_desc->index &&
> +	if (info->phc_index >= 0 && info->phc_index == hwprov_desc->index &&
>  	    net_support_hwtstamp_qualifier(dev, hwprov_desc->qualifier))
>  		return 0;

Re: [PATCH net v2 1/2] net: ethtool: don't match a hwtstamp provider on a device without a PHC

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · 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).

- [High] The fix is incomplete.
- [Medium] The patch deals with the 0xFFFFFFFF/-1 aliasing through an
  open-coded check at one comparison site instead of rejecting the value…

--

net: ethtool: don't match a hwtstamp provider on a device without a PHC

This patch adds a phc_index >= 0 check to ethtool_net_get_ts_info_by_phc().
A requested provider index of 0xFFFFFFFF becomes -1 when stored in an int,
and with this check it no longer matches a netdev that reports no PHC.

> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 23db40618fed1..23e8282563cc3 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -980,7 +980,7 @@ int ethtool_net_get_ts_info_by_phc(struct net_device *dev,
>  	if (err)
>  		return err;
>  
> -	if (info->phc_index == hwprov_desc->index &&
> +	if (info->phc_index >= 0 && info->phc_index == hwprov_desc->index &&
>  	    net_support_hwtstamp_qualifier(dev, hwprov_desc->qualifier))
>  		return 0;
>  

[Severity: High]
Can the same -1 == -1 match still happen in the PHY half of the lookup?

ethtool_phy_get_ts_info_by_phc() in net/ethtool/common.c still compares on
equality alone. It does this in the link topology loop:

	xa_for_each(&dev->link_topo->phys, phy_index, pdn) {
		...
		ethtool_init_tsinfo(info);
		err = phy_ts_info(pdn->phy, info);
		...
		if (info->phc_index == hwprov_desc->index)
			return pdn->phy;
	}

It does the same in the dev->phydev fallback:

	if (info->phc_index == hwprov_desc->index)
		return dev->phydev;

ethtool_init_tsinfo() sets phc_index to -1. So a PHY whose ts_info callback
succeeds without reporting a PHC would match a request for 0xFFFFFFFF.

With this change, the netdev matcher always returns -ENODEV for index -1.
tsconfig_set_hwprov_from_desc() then falls back to the PHY matcher:

	ret = ethtool_net_get_ts_info_by_phc(dev, &ts_info, hwprov_desc);
	if (!ret) {
		/* Found */
		source = HWTSTAMP_SOURCE_NETDEV;
	} else {
		phy = ethtool_phy_get_ts_info_by_phc(dev, &ts_info, hwprov_desc);
		...
		source = HWTSTAMP_SOURCE_PHYLIB;
	}

That builds a hwprov with desc.index = -1, source = HWTSTAMP_SOURCE_PHYLIB,
and phydev pointing at the PHY that has no PHC. ethnl_set_tsconfig() then
installs it in dev->hwprov.

Wouldn't a provider still be selected on hardware that has no PHC, just a
PHYLIB one instead of a NETDEV one? The commit message says "only match a
real PHC index".

One in-tree example looks to be ines_ts_info() in drivers/ptp/ptp_ines.c.
It never writes phc_index and returns 0. The INES timestamper is attached
as phy->mii_ts from the device tree in fwnode_mdiobus_register_phy(), so
phy_has_tsinfo() is true for that PHY.

With CONFIG_PTP_1588_CLOCK=n, bcm-phy-ptp and mscc_ptp also install mii_ts
and report ptp_clock_index(NULL), which is -1.

micrel's lan8841_ts_info() can also report -1. However, its probe only sets
phydev->mii_ts when ptp_clock is non-NULL, so it doesn't seem to be a
trigger.

Here is a possible sequence on a MAC with a real PHC N and a PHY with an
INES mii_ts:

  ETHTOOL_MSG_TSCONFIG_SET provider {N, PRECISE}
    dev->hwprov = {N, PRECISE, NETDEV}
  ETHTOOL_MSG_TSCONFIG_SET provider {0xFFFFFFFF, PRECISE}
    ethnl_update_u32() sets hwprov_mod since N != 0xFFFFFFFF
    ethtool_net_get_ts_info_by_phc() returns -ENODEV
    ethtool_phy_get_ts_info_by_phc() matches -1 == -1
    dev->hwprov = {-1, PRECISE, PHYLIB, INES phy}

TSCONFIG GET only emits the provider when index >= 0, so userspace would
not see this selection afterwards.

The next patch in this series, "net: ethtool: let tsconfig reach a
PHY-only timestamp provider", doesn't change either PHY match site or the
policy. It also drops the ndo_hwtstamp_set/get requirement, so more
devices could reach this fallback.

Should the same >= 0 check also go into both PHY match sites?

[Severity: Medium]
Would it be better to reject 0xFFFFFFFF at the netlink boundary instead of
adding an open-coded check at one comparison site?

The policy in net/ethtool/ts.h leaves the index unbounded:

	[ETHTOOL_A_TS_HWTSTAMP_PROVIDER_INDEX] = { .type = NLA_U32 },

Documentation/netlink/specs/ethtool.yaml also describes the index as a
plain u32. ts_parse_hwtst_provider() copies it into the int
hwtstamp_provider_desc.index, where -1 is also the "no provider requested"
sentinel.

This aliasing is still visible in ETHTOOL_MSG_TSINFO_GET after the patch.
tsinfo_parse_request() starts with:

	req->hwprov_desc.index = -1;

A user-supplied index of 0xFFFFFFFF overwrites that with the same value, so
tsinfo_prepare_data() skips the provider lookup:

	if (req->hwprov_desc.index != -1) {
		ret = ethtool_get_ts_info_by_phc(dev, &data->ts_info,
						 &req->hwprov_desc);
		...
	}

It then returns the default provider's tsinfo instead of an error, even
though the request named a provider.

NLA_POLICY_MAX(NLA_U32, INT_MAX) for ETHTOOL_A_TS_HWTSTAMP_PROVIDER_INDEX
would stop -1 from ever arriving from userspace, for both tsconfig SET and
tsinfo GET. It would cover the PHY match sites too, and the bad attribute
would be reported through extack.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923121644.2194183-2-nb%40tipi-net.de

Re: [PATCH net v2 2/2] net: ethtool: let tsconfig reach a PHY-only timestamp provider

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

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] ethnl_set_tsconfig() can now install an HWTSTAMP_SOURCE_NETDEV
  provider on a device that has no hwtstamp NDOs, and that provider can…
- [Low] The new TSCONFIG_SET gate in ethnl_set_tsconfig()
  (tsconfig.c:303-305) looks only at whether dev->phydev is the default…

--

net: ethtool: let tsconfig reach a PHY-only timestamp provider

This commit lets TSCONFIG_GET and TSCONFIG_SET work on devices that have
no hwtstamp NDOs but whose PHY can timestamp. It drops the NDO check on the
get side. It replaces the set_validate callback with a check inside
ethnl_set_tsconfig() that also accepts a default timestamping PHY, and it
adds a NULL check for ndo_hwtstamp_get in dev_get_hwtstamp_phylib().

> 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]

The commit message says "this makes a netdev provider reachable without
ndo_hwtstamp_get". Should a netdev provider be reachable at all on a device
without the NDOs?

With ethnl_set_tsconfig_validate() removed, the only gate left is the new
check in ethnl_set_tsconfig(). That check lets an NDO-less device through
whenever dev->phydev is the default timestamping PHY.

After that, tsconfig_set_hwprov_from_desc() chooses the netdev source based
only on a PHC index match:

	ret = ethtool_net_get_ts_info_by_phc(dev, &ts_info, hwprov_desc);
	if (!ret) {
		/* Found */
		source = HWTSTAMP_SOURCE_NETDEV;

ethtool_net_get_ts_info_by_phc() compares only the get_ts_info phc_index and
the qualifier. It never looks at the NDOs.

Suppose user space selects the MAC PHC on such a device. Then:

ethnl_set_tsconfig()
  dev_set_hwtstamp_phylib(dev, &zero_config, ...)
    /* no hwprov yet, goes to default PHY, disables PHY timestamping */
  rcu_replace_pointer(dev->hwprov, hwprov, ...)
    /* NETDEV hwprov is committed */
  if (config_mod)
    dev_set_hwtstamp_phylib(dev, &hwtst_config, ...)
      if (!ops->ndo_hwtstamp_set)
          return -EOPNOTSUPP;

That error is returned, but the provider is not rolled back.

If no config attributes are given, tsconfig_send_reply() gets -EOPNOTSUPP
from the new guard above, and ethnl_set_tsconfig() ignores it:

	ret = tsconfig_send_reply(dev, info);
	if (ret && ret != -EOPNOTSUPP) {

The request then returns 0 and no reply is sent.

Can the device get out of this state afterwards? Switching back to the PHY
runs the zero_config disable through the NETDEV branch of
dev_set_hwtstamp_phylib(). That fails at the same !ops->ndo_hwtstamp_set
check.

SIOCSHWTSTAMP, SIOCGHWTSTAMP and TSCONFIG_GET also return -EOPNOTSUPP. Both
skb_defer_rx_timestamp() and skb_clone_tx_timestamp() skip the PHY because
hwprov->source is not PHYLIB. phy_detach() clears dev->hwprov only when
hwprov->phydev matches, so it never clears this provider.

As far as I can tell, PHY timestamping stays off until the netdev is
re-created.

I could not find an in-tree driver that meets all the conditions. ena and
netdevsim report a PHC without the NDOs, but they have no phydev. The
commit message does suggest this state is expected to be reachable, though.

Would it be better to reject a netdev source in
tsconfig_set_hwprov_from_desc() or ethnl_set_tsconfig() when ndo_hwtstamp_set
or ndo_hwtstamp_get is missing, rather than only guarding the call here?

> diff --git a/net/ethtool/tsconfig.c b/net/ethtool/tsconfig.c
> index 6be3aa5d4bc1e..2b1cdd9b5a232 100644
> --- a/net/ethtool/tsconfig.c
> +++ b/net/ethtool/tsconfig.c

[ ... ]

> @@ -313,6 +300,10 @@ static int ethnl_set_tsconfig(struct ethnl_req_info *req_base,
>  	if (!netif_device_present(dev))
>  		return -ENODEV;
>  
> +	if (!dev->netdev_ops->ndo_hwtstamp_set &&
> +	    !phy_is_default_hwtstamp(dev->phydev))
> +		return -EOPNOTSUPP;
> +

[Severity: Low]

This check runs before dev->hwprov or the requested provider is looked at,
and it only considers dev->phydev. Could it reject requests that an already
installed PHYLIB provider can serve?

dev_set_hwtstamp_phylib() serves an installed PHYLIB hwprov without needing
any NDO:

	if (hwprov) {
		if (hwprov->source == HWTSTAMP_SOURCE_PHYLIB &&
		    hwprov->phydev) {
			phy_ts = true;
			phydev = hwprov->phydev;

ethtool_phy_get_ts_info_by_phc() also accepts any timestamping PHY in
dev->link_topo. It does not require default_timestamp.

Take an NDO-less MAC whose dev->phydev is a default timestamper. A PHYLIB
hwprov for a different topology PHY, such as an SFP module PHY, can be
selected on it.

Some drivers disconnect the PHY in ndo_stop. If dev->phydev is detached that
way, phy_detach() leaves the hwprov alone because hwprov->phydev != phydev:

	if (hwprov && hwprov->phydev == phydev) {
		rcu_assign_pointer(dev->hwprov, NULL);

>From then on SIOCSHWTSTAMP still works through hwprov->phydev. TSCONFIG_SET
fails with -EOPNOTSUPP at this check because dev->phydev is NULL. That is the
ioctl/netlink mismatch the commit sets out to remove.

This does not apply when a non-default PHY is selected for the first time.
In that case the zero_config disable step fails with or without this check,
so the check only matters once a PHYLIB hwprov is already installed. I also
have not confirmed the SFP and link topology lifetime details, so this
scenario may be narrower than described.

Would it make sense to move this check after provider resolution and also
accept an installed or requested PHYLIB provider?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923121644.2194183-2-nb%40tipi-net.de