linux-kernel - Re: [PATCH net-next v6] net: dlink: add support for reporting stats via `ethtool -S` and `ip -s -s link show`

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] [day] [month] [year] [list]

Message-ID: <dc15f02e-8411-4f22-b502-fad2cad1870f@lunn.ch>
Date: Tue, 8 Apr 2025 14:37:55 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Moon Yeounsu <yyyynoom@...il.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6] net: dlink: add support for reporting stats
 via `ethtool -S` and `ip -s -s link show`

On Tue, Apr 08, 2025 at 07:57:55AM +0900, Moon Yeounsu wrote:
> On Mon, Apr 07, 2025 at 10:48:01PM +0200, Andrew Lunn wrote:
> > When i see a list like this, it makes me think this should be broken
> > up into multiple patches. Ideally you want lots of simple patches
> > which are obviously correct.
> 
> Would it be appropriate to split this into a patchset, then?
> To be honest, this is my first time creating a patchset, so
> I'm not entirely sure how to divide it properly.
> 
> For now, I'm thinking of splitting it as follows:
> 	1. stat definitions and declarations
> 	2. preprocessor directives (`#ifdef`)
> 	3. `spin_[un]lock_irq()` related changes
> 	4. `get_stats()` implementation
> 	5. `ethtool_ops` implementation
> 
> Is it okay to resend the v7 patchset split as above?

You trimmed too much context, you took away the list, so it is hard
for me to reply. Trimming is good, but thing about what is needed for
the conversation.

One obvious patch is to remove the #ifdef about MMIO. That is one
logical thing.

If the spin_[un]lock_irq() is about existing code, yes that could be a
patch. Does it make sense on its own?

1, 4, and 5 probably go together, since they are one logical thing.

Since you are new, it is worth spending some time reading other
patches on the mailing list and the review comments they get. You can
learn a lot that way.

	Andrew