Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 25 Jan 2020 14:46:31 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Jeffrey Hugo <jhugo@codeaurora.org>
Cc:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        arnd@arndb.de, smohanad@codeaurora.org, kvalo@codeaurora.org,
        bjorn.andersson@linaro.org, hemantk@codeaurora.org,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 05/16] bus: mhi: core: Add support for ringing
 channel/event ring doorbells
Message-ID: <20200125134631.GA3518689@kroah.com>
References: <20200123111836.7414-1-manivannan.sadhasivam@linaro.org>
 <20200123111836.7414-6-manivannan.sadhasivam@linaro.org>
 <beadf428-82db-c89f-22bc-983d7b907bb3@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <beadf428-82db-c89f-22bc-983d7b907bb3@codeaurora.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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