Add support for releasing remote processor firmware through
the Trusted Execution Environment (TEE) interface.
The tee_rproc_release_fw() function is called in the following cases:
- An error occurs in rproc_start() between the loading of the segments and
the start of the remote processor.
- When rproc_release_fw is called on error or after stopping the remote
processor.
Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
drivers/remoteproc/remoteproc_core.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 7694817f25d4..32052dedc149 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -29,6 +29,7 @@
#include <linux/debugfs.h>
#include <linux/rculist.h>
#include <linux/remoteproc.h>
+#include <linux/remoteproc_tee.h>
#include <linux/iommu.h>
#include <linux/idr.h>
#include <linux/elf.h>
@@ -1258,6 +1259,9 @@ static int rproc_alloc_registered_carveouts(struct rproc *rproc)
static void rproc_release_fw(struct rproc *rproc)
{
+ if (rproc->state == RPROC_OFFLINE && rproc->tee_interface)
+ tee_rproc_release_fw(rproc);
+
/* Free the copy of the resource table */
kfree(rproc->cached_table);
rproc->cached_table = NULL;
@@ -1348,7 +1352,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
if (ret) {
dev_err(dev, "failed to prepare subdevices for %s: %d\n",
rproc->name, ret);
- goto reset_table_ptr;
+ goto release_fw;
}
/* power up the remote processor */
@@ -1376,7 +1380,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
rproc->ops->stop(rproc);
unprepare_subdevices:
rproc_unprepare_subdevices(rproc);
-reset_table_ptr:
+release_fw:
+ if (rproc->tee_interface)
+ tee_rproc_release_fw(rproc);
rproc->table_ptr = rproc->cached_table;
return ret;
--
2.25.1On Fri, Aug 30, 2024 at 11:51:44AM GMT, Arnaud Pouliquen wrote:
> Add support for releasing remote processor firmware through
> the Trusted Execution Environment (TEE) interface.
>
> The tee_rproc_release_fw() function is called in the following cases:
>
> - An error occurs in rproc_start() between the loading of the segments and
> the start of the remote processor.
> - When rproc_release_fw is called on error or after stopping the remote
> processor.
>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
> drivers/remoteproc/remoteproc_core.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 7694817f25d4..32052dedc149 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -29,6 +29,7 @@
> #include <linux/debugfs.h>
> #include <linux/rculist.h>
> #include <linux/remoteproc.h>
> +#include <linux/remoteproc_tee.h>
> #include <linux/iommu.h>
> #include <linux/idr.h>
> #include <linux/elf.h>
> @@ -1258,6 +1259,9 @@ static int rproc_alloc_registered_carveouts(struct rproc *rproc)
>
> static void rproc_release_fw(struct rproc *rproc)
> {
> + if (rproc->state == RPROC_OFFLINE && rproc->tee_interface)
> + tee_rproc_release_fw(rproc);
I don't like the idea of having op-tee specific calls made from the
core. If the problem is that we need to unroll something we did at load,
can we instead come up with a more generic mechanism to unload that? Or
can we perhaps postpone the tee interaction until start() to avoid the
gap?
PS. Most of the Qualcomm drivers are TEE-based...so the "tee_interface"
boolean check here is not very nice.
Regards,
Bjorn
> +
> /* Free the copy of the resource table */
> kfree(rproc->cached_table);
> rproc->cached_table = NULL;
> @@ -1348,7 +1352,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> if (ret) {
> dev_err(dev, "failed to prepare subdevices for %s: %d\n",
> rproc->name, ret);
> - goto reset_table_ptr;
> + goto release_fw;
> }
>
> /* power up the remote processor */
> @@ -1376,7 +1380,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> rproc->ops->stop(rproc);
> unprepare_subdevices:
> rproc_unprepare_subdevices(rproc);
> -reset_table_ptr:
> +release_fw:
> + if (rproc->tee_interface)
> + tee_rproc_release_fw(rproc);
> rproc->table_ptr = rproc->cached_table;
>
> return ret;
> --
> 2.25.1
>
Hello Bjorn,
On 9/26/24 05:51, Bjorn Andersson wrote:
> On Fri, Aug 30, 2024 at 11:51:44AM GMT, Arnaud Pouliquen wrote:
>> Add support for releasing remote processor firmware through
>> the Trusted Execution Environment (TEE) interface.
>>
>> The tee_rproc_release_fw() function is called in the following cases:
>>
>> - An error occurs in rproc_start() between the loading of the segments and
>> the start of the remote processor.
>> - When rproc_release_fw is called on error or after stopping the remote
>> processor.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 7694817f25d4..32052dedc149 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -29,6 +29,7 @@
>> #include <linux/debugfs.h>
>> #include <linux/rculist.h>
>> #include <linux/remoteproc.h>
>> +#include <linux/remoteproc_tee.h>
>> #include <linux/iommu.h>
>> #include <linux/idr.h>
>> #include <linux/elf.h>
>> @@ -1258,6 +1259,9 @@ static int rproc_alloc_registered_carveouts(struct rproc *rproc)
>>
>> static void rproc_release_fw(struct rproc *rproc)
>> {
>> + if (rproc->state == RPROC_OFFLINE && rproc->tee_interface)
>> + tee_rproc_release_fw(rproc);
>
> I don't like the idea of having op-tee specific calls made from the
> core. If the problem is that we need to unroll something we did at load,
> can we instead come up with a more generic mechanism to unload that? Or
As proposed in [1] an alternative could be to define a new rproc_ops->release_fw
operation that will be initialized to tee_rproc_release_fw in the platform driver.
> can we perhaps postpone the tee interaction until start() to avoid the
> gap?
In such a case, the management of the resource table should also be postponed
as the firmware has to be authenticated first.
The OP-TEE implementation authenticates the firmware during the load
(in-destination memory authentication), so the sequence is:
1) Load the firmware.
2) Get the resource table and initialize resources.
3) Start the firmware.
The tee_rproc_release_fw() is used if something goes wrong during step 2 an3.
From my perspective, this would result in an alternative boot sequence, as we
have today for "attach". I proposed this approach in my V3 [2]. But this add
complexity in remote proc core.
Please, could you align with Mathieu to define how we should move forward to
address your concerns?
[1]https://lkml.org/lkml/2024/9/18/612
[2]https://lore.kernel.org/lkml/8af59b01-53cf-4fc4-9946-6c630fb7b38e@quicinc.com/T/
Thanks and Regards,
Arnaud
>
>
> PS. Most of the Qualcomm drivers are TEE-based...so the "tee_interface"
> boolean check here is not very nice.
>
> Regards,
> Bjorn
>
>> +
>> /* Free the copy of the resource table */
>> kfree(rproc->cached_table);
>> rproc->cached_table = NULL;
>> @@ -1348,7 +1352,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>> if (ret) {
>> dev_err(dev, "failed to prepare subdevices for %s: %d\n",
>> rproc->name, ret);
>> - goto reset_table_ptr;
>> + goto release_fw;
>> }
>>
>> /* power up the remote processor */
>> @@ -1376,7 +1380,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>> rproc->ops->stop(rproc);
>> unprepare_subdevices:
>> rproc_unprepare_subdevices(rproc);
>> -reset_table_ptr:
>> +release_fw:
>> + if (rproc->tee_interface)
>> + tee_rproc_release_fw(rproc);
>> rproc->table_ptr = rproc->cached_table;
>>
>> return ret;
>> --
>> 2.25.1
>>
Hello Mathieu,
On 8/30/24 11:51, Arnaud Pouliquen wrote:
> Add support for releasing remote processor firmware through
> the Trusted Execution Environment (TEE) interface.
>
> The tee_rproc_release_fw() function is called in the following cases:
>
> - An error occurs in rproc_start() between the loading of the segments and
> the start of the remote processor.
> - When rproc_release_fw is called on error or after stopping the remote
> processor.
>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
> drivers/remoteproc/remoteproc_core.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 7694817f25d4..32052dedc149 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -29,6 +29,7 @@
> #include <linux/debugfs.h>
> #include <linux/rculist.h>
> #include <linux/remoteproc.h>
> +#include <linux/remoteproc_tee.h>
> #include <linux/iommu.h>
> #include <linux/idr.h>
> #include <linux/elf.h>
> @@ -1258,6 +1259,9 @@ static int rproc_alloc_registered_carveouts(struct rproc *rproc)
>
> static void rproc_release_fw(struct rproc *rproc)
> {
> + if (rproc->state == RPROC_OFFLINE && rproc->tee_interface)
> + tee_rproc_release_fw(rproc);
I'm requesting you expertise to fix an issue I'm facing during my test preparing
the V10.
My issue is that here, we can call the tee_rproc_release_fw() function, defined
in remoteproc_tee built as a remoteproc_tee.ko module.
I tried to use the IS_ENABLED and IS_REACHABLE macros in remoteproc_tee.h, but
without success:
- use IS_ENABLED() results in a link error: "undefined reference to
tee_rproc_release_fw."
- use IS_REACHABLE() returns false and remoteproc_core calls the inline
tee_rproc_release_fw function that just call WARN_ON(1).
To solve the issue, I can see three alternatives:
1) Modify Kconfig and remoteproc_tee.c to support only built-in.
2) Use symbol_get/symbol_put.
3) Define a new rproc_ops->release_fw operation that will be initialized to
tee_rproc_release_fw.
From my perspective, the solution 3 seems to be the cleanest way, as it also
removes the dependency between remoteproc_core.c and remoteproc_tee.c. But
regarding previous discussion/series version, it seems that it could not be your
preferred solution.
Please, could you indicate your preference so that I can directly implement the
best solution (or perhaps you have another alternative to propose)?
Thanks in advance!
Arnaud
> +
> /* Free the copy of the resource table */
> kfree(rproc->cached_table);
> rproc->cached_table = NULL;
> @@ -1348,7 +1352,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> if (ret) {
> dev_err(dev, "failed to prepare subdevices for %s: %d\n",
> rproc->name, ret);
> - goto reset_table_ptr;
> + goto release_fw;
> }
>
> /* power up the remote processor */
> @@ -1376,7 +1380,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> rproc->ops->stop(rproc);
> unprepare_subdevices:
> rproc_unprepare_subdevices(rproc);
> -reset_table_ptr:
> +release_fw:
> + if (rproc->tee_interface)
> + tee_rproc_release_fw(rproc);
> rproc->table_ptr = rproc->cached_table;
>
> return ret;
On Wed, Sep 18, 2024 at 04:43:32PM +0200, Arnaud POULIQUEN wrote:
> Hello Mathieu,
>
> On 8/30/24 11:51, Arnaud Pouliquen wrote:
> > Add support for releasing remote processor firmware through
> > the Trusted Execution Environment (TEE) interface.
> >
> > The tee_rproc_release_fw() function is called in the following cases:
> >
> > - An error occurs in rproc_start() between the loading of the segments and
> > the start of the remote processor.
> > - When rproc_release_fw is called on error or after stopping the remote
> > processor.
> >
> > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> > ---
> > drivers/remoteproc/remoteproc_core.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 7694817f25d4..32052dedc149 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -29,6 +29,7 @@
> > #include <linux/debugfs.h>
> > #include <linux/rculist.h>
> > #include <linux/remoteproc.h>
> > +#include <linux/remoteproc_tee.h>
> > #include <linux/iommu.h>
> > #include <linux/idr.h>
> > #include <linux/elf.h>
> > @@ -1258,6 +1259,9 @@ static int rproc_alloc_registered_carveouts(struct rproc *rproc)
> >
> > static void rproc_release_fw(struct rproc *rproc)
> > {
> > + if (rproc->state == RPROC_OFFLINE && rproc->tee_interface)
> > + tee_rproc_release_fw(rproc);
>
> I'm requesting you expertise to fix an issue I'm facing during my test preparing
> the V10.
>
> My issue is that here, we can call the tee_rproc_release_fw() function, defined
> in remoteproc_tee built as a remoteproc_tee.ko module.
>
> I tried to use the IS_ENABLED and IS_REACHABLE macros in remoteproc_tee.h, but
> without success:
> - use IS_ENABLED() results in a link error: "undefined reference to
> tee_rproc_release_fw."
> - use IS_REACHABLE() returns false and remoteproc_core calls the inline
> tee_rproc_release_fw function that just call WARN_ON(1).
>
> To solve the issue, I can see three alternatives:
>
> 1) Modify Kconfig and remoteproc_tee.c to support only built-in.
> 2) Use symbol_get/symbol_put.
> 3) Define a new rproc_ops->release_fw operation that will be initialized to
> tee_rproc_release_fw.
>
Option (1) is best but make sure people can disable the TEE interface if they
don't wish to use it.
> From my perspective, the solution 3 seems to be the cleanest way, as it also
> removes the dependency between remoteproc_core.c and remoteproc_tee.c. But
> regarding previous discussion/series version, it seems that it could not be your
> preferred solution.
>
> Please, could you indicate your preference so that I can directly implement the
> best solution (or perhaps you have another alternative to propose)?
>
> Thanks in advance!
>
> Arnaud
>
>
> > +
> > /* Free the copy of the resource table */
> > kfree(rproc->cached_table);
> > rproc->cached_table = NULL;
> > @@ -1348,7 +1352,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> > if (ret) {
> > dev_err(dev, "failed to prepare subdevices for %s: %d\n",
> > rproc->name, ret);
> > - goto reset_table_ptr;
> > + goto release_fw;
> > }
> >
> > /* power up the remote processor */
> > @@ -1376,7 +1380,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> > rproc->ops->stop(rproc);
> > unprepare_subdevices:
> > rproc_unprepare_subdevices(rproc);
> > -reset_table_ptr:
> > +release_fw:
> > + if (rproc->tee_interface)
> > + tee_rproc_release_fw(rproc);
> > rproc->table_ptr = rproc->cached_table;
> >
> > return ret;
On Fri, Aug 30, 2024 at 11:51:44AM +0200, Arnaud Pouliquen wrote:
> Add support for releasing remote processor firmware through
> the Trusted Execution Environment (TEE) interface.
>
> The tee_rproc_release_fw() function is called in the following cases:
>
> - An error occurs in rproc_start() between the loading of the segments and
> the start of the remote processor.
> - When rproc_release_fw is called on error or after stopping the remote
> processor.
>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
> drivers/remoteproc/remoteproc_core.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 7694817f25d4..32052dedc149 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -29,6 +29,7 @@
> #include <linux/debugfs.h>
> #include <linux/rculist.h>
> #include <linux/remoteproc.h>
> +#include <linux/remoteproc_tee.h>
> #include <linux/iommu.h>
> #include <linux/idr.h>
> #include <linux/elf.h>
> @@ -1258,6 +1259,9 @@ static int rproc_alloc_registered_carveouts(struct rproc *rproc)
>
> static void rproc_release_fw(struct rproc *rproc)
> {
> + if (rproc->state == RPROC_OFFLINE && rproc->tee_interface)
> + tee_rproc_release_fw(rproc);
Function tee_rproc_release_fw() returns a value that is ignored. I don't know
how it passes the Sparse checker but I already see patches coming in my Inbox to
deal with that. In this case there is nothing else to do if there is an error
releasing the firware. As such I would put a (void) in front and add a comment
about the return value being ignore on purpose.
> +
> /* Free the copy of the resource table */
> kfree(rproc->cached_table);
> rproc->cached_table = NULL;
> @@ -1348,7 +1352,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> if (ret) {
> dev_err(dev, "failed to prepare subdevices for %s: %d\n",
> rproc->name, ret);
> - goto reset_table_ptr;
> + goto release_fw;
> }
>
> /* power up the remote processor */
> @@ -1376,7 +1380,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> rproc->ops->stop(rproc);
> unprepare_subdevices:
> rproc_unprepare_subdevices(rproc);
> -reset_table_ptr:
> +release_fw:
> + if (rproc->tee_interface)
> + tee_rproc_release_fw(rproc);
Same here.
> rproc->table_ptr = rproc->cached_table;
>
> return ret;
> --
> 2.25.1
>
Hello Mathieu,
On 9/12/24 17:26, Mathieu Poirier wrote:
> On Fri, Aug 30, 2024 at 11:51:44AM +0200, Arnaud Pouliquen wrote:
>> Add support for releasing remote processor firmware through
>> the Trusted Execution Environment (TEE) interface.
>>
>> The tee_rproc_release_fw() function is called in the following cases:
>>
>> - An error occurs in rproc_start() between the loading of the segments and
>> the start of the remote processor.
>> - When rproc_release_fw is called on error or after stopping the remote
>> processor.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 7694817f25d4..32052dedc149 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -29,6 +29,7 @@
>> #include <linux/debugfs.h>
>> #include <linux/rculist.h>
>> #include <linux/remoteproc.h>
>> +#include <linux/remoteproc_tee.h>
>> #include <linux/iommu.h>
>> #include <linux/idr.h>
>> #include <linux/elf.h>
>> @@ -1258,6 +1259,9 @@ static int rproc_alloc_registered_carveouts(struct rproc *rproc)
>>
>> static void rproc_release_fw(struct rproc *rproc)
>> {
>> + if (rproc->state == RPROC_OFFLINE && rproc->tee_interface)
>> + tee_rproc_release_fw(rproc);
>
> Function tee_rproc_release_fw() returns a value that is ignored. I don't know
> how it passes the Sparse checker but I already see patches coming in my Inbox to
> deal with that. In this case there is nothing else to do if there is an error
> releasing the firware. As such I would put a (void) in front and add a comment
> about the return value being ignore on purpose.
Instead of ignoring the error, I wonder if we should panic in
tee_rproc_release_fw(). Indeed, we would be in an unexpected state without any
possible action to return to a normal state.
Regards,
Arnaud
>
>> +
>> /* Free the copy of the resource table */
>> kfree(rproc->cached_table);
>> rproc->cached_table = NULL;
>> @@ -1348,7 +1352,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>> if (ret) {
>> dev_err(dev, "failed to prepare subdevices for %s: %d\n",
>> rproc->name, ret);
>> - goto reset_table_ptr;
>> + goto release_fw;
>> }
>>
>> /* power up the remote processor */
>> @@ -1376,7 +1380,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>> rproc->ops->stop(rproc);
>> unprepare_subdevices:
>> rproc_unprepare_subdevices(rproc);
>> -reset_table_ptr:
>> +release_fw:
>> + if (rproc->tee_interface)
>> + tee_rproc_release_fw(rproc);
>
> Same here.
>
>> rproc->table_ptr = rproc->cached_table;
>>
>> return ret;
>> --
>> 2.25.1
>>
On Tue, Sep 17, 2024 at 06:56:58PM +0200, Arnaud POULIQUEN wrote:
> Hello Mathieu,
>
> On 9/12/24 17:26, Mathieu Poirier wrote:
> > On Fri, Aug 30, 2024 at 11:51:44AM +0200, Arnaud Pouliquen wrote:
> >> Add support for releasing remote processor firmware through
> >> the Trusted Execution Environment (TEE) interface.
> >>
> >> The tee_rproc_release_fw() function is called in the following cases:
> >>
> >> - An error occurs in rproc_start() between the loading of the segments and
> >> the start of the remote processor.
> >> - When rproc_release_fw is called on error or after stopping the remote
> >> processor.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >> drivers/remoteproc/remoteproc_core.c | 10 ++++++++--
> >> 1 file changed, 8 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >> index 7694817f25d4..32052dedc149 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -29,6 +29,7 @@
> >> #include <linux/debugfs.h>
> >> #include <linux/rculist.h>
> >> #include <linux/remoteproc.h>
> >> +#include <linux/remoteproc_tee.h>
> >> #include <linux/iommu.h>
> >> #include <linux/idr.h>
> >> #include <linux/elf.h>
> >> @@ -1258,6 +1259,9 @@ static int rproc_alloc_registered_carveouts(struct rproc *rproc)
> >>
> >> static void rproc_release_fw(struct rproc *rproc)
> >> {
> >> + if (rproc->state == RPROC_OFFLINE && rproc->tee_interface)
> >> + tee_rproc_release_fw(rproc);
> >
> > Function tee_rproc_release_fw() returns a value that is ignored. I don't know
> > how it passes the Sparse checker but I already see patches coming in my Inbox to
> > deal with that. In this case there is nothing else to do if there is an error
> > releasing the firware. As such I would put a (void) in front and add a comment
> > about the return value being ignore on purpose.
>
> Instead of ignoring the error, I wonder if we should panic in
> tee_rproc_release_fw(). Indeed, we would be in an unexpected state without any
> possible action to return to a normal state.
Nowadays a call to panic() is only used in very dire situations and I don't see
this meeting that requirement. I would just call a dev_err() and let it be.
>
> Regards,
> Arnaud
>
> >
> >> +
> >> /* Free the copy of the resource table */
> >> kfree(rproc->cached_table);
> >> rproc->cached_table = NULL;
> >> @@ -1348,7 +1352,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> >> if (ret) {
> >> dev_err(dev, "failed to prepare subdevices for %s: %d\n",
> >> rproc->name, ret);
> >> - goto reset_table_ptr;
> >> + goto release_fw;
> >> }
> >>
> >> /* power up the remote processor */
> >> @@ -1376,7 +1380,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> >> rproc->ops->stop(rproc);
> >> unprepare_subdevices:
> >> rproc_unprepare_subdevices(rproc);
> >> -reset_table_ptr:
> >> +release_fw:
> >> + if (rproc->tee_interface)
> >> + tee_rproc_release_fw(rproc);
> >
> > Same here.
> >
> >> rproc->table_ptr = rproc->cached_table;
> >>
> >> return ret;
> >> --
> >> 2.25.1
> >>
On Fri, Aug 30, 2024 at 11:51:44AM +0200, Arnaud Pouliquen wrote:
> Add support for releasing remote processor firmware through
> the Trusted Execution Environment (TEE) interface.
>
> The tee_rproc_release_fw() function is called in the following cases:
>
> - An error occurs in rproc_start() between the loading of the segments and
> the start of the remote processor.
> - When rproc_release_fw is called on error or after stopping the remote
> processor.
>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
> drivers/remoteproc/remoteproc_core.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 7694817f25d4..32052dedc149 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -29,6 +29,7 @@
> #include <linux/debugfs.h>
> #include <linux/rculist.h>
> #include <linux/remoteproc.h>
> +#include <linux/remoteproc_tee.h>
> #include <linux/iommu.h>
> #include <linux/idr.h>
> #include <linux/elf.h>
> @@ -1258,6 +1259,9 @@ static int rproc_alloc_registered_carveouts(struct rproc *rproc)
>
> static void rproc_release_fw(struct rproc *rproc)
> {
> + if (rproc->state == RPROC_OFFLINE && rproc->tee_interface)
> + tee_rproc_release_fw(rproc);
> +
If I understand correctly, the first condition is there because the
attach/detach scenario does not yet support management by the TEE. I would
simply move the check to tee_rproc_release_fw() with a comment to that effect.
> /* Free the copy of the resource table */
> kfree(rproc->cached_table);
> rproc->cached_table = NULL;
> @@ -1348,7 +1352,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> if (ret) {
> dev_err(dev, "failed to prepare subdevices for %s: %d\n",
> rproc->name, ret);
> - goto reset_table_ptr;
> + goto release_fw;
> }
>
> /* power up the remote processor */
> @@ -1376,7 +1380,9 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
> rproc->ops->stop(rproc);
> unprepare_subdevices:
> rproc_unprepare_subdevices(rproc);
> -reset_table_ptr:
> +release_fw:
I would have kept the old label.
> + if (rproc->tee_interface)
> + tee_rproc_release_fw(rproc);
> rproc->table_ptr = rproc->cached_table;
>
> return ret;
> --
> 2.25.1
>
Hi Arnaud,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 5be63fc19fcaa4c236b307420483578a56986a37]
url: https://github.com/intel-lab-lkp/linux/commits/Arnaud-Pouliquen/remoteproc-core-Introduce-rproc_pa_to_va-helper/20240830-175609
base: 5be63fc19fcaa4c236b307420483578a56986a37
patch link: https://lore.kernel.org/r/20240830095147.3538047-5-arnaud.pouliquen%40foss.st.com
patch subject: [PATCH v9 4/7] remoteproc: core: Add TEE interface support for firmware release
config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20240901/202409010034.Tln3soEY-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240901/202409010034.Tln3soEY-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409010034.Tln3soEY-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/remoteproc/remoteproc_core.c:32:
>> include/linux/remoteproc_tee.h:52:12: warning: 'tee_rproc_parse_fw' defined but not used [-Wunused-function]
52 | static int tee_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
| ^~~~~~~~~~~~~~~~~~
vim +/tee_rproc_parse_fw +52 include/linux/remoteproc_tee.h
498143a453d14e Arnaud Pouliquen 2024-08-30 51
498143a453d14e Arnaud Pouliquen 2024-08-30 @52 static int tee_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
498143a453d14e Arnaud Pouliquen 2024-08-30 53 {
498143a453d14e Arnaud Pouliquen 2024-08-30 54 /* This shouldn't be possible */
498143a453d14e Arnaud Pouliquen 2024-08-30 55 WARN_ON(1);
498143a453d14e Arnaud Pouliquen 2024-08-30 56
498143a453d14e Arnaud Pouliquen 2024-08-30 57 return 0;
498143a453d14e Arnaud Pouliquen 2024-08-30 58 }
498143a453d14e Arnaud Pouliquen 2024-08-30 59
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 8/31/24 18:43, kernel test robot wrote:
> Hi Arnaud,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on 5be63fc19fcaa4c236b307420483578a56986a37]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Arnaud-Pouliquen/remoteproc-core-Introduce-rproc_pa_to_va-helper/20240830-175609
> base: 5be63fc19fcaa4c236b307420483578a56986a37
> patch link: https://lore.kernel.org/r/20240830095147.3538047-5-arnaud.pouliquen%40foss.st.com
> patch subject: [PATCH v9 4/7] remoteproc: core: Add TEE interface support for firmware release
> config: loongarch-allmodconfig (https://download.01.org/0day-ci/archive/20240901/202409010034.Tln3soEY-lkp@intel.com/config)
> compiler: loongarch64-linux-gcc (GCC) 14.1.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240901/202409010034.Tln3soEY-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202409010034.Tln3soEY-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
> In file included from drivers/remoteproc/remoteproc_core.c:32:
>>> include/linux/remoteproc_tee.h:52:12: warning: 'tee_rproc_parse_fw' defined but not used [-Wunused-function]
> 52 | static int tee_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> | ^~~~~~~~~~~~~~~~~~
>
>
> vim +/tee_rproc_parse_fw +52 include/linux/remoteproc_tee.h
>
> 498143a453d14e Arnaud Pouliquen 2024-08-30 51
> 498143a453d14e Arnaud Pouliquen 2024-08-30 @52 static int tee_rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> 498143a453d14e Arnaud Pouliquen 2024-08-30 53 {
> 498143a453d14e Arnaud Pouliquen 2024-08-30 54 /* This shouldn't be possible */
> 498143a453d14e Arnaud Pouliquen 2024-08-30 55 WARN_ON(1);
> 498143a453d14e Arnaud Pouliquen 2024-08-30 56
> 498143a453d14e Arnaud Pouliquen 2024-08-30 57 return 0;
> 498143a453d14e Arnaud Pouliquen 2024-08-30 58 }
> 498143a453d14e Arnaud Pouliquen 2024-08-30 59
>
The inline attribute is missing. As it is a minor fix, I'm waiting for more
reviews before fixing it in the next version.
Regards,
Arnaud
© 2016 - 2025 Red Hat, Inc.