Re: [PATCH 02/20] bus: mhi: Move common MHI definitions out of host directory

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Alex Elder <elder@ieee.org>
Cc: mhi@lists.linux.dev, hemantk@codeaurora.org,
	bbhatt@codeaurora.org, quic_jhugo@quicinc.com,
	vinod.koul@linaro.org, bjorn.andersson@linaro.org,
	dmitry.baryshkov@linaro.org, skananth@codeaurora.org,
	vpernami@codeaurora.org, vbadigan@codeaurora.org,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 02/20] bus: mhi: Move common MHI definitions out of host directory
Date: Thu, 3 Feb 2022 15:59:17 +0530	[thread overview]
Message-ID: <20220203102917.GA6298@thinkpad> (raw)
In-Reply-To: <cf56733b-7c5d-e5f9-70c4-6f262fa2853b@ieee.org>

Hi Alex,

Thanks a lot for the inputs. I've incorporated a portion of your suggestions
and kept remaining ones post upstream.

On Wed, Jan 05, 2022 at 06:22:25PM -0600, Alex Elder wrote:
> On 12/2/21 5:35 AM, Manivannan Sadhasivam wrote:
> > Move the common MHI definitions in host "internal.h" to "common.h" so
> > that the endpoint code can make use of them. This also avoids
> > duplicating the definitions in the endpoint stack.
> > 
> > Still, the MHI register definitions are not moved since the offsets
> > vary between host and endpoint.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >   drivers/bus/mhi/common.h        | 182 ++++++++++++++++++++++++++++++++
> >   drivers/bus/mhi/host/internal.h | 154 +--------------------------
> >   2 files changed, 183 insertions(+), 153 deletions(-)
> >   create mode 100644 drivers/bus/mhi/common.h
> > 

[...]

> > +
> > +#define EV_CTX_RESERVED_MASK GENMASK(7, 0)
> > +#define EV_CTX_INTMODC_MASK GENMASK(15, 8)
> > +#define EV_CTX_INTMODC_SHIFT 8
> > +#define EV_CTX_INTMODT_MASK GENMASK(31, 16)
> > +#define EV_CTX_INTMODT_SHIFT 16
> > +struct mhi_event_ctxt {
> 
> These fields should all be explicitly marked as little endian.
> It so happens Intel and ARM use that, but defining them as
> simple unsigned values is not correct for an external interface.
> 
> This comment applies to the command and channel context structures
> also.
> 

Ack

> > +	__u32 intmod;
> > +	__u32 ertype;
> > +	__u32 msivec;
> > +
> 
> I think you can just define the entire struct as __packed
> and __aligned(4) rather than defining all of these fields
> with those attributes.
> 

This was suggested by Arnd during the MHI host review. He preferred adding the
aligned parameter only for members that require them.

> > +	__u64 rbase __packed __aligned(4);
> > +	__u64 rlen __packed __aligned(4);
> > +	__u64 rp __packed __aligned(4);
> > +	__u64 wp __packed __aligned(4);
> > +};
> > +
> > +#define CHAN_CTX_CHSTATE_MASK GENMASK(7, 0)
> > +#define CHAN_CTX_CHSTATE_SHIFT 0
> 
> Please eliminate all the _SHIFT definitions like this,
> where you are already defining the corresponding _MASK.
> The _SHIFT is redundant (and could lead to error, and
> takes up extra space).
> 
> You are using bitfield operations (like FIELD_GET()) in
> at least some places already.  Use them consistently
> throughout the driver.  Those macros simplify the code
> and obviate the need for any shift definitions.
> 

Ack.

Thanks,
Mani