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

From: Florian Fainelli
Date: Mon Oct 09 2023 - 18:24:17 EST

On 10/9/23 08:51, Köry Maincent wrote:
From: Kory Maincent <kory.maincent@xxxxxxxxxxx>

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@xxxxxxxxxxx>
---

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: smime.p7s
Description: S/MIME Cryptographic Signature