This patch enhances rproc_put() to support remoteproc clusters
with multiple child nodes as in rproc_get_by_phandle().
Signed-off-by: Tarak Reddy <[email protected]>
Signed-off-by: Tanmay Shah <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index a3e7c8798381..e7e451012615 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2560,6 +2560,19 @@ EXPORT_SYMBOL(rproc_free);
void rproc_put(struct rproc *rproc)
{
module_put(rproc->dev.parent->driver->owner);
+ struct platform_device *cluster_pdev;
+
+ if (rproc->dev.parent) {
+ if (rproc->dev.parent->driver) {
+ module_put(rproc->dev.parent->driver->owner);
+ } else {
+ cluster_pdev = of_find_device_by_node(rproc->dev.parent->of_node->parent);
+ if (cluster_pdev) {
+ module_put(cluster_pdev->dev.driver->owner);
+ put_device(&cluster_pdev->dev);
+ }
+ }
+ }
put_device(&rproc->dev);
}
EXPORT_SYMBOL(rproc_put);
--
2.25.1
Hi Tanmay,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on e19967994d342a5986d950a1bfddf19d7e1191b7]
url: https://github.com/intel-lab-lkp/linux/commits/Tanmay-Shah/remoteproc-Make-rproc_get_by_phandle-work-for-clusters/20230322-121102
base: e19967994d342a5986d950a1bfddf19d7e1191b7
patch link: https://lore.kernel.org/r/20230322040933.924813-3-tanmay.shah%40amd.com
patch subject: [PATCH v2 2/2] remoteproc: enhance rproc_put() for clusters
config: arm-randconfig-r013-20230322 (https://download.01.org/0day-ci/archive/20230322/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
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
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/573d22d13a697097d02d6c29a75fb0fb1ac6d8fe
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Tanmay-Shah/remoteproc-Make-rproc_get_by_phandle-work-for-clusters/20230322-121102
git checkout 573d22d13a697097d02d6c29a75fb0fb1ac6d8fe
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/remoteproc/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/remoteproc/remoteproc_core.c:2563:26: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
struct platform_device *cluster_pdev;
^
1 warning generated.
vim +2563 drivers/remoteproc/remoteproc_core.c
2550
2551 /**
2552 * rproc_put() - release rproc reference
2553 * @rproc: the remote processor handle
2554 *
2555 * This function decrements the rproc dev refcount.
2556 *
2557 * If no one holds any reference to rproc anymore, then its refcount would
2558 * now drop to zero, and it would be freed.
2559 */
2560 void rproc_put(struct rproc *rproc)
2561 {
2562 module_put(rproc->dev.parent->driver->owner);
> 2563 struct platform_device *cluster_pdev;
2564
2565 if (rproc->dev.parent) {
2566 if (rproc->dev.parent->driver) {
2567 module_put(rproc->dev.parent->driver->owner);
2568 } else {
2569 cluster_pdev = of_find_device_by_node(rproc->dev.parent->of_node->parent);
2570 if (cluster_pdev) {
2571 module_put(cluster_pdev->dev.driver->owner);
2572 put_device(&cluster_pdev->dev);
2573 }
2574 }
2575 }
2576 put_device(&rproc->dev);
2577 }
2578 EXPORT_SYMBOL(rproc_put);
2579
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Tanmay,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on e19967994d342a5986d950a1bfddf19d7e1191b7]
url: https://github.com/intel-lab-lkp/linux/commits/Tanmay-Shah/remoteproc-Make-rproc_get_by_phandle-work-for-clusters/20230322-121102
base: e19967994d342a5986d950a1bfddf19d7e1191b7
patch link: https://lore.kernel.org/r/20230322040933.924813-3-tanmay.shah%40amd.com
patch subject: [PATCH v2 2/2] remoteproc: enhance rproc_put() for clusters
config: ia64-allyesconfig (https://download.01.org/0day-ci/archive/20230322/[email protected]/config)
compiler: ia64-linux-gcc (GCC) 12.1.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/intel-lab-lkp/linux/commit/573d22d13a697097d02d6c29a75fb0fb1ac6d8fe
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Tanmay-Shah/remoteproc-Make-rproc_get_by_phandle-work-for-clusters/20230322-121102
git checkout 573d22d13a697097d02d6c29a75fb0fb1ac6d8fe
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/remoteproc/remoteproc_core.c: In function 'rproc_put':
>> drivers/remoteproc/remoteproc_core.c:2563:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
2563 | struct platform_device *cluster_pdev;
| ^~~~~~
vim +2563 drivers/remoteproc/remoteproc_core.c
2550
2551 /**
2552 * rproc_put() - release rproc reference
2553 * @rproc: the remote processor handle
2554 *
2555 * This function decrements the rproc dev refcount.
2556 *
2557 * If no one holds any reference to rproc anymore, then its refcount would
2558 * now drop to zero, and it would be freed.
2559 */
2560 void rproc_put(struct rproc *rproc)
2561 {
2562 module_put(rproc->dev.parent->driver->owner);
> 2563 struct platform_device *cluster_pdev;
2564
2565 if (rproc->dev.parent) {
2566 if (rproc->dev.parent->driver) {
2567 module_put(rproc->dev.parent->driver->owner);
2568 } else {
2569 cluster_pdev = of_find_device_by_node(rproc->dev.parent->of_node->parent);
2570 if (cluster_pdev) {
2571 module_put(cluster_pdev->dev.driver->owner);
2572 put_device(&cluster_pdev->dev);
2573 }
2574 }
2575 }
2576 put_device(&rproc->dev);
2577 }
2578 EXPORT_SYMBOL(rproc_put);
2579
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Tanmay,
url: https://github.com/intel-lab-lkp/linux/commits/Tanmay-Shah/remoteproc-Make-rproc_get_by_phandle-work-for-clusters/20230322-121102
base: e19967994d342a5986d950a1bfddf19d7e1191b7
patch link: https://lore.kernel.org/r/20230322040933.924813-3-tanmay.shah%40amd.com
patch subject: [PATCH v2 2/2] remoteproc: enhance rproc_put() for clusters
config: powerpc-randconfig-m041-20230322 (https://download.01.org/0day-ci/archive/20230322/[email protected]/config)
compiler: powerpc-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Link: https://lore.kernel.org/r/[email protected]/
smatch warnings:
drivers/remoteproc/remoteproc_core.c:2565 rproc_put() warn: variable dereferenced before check 'rproc->dev.parent' (see line 2562)
drivers/remoteproc/remoteproc_core.c:2566 rproc_put() warn: variable dereferenced before check 'rproc->dev.parent->driver' (see line 2562)
vim +2565 drivers/remoteproc/remoteproc_core.c
160e7c840fe858 Ohad Ben-Cohen 2012-07-04 2560 void rproc_put(struct rproc *rproc)
400e64df6b237e Ohad Ben-Cohen 2011-10-20 2561 {
fbb6aacb078285 Bjorn Andersson 2016-10-02 @2562 module_put(rproc->dev.parent->driver->owner);
^^^^^^^^^^^^^^^^^^^^^^^^^^^
Unchecked dereferences.
573d22d13a6970 Tanmay Shah 2023-03-21 2563 struct platform_device *cluster_pdev;
573d22d13a6970 Tanmay Shah 2023-03-21 2564
573d22d13a6970 Tanmay Shah 2023-03-21 @2565 if (rproc->dev.parent) {
^^^^^^^^^^^^^^^^^
Checked too late.
573d22d13a6970 Tanmay Shah 2023-03-21 @2566 if (rproc->dev.parent->driver) {
^^^^^^^^^^^^^^^^^^^^^^^^^
573d22d13a6970 Tanmay Shah 2023-03-21 2567 module_put(rproc->dev.parent->driver->owner);
573d22d13a6970 Tanmay Shah 2023-03-21 2568 } else {
573d22d13a6970 Tanmay Shah 2023-03-21 2569 cluster_pdev = of_find_device_by_node(rproc->dev.parent->of_node->parent);
573d22d13a6970 Tanmay Shah 2023-03-21 2570 if (cluster_pdev) {
573d22d13a6970 Tanmay Shah 2023-03-21 2571 module_put(cluster_pdev->dev.driver->owner);
573d22d13a6970 Tanmay Shah 2023-03-21 2572 put_device(&cluster_pdev->dev);
573d22d13a6970 Tanmay Shah 2023-03-21 2573 }
573d22d13a6970 Tanmay Shah 2023-03-21 2574 }
573d22d13a6970 Tanmay Shah 2023-03-21 2575 }
b5ab5e24e960b9 Ohad Ben-Cohen 2012-05-30 2576 put_device(&rproc->dev);
400e64df6b237e Ohad Ben-Cohen 2011-10-20 2577 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Tanmay,
On Tue, Mar 21, 2023 at 09:09:36PM -0700, Tanmay Shah wrote:
> This patch enhances rproc_put() to support remoteproc clusters
> with multiple child nodes as in rproc_get_by_phandle().
>
> Signed-off-by: Tarak Reddy <[email protected]>
> Signed-off-by: Tanmay Shah <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index a3e7c8798381..e7e451012615 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2560,6 +2560,19 @@ EXPORT_SYMBOL(rproc_free);
> void rproc_put(struct rproc *rproc)
> {
> module_put(rproc->dev.parent->driver->owner);
There is something wrong here - this should have been removed.
> + struct platform_device *cluster_pdev;
> +
> + if (rproc->dev.parent) {
This condition is not needed, please remove.
> + if (rproc->dev.parent->driver) {
> + module_put(rproc->dev.parent->driver->owner);
> + } else {
> + cluster_pdev = of_find_device_by_node(rproc->dev.parent->of_node->parent);
> + if (cluster_pdev) {
> + module_put(cluster_pdev->dev.driver->owner);
> + put_device(&cluster_pdev->dev);
> + }
> + }
> + }
Some in-lined documentation, the way I did in patch 1/2 would be appreciated.
Otherwize I think the above enhancement make sense.
Thanks,
Mathieu
> put_device(&rproc->dev);
> }
> EXPORT_SYMBOL(rproc_put);
> --
> 2.25.1
>
On 3/22/23 9:05 AM, Mathieu Poirier wrote:
> Hi Tanmay,
>
> On Tue, Mar 21, 2023 at 09:09:36PM -0700, Tanmay Shah wrote:
>> This patch enhances rproc_put() to support remoteproc clusters
>> with multiple child nodes as in rproc_get_by_phandle().
>>
>> Signed-off-by: Tarak Reddy <[email protected]>
>> Signed-off-by: Tanmay Shah <[email protected]>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index a3e7c8798381..e7e451012615 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -2560,6 +2560,19 @@ EXPORT_SYMBOL(rproc_free);
>> void rproc_put(struct rproc *rproc)
>> {
>> module_put(rproc->dev.parent->driver->owner);
> There is something wrong here - this should have been removed.
Thanks Mathieu. Sure this needs to be fixed.
This is result of manually picking up patch from my side.
I will try to find better automated way to pick-up patches not available
on mailing list.
>
>> + struct platform_device *cluster_pdev;
>> +
>> + if (rproc->dev.parent) {
> This condition is not needed, please remove.
Ack.
>
>> + if (rproc->dev.parent->driver) {
>> + module_put(rproc->dev.parent->driver->owner);
>> + } else {
>> + cluster_pdev = of_find_device_by_node(rproc->dev.parent->of_node->parent);
>> + if (cluster_pdev) {
>> + module_put(cluster_pdev->dev.driver->owner);
>> + put_device(&cluster_pdev->dev);
I am not sure if cluster_pdev->dev should be dropped here.
Should we drop it in platform driver after rproc_free() ?
>> + }
>> + }
>> + }
> Some in-lined documentation, the way I did in patch 1/2 would be appreciated.
> Otherwize I think the above enhancement make sense.
Ack I will document in next revision.
>
> Thanks,
> Mathieu
>
>> put_device(&rproc->dev);
Also, if we decide to drop cluster->dev here then,
should we drop reference of rproc->dev before cluster->dev ?
>> }
>> EXPORT_SYMBOL(rproc_put);
>> --
>> 2.25.1
>>
On Wed, Mar 22, 2023 at 10:34:57AM -0700, Tanmay Shah wrote:
>
> On 3/22/23 9:05 AM, Mathieu Poirier wrote:
> > Hi Tanmay,
> >
> > On Tue, Mar 21, 2023 at 09:09:36PM -0700, Tanmay Shah wrote:
> > > This patch enhances rproc_put() to support remoteproc clusters
> > > with multiple child nodes as in rproc_get_by_phandle().
> > >
> > > Signed-off-by: Tarak Reddy <[email protected]>
> > > Signed-off-by: Tanmay Shah <[email protected]>
> > > ---
> > > drivers/remoteproc/remoteproc_core.c | 13 +++++++++++++
> > > 1 file changed, 13 insertions(+)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > > index a3e7c8798381..e7e451012615 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -2560,6 +2560,19 @@ EXPORT_SYMBOL(rproc_free);
> > > void rproc_put(struct rproc *rproc)
> > > {
> > > module_put(rproc->dev.parent->driver->owner);
> > There is something wrong here - this should have been removed.
>
>
> Thanks Mathieu. Sure this needs to be fixed.
>
> This is result of manually picking up patch from my side.
>
> I will try to find better automated way to pick-up patches not available on
> mailing list.
>
That would certainly help avoid problems such as this one.
>
> >
> > > + struct platform_device *cluster_pdev;
> > > +
> > > + if (rproc->dev.parent) {
> > This condition is not needed, please remove.
> Ack.
> >
> > > + if (rproc->dev.parent->driver) {
> > > + module_put(rproc->dev.parent->driver->owner);
> > > + } else {
> > > + cluster_pdev = of_find_device_by_node(rproc->dev.parent->of_node->parent);
> > > + if (cluster_pdev) {
> > > + module_put(cluster_pdev->dev.driver->owner);
> > > + put_device(&cluster_pdev->dev);
>
> I am not sure if cluster_pdev->dev should be dropped here.
>
It needs to be done here.
> Should we drop it in platform driver after rproc_free() ?
>
> > > + }
> > > + }
> > > + }
> > Some in-lined documentation, the way I did in patch 1/2 would be appreciated.
> > Otherwize I think the above enhancement make sense.
> Ack I will document in next revision.
> >
> > Thanks,
> > Mathieu
> >
> > > put_device(&rproc->dev);
>
>
> Also, if we decide to drop cluster->dev here? then,
>
> should we drop reference of rproc->dev before cluster->dev ?
>
>
> > > }
> > > EXPORT_SYMBOL(rproc_put);
> > > --
> > > 2.25.1
> > >