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

[netdev-bot+sashiko@kernel.org]

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