Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6423c8ed-8d3a-3f17-9947-1751e7a70a18@linaro.org>
Date:   Mon, 28 Feb 2022 09:51:58 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v4 05/27] bus: mhi: Use bitfield operations for handling
 DWORDs of ring elements
Content-Language: en-US
To:     David Laight <David.Laight@ACULAB.COM>,
        'Manivannan Sadhasivam' <manivannan.sadhasivam@linaro.org>
Cc:     "mhi@lists.linux.dev" <mhi@lists.linux.dev>,
        "quic_hemantk@quicinc.com" <quic_hemantk@quicinc.com>,
        "quic_bbhatt@quicinc.com" <quic_bbhatt@quicinc.com>,
        "quic_jhugo@quicinc.com" <quic_jhugo@quicinc.com>,
        "vinod.koul@linaro.org" <vinod.koul@linaro.org>,
        "bjorn.andersson@linaro.org" <bjorn.andersson@linaro.org>,
        "dmitry.baryshkov@linaro.org" <dmitry.baryshkov@linaro.org>,
        "quic_vbadigan@quicinc.com" <quic_vbadigan@quicinc.com>,
        "quic_cang@quicinc.com" <quic_cang@quicinc.com>,
        "quic_skananth@quicinc.com" <quic_skananth@quicinc.com>,
        "linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20220228124344.77359-1-manivannan.sadhasivam@linaro.org>
 <20220228124344.77359-6-manivannan.sadhasivam@linaro.org>
 <90b1d3954b8c4157a4045db82b562271@AcuMS.aculab.com>
 <20220228144336.GB12451@workstation>
 <8433702975794b5389563393bf7bc405@AcuMS.aculab.com>
From:   Alex Elder <elder@linaro.org>
In-Reply-To: <8433702975794b5389563393bf7bc405@AcuMS.aculab.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/28/22 9:40 AM, David Laight wrote:
> 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.

Although I suggested the use of the bitfield functions, I don't
disagree with the above statement.

The intent was to simplify some code using some standard
helpers.  One benefit of those is that you don't need to
define the shift, because the mask already defines that
(so there is no chance for them mismatching).

The way this got implemented did not line up with what I had
envisioned though (and I had some discussion with Mani about
this earlier).  So this result ended up being messier than
I expected it would.

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

I agree with that.

> 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;
> };

I suggested something similar, and maybe more.  But here
too, Mani felt what he was doing was the right way and
that his way made things simpler overall.

I'm satisfied with the code, and frankly don't want to
delay it getting accepted any further if possible.

So I'm going to say this:

Reviewed-by: Alex Elder <elder@linaro.org>

However, Mani, please consider how you can make this
more readable, and have a plan to update things after
this gets accepted.  I suggested using inline functions
to help break it down a bit.  Or perhaps you could go
back to something like David suggests.

I don't need to review this again; I assume any changes
you make will improve the readability but will not change
the effect of the code.

					-Alex

> 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)
>