Re: [PATCH net-next v7 15/16] net: ethtool: ts: Let the active time stamping layer be selectable

[Köry Maincent]

On Fri, 24 Nov 2023 17:43:43 +0200
Vladimir Oltean <vladimir.oltean@xxxxxxx> wrote:

> On Thu, Nov 23, 2023 at 09:32:05AM -0800, Jakub Kicinski wrote:
> > On Thu, Nov 23, 2023 at 04:00:56PM +0100, Köry Maincent wrote:  
> > > So, do we have a consensus? Vlad, do you agree on putting all under
> > > ethtool?
> > > 
> > > ETHTOOL_GET_TS_INFO will be in charge of replacing the SIOCGHWSTAMP
> > > implementation. Need to add ETHTOOL_A_TSINFO_PHC_INDEX
> > > ETHTOOL_A_TSINFO_QUALIFIER to the request.
> > > 
> > > ETHTOOL_GET_TS_INFO will list all the hwtstamp provider (aka "{phc_index,
> > > qualifier}") through the dumpit callback. I will add a filter to be able
> > > to list only the hwtstamp provider of one netdev.
> > > 
> > > ETHTOOL_SET_TS_INFO will be in charge of replacing the SIOCSHWSTAMP
> > > implementation.  
> > 
> > If not we can do a vote/poll? Maybe others don't find the configuration
> > of timestamping as confusing as me.  
> 
> If you mean the ETHTOOL_MSG_TSINFO_GET netlink message (ETHTOOL_GET_TS_INFO
> is an ioctl), you're saying that you want to move the entire contents of
> SIOCGHWSTAMP there, by making the kernel call ndo_hwtstamp_get() in
> addition to the existing __ethtool_get_ts_info()?

Yes.
 
> Yeah, I don't know, I don't have a real objection, I guess it's fine.
> 
> What will be a bit of an "?!" moment for users is when ethtool gains
> support for the SIOCGHWSTAMP/SIOCSHWSTAMP netlink replacements, but not
> for the original ioctls. So hwstamp_ctl will be able to change timestamping
> configuration, but ethtool wouldn't - all on the same system. Unless
> ethtool gains an ioctl fallback for a ioctl that was never down its alley.

Yes indeed. Would it break things if both ioctls and netlink can get and set
the hwtstamps configuration? It is only configuration. Both happen under
rtnl_lock it should be alright.

The question is which hwtstamp provider will the original ioctls be able to
change? Maybe the default one (MAC with phy whitelist) and only this one.

> But by all means, still hold a poll if you want to. I would vote for
> ethtool netlink, not because it's great, just because I don't have a
> better alternative to propose.

If you agree on that choice, let's go. Jakub and your are the most proactive
reviewers in this patch series. Willem you are the timestamping maintainer do
you also agree on this? 
If anyone have another proposition let them speak now, or forever remain
silent! ;)

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