[PATCH 0/1] rpmsg: core: Add wildcard match for name service

[PATCH 0/1] rpmsg: core: Add wildcard match for name service

[Mathieu Poirier]

This patch is the newest iteration of a feature that has been explored in
the past by Xiang[1] and Suman[2].  It was also deemed useful by Arnaud.

I have decided to use a new approach to avoid potential code duplication
that may happen with the introduction of an rpdev->match() and reduce the
footprint inherent from the addition of an additional description field.

I'm sending this out to restart the conversation on the best way to move
forward with this feature.

I have tested this on ST's mp157c-ev1 board - the kernel was enhanced but
MCU image remained unchanged, proving backward compatibility with the
current implementation.

Thanks,
Mathieu

Mathieu Poirier (1):
  rpmsg: core: Add wildcard match for name service

 drivers/rpmsg/rpmsg_core.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

-- 
2.20.1

[PATCH 1/1] rpmsg: core: Add wildcard match for name service

[Mathieu Poirier]

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>
---
 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

Re: [PATCH 1/1] rpmsg: core: Add wildcard match for name service

[Arnaud POULIQUEN]

Hi Mathieu,

Simple and elegant :)
I tested it with my rpmsg_tty client which defines several IDs: work fine.
  
Just a question regarding the comment else
Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
 

On 2/12/20 10:12 PM, 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>
> ---
>  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
> +	 */
What about adding this as function documentation? i don't know if it makes sense
for a static volatile function...

Regards
Arnaud

> +	return strncmp(id->name, rpdev->id.name, len) == 0;
>  }
>  
>  /* match rpmsg channel and rpmsg driver */
> 

Re: [PATCH 1/1] rpmsg: core: Add wildcard match for name service

[Mathieu Poirier]

On Thu, 13 Feb 2020 at 06:56, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>
> Hi Mathieu,
>
> Simple and elegant :)
> I tested it with my rpmsg_tty client which defines several IDs: work fine.

Perfect - many thanks for giving this a spin.

>
> Just a question regarding the comment else
> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>
>
> On 2/12/20 10:12 PM, 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>
> > ---
> >  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
> > +      */
> What about adding this as function documentation? i don't know if it makes sense
> for a static volatile function...

It didn't cross my mind because (and as you pointed out) it is a
static function.  Let me know if you're keen on seeing this corrected
and I'll go for a respin.

Mathieu

>
> Regards
> Arnaud
>
> > +     return strncmp(id->name, rpdev->id.name, len) == 0;
> >  }
> >
> >  /* match rpmsg channel and rpmsg driver */
> >