* [PATCH v5 0/2] remoteproc sysfs fixes/improvements @ 2022-02-16 8:12 Puranjay Mohan 2022-02-16 8:12 ` [PATCH v5 1/2] remoteproc: Introduce sysfs_read_only flag Puranjay Mohan 2022-02-16 8:12 ` [PATCH v5 2/2] remoteproc: wkup_m3: Set " Puranjay Mohan 0 siblings, 2 replies; 9+ messages in thread From: Puranjay Mohan @ 2022-02-16 8:12 UTC (permalink / raw) To: kishon, vigneshr, s-anna, bjorn.andersson, mathieu.poirier, linux-remoteproc, linux-kernel, linux-arm-kernel Cc: Puranjay Mohan 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/20201121030156.22857-1-s-anna@ti.com/ [2] https://patchwork.kernel.org/project/linux-remoteproc/patch/20201126210642.897302-4-mathieu.poirier@linaro.org/ [3] https://patchwork.kernel.org/project/linux-remoteproc/cover/20201119140850.12268-1-grzegorz.jaszczyk@linaro.org/ 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 1/2] remoteproc: Introduce sysfs_read_only flag 2022-02-16 8:12 [PATCH v5 0/2] remoteproc sysfs fixes/improvements Puranjay Mohan @ 2022-02-16 8:12 ` Puranjay Mohan 2022-02-16 20:04 ` Bjorn Andersson 2022-02-18 5:00 ` Kishon Vijay Abraham I 2022-02-16 8:12 ` [PATCH v5 2/2] remoteproc: wkup_m3: Set " Puranjay Mohan 1 sibling, 2 replies; 9+ messages in thread From: Puranjay Mohan @ 2022-02-16 8:12 UTC (permalink / raw) To: kishon, vigneshr, s-anna, bjorn.andersson, mathieu.poirier, linux-remoteproc, linux-kernel, linux-arm-kernel Cc: Puranjay Mohan 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; + + 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] remoteproc: Introduce sysfs_read_only flag 2022-02-16 8:12 ` [PATCH v5 1/2] remoteproc: Introduce sysfs_read_only flag Puranjay Mohan @ 2022-02-16 20:04 ` Bjorn Andersson 2022-02-17 6:14 ` Puranjay Mohan 2022-02-18 5:00 ` Kishon Vijay Abraham I 1 sibling, 1 reply; 9+ messages in thread From: Bjorn Andersson @ 2022-02-16 20:04 UTC (permalink / raw) To: Puranjay Mohan Cc: kishon, vigneshr, s-anna, mathieu.poirier, linux-remoteproc, linux-kernel, linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] remoteproc: Introduce sysfs_read_only flag 2022-02-16 20:04 ` Bjorn Andersson @ 2022-02-17 6:14 ` Puranjay Mohan 0 siblings, 0 replies; 9+ messages in thread From: Puranjay Mohan @ 2022-02-17 6:14 UTC (permalink / raw) To: Bjorn Andersson Cc: kishon, vigneshr, s-anna, mathieu.poirier, linux-remoteproc, linux-kernel, linux-arm-kernel 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 >> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] remoteproc: Introduce sysfs_read_only flag 2022-02-16 8:12 ` [PATCH v5 1/2] remoteproc: Introduce sysfs_read_only flag Puranjay Mohan 2022-02-16 20:04 ` Bjorn Andersson @ 2022-02-18 5:00 ` Kishon Vijay Abraham I 2022-02-18 5:54 ` Bjorn Andersson 1 sibling, 1 reply; 9+ messages in thread From: Kishon Vijay Abraham I @ 2022-02-18 5:00 UTC (permalink / raw) To: Puranjay Mohan, vigneshr, s-anna, bjorn.andersson, mathieu.poirier, linux-remoteproc, linux-kernel, linux-arm-kernel 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; > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] remoteproc: Introduce sysfs_read_only flag 2022-02-18 5:00 ` Kishon Vijay Abraham I @ 2022-02-18 5:54 ` Bjorn Andersson 2022-03-03 8:00 ` [EXTERNAL] " Puranjay Mohan 0 siblings, 1 reply; 9+ messages in thread From: Bjorn Andersson @ 2022-02-18 5:54 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Puranjay Mohan, vigneshr, s-anna, mathieu.poirier, linux-remoteproc, linux-kernel, linux-arm-kernel 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; > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [EXTERNAL] Re: [PATCH v5 1/2] remoteproc: Introduce sysfs_read_only flag 2022-02-18 5:54 ` Bjorn Andersson @ 2022-03-03 8:00 ` Puranjay Mohan 2022-03-04 16:53 ` Mathieu Poirier 0 siblings, 1 reply; 9+ messages in thread From: Puranjay Mohan @ 2022-03-03 8:00 UTC (permalink / raw) To: Bjorn Andersson, Kishon Vijay Abraham I Cc: vigneshr, s-anna, mathieu.poirier, linux-remoteproc, linux-kernel, linux-arm-kernel 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; >>> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [EXTERNAL] Re: [PATCH v5 1/2] remoteproc: Introduce sysfs_read_only flag 2022-03-03 8:00 ` [EXTERNAL] " Puranjay Mohan @ 2022-03-04 16:53 ` Mathieu Poirier 0 siblings, 0 replies; 9+ messages in thread From: Mathieu Poirier @ 2022-03-04 16:53 UTC (permalink / raw) To: Puranjay Mohan Cc: Bjorn Andersson, Kishon Vijay Abraham I, vigneshr, s-anna, linux-remoteproc, linux-kernel, linux-arm-kernel 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; > >>> ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 2/2] remoteproc: wkup_m3: Set sysfs_read_only flag 2022-02-16 8:12 [PATCH v5 0/2] remoteproc sysfs fixes/improvements Puranjay Mohan 2022-02-16 8:12 ` [PATCH v5 1/2] remoteproc: Introduce sysfs_read_only flag Puranjay Mohan @ 2022-02-16 8:12 ` Puranjay Mohan 1 sibling, 0 replies; 9+ messages in thread From: Puranjay Mohan @ 2022-02-16 8:12 UTC (permalink / raw) To: kishon, vigneshr, s-anna, bjorn.andersson, mathieu.poirier, linux-remoteproc, linux-kernel, linux-arm-kernel Cc: Puranjay Mohan From: Suman Anna <s-anna@ti.com> 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 <s-anna@ti.com> 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 --- 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-03-04 16:53 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-16 8:12 [PATCH v5 0/2] remoteproc sysfs fixes/improvements Puranjay Mohan 2022-02-16 8:12 ` [PATCH v5 1/2] remoteproc: Introduce sysfs_read_only flag Puranjay Mohan 2022-02-16 20:04 ` Bjorn Andersson 2022-02-17 6:14 ` Puranjay Mohan 2022-02-18 5:00 ` Kishon Vijay Abraham I 2022-02-18 5:54 ` Bjorn Andersson 2022-03-03 8:00 ` [EXTERNAL] " Puranjay Mohan 2022-03-04 16:53 ` Mathieu Poirier 2022-02-16 8:12 ` [PATCH v5 2/2] remoteproc: wkup_m3: Set " Puranjay Mohan
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).