Re: [PATCH net-next v19 03/10] ptp: Add phc source and helpers to register specific PTP clock or get information

[Kory Maincent <kory.maincent@bootlin.com>]

On Tue, 12 Nov 2024 18:22:26 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Tue, 12 Nov 2024 11:12:32 +0100 Kory Maincent wrote:
> > > Storing the info about the "user" (netdev, phydev) in the "provider"
> > > (PHC) feels too much like a layering violation. Why do you need this?    
> > 
> > The things is that, the way to manage the phc depends on the "user".
> > ndo_hwtstamp_set for netdev and phy_hwtstamp_set for phydev.
> > https://elixir.bootlin.com/linux/v6.11.6/source/net/core/dev_ioctl.c#L323
> > 
> > Before PHC was managed by the driver "user" so there was no need for this
> > information as the core only gives the task to the single "user". This
> > didn't really works when there is more than one user possible on the net
> > topology.  
> 
> I don't understand. I'm complaining storing netdev state in 
> struct ptp_clock. It's perfectly fine to add the extra info to netdev
> and PHY topology maintained by the core.

Oh okay. Didn't get it the first time. I could move the PHC source with the
phydev pointer in netdev core to avoid this.

> > > In general I can't shake the feeling that we're trying to configure 
> > > the "default" PHC for a narrow use case, while the goal should be 
> > > to let the user pick the PHC per socket.    
> > 
> > Indeed PHC per socket would be neat but it would need a lot more work and I
> > am even not sure how it should be done. Maybe with a new cmsg structure
> > containing the information of the PHC provider?
> > In any case the new ETHTOOL UAPI is ready to support multiple PHC at the
> > same time when it will be supported.
> > This patch series is something in the middle, being able to enable all the
> > PHC on a net topology but only one at a time.  
> 
> I understand, I don't want to push you towards implementing all that.
> But if we keep that in mind as the north star we should try to align
> this default / temporary solution. If the socket API takes a PHC ID
> as an input, the configuration we take in should also be maintained
> as "int default_phc", not pointers to things.

In fact I could remove the hwtstamp pointer from netdevice and add only the
hwtstamp source with the phydev rcu pointer.
We will need the phydev pointer as we don't know to which phy device belong the
PTP. Or we could also use the phyindex instead of the phydev pointer and use
phy_link_topo_get_phy() in the hot path.

> IOW I'm struggling to connect the dots how the code you're adding now
> will be built _upon_ rather than _on the side_ of when socket PHC
> selection is in place.

I see what you mean! It is not something easy to think of as I don't really
know how it would be implemented.
Do you think adding simply the PHC source and the phydev pointer or index would
fit? 

This could be removed from netdev core when we move to socket PHC as it
won't be necessary to save the current PHC.

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