* Re: [PATCH RESEND] rpmsg: Add driver_override device attribute for rpmsg_device
2018-01-10 13:17 [PATCH RESEND] rpmsg: Add driver_override device attribute for rpmsg_device Anup Patel
@ 2018-01-23 13:46 ` Anup Patel
2018-02-06 4:44 ` Anup Patel
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Anup Patel @ 2018-01-23 13:46 UTC (permalink / raw)
To: Ohad Ben-Cohen, Bjorn Andersson
Cc: linux-remoteproc, linux-kernel@vger.kernel.org List, Anup Patel
On Wed, Jan 10, 2018 at 6:47 PM, Anup Patel <anup@brainfault.org> wrote:
> This patch adds "driver_override" device attribute for rpmsg_device which
> will allow users to explicitly specify the rpmsg_driver to be used via
> sysfs entry.
>
> The "driver_override" device attribute implemented here is very similar
> to "driver_override" implemented for platform, pci, and amba bus types.
>
> One important use-case of "driver_override" device attribute is to force
> use of rpmsg_chrdev driver for certain rpmsg_device instances.
>
> Signed-off-by: Anup Patel <anup@brainfault.org>
> ---
> Documentation/ABI/testing/sysfs-bus-rpmsg | 20 ++++++++++++++
> drivers/rpmsg/rpmsg_core.c | 46 +++++++++++++++++++++++++++++--
> 2 files changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-rpmsg b/Documentation/ABI/testing/sysfs-bus-rpmsg
> index 189e419..1c8599e 100644
> --- a/Documentation/ABI/testing/sysfs-bus-rpmsg
> +++ b/Documentation/ABI/testing/sysfs-bus-rpmsg
> @@ -73,3 +73,23 @@ Description:
> This sysfs entry tells us whether the channel is a local
> server channel that is announced (values are either
> true or false).
> +
> +What: /sys/bus/rpmsg/devices/.../driver_override
> +Date: January 2018
> +KernelVersion: 4.16
> +Contact: Ohad Ben-Cohen <ohad@wizery.com>
> +Description:
> + Every rpmsg device is a communication channel with a remote
> + processor. Channels are identified by a textual name (see
> + /sys/bus/rpmsg/devices/.../name above) and have a local
> + ("source") rpmsg address, and remote ("destination") rpmsg
> + address.
> +
> + The listening entity (or client) which communicates with a
> + remote processor is referred as rpmsg driver. The rpmsg device
> + and rpmsg driver are matched based on rpmsg device name and
> + rpmsg driver ID table.
> +
> + This sysfs entry allows the rpmsg driver for a rpmsg device
> + to be specified which will override standard OF, ID table
> + and name matching.
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index dffa3aa..9a25e42 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -321,11 +321,11 @@ struct device *rpmsg_find_device(struct device *parent,
> }
> EXPORT_SYMBOL(rpmsg_find_device);
>
> -/* sysfs show configuration fields */
> +/* sysfs configuration fields */
> #define rpmsg_show_attr(field, path, format_string) \
> static ssize_t \
> field##_show(struct device *dev, \
> - struct device_attribute *attr, char *buf) \
> + struct device_attribute *attr, char *buf) \
> { \
> struct rpmsg_device *rpdev = to_rpmsg_device(dev); \
> \
> @@ -333,11 +333,52 @@ field##_show(struct device *dev, \
> } \
> static DEVICE_ATTR_RO(field);
>
> +#define rpmsg_string_attr(field, path) \
> +static ssize_t \
> +field##_store(struct device *dev, \
> + struct device_attribute *attr, const char *buf, size_t sz)\
> +{ \
> + struct rpmsg_device *rpdev = to_rpmsg_device(dev); \
> + char *new, *old, *cp; \
> + \
> + new = kstrndup(buf, sz, GFP_KERNEL); \
> + if (!new) \
> + return -ENOMEM; \
> + \
> + cp = strchr(new, '\n'); \
> + if (cp) \
> + *cp = '\0'; \
> + \
> + device_lock(dev); \
> + old = rpdev->path; \
> + if (strlen(new)) { \
> + rpdev->path = new; \
> + } else { \
> + kfree(new); \
> + rpdev->path = NULL; \
> + } \
> + device_unlock(dev); \
> + \
> + kfree(old); \
> + \
> + return sz; \
> +} \
> +static ssize_t \
> +field##_show(struct device *dev, \
> + struct device_attribute *attr, char *buf) \
> +{ \
> + struct rpmsg_device *rpdev = to_rpmsg_device(dev); \
> + \
> + return sprintf(buf, "%s\n", rpdev->path); \
> +} \
> +static DEVICE_ATTR_RW(field)
> +
> /* for more info, see Documentation/ABI/testing/sysfs-bus-rpmsg */
> rpmsg_show_attr(name, id.name, "%s\n");
> rpmsg_show_attr(src, src, "0x%x\n");
> rpmsg_show_attr(dst, dst, "0x%x\n");
> rpmsg_show_attr(announce, announce ? "true" : "false", "%s\n");
> +rpmsg_string_attr(driver_override, driver_override);
>
> static ssize_t modalias_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> @@ -359,6 +400,7 @@ static struct attribute *rpmsg_dev_attrs[] = {
> &dev_attr_dst.attr,
> &dev_attr_src.attr,
> &dev_attr_announce.attr,
> + &dev_attr_driver_override.attr,
> NULL,
> };
> ATTRIBUTE_GROUPS(rpmsg_dev);
> --
> 2.7.4
>
Hi All,
Any comments on this??
Regards,
Anup
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RESEND] rpmsg: Add driver_override device attribute for rpmsg_device
2018-01-10 13:17 [PATCH RESEND] rpmsg: Add driver_override device attribute for rpmsg_device Anup Patel
2018-01-23 13:46 ` Anup Patel
@ 2018-02-06 4:44 ` Anup Patel
2018-02-21 11:40 ` Anup Patel
2018-03-18 22:47 ` Bjorn Andersson
3 siblings, 0 replies; 6+ messages in thread
From: Anup Patel @ 2018-02-06 4:44 UTC (permalink / raw)
To: Ohad Ben-Cohen, Bjorn Andersson
Cc: linux-remoteproc, linux-kernel@vger.kernel.org List, Anup Patel
Hi Bjorn,
Can you please have a look at this patch?
Regards,
Anup
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND] rpmsg: Add driver_override device attribute for rpmsg_device
2018-01-10 13:17 [PATCH RESEND] rpmsg: Add driver_override device attribute for rpmsg_device Anup Patel
2018-01-23 13:46 ` Anup Patel
2018-02-06 4:44 ` Anup Patel
@ 2018-02-21 11:40 ` Anup Patel
2018-03-18 22:47 ` Bjorn Andersson
3 siblings, 0 replies; 6+ messages in thread
From: Anup Patel @ 2018-02-21 11:40 UTC (permalink / raw)
To: Ohad Ben-Cohen, Bjorn Andersson
Cc: linux-remoteproc, linux-kernel@vger.kernel.org List, Anup Patel
On Wed, Jan 10, 2018 at 6:47 PM, Anup Patel <anup@brainfault.org> wrote:
> This patch adds "driver_override" device attribute for rpmsg_device which
> will allow users to explicitly specify the rpmsg_driver to be used via
> sysfs entry.
>
> The "driver_override" device attribute implemented here is very similar
> to "driver_override" implemented for platform, pci, and amba bus types.
>
> One important use-case of "driver_override" device attribute is to force
> use of rpmsg_chrdev driver for certain rpmsg_device instances.
>
> Signed-off-by: Anup Patel <anup@brainfault.org>
> ---
> Documentation/ABI/testing/sysfs-bus-rpmsg | 20 ++++++++++++++
> drivers/rpmsg/rpmsg_core.c | 46 +++++++++++++++++++++++++++++--
> 2 files changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-rpmsg b/Documentation/ABI/testing/sysfs-bus-rpmsg
> index 189e419..1c8599e 100644
> --- a/Documentation/ABI/testing/sysfs-bus-rpmsg
> +++ b/Documentation/ABI/testing/sysfs-bus-rpmsg
> @@ -73,3 +73,23 @@ Description:
> This sysfs entry tells us whether the channel is a local
> server channel that is announced (values are either
> true or false).
> +
> +What: /sys/bus/rpmsg/devices/.../driver_override
> +Date: January 2018
> +KernelVersion: 4.16
> +Contact: Ohad Ben-Cohen <ohad@wizery.com>
> +Description:
> + Every rpmsg device is a communication channel with a remote
> + processor. Channels are identified by a textual name (see
> + /sys/bus/rpmsg/devices/.../name above) and have a local
> + ("source") rpmsg address, and remote ("destination") rpmsg
> + address.
> +
> + The listening entity (or client) which communicates with a
> + remote processor is referred as rpmsg driver. The rpmsg device
> + and rpmsg driver are matched based on rpmsg device name and
> + rpmsg driver ID table.
> +
> + This sysfs entry allows the rpmsg driver for a rpmsg device
> + to be specified which will override standard OF, ID table
> + and name matching.
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index dffa3aa..9a25e42 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -321,11 +321,11 @@ struct device *rpmsg_find_device(struct device *parent,
> }
> EXPORT_SYMBOL(rpmsg_find_device);
>
> -/* sysfs show configuration fields */
> +/* sysfs configuration fields */
> #define rpmsg_show_attr(field, path, format_string) \
> static ssize_t \
> field##_show(struct device *dev, \
> - struct device_attribute *attr, char *buf) \
> + struct device_attribute *attr, char *buf) \
> { \
> struct rpmsg_device *rpdev = to_rpmsg_device(dev); \
> \
> @@ -333,11 +333,52 @@ field##_show(struct device *dev, \
> } \
> static DEVICE_ATTR_RO(field);
>
> +#define rpmsg_string_attr(field, path) \
> +static ssize_t \
> +field##_store(struct device *dev, \
> + struct device_attribute *attr, const char *buf, size_t sz)\
> +{ \
> + struct rpmsg_device *rpdev = to_rpmsg_device(dev); \
> + char *new, *old, *cp; \
> + \
> + new = kstrndup(buf, sz, GFP_KERNEL); \
> + if (!new) \
> + return -ENOMEM; \
> + \
> + cp = strchr(new, '\n'); \
> + if (cp) \
> + *cp = '\0'; \
> + \
> + device_lock(dev); \
> + old = rpdev->path; \
> + if (strlen(new)) { \
> + rpdev->path = new; \
> + } else { \
> + kfree(new); \
> + rpdev->path = NULL; \
> + } \
> + device_unlock(dev); \
> + \
> + kfree(old); \
> + \
> + return sz; \
> +} \
> +static ssize_t \
> +field##_show(struct device *dev, \
> + struct device_attribute *attr, char *buf) \
> +{ \
> + struct rpmsg_device *rpdev = to_rpmsg_device(dev); \
> + \
> + return sprintf(buf, "%s\n", rpdev->path); \
> +} \
> +static DEVICE_ATTR_RW(field)
> +
> /* for more info, see Documentation/ABI/testing/sysfs-bus-rpmsg */
> rpmsg_show_attr(name, id.name, "%s\n");
> rpmsg_show_attr(src, src, "0x%x\n");
> rpmsg_show_attr(dst, dst, "0x%x\n");
> rpmsg_show_attr(announce, announce ? "true" : "false", "%s\n");
> +rpmsg_string_attr(driver_override, driver_override);
>
> static ssize_t modalias_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> @@ -359,6 +400,7 @@ static struct attribute *rpmsg_dev_attrs[] = {
> &dev_attr_dst.attr,
> &dev_attr_src.attr,
> &dev_attr_announce.attr,
> + &dev_attr_driver_override.attr,
> NULL,
> };
> ATTRIBUTE_GROUPS(rpmsg_dev);
> --
> 2.7.4
>
Hi Bjorn,
Can you please review this patch?
Regards,
Anup
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RESEND] rpmsg: Add driver_override device attribute for rpmsg_device
2018-01-10 13:17 [PATCH RESEND] rpmsg: Add driver_override device attribute for rpmsg_device Anup Patel
` (2 preceding siblings ...)
2018-02-21 11:40 ` Anup Patel
@ 2018-03-18 22:47 ` Bjorn Andersson
2018-03-21 7:59 ` Anup Patel
3 siblings, 1 reply; 6+ messages in thread
From: Bjorn Andersson @ 2018-03-18 22:47 UTC (permalink / raw)
To: Anup Patel; +Cc: Ohad Ben-Cohen, linux-remoteproc, linux-kernel
On Wed 10 Jan 05:17 PST 2018, Anup Patel wrote:
> This patch adds "driver_override" device attribute for rpmsg_device which
> will allow users to explicitly specify the rpmsg_driver to be used via
> sysfs entry.
>
> The "driver_override" device attribute implemented here is very similar
> to "driver_override" implemented for platform, pci, and amba bus types.
>
> One important use-case of "driver_override" device attribute is to force
> use of rpmsg_chrdev driver for certain rpmsg_device instances.
>
I assume you mean specifically for the case where you want to prevent
some kernel driver to probe for some given channel?
The intention with rpmsg_char is that you through the rpmsg_ctrlX
interface create and destroy endpoints dynamically, so you wouldn't need
to use this mechanism to bind some specific channel to rpmsg_char.
That said, this does make sense for completeness sake.
[..]
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index dffa3aa..9a25e42 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -321,11 +321,11 @@ struct device *rpmsg_find_device(struct device *parent,
> }
> EXPORT_SYMBOL(rpmsg_find_device);
>
> -/* sysfs show configuration fields */
> +/* sysfs configuration fields */
> #define rpmsg_show_attr(field, path, format_string) \
> static ssize_t \
> field##_show(struct device *dev, \
> - struct device_attribute *attr, char *buf) \
> + struct device_attribute *attr, char *buf) \
Seems unnecessary.
> { \
> struct rpmsg_device *rpdev = to_rpmsg_device(dev); \
> \
> @@ -333,11 +333,52 @@ field##_show(struct device *dev, \
> } \
> static DEVICE_ATTR_RO(field);
>
> +#define rpmsg_string_attr(field, path) \
"path" is an odd name for these, I think it's a "member".
> +static ssize_t \
> +field##_store(struct device *dev, \
> + struct device_attribute *attr, const char *buf, size_t sz)\
field##_store(struct device *dev, struct device_attribute *attr, \
const char *buf, size_t sz) \
Is prettier
> +{ \
> + struct rpmsg_device *rpdev = to_rpmsg_device(dev); \
> + char *new, *old, *cp; \
> + \
> + new = kstrndup(buf, sz, GFP_KERNEL); \
> + if (!new) \
> + return -ENOMEM; \
> + \
> + cp = strchr(new, '\n'); \
> + if (cp) \
> + *cp = '\0'; \
I prefer
new[strcspn(new, "\n")] = '\0';
Regards,
Bjorn
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH RESEND] rpmsg: Add driver_override device attribute for rpmsg_device
2018-03-18 22:47 ` Bjorn Andersson
@ 2018-03-21 7:59 ` Anup Patel
0 siblings, 0 replies; 6+ messages in thread
From: Anup Patel @ 2018-03-21 7:59 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, linux-remoteproc, linux-kernel@vger.kernel.org List
On Mon, Mar 19, 2018 at 4:17 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Wed 10 Jan 05:17 PST 2018, Anup Patel wrote:
>
>> This patch adds "driver_override" device attribute for rpmsg_device which
>> will allow users to explicitly specify the rpmsg_driver to be used via
>> sysfs entry.
>>
>> The "driver_override" device attribute implemented here is very similar
>> to "driver_override" implemented for platform, pci, and amba bus types.
>>
>> One important use-case of "driver_override" device attribute is to force
>> use of rpmsg_chrdev driver for certain rpmsg_device instances.
>>
>
> I assume you mean specifically for the case where you want to prevent
> some kernel driver to probe for some given channel?
Yes, there are few use-cases where we want to prevent kernel
driver and rather access rpmsg device from user-space using
rpmsg_chrdev driver.
>
> The intention with rpmsg_char is that you through the rpmsg_ctrlX
> interface create and destroy endpoints dynamically, so you wouldn't need
> to use this mechanism to bind some specific channel to rpmsg_char.
>
>
> That said, this does make sense for completeness sake.
>
> [..]
>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>> index dffa3aa..9a25e42 100644
>> --- a/drivers/rpmsg/rpmsg_core.c
>> +++ b/drivers/rpmsg/rpmsg_core.c
>> @@ -321,11 +321,11 @@ struct device *rpmsg_find_device(struct device *parent,
>> }
>> EXPORT_SYMBOL(rpmsg_find_device);
>>
>> -/* sysfs show configuration fields */
>> +/* sysfs configuration fields */
>> #define rpmsg_show_attr(field, path, format_string) \
>> static ssize_t \
>> field##_show(struct device *dev, \
>> - struct device_attribute *attr, char *buf) \
>> + struct device_attribute *attr, char *buf) \
>
> Seems unnecessary.
OK, I will drop these changes.
>
>> { \
>> struct rpmsg_device *rpdev = to_rpmsg_device(dev); \
>> \
>> @@ -333,11 +333,52 @@ field##_show(struct device *dev, \
>> } \
>> static DEVICE_ATTR_RO(field);
>>
>> +#define rpmsg_string_attr(field, path) \
>
> "path" is an odd name for these, I think it's a "member".
>
>> +static ssize_t \
>> +field##_store(struct device *dev, \
>> + struct device_attribute *attr, const char *buf, size_t sz)\
>
> field##_store(struct device *dev, struct device_attribute *attr, \
> const char *buf, size_t sz) \
>
> Is prettier
OK, I will update this.
>
>> +{ \
>> + struct rpmsg_device *rpdev = to_rpmsg_device(dev); \
>> + char *new, *old, *cp; \
>> + \
>> + new = kstrndup(buf, sz, GFP_KERNEL); \
>> + if (!new) \
>> + return -ENOMEM; \
>> + \
>> + cp = strchr(new, '\n'); \
>> + if (cp) \
>> + *cp = '\0'; \
>
> I prefer
>
> new[strcspn(new, "\n")] = '\0';
Sure, I will update this.
Thanks,
Anup
^ permalink raw reply [flat|nested] 6+ messages in thread