| 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
| ||
|
Message-ID: <cda96440-5c53-4b7a-8b51-51506f5e7cc3@foss.st.com>
Date: Tue, 17 Jun 2025 19:38:19 +0200
From: Arnaud POULIQUEN <arnaud.pouliquen@...s.st.com>
To: Bjorn Andersson <andersson@...nel.org>
CC: Mathieu Poirier <mathieu.poirier@...aro.org>,
Jens Wiklander
<jens.wiklander@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
"Krzysztof
Kozlowski" <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-remoteproc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<op-tee@...ts.trustedfirmware.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v18 3/6] remoteproc: Introduce release_fw optional
operation
Hello Bjorn,
On 6/17/25 06:44, Bjorn Andersson wrote:
> On Mon, Jun 16, 2025 at 09:55:27AM +0200, Arnaud Pouliquen wrote:
>> The release_fw operation is the inverse operation of the load, responsible
>> for releasing the remote processor resources configured from the loading
>> of the remoteproc firmware (e.g., memories).
>>
>
> I was under the impression that we agreed that this would unroll
> rproc_parse_fw() not the "load" in general.
Not Krystal clear to me what you are expecting here.
Is it just on the description or on the design?
Unroll only the rproc_parse_fw is not sufficient. The need here is also
to go back from a LOAD state of the TEE. So in such case the role of
release_fw() would be to unroll the load + the parse of the resource.
Is it your expectation?
>
>> The operation is called in the following cases:
>> - An error occurs on boot of the remote processor.
>> - An error occurs on recovery start of the remote processor.
>> - After stopping the remote processor.
>>
>> This operation is needed for the remoteproc_tee implementation after stop
>> and on error.
>
> And if it's defined to unroll rproc_parse_fw() it can be used for other
> things where some resources was allocated to set up the resource table.
True
>
>> Indeed, as the remoteproc image is loaded when we parse the resource
>> table, there are many situations where something can go wrong before
>> the start of the remote processor(resource handling, carveout allocation,
>> ...).
>
> Unbalanced parenthesis? I think you can write this in less
> conversational style.
>
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@...s.st.com>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 6 ++++++
>> drivers/remoteproc/remoteproc_internal.h | 6 ++++++
>> include/linux/remoteproc.h | 3 +++
>> 3 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index d06eef1fa424..4c1a4bc9e7b7 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1857,6 +1857,8 @@ static int rproc_boot_recovery(struct rproc *rproc)
>>
>> /* boot the remote processor up again */
>> ret = rproc_start(rproc, firmware_p);
>> + if (ret)
>> + rproc_release_fw(rproc);
>>
>> release_firmware(firmware_p);
>>
>> @@ -1998,6 +2000,8 @@ int rproc_boot(struct rproc *rproc)
>> }
>>
>> ret = rproc_fw_boot(rproc, firmware_p);
>> + if (ret)
>> + rproc_release_fw(rproc);
>>
>> release_firmware(firmware_p);
>> }
>> @@ -2067,6 +2071,8 @@ int rproc_shutdown(struct rproc *rproc)
>>
>> rproc_disable_iommu(rproc);
>>
>> + rproc_release_fw(rproc);
>> +
>> /* Free the copy of the resource table */
>> kfree(rproc->cached_table);
>> rproc->cached_table = NULL;
>
> These are allocated in rproc_parse_fw(), would it not make sense to
> clean them up in your newly introduced function?
It seems possible as proposed in v11 3/7[1], but this needs an exception
for rproc_detach().
[1]
https://patchew.org/linux/20241009080108.4170320-1-arnaud.pouliquen@foss.st.com/20241009080108.4170320-4-arnaud.pouliquen@foss.st.com/
>
>> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
>> index 0cd09e67ac14..c7fb908f8652 100644
>> --- a/drivers/remoteproc/remoteproc_internal.h
>> +++ b/drivers/remoteproc/remoteproc_internal.h
>> @@ -221,4 +221,10 @@ bool rproc_u64_fit_in_size_t(u64 val)
>> return (val <= (size_t) -1);
>> }
>>
>> +static inline void rproc_release_fw(struct rproc *rproc)
>> +{
>> + if (rproc->ops->release_fw)
>> + rproc->ops->release_fw(rproc);
>> +}
>> +
>> #endif /* REMOTEPROC_INTERNAL_H */
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 8fd0d7f63c8e..80128461972b 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -381,6 +381,8 @@ enum rsc_handling_status {
>> * @panic: optional callback to react to system panic, core will delay
>> * panic at least the returned number of milliseconds
>> * @coredump: collect firmware dump after the subsystem is shutdown
>> + * @release_fw: optional function to release the loaded firmware, called after
>> + * stopping the remote processor or in case of error
>
> The struct firmware is released at the end of startup and the typical
> carveout memory where the firmware is loaded into is released at
> rproc_shutdown().
>
> As such, this won't help anyone understand the purpose of the ops unless
> they know your system design (and know you added it).
Could you detail which improvement you are expecting here?
Name of the ops, associated comment? both?
Thanks,
Arnaud
>
> Regards,
> Bjorn
>
>> */
>> struct rproc_ops {
>> int (*prepare)(struct rproc *rproc);
>> @@ -403,6 +405,7 @@ struct rproc_ops {
>> u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
>> unsigned long (*panic)(struct rproc *rproc);
>> void (*coredump)(struct rproc *rproc);
>> + void (*release_fw)(struct rproc *rproc);
>> };
>>
>> /**
>> --
>> 2.25.1
>>
Powered by blists - more mailing lists