From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Pingfan Liu <kernelfans@gmail.com>
Cc: linux-kernel@vger.kernel.org,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
Grygorii Strashko <grygorii.strashko@ti.com>,
Christoph Hellwig <hch@infradead.org>,
Bjorn Helgaas <helgaas@kernel.org>,
Dave Young <dyoung@redhat.com>,
linux-pci@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCHv3 2/4] drivers/base: utilize device tree info to shutdown devices
Date: Thu, 05 Jul 2018 12:11:49 +0200 [thread overview]
Message-ID: <2108146.dv4EAOf6IP@aspire.rjw.lan> (raw)
In-Reply-To: <1530600642-25090-3-git-send-email-kernelfans@gmail.com>
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@linuxfoundation.org>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 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 | 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?
> /*
> * 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()?
> +{
> + 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?
> + 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.
> + return 0;
> +}
Well, instead of doing this dance, we might as well walk dpm_list here as it
is in the right order.
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?
Thanks,
Rafael
next prev parent reply other threads:[~2018-07-05 10:13 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-03 6:50 [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset Pingfan Liu
2018-07-03 6:50 ` [PATCHv3 1/4] drivers/base: fold the routine of device's shutdown into a func Pingfan Liu
2018-07-03 6:50 ` [PATCHv3 2/4] drivers/base: utilize device tree info to shutdown devices Pingfan Liu
2018-07-03 7:51 ` Lukas Wunner
2018-07-03 9:26 ` Pingfan Liu
2018-07-04 3:10 ` Pingfan Liu
2018-07-03 10:58 ` Andy Shevchenko
2018-07-03 17:03 ` Pavel Tatashin
2018-07-04 17:04 ` kbuild test robot
2018-07-05 10:11 ` Rafael J. Wysocki [this message]
2018-07-06 3:02 ` Pingfan Liu
2018-07-06 9:53 ` Rafael J. Wysocki
2018-07-07 4:02 ` Pingfan Liu
2018-07-06 10:00 ` [PATCH] driver core: Drop devices_kset_move_last() call from really_probe() Rafael J. Wysocki
2018-07-09 13:57 ` Bjorn Helgaas
2018-07-09 21:35 ` Rafael J. Wysocki
2018-07-09 22:06 ` Bjorn Helgaas
2018-07-10 6:19 ` Kishon Vijay Abraham I
2018-07-10 10:32 ` Rafael J. Wysocki
2018-07-10 10:29 ` Rafael J. Wysocki
2018-07-10 6:33 ` Pingfan Liu
2018-07-10 11:35 ` [PATCH] driver core: Partially revert "driver core: correct device's shutdown order" Rafael J. Wysocki
2018-07-10 12:22 ` Kishon Vijay Abraham I
2018-07-10 12:38 ` Rafael J. Wysocki
2018-07-10 12:51 ` [PATCH v2] " Rafael J. Wysocki
2018-07-10 12:59 ` Greg Kroah-Hartman
2018-07-10 15:40 ` Rafael J. Wysocki
2018-07-10 15:47 ` Greg Kroah-Hartman
2018-07-10 19:13 ` Kishon Vijay Abraham I
2018-07-03 6:50 ` [PATCHv3 3/4] drivers/base: clean up the usage of devices_kset_move_last() Pingfan Liu
2018-07-03 14:26 ` Rafael J. Wysocki
2018-07-04 4:40 ` Pingfan Liu
2018-07-04 10:17 ` Rafael J. Wysocki
2018-07-05 2:32 ` Pingfan Liu
2018-07-03 6:50 ` [PATCHv3 4/4] Revert "driver core: correct device's shutdown order" Pingfan Liu
2018-07-03 14:35 ` [PATCHv3 0/4] drivers/base: bugfix for supplier<-consumer ordering in device_kset Rafael J. Wysocki
2018-07-04 2:47 ` Pingfan Liu
2018-07-04 10:21 ` Rafael J. Wysocki
2018-07-05 2:44 ` Pingfan Liu
2018-07-05 9:18 ` Rafael J. Wysocki
2018-07-06 8:36 ` Lukas Wunner
2018-07-06 8:47 ` Rafael J. Wysocki
2018-07-06 13:55 ` Pingfan Liu
2018-07-07 4:24 ` Pingfan Liu
2018-07-08 8:25 ` Rafael J. Wysocki
2018-07-09 6:48 ` Pingfan Liu
2018-07-09 7:48 ` Rafael J. Wysocki
2018-07-09 8:40 ` Pingfan Liu
2018-07-09 8:58 ` Rafael J. Wysocki
2018-07-06 10:02 ` Kishon Vijay Abraham I
2018-07-06 13:52 ` Pingfan Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2108146.dv4EAOf6IP@aspire.rjw.lan \
--to=rjw@rjwysocki.net \
--cc=dyoung@redhat.com \
--cc=gregkh@linuxfoundation.org \
--cc=grygorii.strashko@ti.com \
--cc=hch@infradead.org \
--cc=helgaas@kernel.org \
--cc=kernelfans@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=rafael.j.wysocki@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).