* [PATCH] rpmsg: virtio: set dst address on first message received @ 2022-03-15 15:38 Arnaud Pouliquen 2022-03-15 22:47 ` kernel test robot 2022-05-06 17:12 ` Mathieu Poirier 0 siblings, 2 replies; 5+ messages in thread From: Arnaud Pouliquen @ 2022-03-15 15:38 UTC (permalink / raw) To: Bjorn Andersson, Mathieu Poirier Cc: linux-remoteproc, linux-kernel, linux-stm32, arnaud.pouliquen when a rpmsg channel has been locally created with a destination address set to RPMSG_ADDR_ANY, a name service announcement message is sent to the remote side. Then the destination address is never updated, making it impossible to send messages to the remote. An example of kernel trace observed: rpmsg_tty virtio0.rpmsg-tty.29.-1: invalid addr (src 0x1d, dst 0xffffffff) Implement same strategy than the open-amp library: On the reception of the first message, if the destination address is RPMSG_ADDR_ANY, then set it to address of the remote endpoint that send the message. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> --- Remark: An alternative (or a complement?) could be to add a NS bind/unbind in the NS announcement channel (in rpmsg_ns.c). This would allow the local and/or the remote processor to inform the remote side the the service announced in bound. --- drivers/rpmsg/virtio_rpmsg_bus.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c index 3ede25b1f2e4..99d2119cc164 100644 --- a/drivers/rpmsg/virtio_rpmsg_bus.c +++ b/drivers/rpmsg/virtio_rpmsg_bus.c @@ -708,6 +708,7 @@ static ssize_t virtio_rpmsg_get_mtu(struct rpmsg_endpoint *ept) static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev, struct rpmsg_hdr *msg, unsigned int len) { + struct rpmsg_device *rpdev; struct rpmsg_endpoint *ept; struct scatterlist sg; bool little_endian = virtio_is_little_endian(vrp->vdev); @@ -746,6 +747,15 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev, mutex_unlock(&vrp->endpoints_lock); if (ept) { + rpdev = ept->rpdev; + if (rpdev->ept == ept && rpdev->dst == RPMSG_ADDR_ANY) { + /* + * First message received from the remote side on the default endpoint, + * update channel destination address. + */ + rpdev->dst = msg->src; + } + /* make sure ept->cb doesn't go away while we use it */ mutex_lock(&ept->cb_lock); -- 2.25.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] rpmsg: virtio: set dst address on first message received 2022-03-15 15:38 [PATCH] rpmsg: virtio: set dst address on first message received Arnaud Pouliquen @ 2022-03-15 22:47 ` kernel test robot 2022-05-06 17:12 ` Mathieu Poirier 1 sibling, 0 replies; 5+ messages in thread From: kernel test robot @ 2022-03-15 22:47 UTC (permalink / raw) To: Arnaud Pouliquen, Bjorn Andersson, Mathieu Poirier Cc: kbuild-all, linux-remoteproc, linux-kernel, linux-stm32, arnaud.pouliquen Hi Arnaud, I love your patch! Perhaps something to improve: [auto build test WARNING on remoteproc/rpmsg-next] [also build test WARNING on v5.17-rc8 next-20220315] [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] url: https://github.com/0day-ci/linux/commits/Arnaud-Pouliquen/rpmsg-virtio-set-dst-address-on-first-message-received/20220315-234049 base: git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rpmsg-next config: s390-randconfig-s031-20220313 (https://download.01.org/0day-ci/archive/20220316/202203160614.sjUMuSy8-lkp@intel.com/config) compiler: s390-linux-gcc (GCC) 11.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.4-dirty # https://github.com/0day-ci/linux/commit/ef182a34773917f6bf876b37485031962393a1cd git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Arnaud-Pouliquen/rpmsg-virtio-set-dst-address-on-first-message-received/20220315-234049 git checkout ef182a34773917f6bf876b37485031962393a1cd # save the config file to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=s390 SHELL=/bin/bash If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> sparse warnings: (new ones prefixed by >>) >> drivers/rpmsg/virtio_rpmsg_bus.c:756:36: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int [usertype] dst @@ got restricted __rpmsg32 [usertype] src @@ drivers/rpmsg/virtio_rpmsg_bus.c:756:36: sparse: expected unsigned int [usertype] dst drivers/rpmsg/virtio_rpmsg_bus.c:756:36: sparse: got restricted __rpmsg32 [usertype] src vim +756 drivers/rpmsg/virtio_rpmsg_bus.c 727 728 /* 729 * We currently use fixed-sized buffers, so trivially sanitize 730 * the reported payload length. 731 */ 732 if (len > vrp->buf_size || 733 msg_len > (len - sizeof(struct rpmsg_hdr))) { 734 dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg_len); 735 return -EINVAL; 736 } 737 738 /* use the dst addr to fetch the callback of the appropriate user */ 739 mutex_lock(&vrp->endpoints_lock); 740 741 ept = idr_find(&vrp->endpoints, __rpmsg32_to_cpu(little_endian, msg->dst)); 742 743 /* let's make sure no one deallocates ept while we use it */ 744 if (ept) 745 kref_get(&ept->refcount); 746 747 mutex_unlock(&vrp->endpoints_lock); 748 749 if (ept) { 750 rpdev = ept->rpdev; 751 if (rpdev->ept == ept && rpdev->dst == RPMSG_ADDR_ANY) { 752 /* 753 * First message received from the remote side on the default endpoint, 754 * update channel destination address. 755 */ > 756 rpdev->dst = msg->src; 757 } 758 759 /* make sure ept->cb doesn't go away while we use it */ 760 mutex_lock(&ept->cb_lock); 761 762 if (ept->cb) 763 ept->cb(ept->rpdev, msg->data, msg_len, ept->priv, 764 __rpmsg32_to_cpu(little_endian, msg->src)); 765 766 mutex_unlock(&ept->cb_lock); 767 768 /* farewell, ept, we don't need you anymore */ 769 kref_put(&ept->refcount, __ept_release); 770 } else 771 dev_warn_ratelimited(dev, "msg received with no recipient\n"); 772 773 /* publish the real size of the buffer */ 774 rpmsg_sg_init(&sg, msg, vrp->buf_size); 775 776 /* add the buffer back to the remote processor's virtqueue */ 777 err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL); 778 if (err < 0) { 779 dev_err(dev, "failed to add a virtqueue buffer: %d\n", err); 780 return err; 781 } 782 783 return 0; 784 } 785 --- 0-DAY CI Kernel Test Service https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rpmsg: virtio: set dst address on first message received 2022-03-15 15:38 [PATCH] rpmsg: virtio: set dst address on first message received Arnaud Pouliquen 2022-03-15 22:47 ` kernel test robot @ 2022-05-06 17:12 ` Mathieu Poirier 2022-05-09 9:18 ` Arnaud POULIQUEN 1 sibling, 1 reply; 5+ messages in thread From: Mathieu Poirier @ 2022-05-06 17:12 UTC (permalink / raw) To: Arnaud Pouliquen Cc: Bjorn Andersson, linux-remoteproc, linux-kernel, linux-stm32 On Tue, Mar 15, 2022 at 04:38:56PM +0100, Arnaud Pouliquen wrote: > when a rpmsg channel has been locally created with a destination address s/when/Wen Also, please be more specific about the "locally created" part, i.e rpmsg_ctrldev_ioctl() -> rpmsg_create_channel(). Otherwise it is really hard to understand the context of this change. > set to RPMSG_ADDR_ANY, a name service announcement message is sent to > the remote side. Then the destination address is never updated, making it > impossible to send messages to the remote. > > An example of kernel trace observed: > rpmsg_tty virtio0.rpmsg-tty.29.-1: invalid addr (src 0x1d, dst 0xffffffff) > > Implement same strategy than the open-amp library: > On the reception of the first message, if the destination address is > RPMSG_ADDR_ANY, then set it to address of the remote endpoint that > send the message. > I would have expected a "Fixes:" tag. > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > > --- > Remark: > An alternative (or a complement?) could be to add a NS bind/unbind in > the NS announcement channel (in rpmsg_ns.c). > This would allow the local and/or the remote processor to inform the > remote side the the service announced in bound. > --- > drivers/rpmsg/virtio_rpmsg_bus.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c > index 3ede25b1f2e4..99d2119cc164 100644 > --- a/drivers/rpmsg/virtio_rpmsg_bus.c > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c > @@ -708,6 +708,7 @@ static ssize_t virtio_rpmsg_get_mtu(struct rpmsg_endpoint *ept) > static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev, > struct rpmsg_hdr *msg, unsigned int len) > { > + struct rpmsg_device *rpdev; > struct rpmsg_endpoint *ept; > struct scatterlist sg; > bool little_endian = virtio_is_little_endian(vrp->vdev); > @@ -746,6 +747,15 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev, > mutex_unlock(&vrp->endpoints_lock); > > if (ept) { > + rpdev = ept->rpdev; > + if (rpdev->ept == ept && rpdev->dst == RPMSG_ADDR_ANY) { Please add a comment to explain the first part of the if() clause. It took me quite some time to understand. > + /* > + * First message received from the remote side on the default endpoint, > + * update channel destination address. > + */ > + rpdev->dst = msg->src; This triggers a bot warning and should be addressed. If it can't be addressed add a comment that clearly explains why so that we don't end up receiving patches for it every 4 weeks. Thanks, Mathieu > + } > + > /* make sure ept->cb doesn't go away while we use it */ > mutex_lock(&ept->cb_lock); > > -- > 2.25.1 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rpmsg: virtio: set dst address on first message received 2022-05-06 17:12 ` Mathieu Poirier @ 2022-05-09 9:18 ` Arnaud POULIQUEN 2022-05-10 14:45 ` Mathieu Poirier 0 siblings, 1 reply; 5+ messages in thread From: Arnaud POULIQUEN @ 2022-05-09 9:18 UTC (permalink / raw) To: Mathieu Poirier Cc: Bjorn Andersson, linux-remoteproc, linux-kernel, linux-stm32 Hello Mathieu, On 5/6/22 19:12, Mathieu Poirier wrote: > On Tue, Mar 15, 2022 at 04:38:56PM +0100, Arnaud Pouliquen wrote: >> when a rpmsg channel has been locally created with a destination address > > s/when/Wen > > Also, please be more specific about the "locally created" part, i.e > rpmsg_ctrldev_ioctl() -> rpmsg_create_channel(). Otherwise it is really hard to > understand the context of this change. > >> set to RPMSG_ADDR_ANY, a name service announcement message is sent to >> the remote side. Then the destination address is never updated, making it >> impossible to send messages to the remote. >> >> An example of kernel trace observed: >> rpmsg_tty virtio0.rpmsg-tty.29.-1: invalid addr (src 0x1d, dst 0xffffffff) >> >> Implement same strategy than the open-amp library: >> On the reception of the first message, if the destination address is >> RPMSG_ADDR_ANY, then set it to address of the remote endpoint that >> send the message. >> > > I would have expected a "Fixes:" tag. Difficult to give a reference. For me the issue exists since the creation of the rpmsg virtio bus. A driver can create a channel that generates a NS announcement leading to this issue. The issue as been highlighted by the creation of the RPMSG_CREATE_DEV_IOCTL control. > >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> >> >> --- >> Remark: >> An alternative (or a complement?) could be to add a NS bind/unbind in >> the NS announcement channel (in rpmsg_ns.c). >> This would allow the local and/or the remote processor to inform the >> remote side the the service announced in bound. >> --- >> drivers/rpmsg/virtio_rpmsg_bus.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c >> index 3ede25b1f2e4..99d2119cc164 100644 >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c >> @@ -708,6 +708,7 @@ static ssize_t virtio_rpmsg_get_mtu(struct rpmsg_endpoint *ept) >> static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev, >> struct rpmsg_hdr *msg, unsigned int len) >> { >> + struct rpmsg_device *rpdev; >> struct rpmsg_endpoint *ept; >> struct scatterlist sg; >> bool little_endian = virtio_is_little_endian(vrp->vdev); >> @@ -746,6 +747,15 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev, >> mutex_unlock(&vrp->endpoints_lock); >> >> if (ept) { >> + rpdev = ept->rpdev; >> + if (rpdev->ept == ept && rpdev->dst == RPMSG_ADDR_ANY) { > > Please add a comment to explain the first part of the if() clause. It took me > quite some time to understand. > >> + /* >> + * First message received from the remote side on the default endpoint, >> + * update channel destination address. >> + */ >> + rpdev->dst = msg->src; > > This triggers a bot warning and should be addressed. If it can't be addressed add > a comment that clearly explains why so that we don't end up receiving patches > for it every 4 weeks. Oops, I missed it, thanks for pointing it out. Concerning the patch itself as discussed in RP open-amp meeting. I wonder if this issue could be addressed by the flow control[1][2][3], or if needed in any case. I propose to send a V2 when ready to propose in parallel the flow control. So both can be addressed at same time to have a global picture of the way to address the use case.. Thanks, Arnaud [1] POC Linux code: https://github.com/arnopo/linux/commits/signalling [2] openamp library associated code: https://github.com/arnopo/open-amp/commits/flow_ctrl [3] overview presentation https://drive.google.com/file/d/1CLU3ybI3oSBGvor18AQ-HOzOJ2nOppEb/view > > Thanks, > Mathieu > >> + } >> + >> /* make sure ept->cb doesn't go away while we use it */ >> mutex_lock(&ept->cb_lock); >> >> -- >> 2.25.1 >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rpmsg: virtio: set dst address on first message received 2022-05-09 9:18 ` Arnaud POULIQUEN @ 2022-05-10 14:45 ` Mathieu Poirier 0 siblings, 0 replies; 5+ messages in thread From: Mathieu Poirier @ 2022-05-10 14:45 UTC (permalink / raw) To: Arnaud POULIQUEN Cc: Bjorn Andersson, linux-remoteproc, linux-kernel, linux-stm32 On Mon, 9 May 2022 at 03:18, Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com> wrote: > > Hello Mathieu, > > On 5/6/22 19:12, Mathieu Poirier wrote: > > On Tue, Mar 15, 2022 at 04:38:56PM +0100, Arnaud Pouliquen wrote: > >> when a rpmsg channel has been locally created with a destination address > > > > s/when/Wen > > > > Also, please be more specific about the "locally created" part, i.e > > rpmsg_ctrldev_ioctl() -> rpmsg_create_channel(). Otherwise it is really hard to > > understand the context of this change. > > > >> set to RPMSG_ADDR_ANY, a name service announcement message is sent to > >> the remote side. Then the destination address is never updated, making it > >> impossible to send messages to the remote. > >> > >> An example of kernel trace observed: > >> rpmsg_tty virtio0.rpmsg-tty.29.-1: invalid addr (src 0x1d, dst 0xffffffff) > >> > >> Implement same strategy than the open-amp library: > >> On the reception of the first message, if the destination address is > >> RPMSG_ADDR_ANY, then set it to address of the remote endpoint that > >> send the message. > >> > > > > I would have expected a "Fixes:" tag. > > Difficult to give a reference. For me the issue exists since the creation > of the rpmsg virtio bus. A driver can create a channel that generates a NS > announcement leading to this issue. > The issue as been highlighted by the creation of the RPMSG_CREATE_DEV_IOCTL > control. > Ok > > > >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com> > >> > >> --- > >> Remark: > >> An alternative (or a complement?) could be to add a NS bind/unbind in > >> the NS announcement channel (in rpmsg_ns.c). > >> This would allow the local and/or the remote processor to inform the > >> remote side the the service announced in bound. > >> --- > >> drivers/rpmsg/virtio_rpmsg_bus.c | 10 ++++++++++ > >> 1 file changed, 10 insertions(+) > >> > >> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c > >> index 3ede25b1f2e4..99d2119cc164 100644 > >> --- a/drivers/rpmsg/virtio_rpmsg_bus.c > >> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c > >> @@ -708,6 +708,7 @@ static ssize_t virtio_rpmsg_get_mtu(struct rpmsg_endpoint *ept) > >> static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev, > >> struct rpmsg_hdr *msg, unsigned int len) > >> { > >> + struct rpmsg_device *rpdev; > >> struct rpmsg_endpoint *ept; > >> struct scatterlist sg; > >> bool little_endian = virtio_is_little_endian(vrp->vdev); > >> @@ -746,6 +747,15 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev, > >> mutex_unlock(&vrp->endpoints_lock); > >> > >> if (ept) { > >> + rpdev = ept->rpdev; > >> + if (rpdev->ept == ept && rpdev->dst == RPMSG_ADDR_ANY) { > > > > Please add a comment to explain the first part of the if() clause. It took me > > quite some time to understand. > > > >> + /* > >> + * First message received from the remote side on the default endpoint, > >> + * update channel destination address. > >> + */ > >> + rpdev->dst = msg->src; > > > > This triggers a bot warning and should be addressed. If it can't be addressed add > > a comment that clearly explains why so that we don't end up receiving patches > > for it every 4 weeks. > > Oops, I missed it, thanks for pointing it out. > > > Concerning the patch itself as discussed in RP open-amp meeting. I wonder if this issue > could be addressed by the flow control[1][2][3], or if needed in any case. > > I propose to send a V2 when ready to propose in parallel the flow control. > So both can be addressed at same time to have a global picture of the way to address the > use case.. > It will be one or the other, we can't have two ways to do things. If the flow control patchset addresses this problem on top of adding more functionality then we should go with that one. > Thanks, > Arnaud > > [1] POC Linux code: > https://github.com/arnopo/linux/commits/signalling > > [2] openamp library associated code: > https://github.com/arnopo/open-amp/commits/flow_ctrl > > [3] overview presentation > https://drive.google.com/file/d/1CLU3ybI3oSBGvor18AQ-HOzOJ2nOppEb/view > > > > > > Thanks, > > Mathieu > > > >> + } > >> + > >> /* make sure ept->cb doesn't go away while we use it */ > >> mutex_lock(&ept->cb_lock); > >> > >> -- > >> 2.25.1 > >> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-05-10 15:09 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-15 15:38 [PATCH] rpmsg: virtio: set dst address on first message received Arnaud Pouliquen 2022-03-15 22:47 ` kernel test robot 2022-05-06 17:12 ` Mathieu Poirier 2022-05-09 9:18 ` Arnaud POULIQUEN 2022-05-10 14:45 ` Mathieu Poirier
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.