* [PATCH v2] rpmsg: core: Add wildcard match for name service @ 2020-03-10 15:50 Mathieu Poirier 2020-03-26 15:06 ` Suman Anna 0 siblings, 1 reply; 11+ messages in thread From: Mathieu Poirier @ 2020-03-10 15:50 UTC (permalink / raw) To: ohad, bjorn.andersson; +Cc: linux-remoteproc, linux-kernel Adding the capability to supplement the base definition published by an rpmsg_driver with a postfix description so that it is possible for several entity to use the same service. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> --- Changes for V2: - Added Arnaud's Acked-by. - Rebased to latest rproc-next. drivers/rpmsg/rpmsg_core.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c index e330ec4dfc33..bfd25978fa35 100644 --- a/drivers/rpmsg/rpmsg_core.c +++ b/drivers/rpmsg/rpmsg_core.c @@ -399,7 +399,25 @@ ATTRIBUTE_GROUPS(rpmsg_dev); static inline int rpmsg_id_match(const struct rpmsg_device *rpdev, const struct rpmsg_device_id *id) { - return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0; + size_t len = min_t(size_t, strlen(id->name), RPMSG_NAME_SIZE); + + /* + * Allow for wildcard matches. For example if rpmsg_driver::id_table + * is: + * + * static struct rpmsg_device_id rpmsg_driver_sample_id_table[] = { + * { .name = "rpmsg-client-sample" }, + * { }, + * } + * + * Then it is possible to support "rpmsg-client-sample*", i.e: + * rpmsg-client-sample + * rpmsg-client-sample_instance0 + * rpmsg-client-sample_instance1 + * ... + * rpmsg-client-sample_instanceX + */ + return strncmp(id->name, rpdev->id.name, len) == 0; } /* match rpmsg channel and rpmsg driver */ -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] rpmsg: core: Add wildcard match for name service 2020-03-10 15:50 [PATCH v2] rpmsg: core: Add wildcard match for name service Mathieu Poirier @ 2020-03-26 15:06 ` Suman Anna 2020-03-26 20:21 ` Mathieu Poirier 0 siblings, 1 reply; 11+ messages in thread From: Suman Anna @ 2020-03-26 15:06 UTC (permalink / raw) To: Mathieu Poirier, ohad, bjorn.andersson; +Cc: linux-remoteproc, linux-kernel Hi Mathieu, On 3/10/20 10:50 AM, Mathieu Poirier wrote: > Adding the capability to supplement the base definition published > by an rpmsg_driver with a postfix description so that it is possible > for several entity to use the same service. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> So, the concern I have here is that we are retrofitting this into the existing 32-byte name field, and the question is if it is going to be enough in general. That's the reason I went with the additional 32-byte field with the "rpmsg: add a description field" patch. regards Suman > --- > Changes for V2: > - Added Arnaud's Acked-by. > - Rebased to latest rproc-next. > > drivers/rpmsg/rpmsg_core.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c > index e330ec4dfc33..bfd25978fa35 100644 > --- a/drivers/rpmsg/rpmsg_core.c > +++ b/drivers/rpmsg/rpmsg_core.c > @@ -399,7 +399,25 @@ ATTRIBUTE_GROUPS(rpmsg_dev); > static inline int rpmsg_id_match(const struct rpmsg_device *rpdev, > const struct rpmsg_device_id *id) > { > - return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0; > + size_t len = min_t(size_t, strlen(id->name), RPMSG_NAME_SIZE); > + > + /* > + * Allow for wildcard matches. For example if rpmsg_driver::id_table > + * is: > + * > + * static struct rpmsg_device_id rpmsg_driver_sample_id_table[] = { > + * { .name = "rpmsg-client-sample" }, > + * { }, > + * } > + * > + * Then it is possible to support "rpmsg-client-sample*", i.e: > + * rpmsg-client-sample > + * rpmsg-client-sample_instance0 > + * rpmsg-client-sample_instance1 > + * ... > + * rpmsg-client-sample_instanceX > + */ > + return strncmp(id->name, rpdev->id.name, len) == 0; > } > > /* match rpmsg channel and rpmsg driver */ > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] rpmsg: core: Add wildcard match for name service 2020-03-26 15:06 ` Suman Anna @ 2020-03-26 20:21 ` Mathieu Poirier 2020-03-26 20:42 ` Suman Anna 0 siblings, 1 reply; 11+ messages in thread From: Mathieu Poirier @ 2020-03-26 20:21 UTC (permalink / raw) To: Suman Anna Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, Linux Kernel Mailing List On Thu, 26 Mar 2020 at 09:06, Suman Anna <s-anna@ti.com> wrote: > > Hi Mathieu, > > On 3/10/20 10:50 AM, Mathieu Poirier wrote: > > Adding the capability to supplement the base definition published > > by an rpmsg_driver with a postfix description so that it is possible > > for several entity to use the same service. > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> > > So, the concern I have here is that we are retrofitting this into the > existing 32-byte name field, and the question is if it is going to be > enough in general. That's the reason I went with the additional 32-byte > field with the "rpmsg: add a description field" patch. > That's a valid concern. Did you consider increasing the size of RPMSG_NAME_SIZE to 64? Have you found cases where that wouldn't work? I did a survey of all the places the #define is used and all destination buffers are also using the same #define in their definition. It would also be backward compatible with firmware implementations that use 32 byte. Thanks, Mathieu > regards > Suman > > > --- > > Changes for V2: > > - Added Arnaud's Acked-by. > > - Rebased to latest rproc-next. > > > > drivers/rpmsg/rpmsg_core.c | 20 +++++++++++++++++++- > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c > > index e330ec4dfc33..bfd25978fa35 100644 > > --- a/drivers/rpmsg/rpmsg_core.c > > +++ b/drivers/rpmsg/rpmsg_core.c > > @@ -399,7 +399,25 @@ ATTRIBUTE_GROUPS(rpmsg_dev); > > static inline int rpmsg_id_match(const struct rpmsg_device *rpdev, > > const struct rpmsg_device_id *id) > > { > > - return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0; > > + size_t len = min_t(size_t, strlen(id->name), RPMSG_NAME_SIZE); > > + > > + /* > > + * Allow for wildcard matches. For example if rpmsg_driver::id_table > > + * is: > > + * > > + * static struct rpmsg_device_id rpmsg_driver_sample_id_table[] = { > > + * { .name = "rpmsg-client-sample" }, > > + * { }, > > + * } > > + * > > + * Then it is possible to support "rpmsg-client-sample*", i.e: > > + * rpmsg-client-sample > > + * rpmsg-client-sample_instance0 > > + * rpmsg-client-sample_instance1 > > + * ... > > + * rpmsg-client-sample_instanceX > > + */ > > + return strncmp(id->name, rpdev->id.name, len) == 0; > > } > > > > /* match rpmsg channel and rpmsg driver */ > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] rpmsg: core: Add wildcard match for name service 2020-03-26 20:21 ` Mathieu Poirier @ 2020-03-26 20:42 ` Suman Anna 2020-03-26 22:01 ` Mathieu Poirier 0 siblings, 1 reply; 11+ messages in thread From: Suman Anna @ 2020-03-26 20:42 UTC (permalink / raw) To: Mathieu Poirier Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, Linux Kernel Mailing List On 3/26/20 3:21 PM, Mathieu Poirier wrote: > On Thu, 26 Mar 2020 at 09:06, Suman Anna <s-anna@ti.com> wrote: >> >> Hi Mathieu, >> >> On 3/10/20 10:50 AM, Mathieu Poirier wrote: >>> Adding the capability to supplement the base definition published >>> by an rpmsg_driver with a postfix description so that it is possible >>> for several entity to use the same service. >>> >>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >>> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> >> >> So, the concern I have here is that we are retrofitting this into the >> existing 32-byte name field, and the question is if it is going to be >> enough in general. That's the reason I went with the additional 32-byte >> field with the "rpmsg: add a description field" patch. >> > > That's a valid concern. > > Did you consider increasing the size of RPMSG_NAME_SIZE to 64? Have > you found cases where that wouldn't work? I did a survey of all the > places the #define is used and all destination buffers are also using > the same #define in their definition. It would also be backward > compatible with firmware implementations that use 32 byte. You can't directly bump the size without breaking the compatibility on the existing rpmsg_ns_msg in firmwares right? All the Linux-side drivers will be ok since they use the same macro but rpmsg_ns_msg has presence on both kernel and firmware-sides. regards Suman > > Thanks, > Mathieu > >> regards >> Suman >> >>> --- >>> Changes for V2: >>> - Added Arnaud's Acked-by. >>> - Rebased to latest rproc-next. >>> >>> drivers/rpmsg/rpmsg_core.c | 20 +++++++++++++++++++- >>> 1 file changed, 19 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c >>> index e330ec4dfc33..bfd25978fa35 100644 >>> --- a/drivers/rpmsg/rpmsg_core.c >>> +++ b/drivers/rpmsg/rpmsg_core.c >>> @@ -399,7 +399,25 @@ ATTRIBUTE_GROUPS(rpmsg_dev); >>> static inline int rpmsg_id_match(const struct rpmsg_device *rpdev, >>> const struct rpmsg_device_id *id) >>> { >>> - return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0; >>> + size_t len = min_t(size_t, strlen(id->name), RPMSG_NAME_SIZE); >>> + >>> + /* >>> + * Allow for wildcard matches. For example if rpmsg_driver::id_table >>> + * is: >>> + * >>> + * static struct rpmsg_device_id rpmsg_driver_sample_id_table[] = { >>> + * { .name = "rpmsg-client-sample" }, >>> + * { }, >>> + * } >>> + * >>> + * Then it is possible to support "rpmsg-client-sample*", i.e: >>> + * rpmsg-client-sample >>> + * rpmsg-client-sample_instance0 >>> + * rpmsg-client-sample_instance1 >>> + * ... >>> + * rpmsg-client-sample_instanceX >>> + */ >>> + return strncmp(id->name, rpdev->id.name, len) == 0; >>> } >>> >>> /* match rpmsg channel and rpmsg driver */ >>> >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] rpmsg: core: Add wildcard match for name service 2020-03-26 20:42 ` Suman Anna @ 2020-03-26 22:01 ` Mathieu Poirier 2020-03-27 9:35 ` Arnaud POULIQUEN 0 siblings, 1 reply; 11+ messages in thread From: Mathieu Poirier @ 2020-03-26 22:01 UTC (permalink / raw) To: Suman Anna Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, Linux Kernel Mailing List On Thu, 26 Mar 2020 at 14:42, Suman Anna <s-anna@ti.com> wrote: > > On 3/26/20 3:21 PM, Mathieu Poirier wrote: > > On Thu, 26 Mar 2020 at 09:06, Suman Anna <s-anna@ti.com> wrote: > >> > >> Hi Mathieu, > >> > >> On 3/10/20 10:50 AM, Mathieu Poirier wrote: > >>> Adding the capability to supplement the base definition published > >>> by an rpmsg_driver with a postfix description so that it is possible > >>> for several entity to use the same service. > >>> > >>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > >>> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> > >> > >> So, the concern I have here is that we are retrofitting this into the > >> existing 32-byte name field, and the question is if it is going to be > >> enough in general. That's the reason I went with the additional 32-byte > >> field with the "rpmsg: add a description field" patch. > >> > > > > That's a valid concern. > > > > Did you consider increasing the size of RPMSG_NAME_SIZE to 64? Have > > you found cases where that wouldn't work? I did a survey of all the > > places the #define is used and all destination buffers are also using > > the same #define in their definition. It would also be backward > > compatible with firmware implementations that use 32 byte. > > You can't directly bump the size without breaking the compatibility on > the existing rpmsg_ns_msg in firmwares right? All the Linux-side drivers > will be ok since they use the same macro but rpmsg_ns_msg has presence > on both kernel and firmware-sides. Ah yes yes... The amount of bytes coming out of the pipe won't match. Let me think a little... > > regards > Suman > > > > > Thanks, > > Mathieu > > > >> regards > >> Suman > >> > >>> --- > >>> Changes for V2: > >>> - Added Arnaud's Acked-by. > >>> - Rebased to latest rproc-next. > >>> > >>> drivers/rpmsg/rpmsg_core.c | 20 +++++++++++++++++++- > >>> 1 file changed, 19 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c > >>> index e330ec4dfc33..bfd25978fa35 100644 > >>> --- a/drivers/rpmsg/rpmsg_core.c > >>> +++ b/drivers/rpmsg/rpmsg_core.c > >>> @@ -399,7 +399,25 @@ ATTRIBUTE_GROUPS(rpmsg_dev); > >>> static inline int rpmsg_id_match(const struct rpmsg_device *rpdev, > >>> const struct rpmsg_device_id *id) > >>> { > >>> - return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0; > >>> + size_t len = min_t(size_t, strlen(id->name), RPMSG_NAME_SIZE); > >>> + > >>> + /* > >>> + * Allow for wildcard matches. For example if rpmsg_driver::id_table > >>> + * is: > >>> + * > >>> + * static struct rpmsg_device_id rpmsg_driver_sample_id_table[] = { > >>> + * { .name = "rpmsg-client-sample" }, > >>> + * { }, > >>> + * } > >>> + * > >>> + * Then it is possible to support "rpmsg-client-sample*", i.e: > >>> + * rpmsg-client-sample > >>> + * rpmsg-client-sample_instance0 > >>> + * rpmsg-client-sample_instance1 > >>> + * ... > >>> + * rpmsg-client-sample_instanceX > >>> + */ > >>> + return strncmp(id->name, rpdev->id.name, len) == 0; > >>> } > >>> > >>> /* match rpmsg channel and rpmsg driver */ > >>> > >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] rpmsg: core: Add wildcard match for name service 2020-03-26 22:01 ` Mathieu Poirier @ 2020-03-27 9:35 ` Arnaud POULIQUEN 2020-03-27 19:36 ` Mathieu Poirier 0 siblings, 1 reply; 11+ messages in thread From: Arnaud POULIQUEN @ 2020-03-27 9:35 UTC (permalink / raw) To: Mathieu Poirier, Suman Anna Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, Linux Kernel Mailing List Hi On 3/26/20 11:01 PM, Mathieu Poirier wrote: > On Thu, 26 Mar 2020 at 14:42, Suman Anna <s-anna@ti.com> wrote: >> >> On 3/26/20 3:21 PM, Mathieu Poirier wrote: >>> On Thu, 26 Mar 2020 at 09:06, Suman Anna <s-anna@ti.com> wrote: >>>> >>>> Hi Mathieu, >>>> >>>> On 3/10/20 10:50 AM, Mathieu Poirier wrote: >>>>> Adding the capability to supplement the base definition published >>>>> by an rpmsg_driver with a postfix description so that it is possible >>>>> for several entity to use the same service. >>>>> >>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >>>>> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> >>>> >>>> So, the concern I have here is that we are retrofitting this into the >>>> existing 32-byte name field, and the question is if it is going to be >>>> enough in general. That's the reason I went with the additional 32-byte >>>> field with the "rpmsg: add a description field" patch. >>>> >>> >>> That's a valid concern. >>> >>> Did you consider increasing the size of RPMSG_NAME_SIZE to 64? Have >>> you found cases where that wouldn't work? I did a survey of all the >>> places the #define is used and all destination buffers are also using >>> the same #define in their definition. It would also be backward >>> compatible with firmware implementations that use 32 byte. >> >> You can't directly bump the size without breaking the compatibility on >> the existing rpmsg_ns_msg in firmwares right? All the Linux-side drivers >> will be ok since they use the same macro but rpmsg_ns_msg has presence >> on both kernel and firmware-sides. > > Ah yes yes... The amount of bytes coming out of the pipe won't match. > Let me think a little... +1 for Suman's concern. Anyway i would like to challenge the need of more than 32 bytes to differentiate service instances. "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", seems to me enough if we only need to differentiate the instances. But perhaps the need is also to provide a short description of the service? Suman, could you share some examples of your need? Regards Arnaud > >> >> regards >> Suman >> >>> >>> Thanks, >>> Mathieu >>> >>>> regards >>>> Suman >>>> >>>>> --- >>>>> Changes for V2: >>>>> - Added Arnaud's Acked-by. >>>>> - Rebased to latest rproc-next. >>>>> >>>>> drivers/rpmsg/rpmsg_core.c | 20 +++++++++++++++++++- >>>>> 1 file changed, 19 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c >>>>> index e330ec4dfc33..bfd25978fa35 100644 >>>>> --- a/drivers/rpmsg/rpmsg_core.c >>>>> +++ b/drivers/rpmsg/rpmsg_core.c >>>>> @@ -399,7 +399,25 @@ ATTRIBUTE_GROUPS(rpmsg_dev); >>>>> static inline int rpmsg_id_match(const struct rpmsg_device *rpdev, >>>>> const struct rpmsg_device_id *id) >>>>> { >>>>> - return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0; >>>>> + size_t len = min_t(size_t, strlen(id->name), RPMSG_NAME_SIZE); >>>>> + >>>>> + /* >>>>> + * Allow for wildcard matches. For example if rpmsg_driver::id_table >>>>> + * is: >>>>> + * >>>>> + * static struct rpmsg_device_id rpmsg_driver_sample_id_table[] = { >>>>> + * { .name = "rpmsg-client-sample" }, >>>>> + * { }, >>>>> + * } >>>>> + * >>>>> + * Then it is possible to support "rpmsg-client-sample*", i.e: >>>>> + * rpmsg-client-sample >>>>> + * rpmsg-client-sample_instance0 >>>>> + * rpmsg-client-sample_instance1 >>>>> + * ... >>>>> + * rpmsg-client-sample_instanceX >>>>> + */ >>>>> + return strncmp(id->name, rpdev->id.name, len) == 0; >>>>> } >>>>> >>>>> /* match rpmsg channel and rpmsg driver */ >>>>> >>>> >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] rpmsg: core: Add wildcard match for name service 2020-03-27 9:35 ` Arnaud POULIQUEN @ 2020-03-27 19:36 ` Mathieu Poirier 2020-04-07 23:07 ` Suman Anna 0 siblings, 1 reply; 11+ messages in thread From: Mathieu Poirier @ 2020-03-27 19:36 UTC (permalink / raw) To: Arnaud POULIQUEN Cc: Suman Anna, Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, Linux Kernel Mailing List On Fri, Mar 27, 2020 at 10:35:34AM +0100, Arnaud POULIQUEN wrote: > Hi > > On 3/26/20 11:01 PM, Mathieu Poirier wrote: > > On Thu, 26 Mar 2020 at 14:42, Suman Anna <s-anna@ti.com> wrote: > >> > >> On 3/26/20 3:21 PM, Mathieu Poirier wrote: > >>> On Thu, 26 Mar 2020 at 09:06, Suman Anna <s-anna@ti.com> wrote: > >>>> > >>>> Hi Mathieu, > >>>> > >>>> On 3/10/20 10:50 AM, Mathieu Poirier wrote: > >>>>> Adding the capability to supplement the base definition published > >>>>> by an rpmsg_driver with a postfix description so that it is possible > >>>>> for several entity to use the same service. > >>>>> > >>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > >>>>> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> > >>>> > >>>> So, the concern I have here is that we are retrofitting this into the > >>>> existing 32-byte name field, and the question is if it is going to be > >>>> enough in general. That's the reason I went with the additional 32-byte > >>>> field with the "rpmsg: add a description field" patch. > >>>> > >>> > >>> That's a valid concern. > >>> > >>> Did you consider increasing the size of RPMSG_NAME_SIZE to 64? Have > >>> you found cases where that wouldn't work? I did a survey of all the > >>> places the #define is used and all destination buffers are also using > >>> the same #define in their definition. It would also be backward > >>> compatible with firmware implementations that use 32 byte. > >> > >> You can't directly bump the size without breaking the compatibility on > >> the existing rpmsg_ns_msg in firmwares right? All the Linux-side drivers > >> will be ok since they use the same macro but rpmsg_ns_msg has presence > >> on both kernel and firmware-sides. > > > > Ah yes yes... The amount of bytes coming out of the pipe won't match. > > Let me think a little... > > +1 for Suman's concern. > > Anyway i would like to challenge the need of more than 32 bytes to > differentiate service instances. > "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", seems to me enough if we only need > to differentiate the instances. > > But perhaps the need is also to provide a short description of the service? > > Suman, could you share some examples of your need? Looking at things further it is possible to extend the name of the service to 64 byte while keeping backward compatibility by looking up the size of @len in function rpmsg_ns_cb(). From there work with an rpmsg_ns_msg or a new rpmsg_ns_msg64, pretty much the way you did in your patch[1]. In fact the approach is the same except you are using 2 arrays of 32 byte and I'm using one of 64. As Arnaud mentioned, is there an immediate need to support a 64-byte name? If not than I suggest to move forward with this patch and address the issue when we get there - at least we know there is room for extention. Otherwise I'll spin off another revision but it will be bigger and more complex. Thanks, Mathieu [1]. https://patchwork.kernel.org/patch/11096599/ > > Regards > Arnaud > > > > >> > >> regards > >> Suman > >> > >>> > >>> Thanks, > >>> Mathieu > >>> > >>>> regards > >>>> Suman > >>>> > >>>>> --- > >>>>> Changes for V2: > >>>>> - Added Arnaud's Acked-by. > >>>>> - Rebased to latest rproc-next. > >>>>> > >>>>> drivers/rpmsg/rpmsg_core.c | 20 +++++++++++++++++++- > >>>>> 1 file changed, 19 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c > >>>>> index e330ec4dfc33..bfd25978fa35 100644 > >>>>> --- a/drivers/rpmsg/rpmsg_core.c > >>>>> +++ b/drivers/rpmsg/rpmsg_core.c > >>>>> @@ -399,7 +399,25 @@ ATTRIBUTE_GROUPS(rpmsg_dev); > >>>>> static inline int rpmsg_id_match(const struct rpmsg_device *rpdev, > >>>>> const struct rpmsg_device_id *id) > >>>>> { > >>>>> - return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0; > >>>>> + size_t len = min_t(size_t, strlen(id->name), RPMSG_NAME_SIZE); > >>>>> + > >>>>> + /* > >>>>> + * Allow for wildcard matches. For example if rpmsg_driver::id_table > >>>>> + * is: > >>>>> + * > >>>>> + * static struct rpmsg_device_id rpmsg_driver_sample_id_table[] = { > >>>>> + * { .name = "rpmsg-client-sample" }, > >>>>> + * { }, > >>>>> + * } > >>>>> + * > >>>>> + * Then it is possible to support "rpmsg-client-sample*", i.e: > >>>>> + * rpmsg-client-sample > >>>>> + * rpmsg-client-sample_instance0 > >>>>> + * rpmsg-client-sample_instance1 > >>>>> + * ... > >>>>> + * rpmsg-client-sample_instanceX > >>>>> + */ > >>>>> + return strncmp(id->name, rpdev->id.name, len) == 0; > >>>>> } > >>>>> > >>>>> /* match rpmsg channel and rpmsg driver */ > >>>>> > >>>> > >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] rpmsg: core: Add wildcard match for name service 2020-03-27 19:36 ` Mathieu Poirier @ 2020-04-07 23:07 ` Suman Anna 2020-04-08 15:59 ` Mathieu Poirier 0 siblings, 1 reply; 11+ messages in thread From: Suman Anna @ 2020-04-07 23:07 UTC (permalink / raw) To: Mathieu Poirier, Arnaud POULIQUEN Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, Linux Kernel Mailing List Hi Mathieu, Arnaud, On 3/27/20 2:36 PM, Mathieu Poirier wrote: > On Fri, Mar 27, 2020 at 10:35:34AM +0100, Arnaud POULIQUEN wrote: >> Hi >> >> On 3/26/20 11:01 PM, Mathieu Poirier wrote: >>> On Thu, 26 Mar 2020 at 14:42, Suman Anna <s-anna@ti.com> wrote: >>>> >>>> On 3/26/20 3:21 PM, Mathieu Poirier wrote: >>>>> On Thu, 26 Mar 2020 at 09:06, Suman Anna <s-anna@ti.com> wrote: >>>>>> >>>>>> Hi Mathieu, >>>>>> >>>>>> On 3/10/20 10:50 AM, Mathieu Poirier wrote: >>>>>>> Adding the capability to supplement the base definition published >>>>>>> by an rpmsg_driver with a postfix description so that it is possible >>>>>>> for several entity to use the same service. >>>>>>> >>>>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >>>>>>> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> >>>>>> >>>>>> So, the concern I have here is that we are retrofitting this into the >>>>>> existing 32-byte name field, and the question is if it is going to be >>>>>> enough in general. That's the reason I went with the additional 32-byte >>>>>> field with the "rpmsg: add a description field" patch. >>>>>> >>>>> >>>>> That's a valid concern. >>>>> >>>>> Did you consider increasing the size of RPMSG_NAME_SIZE to 64? Have >>>>> you found cases where that wouldn't work? I did a survey of all the >>>>> places the #define is used and all destination buffers are also using >>>>> the same #define in their definition. It would also be backward >>>>> compatible with firmware implementations that use 32 byte. >>>> >>>> You can't directly bump the size without breaking the compatibility on >>>> the existing rpmsg_ns_msg in firmwares right? All the Linux-side drivers >>>> will be ok since they use the same macro but rpmsg_ns_msg has presence >>>> on both kernel and firmware-sides. >>> >>> Ah yes yes... The amount of bytes coming out of the pipe won't match. >>> Let me think a little... >> >> +1 for Suman's concern. >> >> Anyway i would like to challenge the need of more than 32 bytes to >> differentiate service instances. >> "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", seems to me enough if we only need >> to differentiate the instances. Remember that the rpmsg_device_id name takes some space within here. So, the shorter the rpmsg_device_id table name, the more room you have. >> >> But perhaps the need is also to provide a short description of the service? I am mostly using it to provide a unique instantiation name. In anycase, I have cross-checked against my current firmwares, and so far all of them happen to have the name + desc < 31 bytes. >> >> Suman, could you share some examples of your need? > > Looking at things further it is possible to extend the name of the service to > 64 byte while keeping backward compatibility by looking up the size of @len > in function rpmsg_ns_cb(). From there work with an rpmsg_ns_msg or a new > rpmsg_ns_msg64, pretty much the way you did in your patch[1]. In fact the > approach is the same except you are using 2 arrays of 32 byte and I'm using one > of 64. > > As Arnaud mentioned, is there an immediate need to support a 64-byte name? If > not than I suggest to move forward with this patch and address the issue when we > get there - at least we know there is room for extention. Otherwise I'll spin > off another revision but it will be bigger and more complex. Yeah ok. I have managed to get my downstream drivers that use the desc field working with this patch after modifying the firmwares to publish using combined name, and adding logic in probe to get the trailing portion of the name. So, the only thing that is missing or content for another patch is if we need to add some tooling/helper stuff for giving the trailing stuff to rpmsg drivers? regards Suman > > Thanks, > Mathieu > > [1]. https://patchwork.kernel.org/patch/11096599/ > >>>>>> >>>>>>> --- >>>>>>> Changes for V2: >>>>>>> - Added Arnaud's Acked-by. >>>>>>> - Rebased to latest rproc-next. >>>>>>> >>>>>>> drivers/rpmsg/rpmsg_core.c | 20 +++++++++++++++++++- >>>>>>> 1 file changed, 19 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c >>>>>>> index e330ec4dfc33..bfd25978fa35 100644 >>>>>>> --- a/drivers/rpmsg/rpmsg_core.c >>>>>>> +++ b/drivers/rpmsg/rpmsg_core.c >>>>>>> @@ -399,7 +399,25 @@ ATTRIBUTE_GROUPS(rpmsg_dev); >>>>>>> static inline int rpmsg_id_match(const struct rpmsg_device *rpdev, >>>>>>> const struct rpmsg_device_id *id) >>>>>>> { >>>>>>> - return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0; >>>>>>> + size_t len = min_t(size_t, strlen(id->name), RPMSG_NAME_SIZE); >>>>>>> + >>>>>>> + /* >>>>>>> + * Allow for wildcard matches. For example if rpmsg_driver::id_table >>>>>>> + * is: >>>>>>> + * >>>>>>> + * static struct rpmsg_device_id rpmsg_driver_sample_id_table[] = { >>>>>>> + * { .name = "rpmsg-client-sample" }, >>>>>>> + * { }, >>>>>>> + * } >>>>>>> + * >>>>>>> + * Then it is possible to support "rpmsg-client-sample*", i.e: >>>>>>> + * rpmsg-client-sample >>>>>>> + * rpmsg-client-sample_instance0 >>>>>>> + * rpmsg-client-sample_instance1 >>>>>>> + * ... >>>>>>> + * rpmsg-client-sample_instanceX >>>>>>> + */ >>>>>>> + return strncmp(id->name, rpdev->id.name, len) == 0; >>>>>>> } >>>>>>> >>>>>>> /* match rpmsg channel and rpmsg driver */ >>>>>>> >>>>>> >>>> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] rpmsg: core: Add wildcard match for name service 2020-04-07 23:07 ` Suman Anna @ 2020-04-08 15:59 ` Mathieu Poirier 2020-04-08 20:52 ` Suman Anna 0 siblings, 1 reply; 11+ messages in thread From: Mathieu Poirier @ 2020-04-08 15:59 UTC (permalink / raw) To: Suman Anna Cc: Arnaud POULIQUEN, Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, Linux Kernel Mailing List On Tue, 7 Apr 2020 at 17:07, Suman Anna <s-anna@ti.com> wrote: > > Hi Mathieu, Arnaud, > > On 3/27/20 2:36 PM, Mathieu Poirier wrote: > > On Fri, Mar 27, 2020 at 10:35:34AM +0100, Arnaud POULIQUEN wrote: > >> Hi > >> > >> On 3/26/20 11:01 PM, Mathieu Poirier wrote: > >>> On Thu, 26 Mar 2020 at 14:42, Suman Anna <s-anna@ti.com> wrote: > >>>> > >>>> On 3/26/20 3:21 PM, Mathieu Poirier wrote: > >>>>> On Thu, 26 Mar 2020 at 09:06, Suman Anna <s-anna@ti.com> wrote: > >>>>>> > >>>>>> Hi Mathieu, > >>>>>> > >>>>>> On 3/10/20 10:50 AM, Mathieu Poirier wrote: > >>>>>>> Adding the capability to supplement the base definition published > >>>>>>> by an rpmsg_driver with a postfix description so that it is possible > >>>>>>> for several entity to use the same service. > >>>>>>> > >>>>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > >>>>>>> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> > >>>>>> > >>>>>> So, the concern I have here is that we are retrofitting this into the > >>>>>> existing 32-byte name field, and the question is if it is going to be > >>>>>> enough in general. That's the reason I went with the additional 32-byte > >>>>>> field with the "rpmsg: add a description field" patch. > >>>>>> > >>>>> > >>>>> That's a valid concern. > >>>>> > >>>>> Did you consider increasing the size of RPMSG_NAME_SIZE to 64? Have > >>>>> you found cases where that wouldn't work? I did a survey of all the > >>>>> places the #define is used and all destination buffers are also using > >>>>> the same #define in their definition. It would also be backward > >>>>> compatible with firmware implementations that use 32 byte. > >>>> > >>>> You can't directly bump the size without breaking the compatibility on > >>>> the existing rpmsg_ns_msg in firmwares right? All the Linux-side drivers > >>>> will be ok since they use the same macro but rpmsg_ns_msg has presence > >>>> on both kernel and firmware-sides. > >>> > >>> Ah yes yes... The amount of bytes coming out of the pipe won't match. > >>> Let me think a little... > >> > >> +1 for Suman's concern. > >> > >> Anyway i would like to challenge the need of more than 32 bytes to > >> differentiate service instances. > >> "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", seems to me enough if we only need > >> to differentiate the instances. > > Remember that the rpmsg_device_id name takes some space within here. So, > the shorter the rpmsg_device_id table name, the more room you have. > > >> > >> But perhaps the need is also to provide a short description of the service? > > I am mostly using it to provide a unique instantiation name. In anycase, > I have cross-checked against my current firmwares, and so far all of > them happen to have the name + desc < 31 bytes. > > > >> > >> Suman, could you share some examples of your need? > > > > Looking at things further it is possible to extend the name of the service to > > 64 byte while keeping backward compatibility by looking up the size of @len > > in function rpmsg_ns_cb(). From there work with an rpmsg_ns_msg or a new > > rpmsg_ns_msg64, pretty much the way you did in your patch[1]. In fact the > > approach is the same except you are using 2 arrays of 32 byte and I'm using one > > of 64. > > > > As Arnaud mentioned, is there an immediate need to support a 64-byte name? If > > not than I suggest to move forward with this patch and address the issue when we > > get there - at least we know there is room for extention. Otherwise I'll spin > > off another revision but it will be bigger and more complex. > > Yeah ok. I have managed to get my downstream drivers that use the desc > field working with this patch after modifying the firmwares to publish > using combined name, and adding logic in probe to get the trailing > portion of the name. Perfect > > So, the only thing that is missing or content for another patch is if we > need to add some tooling/helper stuff for giving the trailing stuff to > rpmsg drivers? So that all rpmsg drivers don't come up with their own parsing that ends up doing the same thing. Let me think about that - I may have to get back to you... > > regards > Suman > > > > > Thanks, > > Mathieu > > > > [1]. https://patchwork.kernel.org/patch/11096599/ > > > >>>>>> > >>>>>>> --- > >>>>>>> Changes for V2: > >>>>>>> - Added Arnaud's Acked-by. > >>>>>>> - Rebased to latest rproc-next. > >>>>>>> > >>>>>>> drivers/rpmsg/rpmsg_core.c | 20 +++++++++++++++++++- > >>>>>>> 1 file changed, 19 insertions(+), 1 deletion(-) > >>>>>>> > >>>>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c > >>>>>>> index e330ec4dfc33..bfd25978fa35 100644 > >>>>>>> --- a/drivers/rpmsg/rpmsg_core.c > >>>>>>> +++ b/drivers/rpmsg/rpmsg_core.c > >>>>>>> @@ -399,7 +399,25 @@ ATTRIBUTE_GROUPS(rpmsg_dev); > >>>>>>> static inline int rpmsg_id_match(const struct rpmsg_device *rpdev, > >>>>>>> const struct rpmsg_device_id *id) > >>>>>>> { > >>>>>>> - return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0; > >>>>>>> + size_t len = min_t(size_t, strlen(id->name), RPMSG_NAME_SIZE); > >>>>>>> + > >>>>>>> + /* > >>>>>>> + * Allow for wildcard matches. For example if rpmsg_driver::id_table > >>>>>>> + * is: > >>>>>>> + * > >>>>>>> + * static struct rpmsg_device_id rpmsg_driver_sample_id_table[] = { > >>>>>>> + * { .name = "rpmsg-client-sample" }, > >>>>>>> + * { }, > >>>>>>> + * } > >>>>>>> + * > >>>>>>> + * Then it is possible to support "rpmsg-client-sample*", i.e: > >>>>>>> + * rpmsg-client-sample > >>>>>>> + * rpmsg-client-sample_instance0 > >>>>>>> + * rpmsg-client-sample_instance1 > >>>>>>> + * ... > >>>>>>> + * rpmsg-client-sample_instanceX > >>>>>>> + */ > >>>>>>> + return strncmp(id->name, rpdev->id.name, len) == 0; > >>>>>>> } > >>>>>>> > >>>>>>> /* match rpmsg channel and rpmsg driver */ > >>>>>>> > >>>>>> > >>>> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] rpmsg: core: Add wildcard match for name service 2020-04-08 15:59 ` Mathieu Poirier @ 2020-04-08 20:52 ` Suman Anna 2020-04-09 8:54 ` Arnaud POULIQUEN 0 siblings, 1 reply; 11+ messages in thread From: Suman Anna @ 2020-04-08 20:52 UTC (permalink / raw) To: Mathieu Poirier Cc: Arnaud POULIQUEN, Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, Linux Kernel Mailing List On 4/8/20 10:59 AM, Mathieu Poirier wrote: > On Tue, 7 Apr 2020 at 17:07, Suman Anna <s-anna@ti.com> wrote: >> >> Hi Mathieu, Arnaud, >> >> On 3/27/20 2:36 PM, Mathieu Poirier wrote: >>> On Fri, Mar 27, 2020 at 10:35:34AM +0100, Arnaud POULIQUEN wrote: >>>> Hi >>>> >>>> On 3/26/20 11:01 PM, Mathieu Poirier wrote: >>>>> On Thu, 26 Mar 2020 at 14:42, Suman Anna <s-anna@ti.com> wrote: >>>>>> >>>>>> On 3/26/20 3:21 PM, Mathieu Poirier wrote: >>>>>>> On Thu, 26 Mar 2020 at 09:06, Suman Anna <s-anna@ti.com> wrote: >>>>>>>> >>>>>>>> Hi Mathieu, >>>>>>>> >>>>>>>> On 3/10/20 10:50 AM, Mathieu Poirier wrote: >>>>>>>>> Adding the capability to supplement the base definition published >>>>>>>>> by an rpmsg_driver with a postfix description so that it is possible >>>>>>>>> for several entity to use the same service. >>>>>>>>> >>>>>>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >>>>>>>>> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> >>>>>>>> >>>>>>>> So, the concern I have here is that we are retrofitting this into the >>>>>>>> existing 32-byte name field, and the question is if it is going to be >>>>>>>> enough in general. That's the reason I went with the additional 32-byte >>>>>>>> field with the "rpmsg: add a description field" patch. >>>>>>>> >>>>>>> >>>>>>> That's a valid concern. >>>>>>> >>>>>>> Did you consider increasing the size of RPMSG_NAME_SIZE to 64? Have >>>>>>> you found cases where that wouldn't work? I did a survey of all the >>>>>>> places the #define is used and all destination buffers are also using >>>>>>> the same #define in their definition. It would also be backward >>>>>>> compatible with firmware implementations that use 32 byte. >>>>>> >>>>>> You can't directly bump the size without breaking the compatibility on >>>>>> the existing rpmsg_ns_msg in firmwares right? All the Linux-side drivers >>>>>> will be ok since they use the same macro but rpmsg_ns_msg has presence >>>>>> on both kernel and firmware-sides. >>>>> >>>>> Ah yes yes... The amount of bytes coming out of the pipe won't match. >>>>> Let me think a little... >>>> >>>> +1 for Suman's concern. >>>> >>>> Anyway i would like to challenge the need of more than 32 bytes to >>>> differentiate service instances. >>>> "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", seems to me enough if we only need >>>> to differentiate the instances. >> >> Remember that the rpmsg_device_id name takes some space within here. So, >> the shorter the rpmsg_device_id table name, the more room you have. >> >>>> >>>> But perhaps the need is also to provide a short description of the service? >> >> I am mostly using it to provide a unique instantiation name. In anycase, >> I have cross-checked against my current firmwares, and so far all of >> them happen to have the name + desc < 31 bytes. >> >> >>>> >>>> Suman, could you share some examples of your need? >>> >>> Looking at things further it is possible to extend the name of the service to >>> 64 byte while keeping backward compatibility by looking up the size of @len >>> in function rpmsg_ns_cb(). From there work with an rpmsg_ns_msg or a new >>> rpmsg_ns_msg64, pretty much the way you did in your patch[1]. In fact the >>> approach is the same except you are using 2 arrays of 32 byte and I'm using one >>> of 64. >>> >>> As Arnaud mentioned, is there an immediate need to support a 64-byte name? If >>> not than I suggest to move forward with this patch and address the issue when we >>> get there - at least we know there is room for extention. Otherwise I'll spin >>> off another revision but it will be bigger and more complex. >> >> Yeah ok. I have managed to get my downstream drivers that use the desc >> field working with this patch after modifying the firmwares to publish >> using combined name, and adding logic in probe to get the trailing >> portion of the name. > > Perfect > >> >> So, the only thing that is missing or content for another patch is if we >> need to add some tooling/helper stuff for giving the trailing stuff to >> rpmsg drivers? > > So that all rpmsg drivers don't come up with their own parsing that > ends up doing the same thing. Let me think about that - I may have to > get back to you... Yep. Sure no problem. It can be a patch on top of this as well. Arnaud, Do you have immediate need for the tooling stuff for the rpmsg-tty driver? regards Suman > >> >> regards >> Suman >> >>> >>> Thanks, >>> Mathieu >>> >>> [1]. https://patchwork.kernel.org/patch/11096599/ >>> >>>>>>>> >>>>>>>>> --- >>>>>>>>> Changes for V2: >>>>>>>>> - Added Arnaud's Acked-by. >>>>>>>>> - Rebased to latest rproc-next. >>>>>>>>> >>>>>>>>> drivers/rpmsg/rpmsg_core.c | 20 +++++++++++++++++++- >>>>>>>>> 1 file changed, 19 insertions(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c >>>>>>>>> index e330ec4dfc33..bfd25978fa35 100644 >>>>>>>>> --- a/drivers/rpmsg/rpmsg_core.c >>>>>>>>> +++ b/drivers/rpmsg/rpmsg_core.c >>>>>>>>> @@ -399,7 +399,25 @@ ATTRIBUTE_GROUPS(rpmsg_dev); >>>>>>>>> static inline int rpmsg_id_match(const struct rpmsg_device *rpdev, >>>>>>>>> const struct rpmsg_device_id *id) >>>>>>>>> { >>>>>>>>> - return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0; >>>>>>>>> + size_t len = min_t(size_t, strlen(id->name), RPMSG_NAME_SIZE); >>>>>>>>> + >>>>>>>>> + /* >>>>>>>>> + * Allow for wildcard matches. For example if rpmsg_driver::id_table >>>>>>>>> + * is: >>>>>>>>> + * >>>>>>>>> + * static struct rpmsg_device_id rpmsg_driver_sample_id_table[] = { >>>>>>>>> + * { .name = "rpmsg-client-sample" }, >>>>>>>>> + * { }, >>>>>>>>> + * } >>>>>>>>> + * >>>>>>>>> + * Then it is possible to support "rpmsg-client-sample*", i.e: >>>>>>>>> + * rpmsg-client-sample >>>>>>>>> + * rpmsg-client-sample_instance0 >>>>>>>>> + * rpmsg-client-sample_instance1 >>>>>>>>> + * ... >>>>>>>>> + * rpmsg-client-sample_instanceX >>>>>>>>> + */ >>>>>>>>> + return strncmp(id->name, rpdev->id.name, len) == 0; >>>>>>>>> } >>>>>>>>> >>>>>>>>> /* match rpmsg channel and rpmsg driver */ >>>>>>>>> >>>>>>>> >>>>>> >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] rpmsg: core: Add wildcard match for name service 2020-04-08 20:52 ` Suman Anna @ 2020-04-09 8:54 ` Arnaud POULIQUEN 0 siblings, 0 replies; 11+ messages in thread From: Arnaud POULIQUEN @ 2020-04-09 8:54 UTC (permalink / raw) To: Suman Anna, Mathieu Poirier Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, Linux Kernel Mailing List Hi Suman, On 4/8/20 10:52 PM, Suman Anna wrote: > On 4/8/20 10:59 AM, Mathieu Poirier wrote: >> On Tue, 7 Apr 2020 at 17:07, Suman Anna <s-anna@ti.com> wrote: >>> >>> Hi Mathieu, Arnaud, >>> >>> On 3/27/20 2:36 PM, Mathieu Poirier wrote: >>>> On Fri, Mar 27, 2020 at 10:35:34AM +0100, Arnaud POULIQUEN wrote: >>>>> Hi >>>>> >>>>> On 3/26/20 11:01 PM, Mathieu Poirier wrote: >>>>>> On Thu, 26 Mar 2020 at 14:42, Suman Anna <s-anna@ti.com> wrote: >>>>>>> >>>>>>> On 3/26/20 3:21 PM, Mathieu Poirier wrote: >>>>>>>> On Thu, 26 Mar 2020 at 09:06, Suman Anna <s-anna@ti.com> wrote: >>>>>>>>> >>>>>>>>> Hi Mathieu, >>>>>>>>> >>>>>>>>> On 3/10/20 10:50 AM, Mathieu Poirier wrote: >>>>>>>>>> Adding the capability to supplement the base definition published >>>>>>>>>> by an rpmsg_driver with a postfix description so that it is possible >>>>>>>>>> for several entity to use the same service. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >>>>>>>>>> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> >>>>>>>>> >>>>>>>>> So, the concern I have here is that we are retrofitting this into the >>>>>>>>> existing 32-byte name field, and the question is if it is going to be >>>>>>>>> enough in general. That's the reason I went with the additional 32-byte >>>>>>>>> field with the "rpmsg: add a description field" patch. >>>>>>>>> >>>>>>>> >>>>>>>> That's a valid concern. >>>>>>>> >>>>>>>> Did you consider increasing the size of RPMSG_NAME_SIZE to 64? Have >>>>>>>> you found cases where that wouldn't work? I did a survey of all the >>>>>>>> places the #define is used and all destination buffers are also using >>>>>>>> the same #define in their definition. It would also be backward >>>>>>>> compatible with firmware implementations that use 32 byte. >>>>>>> >>>>>>> You can't directly bump the size without breaking the compatibility on >>>>>>> the existing rpmsg_ns_msg in firmwares right? All the Linux-side drivers >>>>>>> will be ok since they use the same macro but rpmsg_ns_msg has presence >>>>>>> on both kernel and firmware-sides. >>>>>> >>>>>> Ah yes yes... The amount of bytes coming out of the pipe won't match. >>>>>> Let me think a little... >>>>> >>>>> +1 for Suman's concern. >>>>> >>>>> Anyway i would like to challenge the need of more than 32 bytes to >>>>> differentiate service instances. >>>>> "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", seems to me enough if we only need >>>>> to differentiate the instances. >>> >>> Remember that the rpmsg_device_id name takes some space within here. So, >>> the shorter the rpmsg_device_id table name, the more room you have. >>> >>>>> >>>>> But perhaps the need is also to provide a short description of the service? >>> >>> I am mostly using it to provide a unique instantiation name. In anycase, >>> I have cross-checked against my current firmwares, and so far all of >>> them happen to have the name + desc < 31 bytes. >>> >>> >>>>> >>>>> Suman, could you share some examples of your need? >>>> >>>> Looking at things further it is possible to extend the name of the service to >>>> 64 byte while keeping backward compatibility by looking up the size of @len >>>> in function rpmsg_ns_cb(). From there work with an rpmsg_ns_msg or a new >>>> rpmsg_ns_msg64, pretty much the way you did in your patch[1]. In fact the >>>> approach is the same except you are using 2 arrays of 32 byte and I'm using one >>>> of 64. >>>> >>>> As Arnaud mentioned, is there an immediate need to support a 64-byte name? If >>>> not than I suggest to move forward with this patch and address the issue when we >>>> get there - at least we know there is room for extention. Otherwise I'll spin >>>> off another revision but it will be bigger and more complex. >>> >>> Yeah ok. I have managed to get my downstream drivers that use the desc >>> field working with this patch after modifying the firmwares to publish >>> using combined name, and adding logic in probe to get the trailing >>> portion of the name. >> >> Perfect >> >>> >>> So, the only thing that is missing or content for another patch is if we >>> need to add some tooling/helper stuff for giving the trailing stuff to >>> rpmsg drivers? >> >> So that all rpmsg drivers don't come up with their own parsing that >> ends up doing the same thing. Let me think about that - I may have to >> get back to you... > > Yep. Sure no problem. It can be a patch on top of this as well. > > Arnaud, > Do you have immediate need for the tooling stuff for the rpmsg-tty driver? Before moving forward on rpmsg_tty i would prefer that we are aligned with Bjorn on the implementation of the rpmsg_tty itself and the evolution of the service name... Then rpmsg_tty could be a good threadfor a first implementation... Concerning the name service, having a discussion around the name service skeleton would be nice. This could be an good input for the helpers function definition. Propositions in rpmsg_tty thread are <service_name>-<feature> or <service_name>-<feature>-<sub_service> don't hesitate to comment and/or propose alternatives Regards, Arnaud > > regards > Suman > > >> >>> >>> regards >>> Suman >>> >>>> >>>> Thanks, >>>> Mathieu >>>> >>>> [1]. https://patchwork.kernel.org/patch/11096599/ >>>> >>>>>>>>> >>>>>>>>>> --- >>>>>>>>>> Changes for V2: >>>>>>>>>> - Added Arnaud's Acked-by. >>>>>>>>>> - Rebased to latest rproc-next. >>>>>>>>>> >>>>>>>>>> drivers/rpmsg/rpmsg_core.c | 20 +++++++++++++++++++- >>>>>>>>>> 1 file changed, 19 insertions(+), 1 deletion(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c >>>>>>>>>> index e330ec4dfc33..bfd25978fa35 100644 >>>>>>>>>> --- a/drivers/rpmsg/rpmsg_core.c >>>>>>>>>> +++ b/drivers/rpmsg/rpmsg_core.c >>>>>>>>>> @@ -399,7 +399,25 @@ ATTRIBUTE_GROUPS(rpmsg_dev); >>>>>>>>>> static inline int rpmsg_id_match(const struct rpmsg_device *rpdev, >>>>>>>>>> const struct rpmsg_device_id *id) >>>>>>>>>> { >>>>>>>>>> - return strncmp(id->name, rpdev->id.name, RPMSG_NAME_SIZE) == 0; >>>>>>>>>> + size_t len = min_t(size_t, strlen(id->name), RPMSG_NAME_SIZE); >>>>>>>>>> + >>>>>>>>>> + /* >>>>>>>>>> + * Allow for wildcard matches. For example if rpmsg_driver::id_table >>>>>>>>>> + * is: >>>>>>>>>> + * >>>>>>>>>> + * static struct rpmsg_device_id rpmsg_driver_sample_id_table[] = { >>>>>>>>>> + * { .name = "rpmsg-client-sample" }, >>>>>>>>>> + * { }, >>>>>>>>>> + * } >>>>>>>>>> + * >>>>>>>>>> + * Then it is possible to support "rpmsg-client-sample*", i.e: >>>>>>>>>> + * rpmsg-client-sample >>>>>>>>>> + * rpmsg-client-sample_instance0 >>>>>>>>>> + * rpmsg-client-sample_instance1 >>>>>>>>>> + * ... >>>>>>>>>> + * rpmsg-client-sample_instanceX >>>>>>>>>> + */ >>>>>>>>>> + return strncmp(id->name, rpdev->id.name, len) == 0; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> /* match rpmsg channel and rpmsg driver */ >>>>>>>>>> >>>>>>>>> >>>>>>> >>> > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-04-09 8:54 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-10 15:50 [PATCH v2] rpmsg: core: Add wildcard match for name service Mathieu Poirier 2020-03-26 15:06 ` Suman Anna 2020-03-26 20:21 ` Mathieu Poirier 2020-03-26 20:42 ` Suman Anna 2020-03-26 22:01 ` Mathieu Poirier 2020-03-27 9:35 ` Arnaud POULIQUEN 2020-03-27 19:36 ` Mathieu Poirier 2020-04-07 23:07 ` Suman Anna 2020-04-08 15:59 ` Mathieu Poirier 2020-04-08 20:52 ` Suman Anna 2020-04-09 8:54 ` Arnaud POULIQUEN
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).