| Message ID | 20210310211025.1084636-13-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: Add support for detaching a remote processor | expand |
Hi Mathieu,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.12-rc2 next-20210310]
[cannot apply to remoteproc/for-next rpmsg/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Mathieu-Poirier/remoteproc-Add-support-for-detaching-a-remote-processor/20210311-051242
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 144c79ef33536b4ecb4951e07dbc1f2b7fa99d32
config: arm-randconfig-r036-20210310 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/6715f51e2b7ade7b5121eced81cad8065d4f5525
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Mathieu-Poirier/remoteproc-Add-support-for-detaching-a-remote-processor/20210311-051242
git checkout 6715f51e2b7ade7b5121eced81cad8065d4f5525
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/remoteproc/remoteproc_core.c: In function 'rproc_reset_rsc_table_on_stop':
>> drivers/remoteproc/remoteproc_core.c:1639:25: warning: variable 'table_ptr' set but not used [-Wunused-but-set-variable]
1639 | struct resource_table *table_ptr;
| ^~~~~~~~~
vim +/table_ptr +1639 drivers/remoteproc/remoteproc_core.c
1636
1637 static int rproc_reset_rsc_table_on_stop(struct rproc *rproc)
1638 {
> 1639 struct resource_table *table_ptr;
1640
1641 /* A resource table was never retrieved, nothing to do here */
1642 if (!rproc->table_ptr)
1643 return 0;
1644
1645 /*
1646 * If a cache table exists the remote processor was started by
1647 * the remoteproc core. That cache table should be used for
1648 * the rest of the shutdown process.
1649 */
1650 if (rproc->cached_table)
1651 goto out;
1652
1653 /* Remember where the external entity installed the resource table */
1654 table_ptr = rproc->table_ptr;
1655
1656 /*
1657 * If we made it here the remote processor was started by another
1658 * entity and a cache table doesn't exist. As such make a copy of
1659 * the resource table currently used by the remote processor and
1660 * use that for the rest of the shutdown process. The memory
1661 * allocated here is free'd in rproc_shutdown().
1662 */
1663 rproc->cached_table = kmemdup(rproc->table_ptr,
1664 rproc->table_sz, GFP_KERNEL);
1665 if (!rproc->cached_table)
1666 return -ENOMEM;
1667
1668 /*
1669 * Since the remote processor is being switched off the clean table
1670 * won't be needed. Allocated in rproc_set_rsc_table().
1671 */
1672 kfree(rproc->clean_table);
1673
1674 out:
1675 /*
1676 * Use a copy of the resource table for the remainder of the
1677 * shutdown process.
1678 */
1679 rproc->table_ptr = rproc->cached_table;
1680 return 0;
1681 }
1682
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed 10 Mar 15:10 CST 2021, Mathieu Poirier wrote: > When a remote processor that was attached to is stopped, special care > must be taken to make sure the shutdown process is similar to what > it would be had it been started by the remoteproc core. > > This patch takes care of that by making a copy of the resource > table currently used by the remote processor. From that point on > the copy is used, as if the remote processor had been started by > the remoteproc core. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > --- > New for V7: > New Patch, used to be part of 11/16 in V6. > --- > drivers/remoteproc/remoteproc_core.c | 53 +++++++++++++++++++++++++++- > 1 file changed, 52 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index e9ea2558432d..c488b1aa6119 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1634,6 +1634,52 @@ static int rproc_reset_rsc_table_on_detach(struct rproc *rproc) > return 0; > } > > +static int rproc_reset_rsc_table_on_stop(struct rproc *rproc) > +{ > + struct resource_table *table_ptr; > + > + /* A resource table was never retrieved, nothing to do here */ > + if (!rproc->table_ptr) > + return 0; > + > + /* > + * If a cache table exists the remote processor was started by > + * the remoteproc core. That cache table should be used for > + * the rest of the shutdown process. > + */ > + if (rproc->cached_table) > + goto out; > + > + /* Remember where the external entity installed the resource table */ > + table_ptr = rproc->table_ptr; > + Afaict this is just a remnant from the detach case. I think the series looks really good now, please let me know and I can drop the local "table_ptr" as I apply the patches. Regards, Bjorn > + /* > + * If we made it here the remote processor was started by another > + * entity and a cache table doesn't exist. As such make a copy of > + * the resource table currently used by the remote processor and > + * use that for the rest of the shutdown process. The memory > + * allocated here is free'd in rproc_shutdown(). > + */ > + rproc->cached_table = kmemdup(rproc->table_ptr, > + rproc->table_sz, GFP_KERNEL); > + if (!rproc->cached_table) > + return -ENOMEM; > + > + /* > + * Since the remote processor is being switched off the clean table > + * won't be needed. Allocated in rproc_set_rsc_table(). > + */ > + kfree(rproc->clean_table); > + > +out: > + /* > + * Use a copy of the resource table for the remainder of the > + * shutdown process. > + */ > + rproc->table_ptr = rproc->cached_table; > + return 0; > +} > + > /* > * Attach to remote processor - similar to rproc_fw_boot() but without > * the steps that deal with the firmware image. > @@ -1759,7 +1805,12 @@ static int rproc_stop(struct rproc *rproc, bool crashed) > rproc_stop_subdevices(rproc, crashed); > > /* the installed resource table is no longer accessible */ > - rproc->table_ptr = rproc->cached_table; > + ret = rproc_reset_rsc_table_on_stop(rproc); > + if (ret) { > + dev_err(dev, "can't reset resource table: %d\n", ret); > + return ret; > + } > + > > /* power off the remote processor */ > ret = rproc->ops->stop(rproc); > -- > 2.25.1 >
On 3/11/21 12:53 AM, Bjorn Andersson wrote: > On Wed 10 Mar 15:10 CST 2021, Mathieu Poirier wrote: > >> When a remote processor that was attached to is stopped, special care >> must be taken to make sure the shutdown process is similar to what >> it would be had it been started by the remoteproc core. >> >> This patch takes care of that by making a copy of the resource >> table currently used by the remote processor. From that point on >> the copy is used, as if the remote processor had been started by >> the remoteproc core. >> >> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> >> --- >> New for V7: >> New Patch, used to be part of 11/16 in V6. >> --- >> drivers/remoteproc/remoteproc_core.c | 53 +++++++++++++++++++++++++++- >> 1 file changed, 52 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index e9ea2558432d..c488b1aa6119 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1634,6 +1634,52 @@ static int rproc_reset_rsc_table_on_detach(struct rproc *rproc) >> return 0; >> } >> >> +static int rproc_reset_rsc_table_on_stop(struct rproc *rproc) >> +{ >> + struct resource_table *table_ptr; >> + >> + /* A resource table was never retrieved, nothing to do here */ >> + if (!rproc->table_ptr) >> + return 0; >> + >> + /* >> + * If a cache table exists the remote processor was started by >> + * the remoteproc core. That cache table should be used for >> + * the rest of the shutdown process. >> + */ >> + if (rproc->cached_table) >> + goto out; >> + >> + /* Remember where the external entity installed the resource table */ >> + table_ptr = rproc->table_ptr; >> + > > Afaict this is just a remnant from the detach case. > > I think the series looks really good now, please let me know and I can > drop the local "table_ptr" as I apply the patches. > Just a minor comment on patch 11, then the series LGTM also, For this one Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> Thanks, Arnaud > Regards, > Bjorn > >> + /* >> + * If we made it here the remote processor was started by another >> + * entity and a cache table doesn't exist. As such make a copy of >> + * the resource table currently used by the remote processor and >> + * use that for the rest of the shutdown process. The memory >> + * allocated here is free'd in rproc_shutdown(). >> + */ >> + rproc->cached_table = kmemdup(rproc->table_ptr, >> + rproc->table_sz, GFP_KERNEL); >> + if (!rproc->cached_table) >> + return -ENOMEM; >> + >> + /* >> + * Since the remote processor is being switched off the clean table >> + * won't be needed. Allocated in rproc_set_rsc_table(). >> + */ >> + kfree(rproc->clean_table); >> + >> +out: >> + /* >> + * Use a copy of the resource table for the remainder of the >> + * shutdown process. >> + */ >> + rproc->table_ptr = rproc->cached_table; >> + return 0; >> +} >> + >> /* >> * Attach to remote processor - similar to rproc_fw_boot() but without >> * the steps that deal with the firmware image. >> @@ -1759,7 +1805,12 @@ static int rproc_stop(struct rproc *rproc, bool crashed) >> rproc_stop_subdevices(rproc, crashed); >> >> /* the installed resource table is no longer accessible */ >> - rproc->table_ptr = rproc->cached_table; >> + ret = rproc_reset_rsc_table_on_stop(rproc); >> + if (ret) { >> + dev_err(dev, "can't reset resource table: %d\n", ret); >> + return ret; >> + } >> + >> >> /* power off the remote processor */ >> ret = rproc->ops->stop(rproc); >> -- >> 2.25.1 >>
On Wed, Mar 10, 2021 at 05:53:04PM -0600, Bjorn Andersson wrote: > On Wed 10 Mar 15:10 CST 2021, Mathieu Poirier wrote: > > > When a remote processor that was attached to is stopped, special care > > must be taken to make sure the shutdown process is similar to what > > it would be had it been started by the remoteproc core. > > > > This patch takes care of that by making a copy of the resource > > table currently used by the remote processor. From that point on > > the copy is used, as if the remote processor had been started by > > the remoteproc core. > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > --- > > New for V7: > > New Patch, used to be part of 11/16 in V6. > > --- > > drivers/remoteproc/remoteproc_core.c | 53 +++++++++++++++++++++++++++- > > 1 file changed, 52 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > index e9ea2558432d..c488b1aa6119 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -1634,6 +1634,52 @@ static int rproc_reset_rsc_table_on_detach(struct rproc *rproc) > > return 0; > > } > > > > +static int rproc_reset_rsc_table_on_stop(struct rproc *rproc) > > +{ > > + struct resource_table *table_ptr; > > + > > + /* A resource table was never retrieved, nothing to do here */ > > + if (!rproc->table_ptr) > > + return 0; > > + > > + /* > > + * If a cache table exists the remote processor was started by > > + * the remoteproc core. That cache table should be used for > > + * the rest of the shutdown process. > > + */ > > + if (rproc->cached_table) > > + goto out; > > + > > + /* Remember where the external entity installed the resource table */ > > + table_ptr = rproc->table_ptr; > > + > > Afaict this is just a remnant from the detach case. > > I think the series looks really good now, please let me know and I can > drop the local "table_ptr" as I apply the patches. (sigh) No matter how long you stare at a patchset there is always one that gets away. I will address Arnaud's comment and fix this in a new revision. Thanks, Mathieu > > Regards, > Bjorn > > > + /* > > + * If we made it here the remote processor was started by another > > + * entity and a cache table doesn't exist. As such make a copy of > > + * the resource table currently used by the remote processor and > > + * use that for the rest of the shutdown process. The memory > > + * allocated here is free'd in rproc_shutdown(). > > + */ > > + rproc->cached_table = kmemdup(rproc->table_ptr, > > + rproc->table_sz, GFP_KERNEL); > > + if (!rproc->cached_table) > > + return -ENOMEM; > > + > > + /* > > + * Since the remote processor is being switched off the clean table > > + * won't be needed. Allocated in rproc_set_rsc_table(). > > + */ > > + kfree(rproc->clean_table); > > + > > +out: > > + /* > > + * Use a copy of the resource table for the remainder of the > > + * shutdown process. > > + */ > > + rproc->table_ptr = rproc->cached_table; > > + return 0; > > +} > > + > > /* > > * Attach to remote processor - similar to rproc_fw_boot() but without > > * the steps that deal with the firmware image. > > @@ -1759,7 +1805,12 @@ static int rproc_stop(struct rproc *rproc, bool crashed) > > rproc_stop_subdevices(rproc, crashed); > > > > /* the installed resource table is no longer accessible */ > > - rproc->table_ptr = rproc->cached_table; > > + ret = rproc_reset_rsc_table_on_stop(rproc); > > + if (ret) { > > + dev_err(dev, "can't reset resource table: %d\n", ret); > > + return ret; > > + } > > + > > > > /* power off the remote processor */ > > ret = rproc->ops->stop(rproc); > > -- > > 2.25.1 > >
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index e9ea2558432d..c488b1aa6119 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1634,6 +1634,52 @@ static int rproc_reset_rsc_table_on_detach(struct rproc *rproc) return 0; } +static int rproc_reset_rsc_table_on_stop(struct rproc *rproc) +{ + struct resource_table *table_ptr; + + /* A resource table was never retrieved, nothing to do here */ + if (!rproc->table_ptr) + return 0; + + /* + * If a cache table exists the remote processor was started by + * the remoteproc core. That cache table should be used for + * the rest of the shutdown process. + */ + if (rproc->cached_table) + goto out; + + /* Remember where the external entity installed the resource table */ + table_ptr = rproc->table_ptr; + + /* + * If we made it here the remote processor was started by another + * entity and a cache table doesn't exist. As such make a copy of + * the resource table currently used by the remote processor and + * use that for the rest of the shutdown process. The memory + * allocated here is free'd in rproc_shutdown(). + */ + rproc->cached_table = kmemdup(rproc->table_ptr, + rproc->table_sz, GFP_KERNEL); + if (!rproc->cached_table) + return -ENOMEM; + + /* + * Since the remote processor is being switched off the clean table + * won't be needed. Allocated in rproc_set_rsc_table(). + */ + kfree(rproc->clean_table); + +out: + /* + * Use a copy of the resource table for the remainder of the + * shutdown process. + */ + rproc->table_ptr = rproc->cached_table; + return 0; +} + /* * Attach to remote processor - similar to rproc_fw_boot() but without * the steps that deal with the firmware image. @@ -1759,7 +1805,12 @@ static int rproc_stop(struct rproc *rproc, bool crashed) rproc_stop_subdevices(rproc, crashed); /* the installed resource table is no longer accessible */ - rproc->table_ptr = rproc->cached_table; + ret = rproc_reset_rsc_table_on_stop(rproc); + if (ret) { + dev_err(dev, "can't reset resource table: %d\n", ret); + return ret; + } + /* power off the remote processor */ ret = rproc->ops->stop(rproc);
When a remote processor that was attached to is stopped, special care must be taken to make sure the shutdown process is similar to what it would be had it been started by the remoteproc core. This patch takes care of that by making a copy of the resource table currently used by the remote processor. From that point on the copy is used, as if the remote processor had been started by the remoteproc core. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- New for V7: New Patch, used to be part of 11/16 in V6. --- drivers/remoteproc/remoteproc_core.c | 53 +++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-)