netdev - Re: [PATCH net-next] eea: Add basic driver framework for Alibaba Elastic Ethernet Adaptor

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: <1752735339.8483515-2-xuanzhuo@linux.alibaba.com>
Date: Thu, 17 Jul 2025 14:55:39 +0800
From: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Simon Horman <horms@...nel.org>,
 netdev@...r.kernel.org,
 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>,
 Wen Gu <guwen@...ux.alibaba.com>,
 Philo Lu <lulie@...ux.alibaba.com>,
 Lorenzo Bianconi <lorenzo@...nel.org>,
 Lukas Bulwahn <lukas.bulwahn@...hat.com>,
 Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>,
 Geert Uytterhoeven <geert+renesas@...der.be>,
 Alexander Duyck <alexanderduyck@...com>,
 Dust Li <dust.li@...ux.alibaba.com>
Subject: Re: [PATCH net-next] eea: Add basic driver framework for Alibaba Elastic Ethernet Adaptor

On Wed, 16 Jul 2025 18:32:14 +0200, Andrew Lunn <andrew@...n.ch> wrote:
> > > > +	ret = eea_adminq_submit(enet, cmd, req_addr, res_addr, req_size, res_size);
> > >
> > > Please arrange Networking code so that it is 80 columns wide or less,
> > > where that can be done without reducing readability. E.g. don't split
> > > strings across multiple lines. Do wrap lines like the one above like this:
> > >
> > > 	ret = eea_adminq_submit(enet, cmd, req_addr, res_addr, req_size,
> > > 				res_size);
> > >
> > > Note that the start of the non-whitespace portion of the 2nd line
> > > is aligned to be exactly inside the opening parentheses of the previous
> > > line.
> > >
> > > checkpatch.pl --max-line-length=80 is useful here.
> >
> > We are aware of the current limit of 100 characters, and we have been coding
> > according to that guideline. Of course, we try to keep lines within 80
> > characters where possible. However, in some cases, we find that using up to 100
> > characters improves readability, so 80 is not a strict requirement for us.
> >
> > Is there a specific rule or convention in the networking area that we should
> > follow? Sorry, I have not heard of such a rule before.
>
> That suggests to me you are not subscribed to the netdev list and are
> not reading reviews made to other drivers. This comes up every couple
> of weeks. You should be spending a little bit of time very day just
> looking at the comments other patches get, and make sure you don't
> make the same mistakes.
>
> In this particularly case, i don't think wrapping the line makes any
> difference to readability. There are some cases where it does, which
> is why you don't 100% enforce checkpatch. But in general, you should
> keep with 80 for networking.

OK, I personally also strongly prefer the 80-column limit. However, sometimes I
do think that in certain cases, keeping code on a single line can look cleaner.
Most of this patch should already follow the 80-column rule, although there
might be a few lines between 80 and 100 characters. I'll scan through again for
this issue and try to bring all lines under 80 if possible.

>
> > > > +#define EEA_NET_PT_UDPv6_EX  9
> > > > +	__le16 pkt_type:10,
> > > > +	       reserved1:6;
> > >
> > > Sparse complains about the above. And I'm not at all sure that
> > > a __le16 bitfield works as intended on a big endian system.
> > >
> > > I would suggest some combination of: FIELD_PREP, FIELD_GET, GENMASK,
> > > cpu_to_le16() and le16_to_cpu().
> > >
> > > Also, please do make sure patches don't introduce new Sparse warnings.
> >
> > I will try.
>
> FYI: We take sparse warnings pretty seriously. So please try quite
> hard.

I will add Sparse check.

Thanks.

>
> 	Andrew