On Friday, July 6, 2018 5:02:15 AM CEST Pingfan Liu wrote:
> On Thu, Jul 5, 2018 at 6:13 PM Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> >
> > On Tuesday, July 3, 2018 8:50:40 AM CEST Pingfan Liu wrote:
> > > 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.
> > >
> > > It is a little hard to impose both "parent<-child" and "supplier<-consumer"
> > > order on devices_kset. Take the following scene:
> > > step0: before a consumer's probing, (note child_a is supplier of consumer_a)
> > > [ consumer-X, child_a, ...., child_z] [... consumer_a, ..., consumer_z, ...] supplier-X
> > > ^^^^^^^^^^ affected range ^^^^^^^^^^
> > > step1: when probing, moving consumer-X after supplier-X
> > > [ child_a, ...., child_z] [.... consumer_a, ..., consumer_z, ...] supplier-X, consumer-X
> > > step2: the children of consumer-X should be re-ordered to maintain the seq
> > > [... consumer_a, ..., consumer_z, ....] supplier-X [consumer-X, child_a, ...., child_z]
> > > step3: the consumer_a should be re-ordered to maintain the seq
> > > [... consumer_z, ...] supplier-X [ consumer-X, child_a, consumer_a ..., child_z]
> > >
> > > It requires two nested recursion to drain out all out-of-order item in
> > > "affected range". To avoid such complicated code, this patch suggests
> > > to utilize the info in device tree, instead of using the order of
> > > devices_kset during shutdown. It iterates the device tree, and firstly
> > > shutdown a device's children and consumers. After this patch, the buggy
> > > commit is hollow and left to clean.
> > >
> > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > Cc: Grygorii Strashko <grygorii.strashko@xxxxxx>
> > > Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> > > Cc: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > > Cc: Dave Young <dyoung@xxxxxxxxxx>
> > > Cc: linux-pci@xxxxxxxxxxxxxxx
> > > Cc: linuxppc-dev@xxxxxxxxxxxxxxxx
> > > Signed-off-by: Pingfan Liu <kernelfans@xxxxxxxxx>
> > > ---
> > > drivers/base/core.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
> > > include/linux/device.h | 1 +
> > > 2 files changed, 44 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index a48868f..684b994 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -1446,6 +1446,7 @@ void device_initialize(struct device *dev)
> > > INIT_LIST_HEAD(&dev->links.consumers);
> > > INIT_LIST_HEAD(&dev->links.suppliers);
> > > dev->links.status = DL_DEV_NO_DRIVER;
> > > + dev->shutdown = false;
> > > }
> > > EXPORT_SYMBOL_GPL(device_initialize);
> > >
> > > @@ -2811,7 +2812,6 @@ static void __device_shutdown(struct device *dev)
> > > * lock is to be held
> > > */
> > > parent = get_device(dev->parent);
> > > - get_device(dev);
> >
> > Why is the get_/put_device() not needed any more?
> >
> They are moved upper layer into device_for_each_child_shutdown().
> Since there is lock breakage in __device_shutdown(), resorting to
> ref++ to protect the ancestor. And I think the
> get_device(dev->parent) can be deleted either.
Wouldn't that break USB?
> > > /*
> > > * Make sure the device is off the kset list, in the
> > > * event that dev->*->shutdown() doesn't remove it.
> > > @@ -2842,23 +2842,60 @@ static void __device_shutdown(struct device *dev)
> > > dev_info(dev, "shutdown\n");
> > > dev->driver->shutdown(dev);
> > > }
> > > -
> > > + dev->shutdown = true;
> > > device_unlock(dev);
> > > if (parent)
> > > device_unlock(parent);
> > >
> > > - put_device(dev);
> > > put_device(parent);
> > > spin_lock(&devices_kset->list_lock);
> > > }
> > >
> > > +/* shutdown dev's children and consumer firstly, then itself */
> > > +static int device_for_each_child_shutdown(struct device *dev)
> >
> > Confusing name.
> >
> > What about device_shutdown_subordinate()?
> >
> Fine. My understanding of words is not exact.
>
> > > +{
> > > + struct klist_iter i;
> > > + struct device *child;
> > > + struct device_link *link;
> > > +
> > > + /* already shutdown, then skip this sub tree */
> > > + if (dev->shutdown)
> > > + return 0;
> > > +
> > > + if (!dev->p)
> > > + goto check_consumers;
> > > +
> > > + /* there is breakage of lock in __device_shutdown(), and the redundant
> > > + * ref++ on srcu protected consumer is harmless since shutdown is not
> > > + * hot path.
> > > + */
> > > + get_device(dev);
> > > +
> > > + klist_iter_init(&dev->p->klist_children, &i);
> > > + while ((child = next_device(&i)))
> > > + device_for_each_child_shutdown(child);
> >
> > Why don't you use device_for_each_child() here?
> >
> OK, I will try use it.
Well, hold on.
> > > + klist_iter_exit(&i);
> > > +
> > > +check_consumers:
> > > + list_for_each_entry_rcu(link, &dev->links.consumers, s_node) {
> > > + if (!link->consumer->shutdown)
> > > + device_for_each_child_shutdown(link->consumer);
> > > + }
> > > +
> > > + __device_shutdown(dev);
> > > + put_device(dev);
> >
> > Possible reference counter imbalance AFAICS.
> >
> Yes, get_device() should be ahead of "if (!dev->p)". Is anything else I miss?
Yes, that's it.
> > > + return 0;
> > > +}
> >
> > Well, instead of doing this dance, we might as well walk dpm_list here as it
> > is in the right order.
> >
> Sorry, do you mean that using the same way to manage the dpm_list?
No, I mean to use dpm_list instead of devices_kset for shutdown.
They should be in the same order anyway if all is correct.
> > Of course, that would require dpm_list to be available for CONFIG_PM unset,
> > but it may be a better approach long term.
> >
> > > +
> > > /**
> > > * device_shutdown - call ->shutdown() on each device to shutdown.
> > > */
> > > void device_shutdown(void)
> > > {
> > > struct device *dev;
> > > + int idx;
> > >
> > > + idx = device_links_read_lock();
> > > spin_lock(&devices_kset->list_lock);
> > > /*
> > > * Walk the devices list backward, shutting down each in turn.
> > > @@ -2866,11 +2903,12 @@ void device_shutdown(void)
> > > * devices offline, even as the system is shutting down.
> > > */
> > > while (!list_empty(&devices_kset->list)) {
> > > - dev = list_entry(devices_kset->list.prev, struct device,
> > > + dev = list_entry(devices_kset->list.next, struct device,
> > > kobj.entry);
> > > - __device_shutdown(dev);
> > > + device_for_each_child_shutdown(dev);
> > > }
> > > spin_unlock(&devices_kset->list_lock);
> > > + device_links_read_unlock(idx);
> > > }
> > >
> > > /*
> > > diff --git a/include/linux/device.h b/include/linux/device.h
> > > index 055a69d..8a0f784 100644
> > > --- a/include/linux/device.h
> > > +++ b/include/linux/device.h
> > > @@ -1003,6 +1003,7 @@ struct device {
> > > bool offline:1;
> > > bool of_node_reused:1;
> > > bool dma_32bit_limit:1;
> > > + bool shutdown:1; /* one direction: false->true */
> > > };
> > >
> > > static inline struct device *kobj_to_dev(struct kobject *kobj)
> > >
> >
> > If the device_kset_move_last() in really_probe() is the only problem,
> > I'd rather try to fix that one in the first place.
> >
> > Why is it needed?
> >
> I had tried, but it turns out not easy to archive. The code is
> https://patchwork.kernel.org/patch/10485195/. And I make a detailed
> description of the algorithm in this patch's commit log. To be more
> detailed, we face the potential out of order issue in really_probe()
> like : 0th. [ consumer-X, child_a, ...., child_z] [... consumer_a,
> ..., consumer_z, ...] supplier-X //(note child_a is supplier of
> consumer_a). To address all the potential out of order item in the
> affected section [... consumer_a, ..., consumer_z, ...], it will
> incur two nested recursions. 1st, moving consumer-X and its
> descendants after supplier-X, 2nd, moving consumer_a after child_a,
> 3rd. the 2nd step may pose the same situation of 0th. Besides the two
> interleaved recursion, the breakage of spin lock requires more effort
> to protect the item from disappearing in linked-list (which I did not
> implement in the https://patchwork.kernel.org/patch/10485195/). Hence
> I turn to this cheap method.
So I think that we simply need to drop the devices_kset_move_last() call
from really_probe() as it is plain incorrect and the use case for it is
questionable at best.
And the use case it is supposed to address should be addressed differently.
Thanks,
Rafael
