Re: [PATCH net] net: bcmgenet: add hwtstamp callbacks to restore PHY timestamping

[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] Fix applied at the wrong layer: the regression the commit
  message diagnoses ("the core rejects timestamp configuration requests…
- [Medium] Registering ndo_hwtstamp_get/ndo_hwtstamp_set on a MAC that
  has no PHC opens a one-way trap on the ETHTOOL_MSG_TSCONFIG_SET path.

--

net: bcmgenet: add hwtstamp callbacks to restore PHY timestamping

This adds ndo_hwtstamp_get() and ndo_hwtstamp_set() stubs to bcmgenet that
both return -EOPNOTSUPP, so that the core hwtstamp entry points no longer
reject SIOCSHWTSTAMP/SIOCGHWTSTAMP before the phylib helpers get a chance to
dispatch to a timestamp-capable PHY.  No other behaviour is changed.

> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> index b916080f4ff1..98d861811328 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
> @@ -3721,6 +3721,19 @@ static int bcmgenet_change_carrier(struct net_device *dev, bool new_carrier)
>  	return 0;
>  }
>  
> +static int bcmgenet_hwtstamp_get(struct net_device *dev,
> +				 struct kernel_hwtstamp_config *cfg)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int bcmgenet_hwtstamp_set(struct net_device *dev,
> +				 struct kernel_hwtstamp_config *cfg,
> +				 struct netlink_ext_ack *extack)
> +{
> +	return -EOPNOTSUPP;
> +}
> +

[Severity: High]

Should this be fixed in net/core instead of per driver?  The commit message
describes the failure as "the core rejects timestamp configuration requests
before they reach the PHY", and the rejecting code is provider independent:

net/core/dev_ioctl.c:dev_get_hwtstamp() {
	if (!ops->ndo_hwtstamp_get)
		return -EOPNOTSUPP;
	...
}

net/core/dev_ioctl.c:dev_set_hwtstamp() {
	...
	if (!ops->ndo_hwtstamp_set)
		return -EOPNOTSUPP;
	...
}

The same presence test exists in generic_hwtstamp_get_lower(),
generic_hwtstamp_set_lower() and in ethnl_set_tsconfig_validate()
(net/ethtool/tsconfig.c):

	if (!ops->ndo_hwtstamp_set || !ops->ndo_hwtstamp_get)
		return -EOPNOTSUPP;

These are the lines 5062245a5a7f turned from a dev_eth_ioctl() fallback into
a hard error, so every MAC whose PTP comes only from the PHY is affected, not
just bcmgenet.

Two drivers in the same directory still have no ndo_hwtstamp_* and rely on
phylib ioctl handling:

drivers/net/ethernet/broadcom/bgmac.c:
	.ndo_eth_ioctl           = phy_do_ioctl_running,

drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c:
	.ndo_eth_ioctl		= phy_do_ioctl_running,

arch/arm/boot/dts/broadcom/bcm47189-luxul-xap-1440.dts wires bgmac's gmac0 to
a BCM54210E, and that PHY driver calls bcm_ptp_probe() and sets
phydev->default_timestamp = true, so that board still fails SIOCSHWTSTAMP
after this patch.  Would skipping the NDO presence test when
phy_is_default_hwtstamp(dev->phydev) is true cover all of these at once?

A related question about the stubs themselves: since dev_set_hwtstamp_phylib()
only calls the MAC callback when !phy_ts or dev->see_all_hwtstamp_requests,
and bcmgenet sets neither, are these two functions ever invoked in the PHY
case?  If they exist purely as a capability signal, would a comment saying so
help, so a later "these always fail, remove them" cleanup does not silently
bring the failure back?

>  static const struct net_device_ops bcmgenet_netdev_ops = {
>  	.ndo_open		= bcmgenet_open,
>  	.ndo_stop		= bcmgenet_close,
> @@ -3732,6 +3745,8 @@ static const struct net_device_ops bcmgenet_netdev_ops = {
>  	.ndo_set_features	= bcmgenet_set_features,
>  	.ndo_get_stats64	= bcmgenet_get_stats64,
>  	.ndo_change_carrier	= bcmgenet_change_carrier,
> +	.ndo_hwtstamp_get	= bcmgenet_hwtstamp_get,
> +	.ndo_hwtstamp_set	= bcmgenet_hwtstamp_set,
>  };

[Severity: Medium]

Can advertising these NDOs on a MAC without a PHC make the
ETHTOOL_MSG_TSCONFIG_SET path reachable in a way that cannot be undone?

bcmgenet uses .get_ts_info = ethtool_op_get_ts_info, which reports
info->phc_index = -1.  Before this patch ethnl_set_tsconfig_validate()
returned -EOPNOTSUPP for bcmgenet, so ethnl_set_tsconfig() was never reached.
With both NDOs present, validation passes.

Consider this sequence:

1. TSCONFIG_SET selecting the PHY provider (index = the PHY phc_index), so
   dev->hwprov is non-NULL and PHY timestamping works.

2. TSCONFIG_SET with ETHTOOL_A_TS_HWTSTAMP_PROVIDER_INDEX = 0xFFFFFFFF and
   QUALIFIER = 0.  ts_parse_hwtst_provider() stores that through
   ethnl_update_u32() into the int index field of struct
   hwtstamp_provider_desc, giving -1.

net/ethtool/common.c:ethtool_net_get_ts_info_by_phc() {
	...
	if (info->phc_index == hwprov_desc->index &&
	    net_support_hwtstamp_qualifier(dev, hwprov_desc->qualifier))
		return 0;
	...
}

-1 == -1 matches, and net_support_hwtstamp_qualifier() returns true because
bcmgenet declares no supported_hwtstamp_qualifiers, so the provider becomes
HWTSTAMP_SOURCE_NETDEV with phydev = NULL.

ethnl_set_tsconfig() first disables the working PHY timestamper with
dev_set_hwtstamp_phylib(zero_config) and then installs that provider:

net/ethtool/tsconfig.c:ethnl_set_tsconfig() {
	...
		__hwprov = rcu_replace_pointer(dev->hwprov, hwprov,
					       netdev_is_locked_ops_compat(dev));
	...
}

>From there dev_get_hwtstamp_phylib() and dev_set_hwtstamp_phylib() take the
HWTSTAMP_SOURCE_NETDEV branch:

net/core/dev_ioctl.c:dev_set_hwtstamp_phylib() {
	...
		} else if (hwprov->source == HWTSTAMP_SOURCE_NETDEV) {
			phy_ts = false;
	...
}

so the PHY is never consulted and bcmgenet_hwtstamp_set() returns
-EOPNOTSUPP.

Does that also block recovery?  Reselecting the PHY provider runs the
zero-config disable step first, which now hits the NETDEV branch and the stub,
so ethnl_set_tsconfig() bails out through err_free_hwprov without replacing
dev->hwprov.  dev->hwprov is only cleared in phy_detach() when
hwprov->phydev matches the detaching phydev, which is NULL here, so ifdown
and ifup would not clear it either.

The permissive -1 comparison itself lives in core and predates this change,
but is this patch what makes it reachable for bcmgenet?  Would declaring a
real .get_ts_info (or rejecting an index of -1) avoid it?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918030149.80398-1-jjc%40jclark.com