Re: [PATCH net-next RFC] Add NDOs for hardware timestamp get/set

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Fri, Mar 31, 2023 at 11:10:41AM -0700, Jakub Kicinski wrote:
> On Fri, 31 Mar 2023 11:51:06 -0600 Max Georgiev wrote:
> > On Thu, Mar 30, 2023 at 11:35 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > I wonder if we should pass in
> > >
> > >         struct netlink_ext_ack *extack
> > >
> > > and maybe another structure for future extensions?
> > > So we don't have to change the drivers again when we extend uAPI.  
> > 
> > Would these two extra parameters be ignored by drivers in this initial
> > version of NDO hw timestamp API implementation?
> 
> Yup, and passed in as NULL.
> 
> See struct kernel_ethtool_coalesce for example of a kernel side
> structure extending a fixed-size uAPI struct ethtool_coalesce.
> 
> So we would add a struct kernel_hwtstamp_config which would be 
> empty for now, but we can make it not empty later.
> 
> Vladimir, does that sound reasonable or am I over-thinking?

So in principle I'm okay with the NULL extack (even though we could consider
doing something with the netlink message instead of letting it go to waste;
a suggestion may be to print the _msg to the kernel log, like store_bridge_parm()
does).

I missed the discussions around the creation of struct kernel_ethtool_coalesce,
but I imagine that ethtool_ops->set_coalesce() now takes both struct ethtool_coalesce,
as well as struct kernel_ethtool_coalesce, to avoid refactoring drivers even further
than just patching their function prototype? I don't think that argument would
apply here, for a completely new API?

I'm also okay, in principle, with having a struct kernel_hwtstamp_config, but
I have two problems with the way in which you've suggested it be implemented.
The first is not really only my problem, but rather, I don't think you
can have empty structures in C - I tried!

/* C doesn't allow empty structures, bah! */
struct sja1105_tas_data {
	u8 dummy;
};

The second is that I would actively dislike an ndo_hwtstamp_set() API
where half of the arguments are in one structure and half in the other.
I believe it's much easier, and cleaner, to make struct kernel_hwtstamp_config
duplicate the exact same fields from struct hwtstamp_config, and copy
those fields one by one from one structure to the other (to avoid issues
with UAPI field alignment mismatches). So we could pass only the extensible
kernel_hwtstamp_config to ndo_hwtstamp_set() and ndo_hwtstamp_get().