Message-ID: <000201db2e2d$82ad67d0$88083770$@huawei.com>
Date: Sun, 3 Nov 2024 22:17:55 +0200
From: Gur Stavi <gur.stavi@...wei.com>
To: 'Andrew Lunn' <andrew@...n.ch>
CC: 'Jakub Kicinski' <kuba@...nel.org>, "Gongfan (Eric, Chip)"
	<gongfan1@...wei.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, Cai Huoqing
	<cai.huoqing@...ux.dev>, "Guoxin (D)" <guoxin09@...wei.com>, shenchenyang
	<shenchenyang1@...ilicon.com>, "zhoushuai (A)" <zhoushuai28@...wei.com>,
	"Wulike (Collin)" <wulike1@...wei.com>, "shijing (A)" <shijing34@...wei.com>,
	Meny Yossefi <meny.yossefi@...wei.com>
Subject: RE: [RFC net-next v01 1/1] net: hinic3: Add a driver for Huawei 3rd gen NIC

> On Sun, Nov 03, 2024 at 02:29:27PM +0200, Gur Stavi wrote:
> > > On Wed, 30 Oct 2024 14:25:47 +0200 Gur Stavi wrote:
> > > >  50 files changed, 18058 insertions(+)
> > >
> > > 4kLoC is the right ballpark to target for the initial submission.
> > > Please cut this down and submit a minimal driver, then add the
> > > features.
> >
> > Ack.
> > There is indeed code which is not critical to basic Ethernet
> functionality
> > that can be postponed to later.
> >
> > Our HW management infrastructure is rather large and contains 2
> separate
> > mechanisms (cmdq+mbox). While I hope we can trim the driver to a VF-
> only
> > version with no ethtool support that will fit the 10KLoC ballpark, the
> 4KLoC
> > goal is probably unrealistic for a functional driver.
> 
> It is really all about making you code attractive to reviewers. No
> reviewer is likely to have time to review a single 10KLoc patch. A

The minimal work for a functional Ethernet driver when responding to
probe is:
1. Initialize device management
2. Create IO queues using device management
3. Register interrupts
4. Create netdev on top of the io queues
5. Handle TX+RX

At the moment, just our TX+RX code is ~4KLoC.

> reviewer is more likely to look at the code if it is broken up into 15
> smaller patches, each one which can be reviewed in a coffee break

We can obviously break a larger submission into multiple patches if
requested.
But when researching we saw a comment from David that indicated that there
is no advantage in that for initial submissions.
https://lore.kernel.org/netdev/20170817.220343.905568389038615738.davem@dave
mloft.net/
Quote:
"And this is a fine way to add a huge new driver (although not my
personal preference)."

Breaking a 10KLoC submission into a few 4KLcC (or less) patches helps to
review specific patches (and ignore other patches) but all lines still need
to be approved at once so someone must review them.

Breaking 10KLoC into multiple submissions is easier to review and approve
(in parts), but merged code will be non-functional until the last
submission.
It will compile fine, do no harm, and nobody will pick it except for allyes
builds.

> etc. Also, reviewers have interests. I personally have no interest in
> mailbox APIs, actually moving frames around, etc. I want to easily
> find the ethtool code, have you got pause wrong like nearly everybody
> does, are the statistics correctly broken up into the standard groups,

To properly review (error prone) pause it would be better to remove it from
initial submission and add it in a later dedicated submission.

> are you abusing debugfs? Having little patches with good subject lines
> will draw me towards the patches i want to review.
> 
> 10KLoC is still on the large size. Can you throw VF out, it is just a
> plain borring single function device, like the good old e1000e?

VF driver is just like good old e1000e NIC. It is a driver that registers
A single PCI vid:did, "unaware" that its part of SRIOV, and performs the
initialization sequence described above when probed.

> 
> 	Andrew

Powered by blists - more mailing lists