Re: [PATCH 05/16] bus: mhi: core: Add support for ringing channel/event ring doorbells

[Greg KH]

On Fri, Jan 24, 2020 at 03:51:12PM -0700, Jeffrey Hugo wrote:
> > +struct mhi_event_ctxt {
> > +	u32 reserved : 8;
> > +	u32 intmodc : 8;
> > +	u32 intmodt : 16;
> > +	u32 ertype;
> > +	u32 msivec;
> > +
> > +	u64 rbase __packed __aligned(4);
> > +	u64 rlen __packed __aligned(4);
> > +	u64 rp __packed __aligned(4);
> > +	u64 wp __packed __aligned(4);
> > +};
> 
> This is the struct that is shared with the device, correct?  Surely it needs
> to be packed then?  Seems like you'd expect some padding between msivec and
> rbase on a 64-bit system otherwise, which is probably not intended.
> 
> Also I strongly dislike bitfields in structures which are shared with
> another system since the C specification doesn't define how they are
> implemented, therefore you can run into issues where different compilers
> decide to implement the actual backing memory differently.  I know its less
> convinent, but I would prefer the use of bitmasks for these fields.

You have to use bitmasks in order for all endian cpus to work properly
here, so that needs to be fixed.

Oh, and if these values are in hardware, then the correct types also
need to be used (i.e. __u32 and __u64).

good catch!

greg k-h