diff mbox series

[v7,12/17] remoteproc: Properly deal with the resource table when stopping

Message ID 20210310211025.1084636-13-mathieu.poirier@linaro.org (mailing list archive)
State New, archived
Headers show
Series remoteproc: Add support for detaching a remote processor | expand

Commit Message

Mathieu Poirier March 10, 2021, 9:10 p.m. UTC 
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(-)

Comments

kernel test robot March 10, 2021, 11 p.m. UTC | #1 
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
Bjorn Andersson March 10, 2021, 11:53 p.m. UTC | #2 
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
>
Arnaud POULIQUEN March 11, 2021, 9:14 a.m. UTC | #3 
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
>>
Mathieu Poirier March 11, 2021, 5:13 p.m. UTC | #4 
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 mbox series

Patch

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);