All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation
@ 2016-12-15 14:49 Loic Pallardy
  2016-12-16  5:09 ` Bjorn Andersson
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Loic Pallardy @ 2016-12-15 14:49 UTC (permalink / raw)
  To: bjorn.andersson, ohad, lee.jones
  Cc: loic.pallardy, linux-remoteproc, kernel, patrice.chotard

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>
---
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);
 
 	rpdev->dev.bus = &rpmsg_bus;
 	rpdev->dev.release = rpmsg_release_device;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation
  2016-12-15 14:49 [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation Loic Pallardy
@ 2016-12-16  5:09 ` Bjorn Andersson
  2016-12-27 22:36   ` Suman Anna
  2016-12-16 10:39 ` [STLinux Kernel] " Peter Griffin
  2017-01-10  7:51 ` Lee Jones
  2 siblings, 1 reply; 12+ messages in thread
From: Bjorn Andersson @ 2016-12-16  5:09 UTC (permalink / raw)
  To: Loic Pallardy; +Cc: ohad, lee.jones, linux-remoteproc, kernel, patrice.chotard

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.

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);
>  
>  	rpdev->dev.bus = &rpmsg_bus;
>  	rpdev->dev.release = rpmsg_release_device;
> -- 
> 1.9.1
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [STLinux Kernel] [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation
  2016-12-15 14:49 [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation Loic Pallardy
  2016-12-16  5:09 ` Bjorn Andersson
@ 2016-12-16 10:39 ` Peter Griffin
  2017-01-10  7:51 ` Lee Jones
  2 siblings, 0 replies; 12+ messages in thread
From: Peter Griffin @ 2016-12-16 10:39 UTC (permalink / raw)
  To: Loic Pallardy; +Cc: bjorn.andersson, ohad, lee.jones, linux-remoteproc, kernel

Hi Loic,

On Thu, 15 Dec 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>
> ---
> v2: Update commit header with commit ID generating regression
> Fix rpmsg_core instead of virtio_rpmsg

Acked-by: Peter Griffin <peter.griffin@linaro.org>

regards,

Peter.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation
  2016-12-16  5:09 ` Bjorn Andersson
@ 2016-12-27 22:36   ` Suman Anna
  2016-12-30 19:56     ` Bjorn Andersson
  0 siblings, 1 reply; 12+ messages in thread
From: Suman Anna @ 2016-12-27 22:36 UTC (permalink / raw)
  To: Bjorn Andersson, Loic Pallardy
  Cc: ohad, lee.jones, linux-remoteproc, kernel, patrice.chotard

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)

regards
Suman

>>  
>>  	rpdev->dev.bus = &rpmsg_bus;
>>  	rpdev->dev.release = rpmsg_release_device;
>> -- 
>> 1.9.1
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation
  2016-12-27 22:36   ` Suman Anna
@ 2016-12-30 19:56     ` Bjorn Andersson
  2017-01-09 23:45       ` Bjorn Andersson
  0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Andersson @ 2016-12-30 19:56 UTC (permalink / raw)
  To: Loic Pallardy, Suman Anna
  Cc: ohad, lee.jones, linux-remoteproc, kernel, patrice.chotard

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

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation
  2016-12-30 19:56     ` Bjorn Andersson
@ 2017-01-09 23:45       ` Bjorn Andersson
  2017-01-10  0:47         ` Suman Anna
  2017-01-10  8:18         ` Loic PALLARDY
  0 siblings, 2 replies; 12+ messages in thread
From: Bjorn Andersson @ 2017-01-09 23:45 UTC (permalink / raw)
  To: Loic Pallardy, Suman Anna
  Cc: ohad, lee.jones, linux-remoteproc, kernel, patrice.chotard

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation
  2017-01-09 23:45       ` Bjorn Andersson
@ 2017-01-10  0:47         ` Suman Anna
  2017-01-11 19:57           ` Bjorn Andersson
  2017-01-10  8:18         ` Loic PALLARDY
  1 sibling, 1 reply; 12+ messages in thread
From: Suman Anna @ 2017-01-10  0:47 UTC (permalink / raw)
  To: Bjorn Andersson, Loic Pallardy
  Cc: ohad, lee.jones, linux-remoteproc, kernel, patrice.chotard

On 01/09/2017 05:45 PM, Bjorn Andersson wrote:
> 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)

FWIW, these work fine on TI platforms. I had run into the same issues
that these commits were fixing.

regards
Suman

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation
  2016-12-15 14:49 [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation Loic Pallardy
  2016-12-16  5:09 ` Bjorn Andersson
  2016-12-16 10:39 ` [STLinux Kernel] " Peter Griffin
@ 2017-01-10  7:51 ` Lee Jones
  2017-01-10  8:05   ` Marek Novak
  2 siblings, 1 reply; 12+ messages in thread
From: Lee Jones @ 2017-01-10  7:51 UTC (permalink / raw)
  To: Loic Pallardy
  Cc: bjorn.andersson, ohad, linux-remoteproc, kernel, patrice.chotard

On Thu, 15 Dec 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>
> ---
> 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(-)

Acked-by: Lee Jones <lee.jones@linaro.org>

> 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);
>  
>  	rpdev->dev.bus = &rpmsg_bus;
>  	rpdev->dev.release = rpmsg_release_device;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation
  2017-01-10  7:51 ` Lee Jones
@ 2017-01-10  8:05   ` Marek Novak
  0 siblings, 0 replies; 12+ messages in thread
From: Marek Novak @ 2017-01-10  8:05 UTC (permalink / raw)
  To: Loic Pallardy
  Cc: bjorn.andersson, ohad, linux-remoteproc, kernel, patrice.chotard, Lee

On Thu, 15 Dec 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>
> ---
> 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(-)

Acked-by: Marek Novak <marek.novak@nxp.com>

> 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);
>  
>        rpdev->dev.bus = &rpmsg_bus;
>        rpdev->dev.release = rpmsg_release_device;

-- 
Marek Novak
Multicore Software Engineer
NXP Semiconductors

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation
  2017-01-09 23:45       ` Bjorn Andersson
  2017-01-10  0:47         ` Suman Anna
@ 2017-01-10  8:18         ` Loic PALLARDY
  2017-01-11 22:08           ` Bjorn Andersson
  1 sibling, 1 reply; 12+ messages in thread
From: Loic PALLARDY @ 2017-01-10  8:18 UTC (permalink / raw)
  To: Bjorn Andersson, Suman Anna
  Cc: ohad, lee.jones, linux-remoteproc, kernel, Patrice CHOTARD



> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
> Sent: Tuesday, January 10, 2017 12:46 AM
> To: Loic PALLARDY <loic.pallardy@st.com>; Suman Anna <s-anna@ti.com>
> Cc: ohad@wizery.com; lee.jones@linaro.org; linux-
> remoteproc@vger.kernel.org; kernel@stlinux.com; Patrice CHOTARD
> <patrice.chotard@st.com>
> Subject: Re: [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation
> 
> 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.
Hi Bjorn,

Sorry for my late answer.
Tested on my side. Works fine on ST platform.
Thanks,
Loic 

> 
> (The fixed version is part of linux-next as well)
> 
> Regards,
> Bjorn

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation
  2017-01-10  0:47         ` Suman Anna
@ 2017-01-11 19:57           ` Bjorn Andersson
  0 siblings, 0 replies; 12+ messages in thread
From: Bjorn Andersson @ 2017-01-11 19:57 UTC (permalink / raw)
  To: Suman Anna
  Cc: Loic Pallardy, ohad, lee.jones, linux-remoteproc, kernel,
	patrice.chotard

On Mon 09 Jan 16:47 PST 2017, Suman Anna wrote:

> On 01/09/2017 05:45 PM, Bjorn Andersson wrote:
> > On Fri 30 Dec 11:56 PST 2016, Bjorn Andersson wrote:
[..]
> > 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)
> 
> FWIW, these work fine on TI platforms. I had run into the same issues
> that these commits were fixing.
> 

Thanks Suman!

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation
  2017-01-10  8:18         ` Loic PALLARDY
@ 2017-01-11 22:08           ` Bjorn Andersson
  0 siblings, 0 replies; 12+ messages in thread
From: Bjorn Andersson @ 2017-01-11 22:08 UTC (permalink / raw)
  To: Loic PALLARDY
  Cc: Suman Anna, ohad, lee.jones, linux-remoteproc, kernel, Patrice CHOTARD

On Tue 10 Jan 00:18 PST 2017, Loic PALLARDY wrote:
> > From: Bjorn Andersson [mailto:bjorn.andersson@linaro.org]
[..]
> > Loic, did you have a chance to give this a test run? I would appreciate your
> > feedback before I send this off to Linus.
> Hi Bjorn,
> 
> Sorry for my late answer.
> Tested on my side. Works fine on ST platform.

Thanks Loic!

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2017-01-11 22:08 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-15 14:49 [PATCH v2 1/1] rpmsg: virtio_rpmsg_bus: fix channel creation Loic Pallardy
2016-12-16  5:09 ` Bjorn Andersson
2016-12-27 22:36   ` Suman Anna
2016-12-30 19:56     ` Bjorn Andersson
2017-01-09 23:45       ` Bjorn Andersson
2017-01-10  0:47         ` Suman Anna
2017-01-11 19:57           ` Bjorn Andersson
2017-01-10  8:18         ` Loic PALLARDY
2017-01-11 22:08           ` Bjorn Andersson
2016-12-16 10:39 ` [STLinux Kernel] " Peter Griffin
2017-01-10  7:51 ` Lee Jones
2017-01-10  8:05   ` Marek Novak

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.