| Message ID | 20230322040933.924813-3-tanmay.shah@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: get rproc devices for clusters | expand |
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
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
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 }
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 >
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 >>
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 --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);