netdev - Re: [RFC PATCH net-next] drivers: net: dsa: qca8k: convert to GENMASK/FIELD_PREP/FIELD

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]

Date:   Fri, 1 Oct 2021 14:33:04 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Ansuel Smith <ansuelsmth@...il.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net-next] drivers: net: dsa: qca8k: convert to
 GENMASK/FIELD_PREP/FIELD_GET

On Fri, Oct 01, 2021 at 01:01:02PM +0200, Ansuel Smith wrote:
> On Thu, Sep 30, 2021 at 07:14:13PM -0700, Florian Fainelli wrote:
> > 
> > 
> > On 9/30/2021 6:37 PM, Ansuel Smith wrote:
> > > Convert and try to standardize bit fields using
> > > GENMASK/FIELD_PREP/FIELD_GET macros. Rework some logic to support the
> > > standard macro and tidy things up. No functional change intended.
> > > 
> > > Signed-off-by: Ansuel Smith <ansuelsmth@...il.com>
> > > ---
> > > 
> > > I still need to test this in every part but I would like to have some
> > > approval about this kind of change. Also there are tons of warning by
> > > checkpatch about too long line... Are they accepted for headers? I can't
> > > really find another way to workaround the problem as reducing the define
> > > name would make them less descriptive.
> > > Aside from that I did the conversion as carefully as possible so in
> > > theory nothing should be broken and the conversion should be all
> > > correct. Some real improvement by using filed macro are in the
> > > fdb_read/fdb_write that now are much more readable.
> > 
> > My main concern is that it is going to be a tad harder to back port fixes
> > made to this driver with such changes in place, so unfortunately it is
> > usually a matter of either the initial version of the driver use BIT(),
> > FIELD_{PREP,GET} and GENMASK, or the very few commits following the initial
> > commit take care of that, and then it is all rosy for everyone, or else it
> > may be complicated.
> > 
> > You are one of the active contributors to this driver, so ultimately you
> > should decide.
> > -- 
> > Florian
> 
> Problem I'm trying to solve here is that I notice various name scheme
> used and not an unique one... (many _S and _M mixed with MASK and SHIFT)
> Various shift and strange bits handling. I think this is needed to
> improve the code in all the next release.
> About backports you mean for older kernel (bugfixes)

this

> or for external project (backports for openwrt, for example?) Anyway
> in the main code (the one in theory that should receive backports) I
> just reworked the ACL code that should be stable and the switch ID
> handling (I don't think there is anything to fix there).

Famous last words :)

> Aside from that and some improvement to VLAN, I tried to implement
> FIELD_PREP only in the define without touching qca8k code. 
> I honestly don't know if this would cause that much of a problem with
> backports (assuming they only touch qca8k code and not header).
> Would love to receive some feedback if I'm wrong about my idea.
> Any feedback about the warning for long names in the define? Are they
> accepted? I can't find anywhere how we should handle them.

While it does look pretty ugly, and you could perhaps split them like
this:

#define QCA8K_VTU_FUNC0_EG_MODE_PORT_MASK(_i) \
	(GENMASK(1, 0) << QCA8K_VTU_FUNC0_EG_MODE_PORT_SHIFT(_i))

it's more or less a judgement call, I've seen worse in a header file,
and exceeding a certain line length is certainly not among the worst of
sins - putting complex control flow logic inside the C preprocessor
would take that place, I think.

Anyway, Florian's point is that you're touching up the following
functions:

qca8k_port_vlan_filtering
qca8k_port_vlan_add
qca8k_read_switch_id
qca8k_setup
qca8k_vlan_del
qca8k_vlan_add
qca8k_vlan_access
qca8k_fdb_access
qca8k_fdb_write
qca8k_fdb_read

quite gratuitously, but this is another judgement call for you to make.
The only thing you should consider is that future you might hate you for
giving him extra work when a new bug in those functions is discovered.
You'll have to prepare a fix patch that applies on the refactored code,
to submit to the "net" tree, and and a fix patch that applies to the
pre-refactoring code, to submit to the "stable" tree directly.

As a proponent of code cleanups myself, I would still choose to do the
refactoring, but just be aware that it doesn't come for free.