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

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]

Date:   Thu, 3 Feb 2022 15:59:17 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To:     Alex Elder <elder@...e.org>
Cc:     mhi@...ts.linux.dev, hemantk@...eaurora.org, bbhatt@...eaurora.org,
        quic_jhugo@...cinc.com, vinod.koul@...aro.org,
        bjorn.andersson@...aro.org, dmitry.baryshkov@...aro.org,
        skananth@...eaurora.org, vpernami@...eaurora.org,
        vbadigan@...eaurora.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/20] bus: mhi: Move common MHI definitions out of host
 directory

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@...aro.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