From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Fri, 30 Dec 2016 11:56:24 -0800 From: Bjorn Andersson Subject: Re: [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation Message-ID: <20161230195624.GF10531@minitux> References: <1481813396-1529-1-git-send-email-loic.pallardy@st.com> <20161216050912.GS3439@tuxbot> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Loic Pallardy , Suman Anna Cc: ohad@wizery.com, lee.jones@linaro.org, linux-remoteproc@vger.kernel.org, kernel@stlinux.com, patrice.chotard@st.com List-ID: On Tue 27 Dec 14:36 PST 2016, Suman Anna wrote: > Hi Loic, > > On 12/15/2016 11:09 PM, Bjorn Andersson wrote: > > On Thu 15 Dec 06:49 PST 2016, Loic Pallardy wrote: > > > >> Since commit 4dffed5b3ac796b ("rpmsg: Name rpmsg devices based on > >> channel id"), it is no more possible for a firmware to register twice > >> a service (on different endpoints). rpmsg_register_device function > >> is failing when calling device_add for the second time as second > >> device has the same name as first one already register. > >> It is because name is based only on service name and so is not more > >> unique. Previously name was unique thanks to the use of rpmsg_dev_index. > >> > >> This patch adds destination and source endpoint numbers device name to > >> create an unique identifier. > >> > >> Signed-off-by: Loic Pallardy > > > > Looks good, thanks. > > Thanks for the patch, I have ran into the exact same issue as well. > > > > > Regards, > > Bjorn > > > >> --- > >> v2: Update commit header with commit ID generating regression > >> Fix rpmsg_core instead of virtio_rpmsg > >> > >> drivers/rpmsg/rpmsg_core.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c > >> index a79cb5a..18c73e0 100644 > >> --- a/drivers/rpmsg/rpmsg_core.c > >> +++ b/drivers/rpmsg/rpmsg_core.c > >> @@ -453,8 +453,8 @@ int rpmsg_register_device(struct rpmsg_device *rpdev) > >> struct device *dev = &rpdev->dev; > >> int ret; > >> > >> - dev_set_name(&rpdev->dev, "%s:%s", > >> - dev_name(dev->parent), rpdev->id.name); > >> + dev_set_name(&rpdev->dev, "%s.%d.%d.%s", dev_name(dev->parent), > >> + rpdev->src, rpdev->dst, rpdev->id.name); > > This is fine, though I would have preferred the numbers at the end after > the string literals (%s.%s.%d.%d instead of %s.%d.%d.%s) > When using the Qualcomm SMD backend only the name is used to identify channels, so I set src & dst to RPMSG_ADDR_ANY. Moving the numbers to the end makes this slightly better looking and one could potentially trim those without changing the structure of the name. Loic, I took the liberty of changing the order and have updated my for-next [1] with the following patches: a0c10687ec95 Revert "remoteproc: Merge table_ptr and cached_table pointers" c81c0e0710f0 remoteproc: fix vdev reference management 63447646ac65 rpmsg: virtio_rpmsg_bus: fix channel creation I will let them sit there a few days to get some additional build and boot test and hope you can give them a spin as well; before I send them to Linus. [1] https://github.com/andersson/remoteproc/commits/for-next Regards, Bjorn