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