linux-kernel - RE: [PATCH V2 1/2] remoteproc: introduce rproc features

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DU0PR04MB941787400EF3FDD90AE41634880F9@DU0PR04MB9417.eurprd04.prod.outlook.com>
Date:   Mon, 14 Mar 2022 07:23:15 +0000
From:   Peng Fan <peng.fan@....com>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        "Peng Fan (OSS)" <peng.fan@....nxp.com>
CC:     "mathieu.poirier@...aro.org" <mathieu.poirier@...aro.org>,
        "arnaud.pouliquen@...s.st.com" <arnaud.pouliquen@...s.st.com>,
        "linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH V2 1/2] remoteproc: introduce rproc features

> Subject: Re: [PATCH V2 1/2] remoteproc: introduce rproc features
> 
> On Tue 08 Mar 00:48 CST 2022, Peng Fan (OSS) wrote:
> 
> > From: Peng Fan <peng.fan@....com>
> >
> > remote processor may support:
> >  - firmware recovery with help from main processor
> >  - self recovery without help from main processor
> >  - iommu
> >  - etc
> >
> > Introduce rproc features could simplify code to avoid adding more bool
> > flags and let us optimize current code.
> >
> > Signed-off-by: Peng Fan <peng.fan@....com>
> > ---
> >
> > V2:
> >  New
> >
> >  include/linux/remoteproc.h | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> 
> This is the API that other parts of the kernel use to interact with a struct rproc,
> what feature flags do you have a need for other parts of the kernel to be able
> to query?

I no need other parts of kernel to query rproc features.
In my V1 patch to support i.MX8QM/QXP self recovery, I introduced a new bool
in "struct rproc", but it is not preferred. So I think to use a enum flag, after
looking into struct rproc, I see the possibility to use one API rproc_has_feature
to simplify code, such as move has_iommu into 
rproc_has_feature(rproc, RPROC_FEAT_IOMMU), and in future, when need to
add new flag/capability of rproc, we could reuse the rproc_has_feature API.

> 
> > index 93a1d0050fbc..51edaf80692c 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -417,6 +417,7 @@ struct rproc_ops {
> >   *			has attached to it
> >   * @RPROC_DETACHED:	device has been booted by another entity and
> waiting
> >   *			for the core to attach to it
> > + * @RPROC_CRASHED_ATTACH_RECOVERY: device has crashed and self
> > + recovery
> 
> This seems to belong in the other patch...

I'll move this to patch 2/2

> 
> >   * @RPROC_LAST:		just keep this one at the end
> >   *
> >   * Please note that the values of these states are used as indices @@
> > -489,6 +490,11 @@ struct rproc_dump_segment {
> >  	loff_t offset;
> >  };
> >
> > +enum rproc_features {
> > +	RPROC_FEAT_ATTACH_RECOVERY = 0,
> 
> No need to specify that this is bit 0, and the enum will do that for you.

sure

> 
> > +	RPROC_MAX_FEATURES = 32,
> 
> You're using DECLARE_BITMAP() so why 32?

Just wanna to limit max features to one int, I may mis use this,
will fix in new version.

Thanks,
Peng.

> 
> Regards,
> Bjorn
> 
> > +};
> > +
> >  /**
> >   * struct rproc - represents a physical remote processor device
> >   * @node: list node of this rproc object @@ -530,6 +536,7 @@ struct
> > rproc_dump_segment {
> >   * @elf_machine: firmware ELF machine
> >   * @cdev: character device of the rproc
> >   * @cdev_put_on_release: flag to indicate if remoteproc should be
> > shutdown on @char_dev release
> > + * @features: indicate remoteproc features
> >   */
> >  struct rproc {
> >  	struct list_head node;
> > @@ -570,8 +577,19 @@ struct rproc {
> >  	u16 elf_machine;
> >  	struct cdev cdev;
> >  	bool cdev_put_on_release;
> > +	DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
> >  };
> >
> > +static inline bool rproc_has_feature(struct rproc *rproc, unsigned
> > +int feature) {
> > +	return test_bit(feature, rproc->features); }
> > +
> > +static inline void rproc_set_feature(struct rproc *rproc, unsigned
> > +int feature) {
> > +	set_bit(feature, rproc->features);
> > +}
> > +
> >  /**
> >   * struct rproc_subdev - subdevice tied to a remoteproc
> >   * @node: list node related to the rproc subdevs list
> > --
> > 2.30.0
> >