[06/18] software_node: amend software_node_unregister_node_group() to perform unregistration of array in reverse order to be consistent with software_node_unregister_nodes()

Message ID 20201130133129.1024662-7-djrscally@gmail.com
State New
Headers show
Series
  • Add functionality to ipu3-cio2 driver allowing software_node connections to sensors on platforms designed for Windows
Related show

Commit Message

Daniel Scally Nov. 30, 2020, 1:31 p.m.
To maintain consistency with software_node_unregister_nodes(), reverse
the order in which the software_node_unregister_node_group() function
unregisters nodes.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes since v3:

	Patch introduced

 drivers/base/swnode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart Nov. 30, 2020, 4:17 p.m. | #1
Hi Daniel,

Thank you for the patch.

The subject line is very long. We try to keep it within a 72 characters
limit in the kernel. That can be a challenge sometimes, and expections
can be accepted, but this one is reaaaally long.

(The same comment holds for other patches in the series)

On Mon, Nov 30, 2020 at 01:31:17PM +0000, Daniel Scally wrote:
> To maintain consistency with software_node_unregister_nodes(), reverse
> the order in which the software_node_unregister_node_group() function
> unregisters nodes.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>

I"d squash this with the previous patch to avoid introducing an
inconsistency.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Changes since v3:
> 
> 	Patch introduced
> 
>  drivers/base/swnode.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index d39e1c76d98d..9bd0bb77ad5b 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -782,7 +782,10 @@ void software_node_unregister_node_group(const struct software_node **node_group
>  	if (!node_group)
>  		return;
>  
> -	for (i = 0; node_group[i]; i++)
> +	while (node_group[i]->name)
> +		i++;
> +
> +	while (i--)
>  		software_node_unregister(node_group[i]);
>  }
>  EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
kernel test robot Nov. 30, 2020, 5:19 p.m. | #2
Hi Daniel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on driver-core/driver-core-testing pm/linux-next v5.10-rc6 next-20201130]
[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/Daniel-Scally/Add-functionality-to-ipu3-cio2-driver-allowing-software_node-connections-to-sensors-on-platforms-designed-for-Windows/20201130-214014
base:   git://linuxtv.org/media_tree.git master
config: powerpc-randconfig-r031-20201130 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project dfcf1acf13226be0f599a7ab6b395b66dc9545d6)
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 powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/0day-ci/linux/commit/7c7577c82672f0a0775ac1ad85358e2dc17b2c91
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Scally/Add-functionality-to-ipu3-cio2-driver-allowing-software_node-connections-to-sensors-on-platforms-designed-for-Windows/20201130-214014
        git checkout 7c7577c82672f0a0775ac1ad85358e2dc17b2c91
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc 

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/base/swnode.c:785:20: warning: variable 'i' is uninitialized when used here [-Wuninitialized]
           while (node_group[i]->name)
                             ^
   drivers/base/swnode.c:780:16: note: initialize the variable 'i' to silence this warning
           unsigned int i;
                         ^
                          = 0
   1 warning generated.

vim +/i +785 drivers/base/swnode.c

   771	
   772	/**
   773	 * software_node_unregister_node_group - Unregister a group of software nodes
   774	 * @node_group: NULL terminated array of software node pointers to be unregistered
   775	 *
   776	 * Unregister multiple software nodes at once.
   777	 */
   778	void software_node_unregister_node_group(const struct software_node **node_group)
   779	{
   780		unsigned int i;
   781	
   782		if (!node_group)
   783			return;
   784	
 > 785		while (node_group[i]->name)
   786			i++;
   787	
   788		while (i--)
   789			software_node_unregister(node_group[i]);
   790	}
   791	EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
   792	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Dan Carpenter Dec. 1, 2020, 6:18 p.m. | #3
Hi Daniel,

url:    https://github.com/0day-ci/linux/commits/Daniel-Scally/Add-functionality-to-ipu3-cio2-driver-allowing-software_node-connections-to-sensors-on-platforms-designed-for-Windows/20201130-214014
base:   git://linuxtv.org/media_tree.git master
config: i386-randconfig-m021-20201130 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/base/swnode.c:785 software_node_unregister_node_group() error: uninitialized symbol 'i'.

vim +/i +785 drivers/base/swnode.c

02094d54870590a Andy Shevchenko 2020-04-08  778  void software_node_unregister_node_group(const struct software_node **node_group)
02094d54870590a Andy Shevchenko 2020-04-08  779  {
02094d54870590a Andy Shevchenko 2020-04-08  780  	unsigned int i;
02094d54870590a Andy Shevchenko 2020-04-08  781  
02094d54870590a Andy Shevchenko 2020-04-08  782  	if (!node_group)
02094d54870590a Andy Shevchenko 2020-04-08  783  		return;
02094d54870590a Andy Shevchenko 2020-04-08  784  
7c7577c82672f0a Daniel Scally   2020-11-30 @785  	while (node_group[i]->name)
                                                                          ^
The "i" is never initialized.

7c7577c82672f0a Daniel Scally   2020-11-30  786  		i++;
7c7577c82672f0a Daniel Scally   2020-11-30  787  
7c7577c82672f0a Daniel Scally   2020-11-30  788  	while (i--)
9dcbac84244f32e Andy Shevchenko 2020-06-22  789  		software_node_unregister(node_group[i]);

It's a strange thing that they can only be unregistered in reverse
order...  Walter Harms is right when he points out that programmers are
notoriously bad at counting backwards.

02094d54870590a Andy Shevchenko 2020-04-08  790  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Daniel Scally Dec. 2, 2020, 10:04 a.m. | #4
On 30/11/2020 17:47, Andy Shevchenko wrote:
> On Mon, Nov 30, 2020 at 06:17:16PM +0200, Laurent Pinchart wrote:
>> Hi Daniel,
>>
>> Thank you for the patch.
>>
>> The subject line is very long. We try to keep it within a 72 characters
>> limit in the kernel. That can be a challenge sometimes, and expections
>> can be accepted, but this one is reaaaally long.
>>
>> (The same comment holds for other patches in the series)
> 
> +1.

My bad; I'll go through the series and condense them down as much as
possible.

>> On Mon, Nov 30, 2020 at 01:31:17PM +0000, Daniel Scally wrote:
>>> To maintain consistency with software_node_unregister_nodes(), reverse
>>> the order in which the software_node_unregister_node_group() function
>>> unregisters nodes.
>>>
>>> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>
>> I"d squash this with the previous patch to avoid introducing an
>> inconsistency.
> 
> It's different to previous. It touches not complementary API, but different
> one. However, I would follow your comment about documenting the behaviour of
> these two APIs as well…

I'll update the documentation for this function too.

Patch

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index d39e1c76d98d..9bd0bb77ad5b 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -782,7 +782,10 @@  void software_node_unregister_node_group(const struct software_node **node_group
 	if (!node_group)
 		return;
 
-	for (i = 0; node_group[i]; i++)
+	while (node_group[i]->name)
+		i++;
+
+	while (i--)
 		software_node_unregister(node_group[i]);
 }
 EXPORT_SYMBOL_GPL(software_node_unregister_node_group);