linux-kernel - RE: [PATCH v4 05/27] bus: mhi: Use bitfield operations for handling DWORDs of ring elements

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: <8433702975794b5389563393bf7bc405@AcuMS.aculab.com>
Date:   Mon, 28 Feb 2022 15:40:56 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Manivannan Sadhasivam' <manivannan.sadhasivam@...aro.org>
CC:     "mhi@...ts.linux.dev" <mhi@...ts.linux.dev>,
        "quic_hemantk@...cinc.com" <quic_hemantk@...cinc.com>,
        "quic_bbhatt@...cinc.com" <quic_bbhatt@...cinc.com>,
        "quic_jhugo@...cinc.com" <quic_jhugo@...cinc.com>,
        "vinod.koul@...aro.org" <vinod.koul@...aro.org>,
        "bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
        "dmitry.baryshkov@...aro.org" <dmitry.baryshkov@...aro.org>,
        "quic_vbadigan@...cinc.com" <quic_vbadigan@...cinc.com>,
        "quic_cang@...cinc.com" <quic_cang@...cinc.com>,
        "quic_skananth@...cinc.com" <quic_skananth@...cinc.com>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "elder@...aro.org" <elder@...aro.org>
Subject: RE: [PATCH v4 05/27] bus: mhi: Use bitfield operations for handling
 DWORDs of ring elements

From: 'Manivannan Sadhasivam'
> Sent: 28 February 2022 14:44
> 
> On Mon, Feb 28, 2022 at 02:00:07PM +0000, David Laight wrote:
> > From: Manivannan Sadhasivam
> > > Sent: 28 February 2022 12:43
> > >
> > > Instead of using the hardcoded bits in DWORD definitions, let's use the
> > > bitfield operations to make it more clear how the DWORDs are structured.
> >
> > That all makes it as clear as mud.
> 
> It depends on how you see it ;)
> 
> For instance,
> 
> #define MHI_TRE_GET_CMD_TYPE(tre) ((MHI_TRE_GET_DWORD(tre, 1) >> 16) & 0xFF)
> 
> vs
> 
> #define MHI_TRE_GET_CMD_TYPE(tre) (FIELD_GET(GENMASK(23, 16), (MHI_TRE_GET_DWORD(tre, 1))))
> 
> The later one makes it more obvious that the "type" field resides between bit 23
> and 16. Plus it avoids the extra masking.

No, (x >> 16) & 0xff is obviously bits 23 to 16.
I can guess or try to remember what FIELD_GET() and GENMASK() do
but it is really hard work.

Both lines are actually too long to read - especially given the
number of times they are repeated with very minor changes.

I actually wonder if you shouldn't just have a struct like:
struct mhi_cmd {
	__le64   address;
	__le16   len;
	u8       state;
	u8       vid;
	__le16   xxx; /* I can't see what this is */
	u8       chid;
	u8       cmd;
};

although you might need the odd anonymous union/struct
to get the overlays in.

Even using something like:
#define MAKE_WORD0(len, state, vid) (htole16(len) | state << 16 | vid << 16)
would make for easier reading.

Oh yes, there are some 64bit fields here.
So a 'word' is 64 bits, so a 'double word' would be 128 bits!

WTF is a DWORD anyway????
Are you going to start using DWORD_PTR as well ?????

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)