Re: [PATCHv3 4/4] pci: make reset_subordinate hotplug safe

[<dan.j.williams@intel.com>]

Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> Use the slot reset method when resetting the bridge if the bus contains
> hot plug slots. This fixes spurious hot plug events that are triggered
> by the secondary bus reset that bypasses the slot's detection disabling.
> 
> Resetting a bridge's subordinate bus can be done like this:
> 
>   # echo 1 > /sys/bus/pci/devices/0000:50:01.0/reset_subordinate
> 
> Prior to this patch, an example kernel message may show something like:
> 
>   pcieport 0000:50:01.0: pciehp: Slot(40): Link Down
> 
> With this change, the pciehp driver ignores the link event during the
> reset, so may show this message instead:
> 
>   pcieport 0000:50:01.0: pciehp: Slot(40): Link Down/Up ignored
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/pci/pci-sysfs.c |  3 +-
>  drivers/pci/pci.c       | 71 ++++++++++++++++++++++++++++-------------
>  drivers/pci/pci.h       |  2 +-
>  3 files changed, 50 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index c2df915ad2d29..039d3dc157006 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -553,7 +553,6 @@ static ssize_t reset_subordinate_store(struct device *dev,
>  				const char *buf, size_t count)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct pci_bus *bus = pdev->subordinate;
>  	unsigned long val;
>  
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -563,7 +562,7 @@ static ssize_t reset_subordinate_store(struct device *dev,
>  		return -EINVAL;
>  
>  	if (val) {
> -		int ret = __pci_reset_bus(bus);
> +		int ret = pci_try_reset_bridge(pdev);
>  
>  		if (ret)
>  			return ret;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index df9ed73dad416..27817c6603a5c 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -51,6 +51,10 @@ unsigned int pci_pm_d3hot_delay;
>  
>  static void pci_pme_list_scan(struct work_struct *work);
>  
> +#define PCI_SLOT_RESET_MASKED true
> +#define PCI_SLOT_RESET_UNMASKED false
> +static int pci_reset_bridge(struct pci_dev *bridge, bool save);
> +
>  static LIST_HEAD(pci_pme_list);
>  static DEFINE_MUTEX(pci_pme_list_mutex);
>  static DECLARE_DELAYED_WORK(pci_pme_work, pci_pme_list_scan);
> @@ -5420,29 +5424,7 @@ static int pci_bus_reset(struct pci_bus *bus, bool probe)
>   */
>  int pci_bus_error_reset(struct pci_dev *bridge)
>  {
> -	struct pci_bus *bus = bridge->subordinate;
> -	struct pci_slot *slot;
> -
> -	if (!bus)
> -		return -ENOTTY;
> -
> -	mutex_lock(&pci_slot_mutex);
> -	if (list_empty(&bus->slots))
> -		goto bus_reset;
> -
> -	list_for_each_entry(slot, &bus->slots, list)
> -		if (pci_probe_reset_slot(slot))
> -			goto bus_reset;
> -
> -	list_for_each_entry(slot, &bus->slots, list)
> -		if (pci_slot_reset(slot, PCI_RESET_DO_RESET))
> -			goto bus_reset;
> -
> -	mutex_unlock(&pci_slot_mutex);
> -	return 0;
> -bus_reset:
> -	mutex_unlock(&pci_slot_mutex);
> -	return pci_bus_reset(bridge->subordinate, PCI_RESET_DO_RESET);
> +	return pci_reset_bridge(bridge, PCI_SLOT_RESET_UNMASKED);
>  }
>  
>  /**
> @@ -5483,6 +5465,49 @@ static int pci_try_reset_bus(struct pci_bus *bus)
>  	return rc;
>  }
>  
> +static int pci_reset_bridge(struct pci_dev *bridge, bool masked)

Perhaps a comment clarifying that @masked refers to hotplug events, or a
comment to say "see PCI_SLOT_RESET_MASKED", and put a clarification
there.

> +{
> +	struct pci_bus *bus = bridge->subordinate;
> +	struct pci_slot *slot;
> +
> +	if (!bus)
> +		return -ENOTTY;
> +
> +	mutex_lock(&pci_slot_mutex);
> +	if (list_empty(&bus->slots))
> +		goto bus_reset;
> +
> +	list_for_each_entry(slot, &bus->slots, list)
> +		if (pci_probe_reset_slot(slot))
> +			goto bus_reset;
> +
> +	list_for_each_entry(slot, &bus->slots, list) {
> +		int ret;
> +
> +		if (masked)
> +			ret = pci_try_reset_slot(slot);
> +		else
> +			ret = pci_slot_reset(slot, PCI_RESET_DO_RESET);
> +
> +		if (ret)
> +			goto bus_reset;
> +	}
> +
> +	mutex_unlock(&pci_slot_mutex);

This feels like it wants a helper with a guard() inside to avoid goto,
but I can be convinced that are already too PCI reset helpers. This
otherwise looks ok.

With the @masked comment fixed up at a minimum you can add:

Reviewed-by: Dan Williams <dan.j.williams@intel.com>