| Message ID | 20240325165808.31885-1-afd@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] remoteproc: k3-dsp: Fix usage of omap_mbox_message and mbox_msg_t | expand |
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'... > /* 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. I am puzzled - other than getting rid of a header file I don't see what else this patch does. Thanks, Mathieu > if (ret < 0) > dev_err(dev, "failed to send mailbox message (%pe)\n", > ERR_PTR(ret)); > -- > 2.39.2 >
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 >>
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 > > >
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); 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; /* 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); if (ret < 0) dev_err(dev, "failed to send mailbox message (%pe)\n", ERR_PTR(ret));
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(-)