On Thu, 30 Oct 2025 12:18:50 +0000
Vadim Fedorenko [off-list ref] wrote:

On 30/10/2025 10:30, Kory Maincent wrote:

quoted

On Wed, 29 Oct 2025 20:09:22 +0000
Vadim Fedorenko [off-list ref] wrote:
  

quoted

Convert TI NetCP driver to use ndo_hwtstamp_get()/ndo_hwtstamp_set()
callbacks. The logic is slightly changed, because I believe the original
logic was not really correct. Config reading part is using the very
first module to get the configuration instead of iterating over all of
them and keep the last one as the configuration is supposed to be identical
for all modules. HW timestamp config set path is now trying to configure
all modules, but in case of error from one module it adds extack
message. This way the configuration will be as synchronized as possible.  

On the case the hwtstamp_set return the extack error the hwtstamp_get will
return something that might not be true as not all module will have the same
config. Is it acceptable?  

Well, technically you are right. But this logic was broken from the very
beginning. And as I also mentioned, both modules use the same set
function with the same error path, which means in current situation if
the first call is successful, the second one will also succeed. And
that's why it's acceptable

quoted

quoted

There are only 2 modules using netcp core infrastructure, and both use
the very same function to configure HW timestamping, so no actual
difference in behavior is expected.

Compile test only.

Signed-off-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
b/drivers/net/ethernet/ti/netcp_ethss.c index 55a1a96cd834..0ae44112812c
100644 --- a/drivers/net/ethernet/ti/netcp_ethss.c

+++ b/drivers/net/ethernet/ti/netcp_ethss.c

@@ -2591,20 +2591,26 @@ static int gbe_rxtstamp(struct gbe_intf *gbe_intf,

struct netcp_packet *p_info) return 0;
  }
  
-static int gbe_hwtstamp_get(struct gbe_intf *gbe_intf, struct ifreq *ifr)
+static int gbe_hwtstamp_get(void *intf_priv, struct kernel_hwtstamp_config
*cfg) {
-	struct gbe_priv *gbe_dev = gbe_intf->gbe_dev;
-	struct cpts *cpts = gbe_dev->cpts;
-	struct hwtstamp_config cfg;
+	struct gbe_intf *gbe_intf = intf_priv;
+	struct gbe_priv *gbe_dev;
+	struct phy_device *phy;
+
+	gbe_dev = gbe_intf->gbe_dev;
  
-	if (!cpts)
+	if (!gbe_dev->cpts)
+		return -EOPNOTSUPP;
+
+	phy = gbe_intf->slave->phy;
+	if (phy_has_hwtstamp(phy))
  		return -EOPNOTSUPP;  

This condition should be removed.
The selection between PHY or MAC timestamping is now done in the net core:
https://elixir.bootlin.com/linux/v6.17.1/source/net/core/dev_ioctl.c#L244  

Yeah, but the problem here is that phy device is not really attached to 
netdev, but rather to some private structure, which is not accessible by
the core, only driver can work with it, according to the original code.

The PHY are either attached to gbe_intf->ndev or gbe_dev->dummy_ndev.
Therefore I think it will already attached to the net_device pointer used by
hwtstamp NDOs.
Mmh indeed each slave can have a different PHY. Why is there several slave + phy
per netdev?
https://elixir.bootlin.com/linux/v6.17.1/source/drivers/net/ethernet/ti/netcp_ethss.c#L3163
Shouldn't each netdev be associated to one PHY (or use the phy topology
support). This is kind of weird.

In either case if there is an issue here, it should be split in a second patch
as you are modifying the behavior of the driver.
Maybe you should already split the patch in two to separate the NDO conversion
from the module iteration logic design change.

But I might be missing something obvious here, if someone is at least a
bit aware of this code and can shed some light and confirm that phydev
is correctly set and attached to actual netdev, I'm happy to remove this
ugly part.

Yeah, this driver seems a bit ugly to me also but maybe we are missing
something. 

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com