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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230123143830.60f436ef@kernel.org>
Date:   Mon, 23 Jan 2023 14:38:30 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Aurelien Aptel <aaptel@...dia.com>
Cc:     linux-nvme@...ts.infradead.org, netdev@...r.kernel.org,
        sagi@...mberg.me, hch@....de, kbusch@...nel.org, axboe@...com,
        chaitanyak@...dia.com, davem@...emloft.net,
        aurelien.aptel@...il.com, smalin@...dia.com, malin1024@...il.com,
        ogerlitz@...dia.com, yorayz@...dia.com, borisp@...dia.com
Subject: Re: [PATCH v9 03/25] net/ethtool: add ULP_DDP_{GET,SET} operations
 for caps and stats

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.