Greg: goto GHK
This is v5 version of patches to fix some issues in Xen PCIback.
One of the issues Xen PCI back has that patch:
is fixing is that a deadlock can happen if the PCI device is
assigned to a guest and we try to 'unbind' it from Xen 'pciback' driver.
The issue is rather simple - the SysFS mechanism for the 'unbind' path
takes a device lock and the code in Xen PCI uses the pci_reset_function
which also takes the same lock. Solution is to use the lock-less version
and mandate that callers of said function in Xen pciback take the lock.
Easy enough.
GHK:
To guard against this happening in the future we also add an assert in the
form of lockdep assertion. That is OK except that it looks ugly as we take
it straight from the 'struct device' instead of using an appropriate macro.
See:
+ lockdep_assert_held(&dev->dev.mutex);
(in [PATCH v5 2/6] xen/pciback: Don't deadlock when unbinding).
The patch: [PATCH v5 3/6] driver core: Provide an wrapper around the mutex
to do.
introduces a nice wrapper so it is bit cleaner. Greg, if you are OK with
it could you kindly Ack it as I would prefer to put this patchset
via the Xen tree. It would look now as:
- lockdep_assert_held(&dev->dev.mutex);
+ device_lock_assert(&dev->dev);
I can also squash it in "[PATCH v5 2/6] xen/pciback: Don't deadlock when
unbinding." but since that one is going through the stable tree I wasn't
sure whether you (Greg KH) would be OK with that.
END GHK:
Thank you!
Patches are also available on my git tree
git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git devel/pciback-3.17.v5
Documentation/ABI/testing/sysfs-driver-pciback | 25 +++++++++++++++
drivers/xen/xen-pciback/passthrough.c | 14 +++++++--
drivers/xen/xen-pciback/pci_stub.c | 42 ++++++++++++++------------
drivers/xen/xen-pciback/pciback.h | 7 +++--
drivers/xen/xen-pciback/vpci.c | 14 +++++++--
drivers/xen/xen-pciback/xenbus.c | 4 +--
include/linux/device.h | 5 +++
7 files changed, 81 insertions(+), 30 deletions(-)
Konrad Rzeszutek Wilk (6):
xen-pciback: Document the various parameters and attributes in SysFS
xen/pciback: Don't deadlock when unbinding.
driver core: Provide an wrapper around the mutex to do lockdep warnings
xen/pciback: Include the domain id if removing the device whilst still in use
xen/pciback: Print out the domain owning the device.
xen/pciback: Remove tons of dereferences
Which hadn't been done with the initial commit.
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
v2: Dropped the parameters and one that is unlikeable.
---
Documentation/ABI/testing/sysfs-driver-pciback | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-driver-pciback
diff --git a/Documentation/ABI/testing/sysfs-driver-pciback b/Documentation/ABI/testing/sysfs-driver-pciback
new file mode 100644
index 0000000..cdc8340
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-pciback
@@ -0,0 +1,25 @@
+What: /sys/bus/pci/drivers/pciback/quirks
+Date: Oct 2011
+KernelVersion: 3.1
+Contact: [email protected]
+Description:
+ If the permissive attribute is set, then writing a string in
+ the format of DDDD:BB:DD.F-REG:SIZE:MASK will allow the guest
+ to write and read from the PCI device. That is Domain:Bus:
+ Device.Function-Register:Size:Mask (Domain is optional).
+ For example:
+ #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
+ will allow the guest to read and write to the configuration
+ register 0x0E.
+
+What: /sys/bus/pci/drivers/pciback/irq_handlers
+Date: Oct 2011
+KernelVersion: 3.1
+Contact: [email protected]
+Description:
+ A list of all of the PCI devices owned by Xen PCI back and
+ whether Xen PCI backend will acknowledge the interrupts received
+ and the amount of interrupts received. Xen PCI back acknowledges
+ said interrupts only when they are level, shared with another
+ guest, and enabled by the guest.
+
--
1.9.3
As commit 0a9fd0152929db372ff61b0d6c280fdd34ae8bdb
'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.
CC: [email protected]
Reviewed-by: Boris Ostrovsky <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
[v2: Per David Vrabel's suggestion - use lockless version of reset]
[v3: Per Boris suggestion add assertion mechanism]
---
drivers/xen/xen-pciback/passthrough.c | 14 +++++++++++---
drivers/xen/xen-pciback/pci_stub.c | 12 ++++++------
drivers/xen/xen-pciback/pciback.h | 7 ++++---
drivers/xen/xen-pciback/vpci.c | 14 +++++++++++---
drivers/xen/xen-pciback/xenbus.c | 2 +-
5 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/drivers/xen/xen-pciback/passthrough.c b/drivers/xen/xen-pciback/passthrough.c
index 828dddc..f16a30e 100644
--- a/drivers/xen/xen-pciback/passthrough.c
+++ b/drivers/xen/xen-pciback/passthrough.c
@@ -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);
}
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index d57a173..d4cae5b 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -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);
diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
index f72af87..58e38d5 100644
--- a/drivers/xen/xen-pciback/pciback.h
+++ b/drivers/xen/xen-pciback/pciback.h
@@ -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 *
diff --git a/drivers/xen/xen-pciback/vpci.c b/drivers/xen/xen-pciback/vpci.c
index 51afff9..c99f8bb 100644
--- a/drivers/xen/xen-pciback/vpci.c
+++ b/drivers/xen/xen-pciback/vpci.c
@@ -145,7 +145,7 @@ out:
}
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);
}
}
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index 4a7e6e0..b3318fd 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -290,7 +290,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;
--
1.9.3
A little cleanup. No functional difference.
Reviewed-by: Boris Ostrovsky <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
drivers/xen/xen-pciback/pci_stub.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index 121c725..1ddd22f 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -631,10 +631,12 @@ static pci_ers_result_t common_process(struct pcistub_device *psdev,
{
pci_ers_result_t res = result;
struct xen_pcie_aer_op *aer_op;
+ struct xen_pcibk_device *pdev = psdev->pdev;
+ struct xen_pci_sharedinfo *sh_info = pdev->sh_info;
int ret;
/*with PV AER drivers*/
- aer_op = &(psdev->pdev->sh_info->aer_op);
+ aer_op = &(sh_info->aer_op);
aer_op->cmd = aer_cmd ;
/*useful for error_detected callback*/
aer_op->err = state;
@@ -655,36 +657,36 @@ static pci_ers_result_t common_process(struct pcistub_device *psdev,
* this flag to judge whether we need to check pci-front give aer
* service ack signal
*/
- set_bit(_PCIB_op_pending, (unsigned long *)&psdev->pdev->flags);
+ set_bit(_PCIB_op_pending, (unsigned long *)&pdev->flags);
/*It is possible that a pcifront conf_read_write ops request invokes
* the callback which cause the spurious execution of wake_up.
* Yet it is harmless and better than a spinlock here
*/
set_bit(_XEN_PCIB_active,
- (unsigned long *)&psdev->pdev->sh_info->flags);
+ (unsigned long *)&sh_info->flags);
wmb();
- notify_remote_via_irq(psdev->pdev->evtchn_irq);
+ notify_remote_via_irq(pdev->evtchn_irq);
ret = wait_event_timeout(xen_pcibk_aer_wait_queue,
!(test_bit(_XEN_PCIB_active, (unsigned long *)
- &psdev->pdev->sh_info->flags)), 300*HZ);
+ &sh_info->flags)), 300*HZ);
if (!ret) {
if (test_bit(_XEN_PCIB_active,
- (unsigned long *)&psdev->pdev->sh_info->flags)) {
+ (unsigned long *)&sh_info->flags)) {
dev_err(&psdev->dev->dev,
"pcifront aer process not responding!\n");
clear_bit(_XEN_PCIB_active,
- (unsigned long *)&psdev->pdev->sh_info->flags);
+ (unsigned long *)&sh_info->flags);
aer_op->err = PCI_ERS_RESULT_NONE;
return res;
}
}
- clear_bit(_PCIB_op_pending, (unsigned long *)&psdev->pdev->flags);
+ clear_bit(_PCIB_op_pending, (unsigned long *)&pdev->flags);
if (test_bit(_XEN_PCIF_active,
- (unsigned long *)&psdev->pdev->sh_info->flags)) {
+ (unsigned long *)&sh_info->flags)) {
dev_dbg(&psdev->dev->dev,
"schedule pci_conf service in " DRV_NAME "\n");
xen_pcibk_test_and_schedule_op(psdev->pdev);
--
1.9.3
Cleanup the function a bit - also include the id of the
domain that is using the device.
Reviewed-by: David Vrabel <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
drivers/xen/xen-pciback/pci_stub.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index f9bf793..121c725 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -553,12 +553,14 @@ static void pcistub_remove(struct pci_dev *dev)
spin_unlock_irqrestore(&pcistub_devices_lock, flags);
if (found_psdev) {
- dev_dbg(&dev->dev, "found device to remove - in use? %p\n",
- found_psdev->pdev);
+ dev_dbg(&dev->dev, "found device to remove %s\n",
+ found_psdev->pdev ? "- in-use" : "");
if (found_psdev->pdev) {
- pr_warn("****** removing device %s while still in-use! ******\n",
- pci_name(found_psdev->dev));
+ int domid = xen_find_device_domain_owner(dev);
+
+ pr_warn("****** removing device %s while still in-use by domain %d! ******\n",
+ pci_name(found_psdev->dev), domid);
pr_warn("****** driver domain may still access this device's i/o resources!\n");
pr_warn("****** shutdown driver domain before binding device\n");
pr_warn("****** to other drivers or domains\n");
--
1.9.3
Instead of open-coding it in drivers that want to double check
that their functions are indeed holding the device lock.
CC: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
drivers/xen/xen-pciback/pci_stub.c | 2 +-
include/linux/device.h | 5 +++++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/xen/xen-pciback/pci_stub.c b/drivers/xen/xen-pciback/pci_stub.c
index d4cae5b..f9bf793 100644
--- a/drivers/xen/xen-pciback/pci_stub.c
+++ b/drivers/xen/xen-pciback/pci_stub.c
@@ -278,7 +278,7 @@ void pcistub_put_pci_dev(struct pci_dev *dev)
/* Cleanup our device
* (so it's ready for the next domain)
*/
- lockdep_assert_held(&dev->dev.mutex);
+ device_lock_assert(&dev->dev);
__pci_reset_function_locked(dev);
pci_restore_state(dev);
diff --git a/include/linux/device.h b/include/linux/device.h
index af424ac..1d29fb2 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -908,6 +908,11 @@ static inline void device_unlock(struct device *dev)
mutex_unlock(&dev->mutex);
}
+static inline void device_lock_assert(struct device *dev)
+{
+ lockdep_assert_held(&dev->mutex);
+}
+
void driver_init(void);
/*
--
1.9.3
We had been printing it only if the device was built with
debug enabled. But this information is useful in the field
to troubleshoot.
Reviewed-by: David Vrabel <[email protected]>
Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
---
drivers/xen/xen-pciback/xenbus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/xen/xen-pciback/xenbus.c b/drivers/xen/xen-pciback/xenbus.c
index b3318fd..53e2dda 100644
--- a/drivers/xen/xen-pciback/xenbus.c
+++ b/drivers/xen/xen-pciback/xenbus.c
@@ -246,7 +246,7 @@ static int xen_pcibk_export_device(struct xen_pcibk_device *pdev,
if (err)
goto out;
- dev_dbg(&dev->dev, "registering for %d\n", pdev->xdev->otherend_id);
+ dev_info(&dev->dev, "registering for %d\n", pdev->xdev->otherend_id);
if (xen_register_device_domain_owner(dev,
pdev->xdev->otherend_id) != 0) {
dev_err(&dev->dev, "Stealing ownership from dom%d.\n",
--
1.9.3
On Mon, Jul 14, 2014 at 12:18:53PM -0400, Konrad Rzeszutek Wilk wrote:
> Instead of open-coding it in drivers that want to double check
> that their functions are indeed holding the device lock.
>
> CC: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
On Mon, Jul 14, 2014 at 12:18:50PM -0400, Konrad Rzeszutek Wilk wrote:
> Greg: goto GHK
>
> This is v5 version of patches to fix some issues in Xen PCIback.
>
> One of the issues Xen PCI back has that patch:
>
> is fixing is that a deadlock can happen if the PCI device is
> assigned to a guest and we try to 'unbind' it from Xen 'pciback' driver.
> The issue is rather simple - the SysFS mechanism for the 'unbind' path
> takes a device lock and the code in Xen PCI uses the pci_reset_function
> which also takes the same lock. Solution is to use the lock-less version
> and mandate that callers of said function in Xen pciback take the lock.
> Easy enough.
>
> GHK:
> To guard against this happening in the future we also add an assert in the
> form of lockdep assertion. That is OK except that it looks ugly as we take
> it straight from the 'struct device' instead of using an appropriate macro.
> See:
>
> + lockdep_assert_held(&dev->dev.mutex);
>
> (in [PATCH v5 2/6] xen/pciback: Don't deadlock when unbinding).
>
> The patch: [PATCH v5 3/6] driver core: Provide an wrapper around the mutex
> to do.
>
> introduces a nice wrapper so it is bit cleaner. Greg, if you are OK with
> it could you kindly Ack it as I would prefer to put this patchset
> via the Xen tree. It would look now as:
>
> - lockdep_assert_held(&dev->dev.mutex);
> + device_lock_assert(&dev->dev);
>
> I can also squash it in "[PATCH v5 2/6] xen/pciback: Don't deadlock when
> unbinding." but since that one is going through the stable tree I wasn't
> sure whether you (Greg KH) would be OK with that.
You have my ack now, and feel free to squash it into patch 2/6 if you
want, I don't mind having that in the stable trees.
thanks,
greg k-h
On Mon, Jul 14, 2014 at 10:40:42AM -0700, Greg KH wrote:
> On Mon, Jul 14, 2014 at 12:18:50PM -0400, Konrad Rzeszutek Wilk wrote:
> > Greg: goto GHK
> >
> > This is v5 version of patches to fix some issues in Xen PCIback.
> >
> > One of the issues Xen PCI back has that patch:
> >
> > is fixing is that a deadlock can happen if the PCI device is
> > assigned to a guest and we try to 'unbind' it from Xen 'pciback' driver.
> > The issue is rather simple - the SysFS mechanism for the 'unbind' path
> > takes a device lock and the code in Xen PCI uses the pci_reset_function
> > which also takes the same lock. Solution is to use the lock-less version
> > and mandate that callers of said function in Xen pciback take the lock.
> > Easy enough.
> >
> > GHK:
> > To guard against this happening in the future we also add an assert in the
> > form of lockdep assertion. That is OK except that it looks ugly as we take
> > it straight from the 'struct device' instead of using an appropriate macro.
> > See:
> >
> > + lockdep_assert_held(&dev->dev.mutex);
> >
> > (in [PATCH v5 2/6] xen/pciback: Don't deadlock when unbinding).
> >
> > The patch: [PATCH v5 3/6] driver core: Provide an wrapper around the mutex
> > to do.
> >
> > introduces a nice wrapper so it is bit cleaner. Greg, if you are OK with
> > it could you kindly Ack it as I would prefer to put this patchset
> > via the Xen tree. It would look now as:
> >
> > - lockdep_assert_held(&dev->dev.mutex);
> > + device_lock_assert(&dev->dev);
> >
> > I can also squash it in "[PATCH v5 2/6] xen/pciback: Don't deadlock when
> > unbinding." but since that one is going through the stable tree I wasn't
> > sure whether you (Greg KH) would be OK with that.
>
> You have my ack now, and feel free to squash it into patch 2/6 if you
> want, I don't mind having that in the stable trees.
Fantastic! Thank you.
>
> thanks,
>
> greg k-h
On 14/07/14 17:18, Konrad Rzeszutek Wilk wrote:
> Which hadn't been done with the initial commit.
>
> Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
> ---
> v2: Dropped the parameters and one that is unlikeable.
> ---
> Documentation/ABI/testing/sysfs-driver-pciback | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-pciback
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-pciback b/Documentation/ABI/testing/sysfs-driver-pciback
> new file mode 100644
> index 0000000..cdc8340
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> @@ -0,0 +1,25 @@
> +What: /sys/bus/pci/drivers/pciback/quirks
> +Date: Oct 2011
> +KernelVersion: 3.1
> +Contact: [email protected]
> +Description:
> + If the permissive attribute is set, then writing a string in
> + the format of DDDD:BB:DD.F-REG:SIZE:MASK will allow the guest
> + to write and read from the PCI device. That is Domain:Bus:
"...write and read the PCI device's configuration space."
> + Device.Function-Register:Size:Mask (Domain is optional).
> + For example:
> + #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
> + will allow the guest to read and write to the configuration
> + register 0x0E.
> +
> +What: /sys/bus/pci/drivers/pciback/irq_handlers
> +Date: Oct 2011
> +KernelVersion: 3.1
> +Contact: [email protected]
> +Description:
> + A list of all of the PCI devices owned by Xen PCI back and
> + whether Xen PCI backend will acknowledge the interrupts received
> + and the amount of interrupts received. Xen PCI back acknowledges
> + said interrupts only when they are level, shared with another
> + guest, and enabled by the guest.
That's not very nice sysfs file. This sort of thing ought to be in
debugfs. Perhaps we shouldn't document it for now and try to move it to
debugfs later?
David
On 14/07/14 17:18, Konrad Rzeszutek Wilk wrote:
> As commit 0a9fd0152929db372ff61b0d6c280fdd34ae8bdb
> '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.
>
> CC: [email protected]
This deadlock is for a rather specific and uncommon use case (manually
unbinding a PCI while it is passed-through). Is this critical enough to
warrant a stable backport?
> Reviewed-by: Boris Ostrovsky <[email protected]>
> Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
Reviewed-by: David Vrabel <[email protected]>
David
On Mon, Jul 28, 2014 at 02:04:18PM +0100, David Vrabel wrote:
> On 14/07/14 17:18, Konrad Rzeszutek Wilk wrote:
> > Which hadn't been done with the initial commit.
> >
> > Signed-off-by: Konrad Rzeszutek Wilk <[email protected]>
> > ---
> > v2: Dropped the parameters and one that is unlikeable.
> > ---
> > Documentation/ABI/testing/sysfs-driver-pciback | 25 +++++++++++++++++++++++++
> > 1 file changed, 25 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-driver-pciback
> >
> > diff --git a/Documentation/ABI/testing/sysfs-driver-pciback b/Documentation/ABI/testing/sysfs-driver-pciback
> > new file mode 100644
> > index 0000000..cdc8340
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-pciback
> > @@ -0,0 +1,25 @@
> > +What: /sys/bus/pci/drivers/pciback/quirks
> > +Date: Oct 2011
> > +KernelVersion: 3.1
> > +Contact: [email protected]
> > +Description:
> > + If the permissive attribute is set, then writing a string in
> > + the format of DDDD:BB:DD.F-REG:SIZE:MASK will allow the guest
> > + to write and read from the PCI device. That is Domain:Bus:
>
> "...write and read the PCI device's configuration space."
How is this different from the normal pci device config file?
>
> > + Device.Function-Register:Size:Mask (Domain is optional).
> > + For example:
> > + #echo 00:19.0-E0:2:FF > /sys/bus/pci/drivers/pciback/quirks
> > + will allow the guest to read and write to the configuration
> > + register 0x0E.
> > +
> > +What: /sys/bus/pci/drivers/pciback/irq_handlers
> > +Date: Oct 2011
> > +KernelVersion: 3.1
> > +Contact: [email protected]
> > +Description:
> > + A list of all of the PCI devices owned by Xen PCI back and
> > + whether Xen PCI backend will acknowledge the interrupts received
> > + and the amount of interrupts received. Xen PCI back acknowledges
> > + said interrupts only when they are level, shared with another
> > + guest, and enabled by the guest.
>
> That's not very nice sysfs file. This sort of thing ought to be in
> debugfs. Perhaps we shouldn't document it for now and try to move it to
> debugfs later?
Move it to debugfs now would be better, that's not an acceptable sysfs
file at all, thanks for pointing it out.
thanks,
greg k-h