Re: [RFC PATCH net-next 0/7] 802.1Q Frame Preemption and 802.3 MAC Merge support via ethtool

Vladimir Oltean
2022-10-03T14:51:25Z

On Mon, Oct 03, 2022 at 07:36:03AM -0700, Jakub Kicinski wrote:
> On Sat, 1 Oct 2022 15:53:38 +0000 Vladimir Oltean wrote:
> > > Add a attribute to ETHTOOL_MSG_STATS_GET, let's call it
> > > ETHTOOL_A_STATS_EXPRESS, a flag.
> >
> > I'll add this to the UAPI and to internal data structures, ok?
> >
> > enum ethtool_stats_src {
> > ETHTOOL_STATS_SRC_AGGREGATE = 0,
> > ETHTOOL_STATS_SRC_EMAC,
> > ETHTOOL_STATS_SRC_PMAC,
> > };
>
> Yup!

Ok. I've also added enum ethtool_stats_src as the first member of struct
ethtool_eth_mac_stats, ethtool_eth_phy_stats, ethtool_eth_ctrl_stats,
ethtool_pause_stats, ethtool_rmon_stats. So I am not adding an extra
argument (another "structure for future extensibility" as you wrote
below). Hope that's ok.

> > > Plumb thru to all the stats callback an extra argument
> > > (a structure for future extensibility) with a bool pMAC;
> > >
> > > Add a capability field to ethtool_ops to announce that
> > > driver will pay attention to the bool pMAC / has support.
> >
> > You mean capability field as in ethtool_ops::supported_coalesce_params,
> > right? (we discussed about this separately).
> > This won't fit the enetc driver very well. Some enetc ports on the NXP
> > LS1028A support the MM layer (port 0, port 2) and some don't (port 1,
> > port 3). Yet they share the same PF driver. So populating mm_supported =
> > true in the const struct enetc_pf_ethtool_ops isn't going to cover both.
> > I can, however, key on my ethtool_ops :: get_mm_state() function which
> > lets the driver report a "bool supported". Is this ok?
>
> That happens, I think about the capability in the ops as driver caps
> rather than HW caps. The driver can still return -EOPNOTSUPP, but it
> guarantees to check the field's value.

The stats callbacks return void. We'd be relying on the ETHTOOL_STAT_NOT_SET value.

>
> Most (all but one) datacenter NIC vendors have uber-drivers for all
> their HW generations these days, static per-driver caps can't map to
> HW caps in my world.
>
> So weak preference for sticking to that model to avoid confusion about
> the semantics of existing caps vs caps which should use a function call.

An even bigger uber-driver is DSA, with its own dsa_slave_ethtool_ops.
If I put "supported_mm" in ethtool_ops, and set it to true in DSA,
I become responsible for rejecting everything except ETHTOOL_STATS_SRC_AGGREGATE
for all DSA drivers, which I'd rather not do. Alternatively, I put it to
false in DSA and I won't have pMAC stats callbacks getting called even
if I do support a pMAC. Maybe DSA isn't even the only one in this situation.