Re: [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Fri 30 Dec 11:56 PST 2016, Bjorn Andersson wrote:

> 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 <loic.pallardy@st.com>
> > > 
> > > 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
> 

Loic, did you have a chance to give this a test run? I would appreciate
your feedback before I send this off to Linus.

(The fixed version is part of linux-next as well)

Regards,
Bjorn