Received-SPF: pass (google.com: domain of linux-kernel+bounces-123251-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <0697211f-4a28-4a74-8540-840c075d513c@ti.com>
Date: Thu, 28 Mar 2024 11:26:24 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/3] remoteproc: k3-dsp: Fix usage of omap_mbox_message
 and mbox_msg_t
To: Mathieu Poirier <mathieu.poirier@linaro.org>
CC: Bjorn Andersson <andersson@kernel.org>, <linux-remoteproc@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20240325165808.31885-1-afd@ti.com> <ZgWMi088/zORh0m3@p14s>
Content-Language: en-US
From: Andrew Davis <afd@ti.com>
In-Reply-To: <ZgWMi088/zORh0m3@p14s>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

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.

Thanks,
Andrew

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