This is a refresh of the patches from an old series [1].
Patch 1 of that series is not required now as [2] serves its purpose.
Patch 2 has been improved and is being posted here.
Patch 3 is unchanged, it has been rebased and posted.
The features being introduced here will be needed by the recently posted PRU
remoteproc driver [3] in addition to the existing Wkup M3 remoteproc driver.
Both of these drivers follow a client-driven boot methodology, with the latter
strictly booted by another driver in kernel. The PRU remoteproc driver will be
supporting both in-kernel clients as well as control from userspace orthogonally.
The logic though is applicable and useful to any remoteproc driver not using
'auto-boot' and using an external driver/application to boot the remoteproc.
[1] https://patchwork.kernel.org/project/linux-remoteproc/cover/[email protected]/
[2] https://patchwork.kernel.org/project/linux-remoteproc/patch/[email protected]/
[3] https://patchwork.kernel.org/project/linux-remoteproc/cover/[email protected]/
Puranjay Mohan (1):
remoteproc: Introduce sysfs_read_only flag
Suman Anna (1):
remoteproc: wkup_m3: Set sysfs_read_only flag
drivers/remoteproc/remoteproc_sysfs.c | 19 ++++++++++++++++++-
drivers/remoteproc/wkup_m3_rproc.c | 1 +
include/linux/remoteproc.h | 2 ++
3 files changed, 21 insertions(+), 1 deletion(-)
--
2.17.1
From: Suman Anna <[email protected]>
The Wakeup M3 remote processor is controlled by the wkup_m3_ipc
client driver, so set the newly introduced 'sysfs_read_only' flag
to not allow any overriding of the remoteproc firmware, state,
recovery, or coredump from userspace.
Signed-off-by: Suman Anna <[email protected]>
Signed-off-by: Puranjay Mohan <[email protected]>
Reviewed-by: Mathieu Poirier <[email protected]>
---
Changes in v4->v5
rename deny_sysfs_ops to sysfs_read_only
---
drivers/remoteproc/wkup_m3_rproc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/remoteproc/wkup_m3_rproc.c b/drivers/remoteproc/wkup_m3_rproc.c
index 484f7605823e..a0c204cb0979 100644
--- a/drivers/remoteproc/wkup_m3_rproc.c
+++ b/drivers/remoteproc/wkup_m3_rproc.c
@@ -163,6 +163,7 @@ static int wkup_m3_rproc_probe(struct platform_device *pdev)
}
rproc->auto_boot = false;
+ rproc->sysfs_read_only = true;
wkupm3 = rproc->priv;
wkupm3->rproc = rproc;
--
2.17.1
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 <[email protected]>
Reviewed-by: Mathieu Poirier <[email protected]>
---
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;
+
+ 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
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 <[email protected]>
> Reviewed-by: Mathieu Poirier <[email protected]>
> ---
> 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
>
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 <[email protected]>
>> Reviewed-by: Mathieu Poirier <[email protected]>
>> ---
>> 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/[email protected]/
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
>>
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 <[email protected]>
> Reviewed-by: Mathieu Poirier <[email protected]>
> ---
> 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;
>
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 <[email protected]>
> > Reviewed-by: Mathieu Poirier <[email protected]>
> > ---
> > 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;
> >
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 <[email protected]>
>>> Reviewed-by: Mathieu Poirier <[email protected]>
>>> ---
>>> 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;
>>>
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 <[email protected]>
> >>> Reviewed-by: Mathieu Poirier <[email protected]>
> >>> ---
> >>> 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;
> >>>