Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit e8801a74 authored by Konrad Rzeszutek Wilk's avatar Konrad Rzeszutek Wilk Committed by David Vrabel
Browse files

xen/pciback: Don't deadlock when unbinding. 



As commit 0a9fd015
'xen/pciback: Document the entry points for 'pcistub_put_pci_dev''
explained there are four entry points in this function.
Two of them are when the user fiddles in the SysFS to
unbind a device which might be in use by a guest or not.

Both 'unbind' states will cause a deadlock as the the PCI lock has
already been taken, which then pci_device_reset tries to take.

We can simplify this by requiring that all callers of
pcistub_put_pci_dev MUST hold the device lock. And then
we can just call the lockless version of pci_device_reset.

To make it even simpler we will modify xen_pcibk_release_pci_dev
to quality whether it should take a lock or not - as it ends
up calling xen_pcibk_release_pci_dev and needs to hold the lock.

Reviewed-by: default avatarBoris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: default avatarKonrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: default avatarDavid Vrabel <david.vrabel@citrix.com>
parent 2c3fc8d2

drivers/xen/xen-pciback/passthrough.c

+11 −3
Original line number Diff line number Diff line
@@ -69,7 +69,7 @@ static int __xen_pcibk_add_pci_dev(struct xen_pcibk_device *pdev, 
}



static void __xen_pcibk_release_pci_dev(struct xen_pcibk_device *pdev,

					struct pci_dev *dev)

					struct pci_dev *dev, bool lock)

{

	struct passthrough_dev_data *dev_data = pdev->pci_dev_data;

	struct pci_dev_entry *dev_entry, *t;
@@ -87,8 +87,13 @@ static void __xen_pcibk_release_pci_dev(struct xen_pcibk_device *pdev, 


	mutex_unlock(&dev_data->lock);



	if (found_dev)

	if (found_dev) {

		if (lock)

			device_lock(&found_dev->dev);

		pcistub_put_pci_dev(found_dev);

		if (lock)

			device_unlock(&found_dev->dev);

	}

}



static int __xen_pcibk_init_devices(struct xen_pcibk_device *pdev)
@@ -156,8 +161,11 @@ static void __xen_pcibk_release_devices(struct xen_pcibk_device *pdev) 
	struct pci_dev_entry *dev_entry, *t;



	list_for_each_entry_safe(dev_entry, t, &dev_data->dev_list, list) {

		struct pci_dev *dev = dev_entry->dev;

		list_del(&dev_entry->list);

		pcistub_put_pci_dev(dev_entry->dev);

		device_lock(&dev->dev);

		pcistub_put_pci_dev(dev);

		device_unlock(&dev->dev);

		kfree(dev_entry);

	}

drivers/xen/xen-pciback/pci_stub.c

+6 −6
Original line number Diff line number Diff line
@@ -250,6 +250,8 @@ struct pci_dev *pcistub_get_pci_dev(struct xen_pcibk_device *pdev, 
 *  - 'echo BDF > unbind' with a guest still using it. See pcistub_remove

 *

 *  As such we have to be careful.

 *

 *  To make this easier, the caller has to hold the device lock.

 */

void pcistub_put_pci_dev(struct pci_dev *dev)

{
@@ -276,11 +278,8 @@ void pcistub_put_pci_dev(struct pci_dev *dev) 
	/* Cleanup our device

	 * (so it's ready for the next domain)

	 */



	/* This is OK - we are running from workqueue context

	 * and want to inhibit the user from fiddling with 'reset'

	 */

	pci_reset_function(dev);

	lockdep_assert_held(&dev->dev.mutex);

	__pci_reset_function_locked(dev);

	pci_restore_state(dev);



	/* This disables the device. */
@@ -567,7 +566,8 @@ static void pcistub_remove(struct pci_dev *dev) 
			/* N.B. This ends up calling pcistub_put_pci_dev which ends up

			 * doing the FLR. */

			xen_pcibk_release_pci_dev(found_psdev->pdev,

						found_psdev->dev);

						found_psdev->dev,

						false /* caller holds the lock. */);

		}



		spin_lock_irqsave(&pcistub_devices_lock, flags);

drivers/xen/xen-pciback/pciback.h

+4 −3
Original line number Diff line number Diff line
@@ -99,7 +99,8 @@ struct xen_pcibk_backend { 
		    unsigned int *domain, unsigned int *bus,

		    unsigned int *devfn);

	int (*publish)(struct xen_pcibk_device *pdev, publish_pci_root_cb cb);

	void (*release)(struct xen_pcibk_device *pdev, struct pci_dev *dev);

	void (*release)(struct xen_pcibk_device *pdev, struct pci_dev *dev,

                        bool lock);

	int (*add)(struct xen_pcibk_device *pdev, struct pci_dev *dev,

		   int devid, publish_pci_dev_cb publish_cb);

	struct pci_dev *(*get)(struct xen_pcibk_device *pdev,
@@ -122,10 +123,10 @@ static inline int xen_pcibk_add_pci_dev(struct xen_pcibk_device *pdev, 
}



static inline void xen_pcibk_release_pci_dev(struct xen_pcibk_device *pdev,

					     struct pci_dev *dev)

					     struct pci_dev *dev, bool lock)

{

	if (xen_pcibk_backend && xen_pcibk_backend->release)

		return xen_pcibk_backend->release(pdev, dev);

		return xen_pcibk_backend->release(pdev, dev, lock);

}



static inline struct pci_dev *

drivers/xen/xen-pciback/vpci.c

+11 −3
Original line number Diff line number Diff line
@@ -145,7 +145,7 @@ static int __xen_pcibk_add_pci_dev(struct xen_pcibk_device *pdev, 
}



static void __xen_pcibk_release_pci_dev(struct xen_pcibk_device *pdev,

					struct pci_dev *dev)

					struct pci_dev *dev, bool lock)

{

	int slot;

	struct vpci_dev_data *vpci_dev = pdev->pci_dev_data;
@@ -169,8 +169,13 @@ static void __xen_pcibk_release_pci_dev(struct xen_pcibk_device *pdev, 
out:

	mutex_unlock(&vpci_dev->lock);



	if (found_dev)

	if (found_dev) {

		if (lock)

			device_lock(&found_dev->dev);

		pcistub_put_pci_dev(found_dev);

		if (lock)

			device_unlock(&found_dev->dev);

	}

}



static int __xen_pcibk_init_devices(struct xen_pcibk_device *pdev)
@@ -208,8 +213,11 @@ static void __xen_pcibk_release_devices(struct xen_pcibk_device *pdev) 
		struct pci_dev_entry *e, *tmp;

		list_for_each_entry_safe(e, tmp, &vpci_dev->dev_list[slot],

					 list) {

			struct pci_dev *dev = e->dev;

			list_del(&e->list);

			pcistub_put_pci_dev(e->dev);

			device_lock(&dev->dev);

			pcistub_put_pci_dev(dev);

			device_unlock(&dev->dev);

			kfree(e);

		}

	}

drivers/xen/xen-pciback/xenbus.c

+1 −1
Original line number Diff line number Diff line
@@ -291,7 +291,7 @@ static int xen_pcibk_remove_device(struct xen_pcibk_device *pdev, 


	/* N.B. This ends up calling pcistub_put_pci_dev which ends up

	 * doing the FLR. */

	xen_pcibk_release_pci_dev(pdev, dev);

	xen_pcibk_release_pci_dev(pdev, dev, true /* use the lock. */);



out:

	return err;