| Message ID | 20240830095147.3538047-5-arnaud.pouliquen@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduction of a remoteproc tee to load signed firmware | expand |
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
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
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 >
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 >>
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 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 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 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 >>
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;
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(-)