Re: [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers

[Dan Carpenter <dan.carpenter@oracle.com>]

[ There is a bug with kbuild where it's not showing the Smatch warnings
  but I can probably guess...  - dan ]

Hi Pingfan,

Thank you for the patch! Perhaps something to improve:

url:    https://github.com/0day-ci/linux/commits/Pingfan-Liu/drivers-base-bugfix-for-supplier-consumer-ordering-in-device_kset/20180625-132702


# https://github.com/0day-ci/linux/commit/1b2a1e63898baf80e8e830991284e1534bc54766
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 1b2a1e63898baf80e8e830991284e1534bc54766
vim +/ret +245 drivers/base/core.c

1b2a1e63 Pingfan Liu 2018-06-25  216  
1b2a1e63 Pingfan Liu 2018-06-25  217  /* When reodering, take care of the range of (old_pos(dev), new_pos(dev)),
1b2a1e63 Pingfan Liu 2018-06-25  218   * there may be requirement to recursively move item.
1b2a1e63 Pingfan Liu 2018-06-25  219   */
1b2a1e63 Pingfan Liu 2018-06-25  220  int device_reorder_consumer(struct device *dev)
1b2a1e63 Pingfan Liu 2018-06-25  221  {
1b2a1e63 Pingfan Liu 2018-06-25  222  	struct list_head *iter, *left, *right;
1b2a1e63 Pingfan Liu 2018-06-25  223  	struct device *cur_dev;
1b2a1e63 Pingfan Liu 2018-06-25  224  	struct pos_info info;
1b2a1e63 Pingfan Liu 2018-06-25  225  	int ret, idx;
1b2a1e63 Pingfan Liu 2018-06-25  226  
1b2a1e63 Pingfan Liu 2018-06-25  227  	idx = device_links_read_lock();
1b2a1e63 Pingfan Liu 2018-06-25  228  	if (list_empty(&dev->links.suppliers)) {
1b2a1e63 Pingfan Liu 2018-06-25  229  		device_links_read_unlock(idx);
1b2a1e63 Pingfan Liu 2018-06-25  230  		return 0;
1b2a1e63 Pingfan Liu 2018-06-25  231  	}
1b2a1e63 Pingfan Liu 2018-06-25  232  	spin_lock(&devices_kset->list_lock);
1b2a1e63 Pingfan Liu 2018-06-25  233  	list_for_each_prev(iter, &devices_kset->list) {
1b2a1e63 Pingfan Liu 2018-06-25  234  		cur_dev = list_entry(iter, struct device, kobj.entry);
1b2a1e63 Pingfan Liu 2018-06-25  235  		ret = find_last_supplier(dev, cur_dev);
1b2a1e63 Pingfan Liu 2018-06-25  236  		switch (ret) {
1b2a1e63 Pingfan Liu 2018-06-25  237  		case -1:
1b2a1e63 Pingfan Liu 2018-06-25  238  			goto unlock;
1b2a1e63 Pingfan Liu 2018-06-25  239  		case 1:
1b2a1e63 Pingfan Liu 2018-06-25  240  			break;
1b2a1e63 Pingfan Liu 2018-06-25  241  		case 0:
1b2a1e63 Pingfan Liu 2018-06-25  242  			continue;

The break breaks from the switch and the continue continues the loop so
they're equivalent.  Perhaps you intended to break from the loop?

1b2a1e63 Pingfan Liu 2018-06-25  243  		}
1b2a1e63 Pingfan Liu 2018-06-25  244  	}
1b2a1e63 Pingfan Liu 2018-06-25 @245  	BUG_ON(!ret);

If the list is empty then "ret" can be unitialized.  We test a different
list "dev->links.suppliers" to see if that's empty.  I wrote a bunch of
code to make Smatch try to understand about empty lists, but I don't
think it works...

1b2a1e63 Pingfan Liu 2018-06-25  246  
1b2a1e63 Pingfan Liu 2018-06-25  247  	/* record the affected open section */
1b2a1e63 Pingfan Liu 2018-06-25  248  	left = dev->kobj.entry.prev;
1b2a1e63 Pingfan Liu 2018-06-25  249  	right = iter;
1b2a1e63 Pingfan Liu 2018-06-25  250  	info.pos = list_entry(iter, struct device, kobj.entry);
1b2a1e63 Pingfan Liu 2018-06-25  251  	info.tail = NULL;
1b2a1e63 Pingfan Liu 2018-06-25  252  	/* dry out the consumers in (left,right) */
1b2a1e63 Pingfan Liu 2018-06-25  253  	__device_reorder_consumer(dev, left, right, &info);
1b2a1e63 Pingfan Liu 2018-06-25  254  
1b2a1e63 Pingfan Liu 2018-06-25  255  unlock:
1b2a1e63 Pingfan Liu 2018-06-25  256  	spin_unlock(&devices_kset->list_lock);
1b2a1e63 Pingfan Liu 2018-06-25  257  	device_links_read_unlock(idx);
1b2a1e63 Pingfan Liu 2018-06-25  258  	return 0;
1b2a1e63 Pingfan Liu 2018-06-25  259  }
1b2a1e63 Pingfan Liu 2018-06-25  260  

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation