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@foss.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....
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 errorThe 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