linux-kernel - Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver

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: <20240307194026.GA355455@e130802.arm.com>
Date: Thu, 7 Mar 2024 19:40:26 +0000
From: Abdellatif El Khlifi <abdellatif.elkhlifi@....com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: Bjorn Andersson <andersson@...nel.org>,
	Rob Herring <robh+dt@...nel.org>, Liviu Dudau <liviu.dudau@....com>,
	Sudeep Holla <sudeep.holla@....com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>, Drew.Reed@....com,
	Adam.Johnston@....com, linux-arm-kernel@...ts.infradead.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver

Hi Mathieu,

> > +	do {
> > +		state_reg = readl(priv->reset_cfg.state_reg);
> > +		*rst_ack = EXTSYS_RST_ST_RST_ACK(state_reg);
> > +
> > +		if (*rst_ack == EXTSYS_RST_ACK_RESERVED) {
> > +			dev_err(dev, "unexpected RST_ACK value: 0x%x\n",
> > +				*rst_ack);
> > +			return -EINVAL;
> > +		}
> > +
> > +		/* expected ACK value read */
> > +		if ((*rst_ack & exp_ack) || (*rst_ack == exp_ack))
> 
> I'm not sure why the second condition in this if() statement is needed.  As far
> as I can tell the first condition will trigger and the second one won't be
> reached.

The second condition takes care of the following: exp_ack and  *rst_ack are both 0.
This case happens when RST_REQ bit is cleared (meaning: No reset requested) and
we expect the RST_ACK to be 00 afterwards.

> > +/**
> > + * arm_rproc_load() - Load firmware to memory function for rproc_ops
> > + * @rproc: pointer to the remote processor object
> > + * @fw: pointer to the firmware
> > + *
> > + * Does nothing currently.
> > + *
> > + * Return:
> > + *
> > + * 0 for success.
> > + */
> > +static int arm_rproc_load(struct rproc *rproc, const struct firmware *fw)
> > +{
> 
> What is the point of doing rproc_of_parse_firmware() if the firmware image is
> not loaded to memory?  Does the remote processor have some kind of default ROM
> image to run if it doesn't find anything in memory?

Yes, the remote processor has a default FW image already loaded by default.

rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in the filesystem or a filename
provided.

Please correct me if I'm wrong.

[1]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/remoteproc/remoteproc_core.c#L1947
[2]: https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/base/firmware_loader/main.c#L863

> > +module_platform_driver(arm_rproc_driver);
> > +
> 
> I am echoing Krzysztof view about how generic this driver name is.  This has to
> be related to a family of processors or be made less generic in some way.  Have
> a look at what TI did for their K3 lineup [1] - I would like to see the same
> thing done here.

Thank you, I'll take care of that and of all the other comments made.

Cheers,
Abdellatif