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

[Xuan Zhuo <xuanzhuo@linux.alibaba.com>]

On Wed, 16 Jul 2025 18:32:14 +0200, Andrew Lunn <andrew@lunn.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