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

[Maxime Chevallier]

Hi,

On 9/26/26 19:53, Nicolai Buchwitz wrote:
> Hi (Cl|S)ashiko
> 
> On 26.9.2026 15:54, 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] The removed ethnl_set_tsconfig_validate() required both
>>   ndo_hwtstamp_set and ndo_hwtstamp_get.
>>
>> -- 
>>
>> net: ethtool: let tsconfig reach a PHY-only timestamp provider
>>
>> This patch lets TSCONFIG_GET and TSCONFIG_SET accept a device whose
>> timestamping is served by a default timestamping PHY or an already
>> installed hwprov, even when the netdev has no hwtstamp NDOs. It removes
>> the set_validate callback and moves the check into ethnl_set_tsconfig().
>> It also stops selecting a netdev source without ndo_hwtstamp_set, and
>> adds a NULL check for ndo_hwtstamp_get in dev_get_hwtstamp_phylib().
>>
>>> diff --git a/net/ethtool/tsconfig.c b/net/ethtool/tsconfig.c
>>> index 6be3aa5d4bc1e..2db0e7ba8b9f5 100644
>>> --- a/net/ethtool/tsconfig.c
>>> +++ b/net/ethtool/tsconfig.c
> 
>> [...]
> 
>>> @@ -313,6 +305,11 @@ 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) &&
>>> +        !netdev_ops_lock_dereference(dev->hwprov, dev))
>>> +        return -EOPNOTSUPP;
>>> +
>>
>> [Severity: Medium]
>>
>> The removed ethnl_set_tsconfig_validate() required both ndo_hwtstamp_set
>> and ndo_hwtstamp_get. This new check in ethnl_set_tsconfig() only tests
>> ndo_hwtstamp_set. The matching check in tsconfig_prepare_data() only
>> tests ndo_hwtstamp_get.
>>
>> What happens on a netdev that has ndo_hwtstamp_set but no
>> ndo_hwtstamp_get, and has neither a default timestamping PHY nor a
>> hwprov?
> 
> There is no in-tree driver with ndo_hwtstamp_set but not _get. All 73 files
> have them in pairs...
> 
> Even further, the asymmetry is on purpose. The get side tests ndo_hwtstamp_get
> and the set side tests ndo_hwtstamp_set, each gate testing what its own operation
> needs.
> 
> If anyone insists on the symmetry, a v4 could add || !ndo_hwtstamp_get. But before
> I respin again, let's hear if this is really needed.

The current way LGTM :)

BTW, I raised that point a while ago [1] but now that you're properly handling
the "MAC can't TS, but PHY can" case, we may be able to get rid of the
SIOCSHWTSTAMP code in phy_mii_ioctl() ? as a followup of course :)

Maxime

[1] : https://lore.kernel.org/netdev/20260718143848.677531-1-maxime.chevallier@xxxxxxxxxxx/
> 
>> [...]
> 
> Thanks,
> Nicolai