Re: [PATCH v2] PCI: pciehp: Fix AB-BA deadlock between reset_lock and device_lock

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Apr 11, 2023 at 08:21:02AM +0200, Lukas Wunner wrote:
> In 2013, commits
> 
>   2e35afaefe64 ("PCI: pciehp: Add reset_slot() method")
>   608c388122c7 ("PCI: Add slot reset option to pci_dev_reset()")
> 
> amended PCIe hotplug to mask Presence Detect Changed events during a
> Secondary Bus Reset.  The reset thus no longer causes gratuitous slot
> bringdown and bringup.
> 
> However the commits neglected to serialize reset with code paths reading
> slot registers.  For instance, a slot bringup due to an earlier hotplug
> event may see the Presence Detect State bit cleared during a concurrent
> Secondary Bus Reset.
> ...

> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Applied to pci/hotplug for v6.4, thanks!

> Cc: stable@vger.kernel.org # v4.19+
> Cc: Dan Stein <dstein@hpe.com>
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Alex Michon <amichon@kalrayinc.com>
> Cc: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> Link to v1:
> https://lore.kernel.org/linux-pci/908047f7699d9de9ec2efd6b79aa752d73dab4b6.1595329748.git.lukas@wunner.de/
> 
> In v1 I tried to solve the problem by changing the locking order for
> Secondary Bus Resets.  That didn't really work out.  I've experimented
> with various other approaches and finally came up with this simple one
> which lends itself well for backporting to stable.  I apologize for the
> long time this has taken.
> 
>  drivers/pci/hotplug/pciehp_pci.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp_pci.c b/drivers/pci/hotplug/pciehp_pci.c
> index d17f3bf..ad12515 100644
> --- a/drivers/pci/hotplug/pciehp_pci.c
> +++ b/drivers/pci/hotplug/pciehp_pci.c
> @@ -63,7 +63,14 @@ int pciehp_configure_device(struct controller *ctrl)
>  
>  	pci_assign_unassigned_bridge_resources(bridge);
>  	pcie_bus_configure_settings(parent);
> +
> +	/*
> +	 * Release reset_lock during driver binding
> +	 * to avoid AB-BA deadlock with device_lock.
> +	 */
> +	up_read(&ctrl->reset_lock);
>  	pci_bus_add_devices(parent);
> +	down_read_nested(&ctrl->reset_lock, ctrl->depth);
>  
>   out:
>  	pci_unlock_rescan_remove();
> @@ -104,7 +111,15 @@ void pciehp_unconfigure_device(struct controller *ctrl, bool presence)
>  	list_for_each_entry_safe_reverse(dev, temp, &parent->devices,
>  					 bus_list) {
>  		pci_dev_get(dev);
> +
> +		/*
> +		 * Release reset_lock during driver unbinding
> +		 * to avoid AB-BA deadlock with device_lock.
> +		 */
> +		up_read(&ctrl->reset_lock);
>  		pci_stop_and_remove_bus_device(dev);
> +		down_read_nested(&ctrl->reset_lock, ctrl->depth);
> +
>  		/*
>  		 * Ensure that no new Requests will be generated from
>  		 * the device.
> -- 
> 2.39.1
>