Received-SPF: pass (google.com: domain of linux-kernel+bounces-126841-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 1 Apr 2024 10:01:51 -0600
From: Mathieu Poirier <mathieu.poirier@linaro.org>
To: Andrew Davis <afd@ti.com>
Cc: Bjorn Andersson <andersson@kernel.org>,
	linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] remoteproc: k3-dsp: Fix usage of omap_mbox_message
 and mbox_msg_t
Message-ID: <Zgrab/l1Fp5HPKDb@p14s>
References: <20240325165808.31885-1-afd@ti.com>
 <ZgWMi088/zORh0m3@p14s>
 <0697211f-4a28-4a74-8540-840c075d513c@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0697211f-4a28-4a74-8540-840c075d513c@ti.com>

On Thu, Mar 28, 2024 at 11:26:24AM -0500, Andrew Davis wrote:
> On 3/28/24 10:28 AM, Mathieu Poirier wrote:
> > Hi Andrew,
> > 
> > On Mon, Mar 25, 2024 at 11:58:06AM -0500, Andrew Davis wrote:
> > > The type of message sent using omap-mailbox is always u32. The definition
> > > of mbox_msg_t is uintptr_t which is wrong as that type changes based on
> > > the architecture (32bit vs 64bit). Use u32 unconditionally and remove
> > > the now unneeded omap-mailbox.h include.
> > > 
> > > Signed-off-by: Andrew Davis <afd@ti.com>
> > > ---
> > >   drivers/remoteproc/ti_k3_dsp_remoteproc.c | 7 +++----
> > >   1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> > > index 3555b535b1683..33b30cfb86c9d 100644
> > > --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> > > +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c
> > > @@ -11,7 +11,6 @@
> > >   #include <linux/module.h>
> > >   #include <linux/of.h>
> > >   #include <linux/of_reserved_mem.h>
> > > -#include <linux/omap-mailbox.h>
> > >   #include <linux/platform_device.h>
> > >   #include <linux/remoteproc.h>
> > >   #include <linux/reset.h>
> > > @@ -113,7 +112,7 @@ static void k3_dsp_rproc_mbox_callback(struct mbox_client *client, void *data)
> > >   						  client);
> > >   	struct device *dev = kproc->rproc->dev.parent;
> > >   	const char *name = kproc->rproc->name;
> > > -	u32 msg = omap_mbox_message(data);
> > > +	u32 msg = (u32)(uintptr_t)(data);
> > 
> > Looking at omap-mailbox.h and unless I'm missing something, the end result is
> > the same.
> > 
> > 
> > >   	dev_dbg(dev, "mbox msg: 0x%x\n", msg);
> > > @@ -152,11 +151,11 @@ static void k3_dsp_rproc_kick(struct rproc *rproc, int vqid)
> > >   {
> > >   	struct k3_dsp_rproc *kproc = rproc->priv;
> > >   	struct device *dev = rproc->dev.parent;
> > > -	mbox_msg_t msg = (mbox_msg_t)vqid;
> > > +	u32 msg = vqid;
> > >   	int ret;
> > > 
> > 
> > Here @vqid becomes a 'u32' rather than a 'uintptr'...
> > 
> 
> u32 is the correct type for messages sent with OMAP mailbox. It
> only sends 32bit messages, uintptr is 64bit when compiled on
> 64bit hardware (like our ARM64 cores on K3). mbox_msg_t should
> have been defined as u32, this was a mistake we missed as we only
> ever used to compile it for 32bit cores (where uintptr is 32bit).
> 
> > >   	/* send the index of the triggered virtqueue in the mailbox payload */
> > > -	ret = mbox_send_message(kproc->mbox, (void *)msg);
> > > +	ret = mbox_send_message(kproc->mbox, (void *)(uintptr_t)msg);
> > 
> > ... but here it is casted as a 'uintptr_t', which yields the same result.
> > 
> 
> The function mbox_send_message() takes a void*, so we need to cast our 32bit
> message to that first, it is cast back to u32 inside the OMAP mailbox driver.
> Doing that in one step (u32 -> void*) causes a warning when void* is 64bit
> (cast from int to pointer of different size).
> 
> > 
> > I am puzzled - other than getting rid of a header file I don't see what else
> > this patch does.
> > 
> 
> Getting rid of the header is the main point of this patch (I have a later
> series that needs that header gone). But the difference this patch makes is that
> before we passed a pointer to a 64bit int to OMAP mailbox which takes a pointer
> to a 32bit int. Sure, the result is the same in little-endian systems, but that
> isn't a strictly correct in general.

From your explanation above this patchset is about two things:

1) Getting rid of a compilation warning when void* is 64bit wide
2) Getting rid of omap-mailbox.h

This is what the changelog should describe.  And next time, please add a cover
letter to your work.

Thanks,
Mathieu

> > >   	if (ret < 0)
> > >   		dev_err(dev, "failed to send mailbox message (%pe)\n",
> > >   			ERR_PTR(ret));
> > > -- 
> > > 2.39.2
> > >