| Message ID | 20241011123922.23135-1-richard@nod.at (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rpmsg_ns: Work around TI non-standard message | expand |
Hi Richard, kernel test robot noticed the following build warnings: [auto build test WARNING on remoteproc/rpmsg-next] [also build test WARNING on linus/master v6.12-rc2 next-20241011] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Richard-Weinberger/rpmsg_ns-Work-around-TI-non-standard-message/20241011-204122 base: https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rpmsg-next patch link: https://lore.kernel.org/r/20241011123922.23135-1-richard%40nod.at patch subject: [PATCH] rpmsg_ns: Work around TI non-standard message config: x86_64-randconfig-121-20241012 (https://download.01.org/0day-ci/archive/20241012/202410122348.irTWFe4S-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241012/202410122348.irTWFe4S-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202410122348.irTWFe4S-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/rpmsg/rpmsg_ns.c:55:25: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __rpmsg32 [assigned] [usertype] ns_addr @@ got unsigned int [usertype] addr @@ drivers/rpmsg/rpmsg_ns.c:55:25: sparse: expected restricted __rpmsg32 [assigned] [usertype] ns_addr drivers/rpmsg/rpmsg_ns.c:55:25: sparse: got unsigned int [usertype] addr >> drivers/rpmsg/rpmsg_ns.c:56:26: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __rpmsg32 [assigned] [usertype] ns_flags @@ got unsigned int [usertype] flags @@ drivers/rpmsg/rpmsg_ns.c:56:26: sparse: expected restricted __rpmsg32 [assigned] [usertype] ns_flags drivers/rpmsg/rpmsg_ns.c:56:26: sparse: got unsigned int [usertype] flags vim +55 drivers/rpmsg/rpmsg_ns.c 45 46 if (len == sizeof(struct rpmsg_ns_msg)) { 47 struct rpmsg_ns_msg *msg = data; 48 49 ns_addr = msg->addr; 50 ns_flags = msg->flags; 51 ns_name = msg->name; 52 } else if (len == sizeof(struct __rpmsg_ns_msg_ti)) { 53 struct __rpmsg_ns_msg_ti *msg = data; 54 > 55 ns_addr = msg->addr; > 56 ns_flags = msg->flags; 57 ns_name = msg->name; 58 dev_warn(dev, "non-standard ns msg found\n"); 59 } else { 60 dev_err(dev, "malformed ns msg (%d)\n", len); 61 return -EINVAL; 62 } 63 64 /* don't trust the remote processor for null terminating the name */ 65 ns_name[RPMSG_NAME_SIZE - 1] = '\0'; 66 67 strscpy_pad(chinfo.name, ns_name, sizeof(chinfo.name)); 68 chinfo.src = RPMSG_ADDR_ANY; 69 chinfo.dst = rpmsg32_to_cpu(rpdev, ns_addr); 70 71 dev_info(dev, "%sing channel %s addr 0x%x\n", 72 rpmsg32_to_cpu(rpdev, ns_flags) & RPMSG_NS_DESTROY ? 73 "destroy" : "creat", ns_name, chinfo.dst); 74 75 if (rpmsg32_to_cpu(rpdev, ns_flags) & RPMSG_NS_DESTROY) { 76 ret = rpmsg_release_channel(rpdev, &chinfo); 77 if (ret) 78 dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret); 79 } else { 80 newch = rpmsg_create_channel(rpdev, &chinfo); 81 if (!newch) 82 dev_err(dev, "rpmsg_create_channel failed\n"); 83 } 84 85 return 0; 86 } 87
Am Samstag, 12. Oktober 2024, 17:53:16 CEST schrieb kernel test robot: > sparse warnings: (new ones prefixed by >>) > >> drivers/rpmsg/rpmsg_ns.c:55:25: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __rpmsg32 [assigned] [usertype] ns_addr @@ got unsigned int [usertype] addr @@ > drivers/rpmsg/rpmsg_ns.c:55:25: sparse: expected restricted __rpmsg32 [assigned] [usertype] ns_addr > drivers/rpmsg/rpmsg_ns.c:55:25: sparse: got unsigned int [usertype] addr > >> drivers/rpmsg/rpmsg_ns.c:56:26: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __rpmsg32 [assigned] [usertype] ns_flags @@ got unsigned int [usertype] flags @@ > drivers/rpmsg/rpmsg_ns.c:56:26: sparse: expected restricted __rpmsg32 [assigned] [usertype] ns_flags > drivers/rpmsg/rpmsg_ns.c:56:26: sparse: got unsigned int [usertype] flags sprase is right. I missed to replace u32 in the struct by __rpmsg32. Thanks, //richard
Good morning Richard, On Fri, Oct 11, 2024 at 02:39:22PM +0200, Richard Weinberger wrote: > Texas Instruments ships a patch in their vendor kernels, > which adds a new NS message that includes a description field. > While TI is free to do whatever they want in their copy of the kernel, > it becomes a mess when people switch to a mainline kernel and want > to use their existing DSP programs with it. I suspect there is a lot more things to change when going from downstream to a mainline kernel. > > To make it easier to migrate to a mainline kernel, > let's make the kernel aware of their non-standard extension but > briefly ignore the description field. In my opinion the real fix here is to get TI to use the standard message announcement structure. The ->desc field doesn't seem to be that useful since it gets discarted. Thanks, Mathieu > > [0] https://patchwork.kernel.org/project/linux-remoteproc/patch/20190815231448.10100-1-s-anna@ti.com/ > [1] https://stash.phytec.com/projects/PUB/repos/linux-phytec-ti/commits/aeded1f439effc84aa9f4e341a6e92ce1844ab98#drivers/rpmsg/virtio_rpmsg_bus.c > > Cc: ohad@wizery.com > Cc: s-anna@ti.com > Cc: t-kristo@ti.com > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > FWIW, this is a forward port of a patch I'm using on v6.6. > > Thanks, > //richard > --- > drivers/rpmsg/rpmsg_ns.c | 30 ++++++++++++++++++++++-------- > include/linux/rpmsg/ns.h | 8 ++++++++ > 2 files changed, 30 insertions(+), 8 deletions(-) > > diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c > index bde8c8d433e0a..2fb3721eb0141 100644 > --- a/drivers/rpmsg/rpmsg_ns.c > +++ b/drivers/rpmsg/rpmsg_ns.c > @@ -31,10 +31,11 @@ EXPORT_SYMBOL(rpmsg_ns_register_device); > static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len, > void *priv, u32 src) > { > - struct rpmsg_ns_msg *msg = data; > struct rpmsg_device *newch; > struct rpmsg_channel_info chinfo; > struct device *dev = rpdev->dev.parent; > + __rpmsg32 ns_addr, ns_flags; > + char *ns_name; > int ret; > > #if defined(CONFIG_DYNAMIC_DEBUG) > @@ -42,23 +43,36 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len, > data, len, true); > #endif > > - if (len != sizeof(*msg)) { > + if (len == sizeof(struct rpmsg_ns_msg)) { > + struct rpmsg_ns_msg *msg = data; > + > + ns_addr = msg->addr; > + ns_flags = msg->flags; > + ns_name = msg->name; > + } else if (len == sizeof(struct __rpmsg_ns_msg_ti)) { > + struct __rpmsg_ns_msg_ti *msg = data; > + > + ns_addr = msg->addr; > + ns_flags = msg->flags; > + ns_name = msg->name; > + dev_warn(dev, "non-standard ns msg found\n"); > + } else { > dev_err(dev, "malformed ns msg (%d)\n", len); > return -EINVAL; > } > > /* don't trust the remote processor for null terminating the name */ > - msg->name[RPMSG_NAME_SIZE - 1] = '\0'; > + ns_name[RPMSG_NAME_SIZE - 1] = '\0'; > > - strscpy_pad(chinfo.name, msg->name, sizeof(chinfo.name)); > + strscpy_pad(chinfo.name, ns_name, sizeof(chinfo.name)); > chinfo.src = RPMSG_ADDR_ANY; > - chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr); > + chinfo.dst = rpmsg32_to_cpu(rpdev, ns_addr); > > dev_info(dev, "%sing channel %s addr 0x%x\n", > - rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ? > - "destroy" : "creat", msg->name, chinfo.dst); > + rpmsg32_to_cpu(rpdev, ns_flags) & RPMSG_NS_DESTROY ? > + "destroy" : "creat", ns_name, chinfo.dst); > > - if (rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY) { > + if (rpmsg32_to_cpu(rpdev, ns_flags) & RPMSG_NS_DESTROY) { > ret = rpmsg_release_channel(rpdev, &chinfo); > if (ret) > dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret); > diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h > index a7804edd6d58f..60fca84ad4cea 100644 > --- a/include/linux/rpmsg/ns.h > +++ b/include/linux/rpmsg/ns.h > @@ -26,6 +26,14 @@ struct rpmsg_ns_msg { > __rpmsg32 flags; > } __packed; > > +/* Non-standard extended ns message by Texas Instruments */ > +struct __rpmsg_ns_msg_ti { > + char name[RPMSG_NAME_SIZE]; > + char desc[RPMSG_NAME_SIZE]; /* ignored */ > + u32 addr; > + u32 flags; > +} __packed; > + > /** > * enum rpmsg_ns_flags - dynamic name service announcement flags > * > -- > 2.35.3 >
Mathieu, Am Dienstag, 15. Oktober 2024, 18:48:08 CEST schrieb Mathieu Poirier: > Good morning Richard, > > On Fri, Oct 11, 2024 at 02:39:22PM +0200, Richard Weinberger wrote: > > Texas Instruments ships a patch in their vendor kernels, > > which adds a new NS message that includes a description field. > > While TI is free to do whatever they want in their copy of the kernel, > > it becomes a mess when people switch to a mainline kernel and want > > to use their existing DSP programs with it. > > I suspect there is a lot more things to change when going from downstream to a > mainline kernel. Not really. I had to revert c6aed238b7a9b ("remoteproc: modify vring allocation to rely on centralized carveout allocator") because the DSP has a sub-optimal resource table, and this workaround. With that the DSP program worked as-is on kernel 6.6. Downstream was 4.19 TI. > > > > To make it easier to migrate to a mainline kernel, > > let's make the kernel aware of their non-standard extension but > > briefly ignore the description field. > > In my opinion the real fix here is to get TI to use the standard message > announcement structure. The ->desc field doesn't seem to be that useful since > it gets discarted. This is for the future, the goal of my patch is helping people to get existing DSP programs work with mainline. Not everyone can or want to rebuild theirs DSP programs when moving to a mainline kernel. Thanks, //richard
On Tue, Oct 15, 2024 at 06:58:33PM +0200, Richard Weinberger wrote: > Mathieu, > > Am Dienstag, 15. Oktober 2024, 18:48:08 CEST schrieb Mathieu Poirier: > > Good morning Richard, > > > > On Fri, Oct 11, 2024 at 02:39:22PM +0200, Richard Weinberger wrote: > > > Texas Instruments ships a patch in their vendor kernels, > > > which adds a new NS message that includes a description field. > > > While TI is free to do whatever they want in their copy of the kernel, > > > it becomes a mess when people switch to a mainline kernel and want > > > to use their existing DSP programs with it. > > > > I suspect there is a lot more things to change when going from downstream to a > > mainline kernel. > > Not really. > I had to revert c6aed238b7a9b ("remoteproc: modify vring allocation to rely on centralized carveout allocator") > because the DSP has a sub-optimal resource table, and this workaround. > With that the DSP program worked as-is on kernel 6.6. > Downstream was 4.19 TI. > > > > > > > To make it easier to migrate to a mainline kernel, > > > let's make the kernel aware of their non-standard extension but > > > briefly ignore the description field. > > > > In my opinion the real fix here is to get TI to use the standard message > > announcement structure. The ->desc field doesn't seem to be that useful since > > it gets discarted. > > This is for the future, the goal of my patch is helping people to > get existing DSP programs work with mainline. > Not everyone can or want to rebuild theirs DSP programs when moving to a mainline > kernel. That's an even better argument to adopt the standard structure as soon as possible. Modifying the mainline kernel to adapt to vendors' quirks doesn't scale. > > Thanks, > //richard > > -- > sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, AUT > UID/VAT Nr: ATU 66964118 | FN: 374287y > >
Am Dienstag, 15. Oktober 2024, 19:56:08 CEST schrieb Mathieu Poirier: > > > In my opinion the real fix here is to get TI to use the standard message > > > announcement structure. The ->desc field doesn't seem to be that useful since > > > it gets discarted. > > > > This is for the future, the goal of my patch is helping people to > > get existing DSP programs work with mainline. > > Not everyone can or want to rebuild theirs DSP programs when moving to a mainline > > kernel. > > That's an even better argument to adopt the standard structure as soon as > possible. Modifying the mainline kernel to adapt to vendors' quirks doesn't > scale. Well, I can't speak for TI. But I have little hope. Thanks, //richard
Hello Richard, All, Le 15/10/2024 à 20:00, Richard Weinberger a écrit : > Am Dienstag, 15. Oktober 2024, 19:56:08 CEST schrieb Mathieu Poirier: >>>> In my opinion the real fix here is to get TI to use the standard message >>>> announcement structure. The ->desc field doesn't seem to be that useful since >>>> it gets discarted. >>> >>> This is for the future, the goal of my patch is helping people to >>> get existing DSP programs work with mainline. >>> Not everyone can or want to rebuild theirs DSP programs when moving to a mainline >>> kernel. >> >> That's an even better argument to adopt the standard structure as soon as >> possible. Modifying the mainline kernel to adapt to vendors' quirks doesn't >> scale. > > Well, I can't speak for TI. > But I have little hope. I'm also using an AM57xx SoC with DSP firmware and I had the same issue while updating the kernel from 5.10 to a newer version. remoteproc rpmsg description field changes [1] is required by the DSP firmware based on TI-RTOS that is loaded by remoteproc using the new (as of 2013) but still experimental RPMSG_NS_2_0 [2]. RPMSG_NS_2_0 broke compatibility with existing rpmsg structs [3] (defined in upstream kernel) for all devices except OMAP5 and newer SoC (Newer 64bits SoC DRA8 and Jacinto doesn’t need this change thanks to the new IPC-lld implementation). This rpmsg description field has been added to ipcdev long time ago (14/03/2013) and requires this kernel vendor change since then (all DSP firmware generated by AM57xx TI SDK need it). Note: RPMSG_NS_2_0 is not the only vendor changes you need to rebase on newer kernels... DSP firmware generated by TI SDK are using the vendor MessageQ API [4] that requires AF_RPMSG socket support [5] in the kernel. This driver was never upstreamed since the IPC 3.x is deprecated nowadays and replaced by the IPC-lld on newer SoC (IPC-lld uses the upstream generic rpmg-char driver). All theses patches were moved out of ti-linux-kernel (6.1 since) to a meta-tisdk yocto layer that is used only for am57xx vendor kernel. See the latest SDK release for AM57xx [7]. So, DSP firmwares on AM57xx will requires theses two vendor patches since it's how the TI IPC stack was designed more than 10 years ago. It's nice to see newer TI SoC able to use upstream kernel using the standard structure. [1] https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-rt-linux-5.10.y&id=7e3ea0d62a4bf0ca04be9bc320d13f564aab0a92 [2] https://git.ti.com/cgit/ipc/ipcdev/commit/?id=e8a33844cd2faa404e457d13f9d54478ec8129e7 [3] https://git.ti.com/cgit/ipc/ipcdev/commit/?id=1264ed112ef8c0eed6ff30503b14f81b8ff11dd7 [4] http://downloads.ti.com/dsps/dsps_public_sw/sdo_sb/targetcontent/ipc/latest/docs/doxygen/html/_message_q_8h.html [5] https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-rt-linux-5.10.y&id=f4b978a978c38149f712ddd137f12ed5fb914161 [6] https://git.ti.com/cgit/ti-sdk-linux/meta-tisdk/commit/?h=am57x-9.x&id=25e56a0615fb8e973e516b5a225ee81f655f98db [7] https://www.ti.com/tool/download/PROCESSOR-SDK-LINUX-AM57X/09.02.00.133 Best regards, Romain > > Thanks, > //richard >
diff --git a/drivers/rpmsg/rpmsg_ns.c b/drivers/rpmsg/rpmsg_ns.c index bde8c8d433e0a..2fb3721eb0141 100644 --- a/drivers/rpmsg/rpmsg_ns.c +++ b/drivers/rpmsg/rpmsg_ns.c @@ -31,10 +31,11 @@ EXPORT_SYMBOL(rpmsg_ns_register_device); static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len, void *priv, u32 src) { - struct rpmsg_ns_msg *msg = data; struct rpmsg_device *newch; struct rpmsg_channel_info chinfo; struct device *dev = rpdev->dev.parent; + __rpmsg32 ns_addr, ns_flags; + char *ns_name; int ret; #if defined(CONFIG_DYNAMIC_DEBUG) @@ -42,23 +43,36 @@ static int rpmsg_ns_cb(struct rpmsg_device *rpdev, void *data, int len, data, len, true); #endif - if (len != sizeof(*msg)) { + if (len == sizeof(struct rpmsg_ns_msg)) { + struct rpmsg_ns_msg *msg = data; + + ns_addr = msg->addr; + ns_flags = msg->flags; + ns_name = msg->name; + } else if (len == sizeof(struct __rpmsg_ns_msg_ti)) { + struct __rpmsg_ns_msg_ti *msg = data; + + ns_addr = msg->addr; + ns_flags = msg->flags; + ns_name = msg->name; + dev_warn(dev, "non-standard ns msg found\n"); + } else { dev_err(dev, "malformed ns msg (%d)\n", len); return -EINVAL; } /* don't trust the remote processor for null terminating the name */ - msg->name[RPMSG_NAME_SIZE - 1] = '\0'; + ns_name[RPMSG_NAME_SIZE - 1] = '\0'; - strscpy_pad(chinfo.name, msg->name, sizeof(chinfo.name)); + strscpy_pad(chinfo.name, ns_name, sizeof(chinfo.name)); chinfo.src = RPMSG_ADDR_ANY; - chinfo.dst = rpmsg32_to_cpu(rpdev, msg->addr); + chinfo.dst = rpmsg32_to_cpu(rpdev, ns_addr); dev_info(dev, "%sing channel %s addr 0x%x\n", - rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY ? - "destroy" : "creat", msg->name, chinfo.dst); + rpmsg32_to_cpu(rpdev, ns_flags) & RPMSG_NS_DESTROY ? + "destroy" : "creat", ns_name, chinfo.dst); - if (rpmsg32_to_cpu(rpdev, msg->flags) & RPMSG_NS_DESTROY) { + if (rpmsg32_to_cpu(rpdev, ns_flags) & RPMSG_NS_DESTROY) { ret = rpmsg_release_channel(rpdev, &chinfo); if (ret) dev_err(dev, "rpmsg_destroy_channel failed: %d\n", ret); diff --git a/include/linux/rpmsg/ns.h b/include/linux/rpmsg/ns.h index a7804edd6d58f..60fca84ad4cea 100644 --- a/include/linux/rpmsg/ns.h +++ b/include/linux/rpmsg/ns.h @@ -26,6 +26,14 @@ struct rpmsg_ns_msg { __rpmsg32 flags; } __packed; +/* Non-standard extended ns message by Texas Instruments */ +struct __rpmsg_ns_msg_ti { + char name[RPMSG_NAME_SIZE]; + char desc[RPMSG_NAME_SIZE]; /* ignored */ + u32 addr; + u32 flags; +} __packed; + /** * enum rpmsg_ns_flags - dynamic name service announcement flags *
Texas Instruments ships a patch in their vendor kernels, which adds a new NS message that includes a description field. While TI is free to do whatever they want in their copy of the kernel, it becomes a mess when people switch to a mainline kernel and want to use their existing DSP programs with it. To make it easier to migrate to a mainline kernel, let's make the kernel aware of their non-standard extension but briefly ignore the description field. [0] https://patchwork.kernel.org/project/linux-remoteproc/patch/20190815231448.10100-1-s-anna@ti.com/ [1] https://stash.phytec.com/projects/PUB/repos/linux-phytec-ti/commits/aeded1f439effc84aa9f4e341a6e92ce1844ab98#drivers/rpmsg/virtio_rpmsg_bus.c Cc: ohad@wizery.com Cc: s-anna@ti.com Cc: t-kristo@ti.com Signed-off-by: Richard Weinberger <richard@nod.at> --- FWIW, this is a forward port of a patch I'm using on v6.6. Thanks, //richard --- drivers/rpmsg/rpmsg_ns.c | 30 ++++++++++++++++++++++-------- include/linux/rpmsg/ns.h | 8 ++++++++ 2 files changed, 30 insertions(+), 8 deletions(-)