Re: [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers
On Wed, Jun 27, 2018 at 4:35 PM Dan Carpenter wrote: > > On Wed, Jun 27, 2018 at 10:34:54AM +0800, Pingfan Liu wrote: > > > 1b2a1e63 Pingfan Liu 2018-06-25 243} > > > 1b2a1e63 Pingfan Liu 2018-06-25 244} > > > 1b2a1e63 Pingfan Liu 2018-06-25 @245BUG_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... > > > > > Yes, if list_empty, then the code can not touch ret. But ret is > > useless in this scene. Does it matter? > > > > I'm not sure I understand what you're asking? Of course, it matters? > Oh, I misunderstood your original comment. Yes, you are right. I will fix it in next version, if this code section is still used. Thanks and regards, Pingfan
Re: [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers
On Wed, Jun 27, 2018 at 10:34:54AM +0800, Pingfan Liu wrote: > > 1b2a1e63 Pingfan Liu 2018-06-25 243} > > 1b2a1e63 Pingfan Liu 2018-06-25 244} > > 1b2a1e63 Pingfan Liu 2018-06-25 @245BUG_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... > > > Yes, if list_empty, then the code can not touch ret. But ret is > useless in this scene. Does it matter? > I'm not sure I understand what you're asking? Of course, it matters? regards, dan carpenter
Re: [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers
Hi Dan,
Thanks for your hints, see the comment in lines.
On Tue, Jun 26, 2018 at 3:44 PM Dan Carpenter wrote:
>
> [ 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 222struct list_head *iter, *left, *right;
> 1b2a1e63 Pingfan Liu 2018-06-25 223struct device *cur_dev;
> 1b2a1e63 Pingfan Liu 2018-06-25 224struct pos_info info;
> 1b2a1e63 Pingfan Liu 2018-06-25 225int ret, idx;
> 1b2a1e63 Pingfan Liu 2018-06-25 226
> 1b2a1e63 Pingfan Liu 2018-06-25 227idx = device_links_read_lock();
> 1b2a1e63 Pingfan Liu 2018-06-25 228if
> (list_empty(>links.suppliers)) {
> 1b2a1e63 Pingfan Liu 2018-06-25 229device_links_read_unlock(idx);
> 1b2a1e63 Pingfan Liu 2018-06-25 230return 0;
> 1b2a1e63 Pingfan Liu 2018-06-25 231}
> 1b2a1e63 Pingfan Liu 2018-06-25 232spin_lock(_kset->list_lock);
> 1b2a1e63 Pingfan Liu 2018-06-25 233list_for_each_prev(iter,
> _kset->list) {
> 1b2a1e63 Pingfan Liu 2018-06-25 234cur_dev = list_entry(iter,
> struct device, kobj.entry);
> 1b2a1e63 Pingfan Liu 2018-06-25 235ret = find_last_supplier(dev,
> cur_dev);
> 1b2a1e63 Pingfan Liu 2018-06-25 236switch (ret) {
> 1b2a1e63 Pingfan Liu 2018-06-25 237case -1:
> 1b2a1e63 Pingfan Liu 2018-06-25 238goto unlock;
> 1b2a1e63 Pingfan Liu 2018-06-25 239case 1:
> 1b2a1e63 Pingfan Liu 2018-06-25 240break;
> 1b2a1e63 Pingfan Liu 2018-06-25 241case 0:
> 1b2a1e63 Pingfan Liu 2018-06-25 242continue;
>
> The break breaks from the switch and the continue continues the loop so
> they're equivalent. Perhaps you intended to break from the loop?
>
Yes, you are right.
> 1b2a1e63 Pingfan Liu 2018-06-25 243}
> 1b2a1e63 Pingfan Liu 2018-06-25 244}
> 1b2a1e63 Pingfan Liu 2018-06-25 @245BUG_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...
>
Yes, if list_empty, then the code can not touch ret. But ret is
useless in this scene. Does it matter?
Thanks and regards,
Pingfan
> 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 248left = dev->kobj.entry.prev;
> 1b2a1e63 Pingfan Liu 2018-06-25 249right = iter;
> 1b2a1e63 Pingfan Liu 2018-06-25 250info.pos = list_entry(iter, struct
> device, kobj.entry);
> 1b2a1e63 Pingfan Liu 2018-06-25 251info.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, );
> 1b2a1e63 Pingfan Liu 2018-06-25 254
> 1b2a1e63 Pingfan Liu 2018-06-25 255 unlock:
> 1b2a1e63 Pingfan Liu 2018-06-25 256spin_unlock(_kset->list_lock);
> 1b2a1e63 Pingfan Liu 2018-06-25 257device_links_read_unlock(idx);
> 1b2a1e63 Pingfan Liu 2018-06-25 258return 0;
> 1b2a1e63 Pingfan Liu 2018-06-25 259 }
> 1b2a1e63 Pingfan Liu 2018-06-25 260
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers
[ 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 222struct list_head *iter, *left, *right;
1b2a1e63 Pingfan Liu 2018-06-25 223struct device *cur_dev;
1b2a1e63 Pingfan Liu 2018-06-25 224struct pos_info info;
1b2a1e63 Pingfan Liu 2018-06-25 225int ret, idx;
1b2a1e63 Pingfan Liu 2018-06-25 226
1b2a1e63 Pingfan Liu 2018-06-25 227idx = device_links_read_lock();
1b2a1e63 Pingfan Liu 2018-06-25 228if (list_empty(>links.suppliers)) {
1b2a1e63 Pingfan Liu 2018-06-25 229device_links_read_unlock(idx);
1b2a1e63 Pingfan Liu 2018-06-25 230return 0;
1b2a1e63 Pingfan Liu 2018-06-25 231}
1b2a1e63 Pingfan Liu 2018-06-25 232spin_lock(_kset->list_lock);
1b2a1e63 Pingfan Liu 2018-06-25 233list_for_each_prev(iter,
_kset->list) {
1b2a1e63 Pingfan Liu 2018-06-25 234cur_dev = list_entry(iter,
struct device, kobj.entry);
1b2a1e63 Pingfan Liu 2018-06-25 235ret = find_last_supplier(dev,
cur_dev);
1b2a1e63 Pingfan Liu 2018-06-25 236switch (ret) {
1b2a1e63 Pingfan Liu 2018-06-25 237case -1:
1b2a1e63 Pingfan Liu 2018-06-25 238goto unlock;
1b2a1e63 Pingfan Liu 2018-06-25 239case 1:
1b2a1e63 Pingfan Liu 2018-06-25 240break;
1b2a1e63 Pingfan Liu 2018-06-25 241case 0:
1b2a1e63 Pingfan Liu 2018-06-25 242continue;
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 @245BUG_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 248left = dev->kobj.entry.prev;
1b2a1e63 Pingfan Liu 2018-06-25 249right = iter;
1b2a1e63 Pingfan Liu 2018-06-25 250info.pos = list_entry(iter, struct
device, kobj.entry);
1b2a1e63 Pingfan Liu 2018-06-25 251info.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, );
1b2a1e63 Pingfan Liu 2018-06-25 254
1b2a1e63 Pingfan Liu 2018-06-25 255 unlock:
1b2a1e63 Pingfan Liu 2018-06-25 256spin_unlock(_kset->list_lock);
1b2a1e63 Pingfan Liu 2018-06-25 257device_links_read_unlock(idx);
1b2a1e63 Pingfan Liu 2018-06-25 258return 0;
1b2a1e63 Pingfan Liu 2018-06-25 259 }
1b2a1e63 Pingfan Liu 2018-06-25 260
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers
commit 52cdbdd49853 ("driver core: correct device's shutdown order")
introduces supplier<-consumer order in devices_kset. The commit tries
to cleverly maintain both parent<-child and supplier<-consumer order by
reordering a device when probing. This method makes things simple and
clean, but unfortunately, breaks parent<-child order in some case,
which is described in next patch in this series.
Here this patch tries to resolve supplier<-consumer by only reordering a
device when it has suppliers, and takes care of the following scenario:
[consumer, children] [ ... potential ... ] supplier
^ ^
After moving the consumer and its children after the supplier, the
potentail section may contain consumers whose supplier is inside
children, and this poses the requirement to dry out all consumpers in
the section recursively.
Cc: Greg Kroah-Hartman
Cc: Grygorii Strashko
Cc: Christoph Hellwig
Cc: Bjorn Helgaas
Cc: Dave Young
Cc: linux-...@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Pingfan Liu
---
drivers/base/base.h | 1 +
drivers/base/core.c | 97 +
2 files changed, 98 insertions(+)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index a75c302..37f86ca 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -135,6 +135,7 @@ extern void device_unblock_probing(void);
/* /sys/devices directory */
extern struct kset *devices_kset;
+extern int device_reorder_consumer(struct device *dev);
extern void devices_kset_move_last(struct device *dev);
#if defined(CONFIG_MODULES) && defined(CONFIG_SYSFS)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 8113d2c..db30e86 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -161,6 +161,103 @@ static bool is_consumer(struct device *query, struct
device *supplier)
return false;
}
+/* recursively move the potential consumers in open section (left, right)
+ * after the barrier
+ */
+static int __device_reorder_consumer(struct device *consumer,
+ struct list_head *left, struct list_head *right,
+ struct pos_info *p)
+{
+ struct list_head *iter;
+ struct device *c_dev, *s_dev, *tail_dev;
+
+ descendants_reorder_after_pos(consumer, p);
+ tail_dev = p->tail;
+ /* (left, right) may contain consumers, hence checking if any moved
+* child serving as supplier. The reversing order help us to meet
+* the last supplier of a consumer.
+*/
+ list_opensect_for_each_reverse(iter, left, right) {
+ struct list_head *l_iter, *moved_left, *moved_right;
+
+ moved_left = (>kobj.entry)->prev;
+ moved_right = tail_dev->kobj.entry.next;
+ /* the moved section may contain potential suppliers */
+ list_opensect_for_each_reverse(l_iter, moved_left,
+ moved_right) {
+ s_dev = list_entry(l_iter, struct device, kobj.entry);
+ c_dev = list_entry(iter, struct device, kobj.entry);
+ /* to fix: this poses extra effort for locking */
+ if (is_consumer(c_dev, s_dev)) {
+ p->tail = NULL;
+ /* to fix: lock issue */
+ p->pos = s_dev;
+ /* reorder after the last supplier */
+ __device_reorder_consumer(c_dev,
+ l_iter, right, p);
+ }
+ }
+ }
+ return 0;
+}
+
+static int find_last_supplier(struct device *dev, struct device *supplier)
+{
+ struct device_link *link;
+
+ list_for_each_entry_reverse(link, >links.suppliers, c_node) {
+ if (link->supplier == supplier)
+ return 1;
+ }
+ if (dev == supplier)
+ return -1;
+ return 0;
+}
+
+/* When reodering, take care of the range of (old_pos(dev), new_pos(dev)),
+ * there may be requirement to recursively move item.
+ */
+int device_reorder_consumer(struct device *dev)
+{
+ struct list_head *iter, *left, *right;
+ struct device *cur_dev;
+ struct pos_info info;
+ int ret, idx;
+
+ idx = device_links_read_lock();
+ if (list_empty(>links.suppliers)) {
+ device_links_read_unlock(idx);
+ return 0;
+ }
+ spin_lock(_kset->list_lock);
+ list_for_each_prev(iter, _kset->list) {
+ cur_dev = list_entry(iter, struct device, kobj.entry);
+ ret = find_last_supplier(dev, cur_dev);
+ switch (ret) {
+ case -1:
+ goto unlock;
+ case 1:
+ break;
+ case 0:
+ continue;
+ }
+ }
+ BUG_ON(!ret);
+
+ /* record the
