linux-kernel - Re: [PATCH net-next v06 1/1] hinic3: module initialization and tx/rx logic

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: <20250226084124.650275-1-gur.stavi@huawei.com>
Date: Wed, 26 Feb 2025 10:41:24 +0200
From: Gur Stavi <gur.stavi@...wei.com>
To: <jdamato@...tly.com>
CC: <andrew+netdev@...n.ch>, <cai.huoqing@...ux.dev>, <corbet@....net>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <gongfan1@...wei.com>,
	<guoxin09@...wei.com>, <gur.stavi@...wei.com>, <helgaas@...nel.org>,
	<horms@...nel.org>, <kuba@...nel.org>, <linux-doc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <luosifu@...wei.com>,
	<meny.yossefi@...wei.com>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<przemyslaw.kitszel@...el.com>, <shenchenyang1@...ilicon.com>,
	<shijing34@...wei.com>, <sumang@...vell.com>, <wulike1@...wei.com>,
	<zhoushuai28@...wei.com>
Subject: Re: [PATCH net-next v06 1/1] hinic3: module initialization and tx/rx logic

> On Tue, Feb 25, 2025 at 04:53:30PM +0200, Gur Stavi wrote:
> > From: Fan Gong <gongfan1@...wei.com>
> >
> > This is [1/3] part of hinic3 Ethernet driver initial submission.
> > With this patch hinic3 is a valid kernel module but non-functional
> > driver.
>
> IMHO, there's a huge amount of code so it makes reviewing pretty
> difficult.
>
> Is there no way to split this into multiple smaller patches? I am
> sure his was asked and answered in a previous thread that I missed.
>
> I took a quick pass over the code, but probably missed many things
> due to the large amount of code in a single patch.

>
> Doesn't this function need to re-enable hw IRQs? Maybe it does
> somewhere in one of the helpers and I missed it?
>
> Even so, it should probably be checking napi_complete_done before
> re-enabling IRQs and I don't see a call to that anywhere, but maybe
> I missed it?
>
> I also don't see any calls to netif_napi_add, so I'm not sure if
> this code needs to be included in this patch ?
>

Hi Joe, thanks for the review.

There was a discussion during our initial submission where we were required to
limit submission size to 4k LoC.
https://lore.kernel.org/netdev/cover.1730290527.git.gur.stavi@huawei.com

After removing almost anything optional from the driver we reached ~12K LoC
and broke it into 3 submissions of 4K LoC that compile. This is the 1st
submission.

The rational for patch ordering was to start with the code that have lots of
interaction with Linux interfaces and therefore is of more interest to the
community and keep the code that deals with proprietary HW interfaces for later.
We also try to make the patches incremental and avoid introducing ad-hoc code
that is being deleted or modified in later submission.

The actual calls to rx/tx poll and napi require IRQs that require hw management
interfaces.
I can try to pull these functions from later submission (without breaking the 4K
limit) into this patch. They are currently static functions so I will have to
un-static them to avoid compilation errors and then re-static them at a later
submission.

As for breaking the 4K submission into smaller patches, I will try to find
some finer granularity that makes sense. When I studied the mailing list,
there were conflicting opinions about the right way for initial driver
submission. For example:
https://lore.kernel.org/netdev/20170817.220343.905568389038615738.davem@davemloft.net