Re: [PATCHv3 2/4] drivers/base: utilize device tree info to shutdown devices

[Rafael J. Wysocki]

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?

>  	/*
>  	 * 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