[PATCH v9 03/25] net/ethtool: add ULP_DDP_{GET,SET} operations for caps and stats

[Jakub Kicinski]

On Mon, 23 Jan 2023 20:36:21 +0200 Aurelien Aptel wrote:
> >> Compact statistics are nested as follows:
> >>
> >>     STATS (nest)
> >>         COUNT (u32)
> >>         COMPACT_VALUES (array of u64)  
> >
> > That's not how other per-cmd stats work, why are you inventing
> > new ways..  
> 
> As we commented in patch 2, dynamic strings are used for ethtool
> forward-compability (being able to list future stats, which we are
> planning) without updating or recompiling.

But this is not how they should be carried.

The string set is retrieved by a separate command, then you request
a string based on the attribute ID (global_stringset() + get_string() 
in ethtool CLI code).

That way long running code or code dumping muliple interfaces can load
strings once and dumps are kept smaller.

> >> +     int     (*get_ulp_ddp_stats)(struct net_device *dev, struct ethtool_ulp_ddp_stats *stats);
> >> +     int     (*set_ulp_ddp_capabilities)(struct net_device *dev, unsigned long *bits);  
> >
> > Why are these two callbacks not in struct ulp_ddp_dev_ops?  
> 
> We were trying to implement these callbacks in alignment with the
> existing ethtool commands, for this reason we implemented it in the
> ethtool API.

ethtool commands mostly talk to HW, note that the feature configuration
(ethtool -k/-K) does not use ethtool ops either.

> > Why does the ethtool API not expose limits?  
> 
> Originally, and before we started adding the netlink interface, we were
> not planning to include the ability to modify the limits as part of this
> series.  We do agree that it now makes sense, but we will add, some
> limits reflect hardware limitations while other could be tweaked by
> users.  Those limits will be per-device and per-protocol. We will
> suggest how to design it.

Alright, I was mostly curious, it's not a requirement for initial
support.