| Message ID | 20201130133129.1024662-7-djrscally@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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);
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. > 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…
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
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.
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);
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(-)