Message ID | 20240830095147.3538047-5-arnaud.pouliquen@foss.st.com (mailing list archive) |
---|---|
State | New |
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;
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(-)