diff mbox series

[v5,1/2] remoteproc: Introduce sysfs_read_only flag

Message ID 20220216081224.9956-2-p-mohan@ti.com (mailing list archive)
State Accepted
Headers show
Series remoteproc sysfs fixes/improvements | expand

Commit Message

Puranjay Mohan Feb. 16, 2022, 8:12 a.m. UTC
The remoteproc framework provides sysfs interfaces for changing
the firmware name and for starting/stopping a remote processor
through the sysfs files 'state' and 'firmware'. The 'coredump'
file is used to set the coredump configuration. The 'recovery'
sysfs file can also be used similarly to control the error recovery
state machine of a remoteproc. These interfaces are currently
allowed irrespective of how the remoteprocs were booted (like
remoteproc self auto-boot, remoteproc client-driven boot etc).
These interfaces can adversely affect a remoteproc and its clients
especially when a remoteproc is being controlled by a remoteproc
client driver(s). Also, not all remoteproc drivers may want to
support the sysfs interfaces by default.

Add support to make the remoteproc sysfs files read only by
introducing a state flag 'sysfs_read_only' that the individual
remoteproc drivers can set based on their usage needs. The default
behavior is to allow the sysfs operations as before.

Implement attribute_group->is_visible() to make the sysfs
entries read only when 'sysfs_read_only' flag is set.

Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
Changes in v4->v5:
Rename deny_sysfs_ops to sysfs_read_only.
Make coredump readonly with other files.

Changes in v3->v4:
Use mode = 0444 in rproc_is_visible() to make the sysfs entries
read-only when the deny_sysfs_ops flag is set.
---
 drivers/remoteproc/remoteproc_sysfs.c | 19 ++++++++++++++++++-
 include/linux/remoteproc.h            |  2 ++
 2 files changed, 20 insertions(+), 1 deletion(-)

Comments

Bjorn Andersson Feb. 16, 2022, 8:04 p.m. UTC | #1
On Wed 16 Feb 02:12 CST 2022, Puranjay Mohan wrote:

> The remoteproc framework provides sysfs interfaces for changing
> the firmware name and for starting/stopping a remote processor
> through the sysfs files 'state' and 'firmware'. The 'coredump'
> file is used to set the coredump configuration. The 'recovery'
> sysfs file can also be used similarly to control the error recovery
> state machine of a remoteproc. These interfaces are currently
> allowed irrespective of how the remoteprocs were booted (like
> remoteproc self auto-boot, remoteproc client-driven boot etc).
> These interfaces can adversely affect a remoteproc and its clients
> especially when a remoteproc is being controlled by a remoteproc
> client driver(s). Also, not all remoteproc drivers may want to
> support the sysfs interfaces by default.
> 
> Add support to make the remoteproc sysfs files read only by
> introducing a state flag 'sysfs_read_only' that the individual
> remoteproc drivers can set based on their usage needs. The default
> behavior is to allow the sysfs operations as before.
> 
> Implement attribute_group->is_visible() to make the sysfs
> entries read only when 'sysfs_read_only' flag is set.
> 
> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
> Changes in v4->v5:
> Rename deny_sysfs_ops to sysfs_read_only.
> Make coredump readonly with other files.
> 
> Changes in v3->v4:
> Use mode = 0444 in rproc_is_visible() to make the sysfs entries
> read-only when the deny_sysfs_ops flag is set.
> ---
>  drivers/remoteproc/remoteproc_sysfs.c | 19 ++++++++++++++++++-
>  include/linux/remoteproc.h            |  2 ++
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index ea8b89f97d7b..abf0cd05d5e1 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -230,6 +230,22 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(name);
>  
> +static umode_t rproc_is_visible(struct kobject *kobj, struct attribute *attr,
> +				int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct rproc *rproc = to_rproc(dev);
> +	umode_t mode = attr->mode;
> +
> +	if (rproc->sysfs_read_only && (attr == &dev_attr_recovery.attr ||
> +				       attr == &dev_attr_firmware.attr ||
> +				       attr == &dev_attr_state.attr ||
> +				       attr == &dev_attr_coredump.attr))
> +		mode = 0444;

Change looks good now, but just to make sure, you actually care about
the content of these files for the Wakup M3? Read-only vs hiding them...

regards,
Bjorn

> +
> +	return mode;
> +}
> +
>  static struct attribute *rproc_attrs[] = {
>  	&dev_attr_coredump.attr,
>  	&dev_attr_recovery.attr,
> @@ -240,7 +256,8 @@ static struct attribute *rproc_attrs[] = {
>  };
>  
>  static const struct attribute_group rproc_devgroup = {
> -	.attrs = rproc_attrs
> +	.attrs = rproc_attrs,
> +	.is_visible = rproc_is_visible,
>  };
>  
>  static const struct attribute_group *rproc_devgroups[] = {
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e0600e1e5c17..93a1d0050fbc 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -523,6 +523,7 @@ struct rproc_dump_segment {
>   * @table_sz: size of @cached_table
>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>   * @auto_boot: flag to indicate if remote processor should be auto-started
> + * @sysfs_read_only: flag to make remoteproc sysfs files read only
>   * @dump_segments: list of segments in the firmware
>   * @nb_vdev: number of vdev currently handled by rproc
>   * @elf_class: firmware ELF class
> @@ -562,6 +563,7 @@ struct rproc {
>  	size_t table_sz;
>  	bool has_iommu;
>  	bool auto_boot;
> +	bool sysfs_read_only;
>  	struct list_head dump_segments;
>  	int nb_vdev;
>  	u8 elf_class;
> -- 
> 2.17.1
>
Puranjay Mohan Feb. 17, 2022, 6:14 a.m. UTC | #2
Hi Bjorn,

On 17/02/22 01:34, Bjorn Andersson wrote:
> On Wed 16 Feb 02:12 CST 2022, Puranjay Mohan wrote:
> 
>> The remoteproc framework provides sysfs interfaces for changing
>> the firmware name and for starting/stopping a remote processor
>> through the sysfs files 'state' and 'firmware'. The 'coredump'
>> file is used to set the coredump configuration. The 'recovery'
>> sysfs file can also be used similarly to control the error recovery
>> state machine of a remoteproc. These interfaces are currently
>> allowed irrespective of how the remoteprocs were booted (like
>> remoteproc self auto-boot, remoteproc client-driven boot etc).
>> These interfaces can adversely affect a remoteproc and its clients
>> especially when a remoteproc is being controlled by a remoteproc
>> client driver(s). Also, not all remoteproc drivers may want to
>> support the sysfs interfaces by default.
>>
>> Add support to make the remoteproc sysfs files read only by
>> introducing a state flag 'sysfs_read_only' that the individual
>> remoteproc drivers can set based on their usage needs. The default
>> behavior is to allow the sysfs operations as before.
>>
>> Implement attribute_group->is_visible() to make the sysfs
>> entries read only when 'sysfs_read_only' flag is set.
>>
>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>> ---
>> Changes in v4->v5:
>> Rename deny_sysfs_ops to sysfs_read_only.
>> Make coredump readonly with other files.
>>
>> Changes in v3->v4:
>> Use mode = 0444 in rproc_is_visible() to make the sysfs entries
>> read-only when the deny_sysfs_ops flag is set.
>> ---
>>  drivers/remoteproc/remoteproc_sysfs.c | 19 ++++++++++++++++++-
>>  include/linux/remoteproc.h            |  2 ++
>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
>> index ea8b89f97d7b..abf0cd05d5e1 100644
>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>> @@ -230,6 +230,22 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
>>  }
>>  static DEVICE_ATTR_RO(name);
>>  
>> +static umode_t rproc_is_visible(struct kobject *kobj, struct attribute *attr,
>> +				int n)
>> +{
>> +	struct device *dev = kobj_to_dev(kobj);
>> +	struct rproc *rproc = to_rproc(dev);
>> +	umode_t mode = attr->mode;
>> +
>> +	if (rproc->sysfs_read_only && (attr == &dev_attr_recovery.attr ||
>> +				       attr == &dev_attr_firmware.attr ||
>> +				       attr == &dev_attr_state.attr ||
>> +				       attr == &dev_attr_coredump.attr))
>> +		mode = 0444;
> 
> Change looks good now, but just to make sure, you actually care about
> the content of these files for the Wakup M3? Read-only vs hiding them...

Yes, it is useful for debugging purposes, hence making it read-only is
the correct approach.

Also, I will be re-sending PRU remoteproc consumer API patch series[1]
that depends on this change.

[1]https://patchwork.kernel.org/project/linux-remoteproc/cover/20201216165239.2744-1-grzegorz.jaszczyk@linaro.org/

Thanks,
Puranjay Mohan

> 
> regards,
> Bjorn
> 
>> +
>> +	return mode;
>> +}
>> +
>>  static struct attribute *rproc_attrs[] = {
>>  	&dev_attr_coredump.attr,
>>  	&dev_attr_recovery.attr,
>> @@ -240,7 +256,8 @@ static struct attribute *rproc_attrs[] = {
>>  };
>>  
>>  static const struct attribute_group rproc_devgroup = {
>> -	.attrs = rproc_attrs
>> +	.attrs = rproc_attrs,
>> +	.is_visible = rproc_is_visible,
>>  };
>>  
>>  static const struct attribute_group *rproc_devgroups[] = {
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index e0600e1e5c17..93a1d0050fbc 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -523,6 +523,7 @@ struct rproc_dump_segment {
>>   * @table_sz: size of @cached_table
>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>>   * @auto_boot: flag to indicate if remote processor should be auto-started
>> + * @sysfs_read_only: flag to make remoteproc sysfs files read only
>>   * @dump_segments: list of segments in the firmware
>>   * @nb_vdev: number of vdev currently handled by rproc
>>   * @elf_class: firmware ELF class
>> @@ -562,6 +563,7 @@ struct rproc {
>>  	size_t table_sz;
>>  	bool has_iommu;
>>  	bool auto_boot;
>> +	bool sysfs_read_only;
>>  	struct list_head dump_segments;
>>  	int nb_vdev;
>>  	u8 elf_class;
>> -- 
>> 2.17.1
>>
Kishon Vijay Abraham I Feb. 18, 2022, 5 a.m. UTC | #3
On 16/02/22 1:42 pm, Puranjay Mohan wrote:
> The remoteproc framework provides sysfs interfaces for changing
> the firmware name and for starting/stopping a remote processor
> through the sysfs files 'state' and 'firmware'. The 'coredump'
> file is used to set the coredump configuration. The 'recovery'
> sysfs file can also be used similarly to control the error recovery
> state machine of a remoteproc. These interfaces are currently
> allowed irrespective of how the remoteprocs were booted (like
> remoteproc self auto-boot, remoteproc client-driven boot etc).
> These interfaces can adversely affect a remoteproc and its clients
> especially when a remoteproc is being controlled by a remoteproc
> client driver(s). Also, not all remoteproc drivers may want to
> support the sysfs interfaces by default.
> 
> Add support to make the remoteproc sysfs files read only by
> introducing a state flag 'sysfs_read_only' that the individual
> remoteproc drivers can set based on their usage needs. The default
> behavior is to allow the sysfs operations as before.
> 
> Implement attribute_group->is_visible() to make the sysfs
> entries read only when 'sysfs_read_only' flag is set.
> 
> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
> Changes in v4->v5:
> Rename deny_sysfs_ops to sysfs_read_only.
> Make coredump readonly with other files.
> 
> Changes in v3->v4:
> Use mode = 0444 in rproc_is_visible() to make the sysfs entries
> read-only when the deny_sysfs_ops flag is set.
> ---
>  drivers/remoteproc/remoteproc_sysfs.c | 19 ++++++++++++++++++-
>  include/linux/remoteproc.h            |  2 ++
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index ea8b89f97d7b..abf0cd05d5e1 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -230,6 +230,22 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(name);
>  
> +static umode_t rproc_is_visible(struct kobject *kobj, struct attribute *attr,
> +				int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct rproc *rproc = to_rproc(dev);
> +	umode_t mode = attr->mode;
> +
> +	if (rproc->sysfs_read_only && (attr == &dev_attr_recovery.attr ||
> +				       attr == &dev_attr_firmware.attr ||
> +				       attr == &dev_attr_state.attr ||
> +				       attr == &dev_attr_coredump.attr))
> +		mode = 0444;

Nitpick: use S_IRUGO instead of 0444.

Thanks,
Kishon
> +
> +	return mode;
> +}
> +
>  static struct attribute *rproc_attrs[] = {
>  	&dev_attr_coredump.attr,
>  	&dev_attr_recovery.attr,
> @@ -240,7 +256,8 @@ static struct attribute *rproc_attrs[] = {
>  };
>  
>  static const struct attribute_group rproc_devgroup = {
> -	.attrs = rproc_attrs
> +	.attrs = rproc_attrs,
> +	.is_visible = rproc_is_visible,
>  };
>  
>  static const struct attribute_group *rproc_devgroups[] = {
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e0600e1e5c17..93a1d0050fbc 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -523,6 +523,7 @@ struct rproc_dump_segment {
>   * @table_sz: size of @cached_table
>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>   * @auto_boot: flag to indicate if remote processor should be auto-started
> + * @sysfs_read_only: flag to make remoteproc sysfs files read only
>   * @dump_segments: list of segments in the firmware
>   * @nb_vdev: number of vdev currently handled by rproc
>   * @elf_class: firmware ELF class
> @@ -562,6 +563,7 @@ struct rproc {
>  	size_t table_sz;
>  	bool has_iommu;
>  	bool auto_boot;
> +	bool sysfs_read_only;
>  	struct list_head dump_segments;
>  	int nb_vdev;
>  	u8 elf_class;
>
Bjorn Andersson Feb. 18, 2022, 5:54 a.m. UTC | #4
On Thu 17 Feb 21:00 PST 2022, Kishon Vijay Abraham I wrote:

> 
> 
> On 16/02/22 1:42 pm, Puranjay Mohan wrote:
> > The remoteproc framework provides sysfs interfaces for changing
> > the firmware name and for starting/stopping a remote processor
> > through the sysfs files 'state' and 'firmware'. The 'coredump'
> > file is used to set the coredump configuration. The 'recovery'
> > sysfs file can also be used similarly to control the error recovery
> > state machine of a remoteproc. These interfaces are currently
> > allowed irrespective of how the remoteprocs were booted (like
> > remoteproc self auto-boot, remoteproc client-driven boot etc).
> > These interfaces can adversely affect a remoteproc and its clients
> > especially when a remoteproc is being controlled by a remoteproc
> > client driver(s). Also, not all remoteproc drivers may want to
> > support the sysfs interfaces by default.
> > 
> > Add support to make the remoteproc sysfs files read only by
> > introducing a state flag 'sysfs_read_only' that the individual
> > remoteproc drivers can set based on their usage needs. The default
> > behavior is to allow the sysfs operations as before.
> > 
> > Implement attribute_group->is_visible() to make the sysfs
> > entries read only when 'sysfs_read_only' flag is set.
> > 
> > Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> > Changes in v4->v5:
> > Rename deny_sysfs_ops to sysfs_read_only.
> > Make coredump readonly with other files.
> > 
> > Changes in v3->v4:
> > Use mode = 0444 in rproc_is_visible() to make the sysfs entries
> > read-only when the deny_sysfs_ops flag is set.
> > ---
> >  drivers/remoteproc/remoteproc_sysfs.c | 19 ++++++++++++++++++-
> >  include/linux/remoteproc.h            |  2 ++
> >  2 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> > index ea8b89f97d7b..abf0cd05d5e1 100644
> > --- a/drivers/remoteproc/remoteproc_sysfs.c
> > +++ b/drivers/remoteproc/remoteproc_sysfs.c
> > @@ -230,6 +230,22 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RO(name);
> >  
> > +static umode_t rproc_is_visible(struct kobject *kobj, struct attribute *attr,
> > +				int n)
> > +{
> > +	struct device *dev = kobj_to_dev(kobj);
> > +	struct rproc *rproc = to_rproc(dev);
> > +	umode_t mode = attr->mode;
> > +
> > +	if (rproc->sysfs_read_only && (attr == &dev_attr_recovery.attr ||
> > +				       attr == &dev_attr_firmware.attr ||
> > +				       attr == &dev_attr_state.attr ||
> > +				       attr == &dev_attr_coredump.attr))
> > +		mode = 0444;
> 
> Nitpick: use S_IRUGO instead of 0444.
> 

Thanks for the suggestion Kishon, but I like 0444, it has direct meaning
to me.

So unless there's some directive to use S_I*** throughout the kernel I
would prefer this.

Regards,
Bjorn

> Thanks,
> Kishon
> > +
> > +	return mode;
> > +}
> > +
> >  static struct attribute *rproc_attrs[] = {
> >  	&dev_attr_coredump.attr,
> >  	&dev_attr_recovery.attr,
> > @@ -240,7 +256,8 @@ static struct attribute *rproc_attrs[] = {
> >  };
> >  
> >  static const struct attribute_group rproc_devgroup = {
> > -	.attrs = rproc_attrs
> > +	.attrs = rproc_attrs,
> > +	.is_visible = rproc_is_visible,
> >  };
> >  
> >  static const struct attribute_group *rproc_devgroups[] = {
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index e0600e1e5c17..93a1d0050fbc 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -523,6 +523,7 @@ struct rproc_dump_segment {
> >   * @table_sz: size of @cached_table
> >   * @has_iommu: flag to indicate if remote processor is behind an MMU
> >   * @auto_boot: flag to indicate if remote processor should be auto-started
> > + * @sysfs_read_only: flag to make remoteproc sysfs files read only
> >   * @dump_segments: list of segments in the firmware
> >   * @nb_vdev: number of vdev currently handled by rproc
> >   * @elf_class: firmware ELF class
> > @@ -562,6 +563,7 @@ struct rproc {
> >  	size_t table_sz;
> >  	bool has_iommu;
> >  	bool auto_boot;
> > +	bool sysfs_read_only;
> >  	struct list_head dump_segments;
> >  	int nb_vdev;
> >  	u8 elf_class;
> >
Puranjay Mohan March 3, 2022, 8 a.m. UTC | #5
Hi Bjorn,
Hi Mathieu,

When is this series expected to be applied?

I am going to post another series titled "PRU Consumer API".
One patch from that series depends on this "Introduce sysfs_read_only
flag" patch.

Please let me know so I can rebase and post that series.

Thanks,
Puranjay Mohan

On 18/02/22 11:24, Bjorn Andersson wrote:
> On Thu 17 Feb 21:00 PST 2022, Kishon Vijay Abraham I wrote:
> 
>>
>>
>> On 16/02/22 1:42 pm, Puranjay Mohan wrote:
>>> The remoteproc framework provides sysfs interfaces for changing
>>> the firmware name and for starting/stopping a remote processor
>>> through the sysfs files 'state' and 'firmware'. The 'coredump'
>>> file is used to set the coredump configuration. The 'recovery'
>>> sysfs file can also be used similarly to control the error recovery
>>> state machine of a remoteproc. These interfaces are currently
>>> allowed irrespective of how the remoteprocs were booted (like
>>> remoteproc self auto-boot, remoteproc client-driven boot etc).
>>> These interfaces can adversely affect a remoteproc and its clients
>>> especially when a remoteproc is being controlled by a remoteproc
>>> client driver(s). Also, not all remoteproc drivers may want to
>>> support the sysfs interfaces by default.
>>>
>>> Add support to make the remoteproc sysfs files read only by
>>> introducing a state flag 'sysfs_read_only' that the individual
>>> remoteproc drivers can set based on their usage needs. The default
>>> behavior is to allow the sysfs operations as before.
>>>
>>> Implement attribute_group->is_visible() to make the sysfs
>>> entries read only when 'sysfs_read_only' flag is set.
>>>
>>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> ---
>>> Changes in v4->v5:
>>> Rename deny_sysfs_ops to sysfs_read_only.
>>> Make coredump readonly with other files.
>>>
>>> Changes in v3->v4:
>>> Use mode = 0444 in rproc_is_visible() to make the sysfs entries
>>> read-only when the deny_sysfs_ops flag is set.
>>> ---
>>>  drivers/remoteproc/remoteproc_sysfs.c | 19 ++++++++++++++++++-
>>>  include/linux/remoteproc.h            |  2 ++
>>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
>>> index ea8b89f97d7b..abf0cd05d5e1 100644
>>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>>> @@ -230,6 +230,22 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
>>>  }
>>>  static DEVICE_ATTR_RO(name);
>>>  
>>> +static umode_t rproc_is_visible(struct kobject *kobj, struct attribute *attr,
>>> +				int n)
>>> +{
>>> +	struct device *dev = kobj_to_dev(kobj);
>>> +	struct rproc *rproc = to_rproc(dev);
>>> +	umode_t mode = attr->mode;
>>> +
>>> +	if (rproc->sysfs_read_only && (attr == &dev_attr_recovery.attr ||
>>> +				       attr == &dev_attr_firmware.attr ||
>>> +				       attr == &dev_attr_state.attr ||
>>> +				       attr == &dev_attr_coredump.attr))
>>> +		mode = 0444;
>>
>> Nitpick: use S_IRUGO instead of 0444.
>>
> 
> Thanks for the suggestion Kishon, but I like 0444, it has direct meaning
> to me.
> 
> So unless there's some directive to use S_I*** throughout the kernel I
> would prefer this.
> 
> Regards,
> Bjorn
> 
>> Thanks,
>> Kishon
>>> +
>>> +	return mode;
>>> +}
>>> +
>>>  static struct attribute *rproc_attrs[] = {
>>>  	&dev_attr_coredump.attr,
>>>  	&dev_attr_recovery.attr,
>>> @@ -240,7 +256,8 @@ static struct attribute *rproc_attrs[] = {
>>>  };
>>>  
>>>  static const struct attribute_group rproc_devgroup = {
>>> -	.attrs = rproc_attrs
>>> +	.attrs = rproc_attrs,
>>> +	.is_visible = rproc_is_visible,
>>>  };
>>>  
>>>  static const struct attribute_group *rproc_devgroups[] = {
>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>> index e0600e1e5c17..93a1d0050fbc 100644
>>> --- a/include/linux/remoteproc.h
>>> +++ b/include/linux/remoteproc.h
>>> @@ -523,6 +523,7 @@ struct rproc_dump_segment {
>>>   * @table_sz: size of @cached_table
>>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>>>   * @auto_boot: flag to indicate if remote processor should be auto-started
>>> + * @sysfs_read_only: flag to make remoteproc sysfs files read only
>>>   * @dump_segments: list of segments in the firmware
>>>   * @nb_vdev: number of vdev currently handled by rproc
>>>   * @elf_class: firmware ELF class
>>> @@ -562,6 +563,7 @@ struct rproc {
>>>  	size_t table_sz;
>>>  	bool has_iommu;
>>>  	bool auto_boot;
>>> +	bool sysfs_read_only;
>>>  	struct list_head dump_segments;
>>>  	int nb_vdev;
>>>  	u8 elf_class;
>>>
Mathieu Poirier March 4, 2022, 4:53 p.m. UTC | #6
On Thu, Mar 03, 2022 at 01:30:08PM +0530, Puranjay Mohan wrote:
> Hi Bjorn,
> Hi Mathieu,
> 
> When is this series expected to be applied?
> 
> I am going to post another series titled "PRU Consumer API".
> One patch from that series depends on this "Introduce sysfs_read_only
> flag" patch.
> 
> Please let me know so I can rebase and post that series.

Applied and pushed.

Thanks,
Mathieu

> 
> Thanks,
> Puranjay Mohan
> 
> On 18/02/22 11:24, Bjorn Andersson wrote:
> > On Thu 17 Feb 21:00 PST 2022, Kishon Vijay Abraham I wrote:
> > 
> >>
> >>
> >> On 16/02/22 1:42 pm, Puranjay Mohan wrote:
> >>> The remoteproc framework provides sysfs interfaces for changing
> >>> the firmware name and for starting/stopping a remote processor
> >>> through the sysfs files 'state' and 'firmware'. The 'coredump'
> >>> file is used to set the coredump configuration. The 'recovery'
> >>> sysfs file can also be used similarly to control the error recovery
> >>> state machine of a remoteproc. These interfaces are currently
> >>> allowed irrespective of how the remoteprocs were booted (like
> >>> remoteproc self auto-boot, remoteproc client-driven boot etc).
> >>> These interfaces can adversely affect a remoteproc and its clients
> >>> especially when a remoteproc is being controlled by a remoteproc
> >>> client driver(s). Also, not all remoteproc drivers may want to
> >>> support the sysfs interfaces by default.
> >>>
> >>> Add support to make the remoteproc sysfs files read only by
> >>> introducing a state flag 'sysfs_read_only' that the individual
> >>> remoteproc drivers can set based on their usage needs. The default
> >>> behavior is to allow the sysfs operations as before.
> >>>
> >>> Implement attribute_group->is_visible() to make the sysfs
> >>> entries read only when 'sysfs_read_only' flag is set.
> >>>
> >>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> >>> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>> ---
> >>> Changes in v4->v5:
> >>> Rename deny_sysfs_ops to sysfs_read_only.
> >>> Make coredump readonly with other files.
> >>>
> >>> Changes in v3->v4:
> >>> Use mode = 0444 in rproc_is_visible() to make the sysfs entries
> >>> read-only when the deny_sysfs_ops flag is set.
> >>> ---
> >>>  drivers/remoteproc/remoteproc_sysfs.c | 19 ++++++++++++++++++-
> >>>  include/linux/remoteproc.h            |  2 ++
> >>>  2 files changed, 20 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> >>> index ea8b89f97d7b..abf0cd05d5e1 100644
> >>> --- a/drivers/remoteproc/remoteproc_sysfs.c
> >>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> >>> @@ -230,6 +230,22 @@ static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> >>>  }
> >>>  static DEVICE_ATTR_RO(name);
> >>>  
> >>> +static umode_t rproc_is_visible(struct kobject *kobj, struct attribute *attr,
> >>> +				int n)
> >>> +{
> >>> +	struct device *dev = kobj_to_dev(kobj);
> >>> +	struct rproc *rproc = to_rproc(dev);
> >>> +	umode_t mode = attr->mode;
> >>> +
> >>> +	if (rproc->sysfs_read_only && (attr == &dev_attr_recovery.attr ||
> >>> +				       attr == &dev_attr_firmware.attr ||
> >>> +				       attr == &dev_attr_state.attr ||
> >>> +				       attr == &dev_attr_coredump.attr))
> >>> +		mode = 0444;
> >>
> >> Nitpick: use S_IRUGO instead of 0444.
> >>
> > 
> > Thanks for the suggestion Kishon, but I like 0444, it has direct meaning
> > to me.
> > 
> > So unless there's some directive to use S_I*** throughout the kernel I
> > would prefer this.
> > 
> > Regards,
> > Bjorn
> > 
> >> Thanks,
> >> Kishon
> >>> +
> >>> +	return mode;
> >>> +}
> >>> +
> >>>  static struct attribute *rproc_attrs[] = {
> >>>  	&dev_attr_coredump.attr,
> >>>  	&dev_attr_recovery.attr,
> >>> @@ -240,7 +256,8 @@ static struct attribute *rproc_attrs[] = {
> >>>  };
> >>>  
> >>>  static const struct attribute_group rproc_devgroup = {
> >>> -	.attrs = rproc_attrs
> >>> +	.attrs = rproc_attrs,
> >>> +	.is_visible = rproc_is_visible,
> >>>  };
> >>>  
> >>>  static const struct attribute_group *rproc_devgroups[] = {
> >>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >>> index e0600e1e5c17..93a1d0050fbc 100644
> >>> --- a/include/linux/remoteproc.h
> >>> +++ b/include/linux/remoteproc.h
> >>> @@ -523,6 +523,7 @@ struct rproc_dump_segment {
> >>>   * @table_sz: size of @cached_table
> >>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
> >>>   * @auto_boot: flag to indicate if remote processor should be auto-started
> >>> + * @sysfs_read_only: flag to make remoteproc sysfs files read only
> >>>   * @dump_segments: list of segments in the firmware
> >>>   * @nb_vdev: number of vdev currently handled by rproc
> >>>   * @elf_class: firmware ELF class
> >>> @@ -562,6 +563,7 @@ struct rproc {
> >>>  	size_t table_sz;
> >>>  	bool has_iommu;
> >>>  	bool auto_boot;
> >>> +	bool sysfs_read_only;
> >>>  	struct list_head dump_segments;
> >>>  	int nb_vdev;
> >>>  	u8 elf_class;
> >>>
diff mbox series

Patch

diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index ea8b89f97d7b..abf0cd05d5e1 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -230,6 +230,22 @@  static ssize_t name_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(name);
 
+static umode_t rproc_is_visible(struct kobject *kobj, struct attribute *attr,
+				int n)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct rproc *rproc = to_rproc(dev);
+	umode_t mode = attr->mode;
+
+	if (rproc->sysfs_read_only && (attr == &dev_attr_recovery.attr ||
+				       attr == &dev_attr_firmware.attr ||
+				       attr == &dev_attr_state.attr ||
+				       attr == &dev_attr_coredump.attr))
+		mode = 0444;
+
+	return mode;
+}
+
 static struct attribute *rproc_attrs[] = {
 	&dev_attr_coredump.attr,
 	&dev_attr_recovery.attr,
@@ -240,7 +256,8 @@  static struct attribute *rproc_attrs[] = {
 };
 
 static const struct attribute_group rproc_devgroup = {
-	.attrs = rproc_attrs
+	.attrs = rproc_attrs,
+	.is_visible = rproc_is_visible,
 };
 
 static const struct attribute_group *rproc_devgroups[] = {
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e0600e1e5c17..93a1d0050fbc 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -523,6 +523,7 @@  struct rproc_dump_segment {
  * @table_sz: size of @cached_table
  * @has_iommu: flag to indicate if remote processor is behind an MMU
  * @auto_boot: flag to indicate if remote processor should be auto-started
+ * @sysfs_read_only: flag to make remoteproc sysfs files read only
  * @dump_segments: list of segments in the firmware
  * @nb_vdev: number of vdev currently handled by rproc
  * @elf_class: firmware ELF class
@@ -562,6 +563,7 @@  struct rproc {
 	size_t table_sz;
 	bool has_iommu;
 	bool auto_boot;
+	bool sysfs_read_only;
 	struct list_head dump_segments;
 	int nb_vdev;
 	u8 elf_class;