Re: [PATCH net-next v5 13/16] net: Change the API of PHY default timestamp to MAC

[Florian Fainelli <florian.fainelli@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 3986 bytes --]

On 10/9/23 08:51, Köry Maincent wrote:
> From: Kory Maincent <kory.maincent@bootlin.com>
> 
> Change the API to select MAC default time stamping instead of the PHY.
> Indeed the PHY is closer to the wire therefore theoretically it have less
> delay than the MAC timestamping but the reality is different.

s/have/has/ or you need to make the subject plural.

> Due to lower
> time stamping clock frequency, latency in the MDIO bus and no PHC hardware
> synchronization between different PHY, the PHY PTP is often less precise
> than the MAC. 

-> different PHYs

> The exception is for PHY designed specially for PTP case but
> these board are not very widespread. 

s/board/devices/ maybe?

> For not breaking the compatibility I
> introduce an allowlist to reference all current PHYs that support
> time stamping and let them keep the old API behavior.
> 
> The phy_set_timestamp function is called at each call of phy_attach_direct.
> In case of MAC driver using phylink this function is called when the
> interface is turned up. Then if the interface goes down and up again the
> last choice of timestamp will be overwritten by the default choice.
> A solution could be to cache the timestamp status but it can bring other
> issues. In case of SFP, if we change the module, it doesn't make sense to
> blindly re-set the timestamp back to PHY, if the new module has a PHY with
> mediocre timestamping capabilities.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
> 
> Changes in v5:
> - Extract the API change in this patch.
> - Rename whitelist to allowlist.
> - Set NETDEV_TIMESTAMPING in register_netdevice function.
> - Add software timestamping case description in ts_info.
> ---
>   drivers/net/phy/phy_device.c | 68 ++++++++++++++++++++++++++++++++++++
>   include/linux/netdevice.h    |  5 +++
>   net/core/dev.c               |  3 ++
>   net/core/dev_ioctl.c         | 36 +++++++++++--------
>   net/core/timestamping.c      |  9 +++++
>   net/ethtool/common.c         | 16 +++++++--
>   6 files changed, 121 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 2ce74593d6e4..2d5a6d57acb3 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1411,6 +1411,68 @@ int phy_sfp_probe(struct phy_device *phydev,
>   }
>   EXPORT_SYMBOL(phy_sfp_probe);
>   
> +/* An allowlist for PHYs selected as default timesetamping.
> + * Its use is to keep compatibility with old PTP API which is selecting
> + * these PHYs as default timestamping.
> + * The new API is selecting the MAC as default timestamping.
> + */
> +const char * const phy_timestamping_allowlist[] = {
> +	"Broadcom BCM5411",
> +	"Broadcom BCM5421",
> +	"Broadcom BCM54210E",
> +	"Broadcom BCM5461",
> +	"Broadcom BCM54612E",
> +	"Broadcom BCM5464",
> +	"Broadcom BCM5481",
> +	"Broadcom BCM54810",
> +	"Broadcom BCM54811",
> +	"Broadcom BCM5482",
> +	"Broadcom BCM50610",
> +	"Broadcom BCM50610M",
> +	"Broadcom BCM57780",
> +	"Broadcom BCM5395",
> +	"Broadcom BCM53125",
> +	"Broadcom BCM53128",
> +	"Broadcom BCM89610",

The list of Broadcom PHYs that you have is too broad, if you look at 
drivers/net/phy/bcm-phy-ptp.c only PHY_ID_BCM54210E is actually 
supported as of now.

The allowlist is not maintainable using the PHY device name, especially 
not without having a shared header file that is used by both 
phy_device.c and the individual PHY device driver. You are guaranteed 
that a name change on one side can be missed on the other.

Using PHY OUIs can be a tad complicated with C45 PHYs, so rather, I 
would suggest that each of those PHY device drivers be responsible for 
overidding the timestamping selection, you could even consider inventing 
a specific PHYLIB_TIMESTAMPING_LEGACY and then issue a warning within 
the PHY library to encourage a change (if this is even relevant).

>   #endif
> +
> +	u32			ts_layer;

Likewise, would be preferable to use an enum type.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]