diff mbox series

[v2,2/2] remoteproc: enhance rproc_put() for clusters

Message ID	20230322040933.924813-3-tanmay.shah@amd.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Received-SPF: Pass (protection.outlook.com: domain of amd.com designates 165.204.84.17 as permitted sender) receiver=protection.outlook.com; client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C From: Tanmay Shah <tanmay.shah@amd.com> To: <andersson@kernel.org>, <mathieu.poirier@linaro.org> CC: <linux-remoteproc@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Tanmay Shah <tanmay.shah@amd.com>, Tarak Reddy <tarak.reddy@amd.com> Subject: [PATCH v2 2/2] remoteproc: enhance rproc_put() for clusters Date: Tue, 21 Mar 2023 21:09:36 -0700 Message-ID: <20230322040933.924813-3-tanmay.shah@amd.com> In-Reply-To: <20230322040933.924813-1-tanmay.shah@amd.com> References: <20230322040933.924813-1-tanmay.shah@amd.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain Precedence: bulk
Series	remoteproc: get rproc devices for clusters \| [v2,0/2] remoteproc: get rproc devices for clusters [v2,1/2] remoteproc: Make rproc_get_by_phandle() work for clusters [v2,2/2] remoteproc: enhance rproc_put() for clusters

Commit Message

Tanmay Shah March 22, 2023, 4:09 a.m. UTC

  This patch enhances rproc_put() to support remoteproc clusters
with multiple child nodes as in rproc_get_by_phandle().

Signed-off-by: Tarak Reddy <tarak.reddy@amd.com>
Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---
 drivers/remoteproc/remoteproc_core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

kernel test robot March 22, 2023, 6:20 a.m. UTC | #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/202303221441.cuBnpvye-lkp@intel.com/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 <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303221441.cuBnpvye-lkp@intel.com/

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

kernel test robot March 22, 2023, 7:31 a.m. UTC | #2

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/202303221514.3xIiCbpk-lkp@intel.com/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 <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303221514.3xIiCbpk-lkp@intel.com/

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

Dan Carpenter March 22, 2023, 11:52 a.m. UTC | #3

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/202303221916.LgKkr8Gk-lkp@intel.com/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 <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202303221916.LgKkr8Gk-lkp@intel.com/

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  }

Mathieu Poirier March 22, 2023, 4:05 p.m. UTC | #4

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 <tarak.reddy@amd.com>
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
>  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
>

Tanmay Shah March 22, 2023, 5:34 p.m. UTC | #5

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 <tarak.reddy@amd.com>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>   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
>>

Mathieu Poirier March 23, 2023, 10:45 p.m. UTC | #6

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 <tarak.reddy@amd.com>
> > > Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> > > ---
> > >   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
> > >

diff mbox series

Patch

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