* [PATCH 1/3] drivers/base: introduce some help routines for reordering a group of dev
2018-06-25 5:23 [PATCH 0/3] drivers/base: bugfix for supplier<-consumer ordering in device_kset Pingfan Liu
@ 2018-06-25 5:23 ` Pingfan Liu
2018-06-25 6:41 ` Greg Kroah-Hartman
2018-06-25 5:23 ` [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers Pingfan Liu
2018-06-25 5:23 ` [PATCH 3/3] drivers/base: only reordering consumer device when probing Pingfan Liu
2 siblings, 1 reply; 10+ messages in thread
From: Pingfan Liu @ 2018-06-25 5:23 UTC (permalink / raw)
To: linux-kernel
Cc: Pingfan Liu, Greg Kroah-Hartman, Grygorii Strashko,
Christoph Hellwig, Bjorn Helgaas, Dave Young, linux-pci,
linuxppc-dev
This patch introduce some help routines used by next patch. It aims to
ease reviewing, while the next patch will concentrate on algorithm.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Bjorn Helgaas <helgaas@kernel.org>
Cc: Dave Young <dyoung@redhat.com>
Cc: linux-pci@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
---
drivers/base/core.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 36622b5..8113d2c 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -123,6 +123,44 @@ static int device_is_dependent(struct device *dev, void *target)
return ret;
}
+struct pos_info {
+ struct device *pos;
+ struct device *tail;
+};
+
+/* caller takes the devices_kset->list_lock */
+static int descendants_reorder_after_pos(struct device *dev,
+ void *data)
+{
+ struct device *pos;
+ struct pos_info *p = data;
+
+ pos = p->pos;
+ pr_debug("devices_kset: Moving %s after %s\n",
+ dev_name(dev), dev_name(pos));
+ device_for_each_child(dev, p, descendants_reorder_after_pos);
+ /* children at the tail */
+ list_move(&dev->kobj.entry, &pos->kobj.entry);
+ /* record the right boundary of the section */
+ if (p->tail == NULL)
+ p->tail = dev;
+ return 0;
+}
+
+/* iterate over an open section */
+#define list_opensect_for_each_reverse(cur, left, right) \
+ for (cur = right->prev; cur == left; cur = cur->prev)
+
+static bool is_consumer(struct device *query, struct device *supplier)
+{
+ struct device_link *link;
+ /* todo, lock protection */
+ list_for_each_entry(link, &supplier->links.consumers, s_node)
+ if (link->consumer == query)
+ return true;
+ return false;
+}
+
static int device_reorder_to_tail(struct device *dev, void *not_used)
{
struct device_link *link;
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] drivers/base: introduce some help routines for reordering a group of dev
2018-06-25 5:23 ` [PATCH 1/3] drivers/base: introduce some help routines for reordering a group of dev Pingfan Liu
@ 2018-06-25 6:41 ` Greg Kroah-Hartman
2018-06-25 7:08 ` Pingfan Liu
0 siblings, 1 reply; 10+ messages in thread
From: Greg Kroah-Hartman @ 2018-06-25 6:41 UTC (permalink / raw)
To: Pingfan Liu
Cc: linux-kernel, Grygorii Strashko, Christoph Hellwig,
Bjorn Helgaas, Dave Young, linux-pci, linuxppc-dev
On Mon, Jun 25, 2018 at 01:23:05PM +0800, Pingfan Liu wrote:
> This patch introduce some help routines used by next patch. It aims to
> ease reviewing, while the next patch will concentrate on algorithm.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Bjorn Helgaas <helgaas@kernel.org>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: linux-pci@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> ---
> drivers/base/core.c | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 36622b5..8113d2c 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -123,6 +123,44 @@ static int device_is_dependent(struct device *dev, void *target)
> return ret;
> }
>
> +struct pos_info {
> + struct device *pos;
> + struct device *tail;
> +};
> +
> +/* caller takes the devices_kset->list_lock */
> +static int descendants_reorder_after_pos(struct device *dev,
> + void *data)
> +{
> + struct device *pos;
> + struct pos_info *p = data;
> +
> + pos = p->pos;
> + pr_debug("devices_kset: Moving %s after %s\n",
> + dev_name(dev), dev_name(pos));
> + device_for_each_child(dev, p, descendants_reorder_after_pos);
> + /* children at the tail */
> + list_move(&dev->kobj.entry, &pos->kobj.entry);
> + /* record the right boundary of the section */
> + if (p->tail == NULL)
> + p->tail = dev;
> + return 0;
> +}
> +
> +/* iterate over an open section */
> +#define list_opensect_for_each_reverse(cur, left, right) \
> + for (cur = right->prev; cur == left; cur = cur->prev)
> +
> +static bool is_consumer(struct device *query, struct device *supplier)
> +{
> + struct device_link *link;
> + /* todo, lock protection */
> + list_for_each_entry(link, &supplier->links.consumers, s_node)
> + if (link->consumer == query)
> + return true;
> + return false;
> +}
You are adding code that no one uses yet, making this impossible to
review as I don't know what to expect. I shouldn't have to read the
second patch and have to flip back and forth to try to figure it out :(
sorry, please break this series up in a better way to make it simpler to
review.
greg k-h
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] drivers/base: introduce some help routines for reordering a group of dev
2018-06-25 6:41 ` Greg Kroah-Hartman
@ 2018-06-25 7:08 ` Pingfan Liu
0 siblings, 0 replies; 10+ messages in thread
From: Pingfan Liu @ 2018-06-25 7:08 UTC (permalink / raw)
To: gregkh
Cc: linux-kernel, grygorii.strashko, Christoph Hellwig, helgaas,
Dave Young, linux-pci, linuxppc-dev
On Mon, Jun 25, 2018 at 2:41 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jun 25, 2018 at 01:23:05PM +0800, Pingfan Liu wrote:
> > This patch introduce some help routines used by next patch. It aims to
> > ease reviewing, while the next patch will concentrate on algorithm.
> >
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> > Cc: Christoph Hellwig <hch@infradead.org>
> > Cc: Bjorn Helgaas <helgaas@kernel.org>
> > Cc: Dave Young <dyoung@redhat.com>
> > Cc: linux-pci@vger.kernel.org
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> > ---
> > drivers/base/core.c | 38 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 38 insertions(+)
> >
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 36622b5..8113d2c 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -123,6 +123,44 @@ static int device_is_dependent(struct device *dev, void *target)
> > return ret;
> > }
> >
> > +struct pos_info {
> > + struct device *pos;
> > + struct device *tail;
> > +};
> > +
> > +/* caller takes the devices_kset->list_lock */
> > +static int descendants_reorder_after_pos(struct device *dev,
> > + void *data)
> > +{
> > + struct device *pos;
> > + struct pos_info *p = data;
> > +
> > + pos = p->pos;
> > + pr_debug("devices_kset: Moving %s after %s\n",
> > + dev_name(dev), dev_name(pos));
> > + device_for_each_child(dev, p, descendants_reorder_after_pos);
> > + /* children at the tail */
> > + list_move(&dev->kobj.entry, &pos->kobj.entry);
> > + /* record the right boundary of the section */
> > + if (p->tail == NULL)
> > + p->tail = dev;
> > + return 0;
> > +}
> > +
> > +/* iterate over an open section */
> > +#define list_opensect_for_each_reverse(cur, left, right) \
> > + for (cur = right->prev; cur == left; cur = cur->prev)
> > +
> > +static bool is_consumer(struct device *query, struct device *supplier)
> > +{
> > + struct device_link *link;
> > + /* todo, lock protection */
> > + list_for_each_entry(link, &supplier->links.consumers, s_node)
> > + if (link->consumer == query)
> > + return true;
> > + return false;
> > +}
>
> You are adding code that no one uses yet, making this impossible to
> review as I don't know what to expect. I shouldn't have to read the
> second patch and have to flip back and forth to try to figure it out :(
>
OK, I had thought the less code in [2/3] will ease the reviewing
> sorry, please break this series up in a better way to make it simpler to
> review.
>
OK.
Regards,
Pingfan
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers
2018-06-25 5:23 [PATCH 0/3] drivers/base: bugfix for supplier<-consumer ordering in device_kset Pingfan Liu
2018-06-25 5:23 ` [PATCH 1/3] drivers/base: introduce some help routines for reordering a group of dev Pingfan Liu
@ 2018-06-25 5:23 ` Pingfan Liu
2018-06-26 7:44 ` Dan Carpenter
2018-06-25 5:23 ` [PATCH 3/3] drivers/base: only reordering consumer device when probing Pingfan Liu
2 siblings, 1 reply; 10+ messages in thread
From: Pingfan Liu @ 2018-06-25 5:23 UTC (permalink / raw)
To: linux-kernel
Cc: Pingfan Liu, Greg Kroah-Hartman, Grygorii Strashko,
Christoph Hellwig, Bjorn Helgaas, Dave Young, linux-pci,
linuxppc-dev
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 <gregkh@linuxfoundation.org>
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Bjorn Helgaas <helgaas@kernel.org>
Cc: Dave Young <dyoung@redhat.com>
Cc: linux-pci@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
---
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 = (&consumer->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, &dev->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(&dev->links.suppliers)) {
+ device_links_read_unlock(idx);
+ return 0;
+ }
+ spin_lock(&devices_kset->list_lock);
+ list_for_each_prev(iter, &devices_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 affected open section */
+ left = dev->kobj.entry.prev;
+ right = iter;
+ info.pos = list_entry(iter, struct device, kobj.entry);
+ info.tail = NULL;
+ /* dry out the consumers in (left,right) */
+ __device_reorder_consumer(dev, left, right, &info);
+
+unlock:
+ spin_unlock(&devices_kset->list_lock);
+ device_links_read_unlock(idx);
+ return 0;
+}
+
static int device_reorder_to_tail(struct device *dev, void *not_used)
{
struct device_link *link;
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers
2018-06-25 5:23 ` [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers Pingfan Liu
@ 2018-06-26 7:44 ` Dan Carpenter
2018-06-27 2:34 ` Pingfan Liu
0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2018-06-26 7:44 UTC (permalink / raw)
To: kbuild, Pingfan Liu
Cc: kbuild-all, linux-kernel, Pingfan Liu, Greg Kroah-Hartman,
Grygorii Strashko, Christoph Hellwig, Bjorn Helgaas, Dave Young,
linux-pci, linuxppc-dev
[ 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers
2018-06-26 7:44 ` Dan Carpenter
@ 2018-06-27 2:34 ` Pingfan Liu
2018-06-27 8:34 ` Dan Carpenter
0 siblings, 1 reply; 10+ messages in thread
From: Pingfan Liu @ 2018-06-27 2:34 UTC (permalink / raw)
To: dan.carpenter
Cc: kbuild, kbuild-all, linux-kernel, Greg Kroah-Hartman,
Grygorii Strashko, Christoph Hellwig, Bjorn Helgaas, Dave Young,
linux-pci, linuxppc-dev
Hi Dan,
Thanks for your hints, see the comment in lines.
On Tue, Jun 26, 2018 at 3:44 PM Dan Carpenter <dan.carpenter@oracle.com> 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 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?
>
Yes, you are right.
> 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...
>
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 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers
2018-06-27 2:34 ` Pingfan Liu
@ 2018-06-27 8:34 ` Dan Carpenter
2018-06-28 13:47 ` Pingfan Liu
0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2018-06-27 8:34 UTC (permalink / raw)
To: Pingfan Liu
Cc: kbuild, kbuild-all, linux-kernel, Greg Kroah-Hartman,
Grygorii Strashko, Christoph Hellwig, Bjorn Helgaas, Dave Young,
linux-pci, linuxppc-dev
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 @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...
> >
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers
2018-06-27 8:34 ` Dan Carpenter
@ 2018-06-28 13:47 ` Pingfan Liu
0 siblings, 0 replies; 10+ messages in thread
From: Pingfan Liu @ 2018-06-28 13:47 UTC (permalink / raw)
To: dan.carpenter
Cc: kbuild, kbuild-all, linux-kernel, Greg Kroah-Hartman,
Grygorii Strashko, Christoph Hellwig, Bjorn Helgaas, Dave Young,
linux-pci, linuxppc-dev
On Wed, Jun 27, 2018 at 4:35 PM Dan Carpenter <dan.carpenter@oracle.com> 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 @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...
> > >
> > 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] drivers/base: only reordering consumer device when probing
2018-06-25 5:23 [PATCH 0/3] drivers/base: bugfix for supplier<-consumer ordering in device_kset Pingfan Liu
2018-06-25 5:23 ` [PATCH 1/3] drivers/base: introduce some help routines for reordering a group of dev Pingfan Liu
2018-06-25 5:23 ` [PATCH 2/3] drivers/base: reorder consumer and its children behind suppliers Pingfan Liu
@ 2018-06-25 5:23 ` Pingfan Liu
2 siblings, 0 replies; 10+ messages in thread
From: Pingfan Liu @ 2018-06-25 5:23 UTC (permalink / raw)
To: linux-kernel
Cc: Pingfan Liu, Greg Kroah-Hartman, Grygorii Strashko,
Christoph Hellwig, Bjorn Helgaas, Dave Young, linux-pci,
linuxppc-dev
commit 52cdbdd49853 ("driver core: correct device's shutdown order")
places an assumption of supplier<-consumer order on the process of probe.
But it turns out to break down the parent <- child order in some scene.
E.g in pci, a bridge is enabled by pci core, and behind it, the devices
have been probed. Then comes the bridge's module, which enables extra
feature(such as hotplug) on this bridge. This will break the
parent<-children order and cause failure when "kexec -e" in some scenario.
The detailed description of the scenario:
An IBM Power9 machine on which, two drivers portdrv_pci and shpchp(a mod)
match the PCI_CLASS_BRIDGE_PCI, but neither of them success to probe due
to some issue. For this case, the bridge is moved after its children in
devices_kset. Then, when "kexec -e", a ata-disk behind the bridge can not
write back buffer in flight due to the former shutdown of the bridge which
clears the BusMaster bit.
To fix this issue, only reordering a device and all of its children
if it is a consumer.
Note, the bridge involved:
0004:00:00.0 PCI bridge: IBM Device 04c1 (prog-if 00 [Normal decode])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR+ FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
NUMA node: 0
Bus: primary=00, secondary=01, subordinate=12, sec-latency=0
I/O behind bridge: 00000000-00000fff
Memory behind bridge: 80000000-ffefffff
Prefetchable memory behind bridge: 0006024000000000-0006027f7fffffff
Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- <SERR- <PERR-
BridgeCtl: Parity- SERR+ NoISA- VGA- MAbort- >Reset- FastB2B-
PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
Capabilities: [40] Power Management version 3
Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [48] Express (v2) Root Port (Slot-), MSI 00
DevCap: MaxPayload 512 bytes, PhantFunc 0
ExtTag- RBE+
DevCtl: Report errors: Correctable- Non-Fatal- Fatal- Unsupported-
RlxdOrd- ExtTag- PhantFunc- AuxPwr- NoSnoop-
MaxPayload 256 bytes, MaxReadReq 128 bytes
DevSta: CorrErr- UncorrErr- FatalErr- UnsuppReq- AuxPwr- TransPend-
LnkCap: Port #0, Speed 8GT/s, Width x8, ASPM not supported, Exit Latency L0s <64ns, L1 <1us
ClockPM- Surprise- LLActRep+ BwNot+ ASPMOptComp-
LnkCtl: ASPM Disabled; RCB 128 bytes Disabled- CommClk-
ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
LnkSta: Speed 8GT/s, Width x4, TrErr- Train- SlotClk- DLActive+ BWMgmt- ABWMgmt+
RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna- CRSVisible-
RootCap: CRSVisible-
RootSta: PME ReqID 0000, PMEStatus- PMEPending-
DevCap2: Completion Timeout: Range ABCD, TimeoutDis+, LTR-, OBFF Not Supported ARIFwd+
DevCtl2: Completion Timeout: 16ms to 55ms, TimeoutDis+, LTR-, OBFF Disabled ARIFwd+
LnkCtl2: Target Link Speed: 8GT/s, EnterCompliance- SpeedDis-
Transmit Margin: Normal Operating Range, EnterModifiedCompliance- ComplianceSOS-
Compliance De-emphasis: -6dB
LnkSta2: Current De-emphasis Level: -3.5dB, EqualizationComplete+, EqualizationPhase1+
EqualizationPhase2+, EqualizationPhase3+, LinkEqualizationRequest-
Capabilities: [100 v1] Advanced Error Reporting
UESta: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
UEMsk: DLP- SDES- TLP+ FCP- CmpltTO+ CmpltAbrt+ UnxCmplt- RxOF- MalfTLP- ECRC+ UnsupReq- ACSViol-
UESvrt: DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF- MalfTLP- ECRC- UnsupReq- ACSViol-
CESta: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
CEMsk: RxErr- BadTLP- BadDLLP- Rollover- Timeout- NonFatalErr-
AERCap: First Error Pointer: 00, GenCap+ CGenEn+ ChkCap+ ChkEn+
Capabilities: [148 v1] #19
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Bjorn Helgaas <helgaas@kernel.org>
Cc: Dave Young <dyoung@redhat.com>
Cc: linux-pci@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
---
drivers/base/dd.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 1435d72..c74f23c 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -434,13 +434,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
goto probe_failed;
}
- /*
- * Ensure devices are listed in devices_kset in correct order
- * It's important to move Dev to the end of devices_kset before
- * calling .probe, because it could be recursive and parent Dev
- * should always go first
- */
- devices_kset_move_last(dev);
+ /* only reoder consumer and its children after suppliers.*/
+ device_reorder_consumer(dev);
if (dev->bus->probe) {
ret = dev->bus->probe(dev);
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread