| Message ID | 20150305210529.6393.29546.stgit@gimli.home (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 2015/3/6 5:06, Alex Williamson wrote: > The IRQ resource for a device is established when pci_enabled_device() > is called on a fully disabled device (ie. enable_cnt == 0). With > commit b4b55cda5874 ("x86/PCI: Refine the way to release PCI IRQ > resources") this same IRQ resource is released when the driver is > unbound from the device, regardless of the enable_cnt. This presents > the situation that an ill-behaved driver can now make a device > unusable to subsequent drivers by an imbalance in their use of > pci_enable/disable_device(). It's one thing to break your own device > if you're one of these ill-behaved drivers, but it's a serious > regression for secondary drivers like vfio-pci, which are innocent > of the transgressions of the previous driver. > > Resolve by pushing the device to a fully disabled state before > releasing the IRQ resource. > > Fixes: b4b55cda5874 ("x86/PCI: Refine the way to release PCI IRQ resources") > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > Cc: Jiang Liu <jiang.liu@linux.intel.com> > --- > arch/x86/pci/common.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > index 3d2612b..4810194 100644 > --- a/arch/x86/pci/common.c > +++ b/arch/x86/pci/common.c > @@ -527,8 +527,19 @@ static int pci_irq_notifier(struct notifier_block *nb, unsigned long action, > if (action != BUS_NOTIFY_UNBOUND_DRIVER) > return NOTIFY_DONE; > > - if (pcibios_disable_irq) > + if (pcibios_disable_irq) { > + /* > + * Broken drivers may allow a device to be .remove()'d while > + * still enabled. pci_enable_device() will only re-establish > + * dev->irq if the devices is fully disabled. So if we want > + * to release the IRQ, we need to make sure the next driver > + * can re-establish it using pci_enable_device(). > + */ > + while (pci_is_enabled(dev)) > + pci_disable_device(dev); > + > pcibios_disable_irq(dev); > + } Hi Alex, Thanks for debugging and fixing it. Will it be feasible to give a debug message to remind those driver authors to correctly disable PCI when unbinding? Regards! Gerry > > return NOTIFY_OK; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2015-03-06 at 09:49 +0800, Jiang Liu wrote: > On 2015/3/6 5:06, Alex Williamson wrote: > > The IRQ resource for a device is established when pci_enabled_device() > > is called on a fully disabled device (ie. enable_cnt == 0). With > > commit b4b55cda5874 ("x86/PCI: Refine the way to release PCI IRQ > > resources") this same IRQ resource is released when the driver is > > unbound from the device, regardless of the enable_cnt. This presents > > the situation that an ill-behaved driver can now make a device > > unusable to subsequent drivers by an imbalance in their use of > > pci_enable/disable_device(). It's one thing to break your own device > > if you're one of these ill-behaved drivers, but it's a serious > > regression for secondary drivers like vfio-pci, which are innocent > > of the transgressions of the previous driver. > > > > Resolve by pushing the device to a fully disabled state before > > releasing the IRQ resource. > > > > Fixes: b4b55cda5874 ("x86/PCI: Refine the way to release PCI IRQ resources") > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > Cc: Jiang Liu <jiang.liu@linux.intel.com> > > --- > > arch/x86/pci/common.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > > index 3d2612b..4810194 100644 > > --- a/arch/x86/pci/common.c > > +++ b/arch/x86/pci/common.c > > @@ -527,8 +527,19 @@ static int pci_irq_notifier(struct notifier_block *nb, unsigned long action, > > if (action != BUS_NOTIFY_UNBOUND_DRIVER) > > return NOTIFY_DONE; > > > > - if (pcibios_disable_irq) > > + if (pcibios_disable_irq) { > > + /* > > + * Broken drivers may allow a device to be .remove()'d while > > + * still enabled. pci_enable_device() will only re-establish > > + * dev->irq if the devices is fully disabled. So if we want > > + * to release the IRQ, we need to make sure the next driver > > + * can re-establish it using pci_enable_device(). > > + */ > > + while (pci_is_enabled(dev)) > > + pci_disable_device(dev); > > + > > pcibios_disable_irq(dev); > > + } > Hi Alex, > Thanks for debugging and fixing it. > Will it be feasible to give a debug message to remind those > driver authors to correctly disable PCI when unbinding? I can certainly add a warning to the loop, it loses a bit of its teeth here though since we can't specify which driver to blame at this point. Maybe that warning and perhaps this enabling roll-back should happen in drivers/pci/pci-driver.c:pci_device_remove(). Bjorn, would you prefer it be done generically there? Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2015-03-05 at 20:51 -0700, Alex Williamson wrote: > On Fri, 2015-03-06 at 09:49 +0800, Jiang Liu wrote: > > On 2015/3/6 5:06, Alex Williamson wrote: > > > The IRQ resource for a device is established when pci_enabled_device() > > > is called on a fully disabled device (ie. enable_cnt == 0). With > > > commit b4b55cda5874 ("x86/PCI: Refine the way to release PCI IRQ > > > resources") this same IRQ resource is released when the driver is > > > unbound from the device, regardless of the enable_cnt. This presents > > > the situation that an ill-behaved driver can now make a device > > > unusable to subsequent drivers by an imbalance in their use of > > > pci_enable/disable_device(). It's one thing to break your own device > > > if you're one of these ill-behaved drivers, but it's a serious > > > regression for secondary drivers like vfio-pci, which are innocent > > > of the transgressions of the previous driver. > > > > > > Resolve by pushing the device to a fully disabled state before > > > releasing the IRQ resource. > > > > > > Fixes: b4b55cda5874 ("x86/PCI: Refine the way to release PCI IRQ resources") > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > Cc: Jiang Liu <jiang.liu@linux.intel.com> > > > --- > > > arch/x86/pci/common.c | 13 ++++++++++++- > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > > > index 3d2612b..4810194 100644 > > > --- a/arch/x86/pci/common.c > > > +++ b/arch/x86/pci/common.c > > > @@ -527,8 +527,19 @@ static int pci_irq_notifier(struct notifier_block *nb, unsigned long action, > > > if (action != BUS_NOTIFY_UNBOUND_DRIVER) > > > return NOTIFY_DONE; > > > > > > - if (pcibios_disable_irq) > > > + if (pcibios_disable_irq) { > > > + /* > > > + * Broken drivers may allow a device to be .remove()'d while > > > + * still enabled. pci_enable_device() will only re-establish > > > + * dev->irq if the devices is fully disabled. So if we want > > > + * to release the IRQ, we need to make sure the next driver > > > + * can re-establish it using pci_enable_device(). > > > + */ > > > + while (pci_is_enabled(dev)) > > > + pci_disable_device(dev); > > > + > > > pcibios_disable_irq(dev); > > > + } > > Hi Alex, > > Thanks for debugging and fixing it. > > Will it be feasible to give a debug message to remind those > > driver authors to correctly disable PCI when unbinding? > > I can certainly add a warning to the loop, it loses a bit of its teeth > here though since we can't specify which driver to blame at this point. > Maybe that warning and perhaps this enabling roll-back should happen in > drivers/pci/pci-driver.c:pci_device_remove(). Bjorn, would you prefer > it be done generically there? Thanks, Unfortunately there's a long standing comment in pci_device_remove(): /* * We would love to complain here if pci_dev->is_enabled is set, that * the driver should have called pci_disable_device(), but the * unfortunate fact is there are too many odd BIOS and bridge setups * that don't like drivers doing that all of the time. * Oh well, we can dream of sane hardware when we sleep, no matter how * horrible the crap we have to deal with is when we are awake... */ So, unless we can somehow ignore that comment, I suspect forcing the device to be disabled on driver remove, whether done from pci-core or from x86/pci, is going to cause all sorts of breakage. Are the expectations set by b4b55cda5874 really valid? It seems like something needs to be done to allow the IRQ to be automatically re-established on x86 regardless of the driver doing the right thing when releasing the device. We're still looking at a regression for v4.0 as a result of b4b55cda5874. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, March 11, 2015 10:47:30 AM Alex Williamson wrote: > On Thu, 2015-03-05 at 20:51 -0700, Alex Williamson wrote: > > On Fri, 2015-03-06 at 09:49 +0800, Jiang Liu wrote: > > > On 2015/3/6 5:06, Alex Williamson wrote: > > > > The IRQ resource for a device is established when pci_enabled_device() > > > > is called on a fully disabled device (ie. enable_cnt == 0). With > > > > commit b4b55cda5874 ("x86/PCI: Refine the way to release PCI IRQ > > > > resources") this same IRQ resource is released when the driver is > > > > unbound from the device, regardless of the enable_cnt. This presents > > > > the situation that an ill-behaved driver can now make a device > > > > unusable to subsequent drivers by an imbalance in their use of > > > > pci_enable/disable_device(). It's one thing to break your own device > > > > if you're one of these ill-behaved drivers, but it's a serious > > > > regression for secondary drivers like vfio-pci, which are innocent > > > > of the transgressions of the previous driver. > > > > > > > > Resolve by pushing the device to a fully disabled state before > > > > releasing the IRQ resource. > > > > > > > > Fixes: b4b55cda5874 ("x86/PCI: Refine the way to release PCI IRQ resources") > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com> > > > > Cc: Jiang Liu <jiang.liu@linux.intel.com> > > > > --- > > > > arch/x86/pci/common.c | 13 ++++++++++++- > > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > > > > index 3d2612b..4810194 100644 > > > > --- a/arch/x86/pci/common.c > > > > +++ b/arch/x86/pci/common.c > > > > @@ -527,8 +527,19 @@ static int pci_irq_notifier(struct notifier_block *nb, unsigned long action, > > > > if (action != BUS_NOTIFY_UNBOUND_DRIVER) > > > > return NOTIFY_DONE; > > > > > > > > - if (pcibios_disable_irq) > > > > + if (pcibios_disable_irq) { > > > > + /* > > > > + * Broken drivers may allow a device to be .remove()'d while > > > > + * still enabled. pci_enable_device() will only re-establish > > > > + * dev->irq if the devices is fully disabled. So if we want > > > > + * to release the IRQ, we need to make sure the next driver > > > > + * can re-establish it using pci_enable_device(). > > > > + */ > > > > + while (pci_is_enabled(dev)) > > > > + pci_disable_device(dev); > > > > + > > > > pcibios_disable_irq(dev); > > > > + } > > > Hi Alex, > > > Thanks for debugging and fixing it. > > > Will it be feasible to give a debug message to remind those > > > driver authors to correctly disable PCI when unbinding? > > > > I can certainly add a warning to the loop, it loses a bit of its teeth > > here though since we can't specify which driver to blame at this point. > > Maybe that warning and perhaps this enabling roll-back should happen in > > drivers/pci/pci-driver.c:pci_device_remove(). Bjorn, would you prefer > > it be done generically there? Thanks, > > Unfortunately there's a long standing comment in pci_device_remove(): > > /* > * We would love to complain here if pci_dev->is_enabled is set, that > * the driver should have called pci_disable_device(), but the > * unfortunate fact is there are too many odd BIOS and bridge setups > * that don't like drivers doing that all of the time. > * Oh well, we can dream of sane hardware when we sleep, no matter how > * horrible the crap we have to deal with is when we are awake... > */ > > So, unless we can somehow ignore that comment, I suspect forcing the > device to be disabled on driver remove, whether done from pci-core or > from x86/pci, is going to cause all sorts of breakage. Are the > expectations set by b4b55cda5874 really valid? It seems like something > needs to be done to allow the IRQ to be automatically re-established on > x86 regardless of the driver doing the right thing when releasing the > device. We're still looking at a regression for v4.0 as a result of > b4b55cda5874. In which case we probably should revert commit b4b55cda5874 for the time being. At least I'd be very nervous about any ad-hoc fixes at this stage of the cycle. Gerry?
Pj4gVW5mb3J0dW5hdGVseSB0aGVyZSdzIGEgbG9uZyBzdGFuZGluZyBjb21tZW50IGluIHBjaV9k ZXZpY2VfcmVtb3ZlKCk6DQo+PiANCj4+ICAgICAgICAgLyoNCj4+ICAgICAgICAgICogV2Ugd291 bGQgbG92ZSB0byBjb21wbGFpbiBoZXJlIGlmIHBjaV9kZXYtPmlzX2VuYWJsZWQgaXMgc2V0LCB0 aGF0DQo+PiAgICAgICAgICAqIHRoZSBkcml2ZXIgc2hvdWxkIGhhdmUgY2FsbGVkIHBjaV9kaXNh YmxlX2RldmljZSgpLCBidXQgdGhlDQo+PiAgICAgICAgICAqIHVuZm9ydHVuYXRlIGZhY3QgaXMg dGhlcmUgYXJlIHRvbyBtYW55IG9kZCBCSU9TIGFuZCBicmlkZ2Ugc2V0dXBzDQo+PiAgICAgICAg ICAqIHRoYXQgZG9uJ3QgbGlrZSBkcml2ZXJzIGRvaW5nIHRoYXQgYWxsIG9mIHRoZSB0aW1lLg0K Pj4gICAgICAgICAgKiBPaCB3ZWxsLCB3ZSBjYW4gZHJlYW0gb2Ygc2FuZSBoYXJkd2FyZSB3aGVu IHdlIHNsZWVwLCBubyBtYXR0ZXIgaG93DQo+PiAgICAgICAgICAqIGhvcnJpYmxlIHRoZSBjcmFw IHdlIGhhdmUgdG8gZGVhbCB3aXRoIGlzIHdoZW4gd2UgYXJlIGF3YWtlLi4uDQo+PiAgICAgICAg ICAqLw0KPj4gDQo+PiBTbywgdW5sZXNzIHdlIGNhbiBzb21laG93IGlnbm9yZSB0aGF0IGNvbW1l bnQsIEkgc3VzcGVjdCBmb3JjaW5nIHRoZQ0KPj4gZGV2aWNlIHRvIGJlIGRpc2FibGVkIG9uIGRy aXZlciByZW1vdmUsIHdoZXRoZXIgZG9uZSBmcm9tIHBjaS1jb3JlIG9yDQo+PiBmcm9tIHg4Ni9w Y2ksIGlzIGdvaW5nIHRvIGNhdXNlIGFsbCBzb3J0cyBvZiBicmVha2FnZS4gIEFyZSB0aGUNCj4+ IGV4cGVjdGF0aW9ucyBzZXQgYnkgYjRiNTVjZGE1ODc0IHJlYWxseSB2YWxpZD8gIEl0IHNlZW1z IGxpa2Ugc29tZXRoaW5nDQo+PiBuZWVkcyB0byBiZSBkb25lIHRvIGFsbG93IHRoZSBJUlEgdG8g YmUgYXV0b21hdGljYWxseSByZS1lc3RhYmxpc2hlZCBvbg0KPj4geDg2IHJlZ2FyZGxlc3Mgb2Yg dGhlIGRyaXZlciBkb2luZyB0aGUgcmlnaHQgdGhpbmcgd2hlbiByZWxlYXNpbmcgdGhlDQo+PiBk ZXZpY2UuICBXZSdyZSBzdGlsbCBsb29raW5nIGF0IGEgcmVncmVzc2lvbiBmb3IgdjQuMCBhcyBh IHJlc3VsdCBvZg0KPj4gYjRiNTVjZGE1ODc0Lg0KPg0KPiBJbiB3aGljaCBjYXNlIHdlIHByb2Jh Ymx5IHNob3VsZCByZXZlcnQgY29tbWl0IGI0YjU1Y2RhNTg3NCBmb3IgdGhlIHRpbWUgYmVpbmcu DQo+DQo+IEF0IGxlYXN0IEknZCBiZSB2ZXJ5IG5lcnZvdXMgYWJvdXQgYW55IGFkLWhvYyBmaXhl cyBhdCB0aGlzIHN0YWdlIG9mIHRoZSBjeWNsZS4NCg0KVGhlIGNvbW1lbnQgZ29lcyBiYWNrIHRv IHRoZSBkYXduIG9mICJnaXQiIHRpbWUgLi4uIG5vdCBzdXJlIGhvdyBtdWNoIGZ1cnRoZXINCmJh Y2suDQoNCklzIHRoaXMgYWN0dWFsbHkgc3RpbGwgYW4gaXNzdWUgb24gbW9kZXJuIHN5c3RlbXM/ ICBNYXliZSB3ZSBuZWVkIGEgYmxhY2sgbGlzdA0Kb3Igd2hpdGUgbGlzdCB0byBzZXBhcmF0ZSB0 aGUgZ29vZCBmcm9tIGJhZCBzeXN0ZW1zPw0KDQotVG9ueQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, March 11, 2015 10:04:42 PM Luck, Tony wrote: > >> Unfortunately there's a long standing comment in pci_device_remove(): > >> > >> /* > >> * We would love to complain here if pci_dev->is_enabled is set, that > >> * the driver should have called pci_disable_device(), but the > >> * unfortunate fact is there are too many odd BIOS and bridge setups > >> * that don't like drivers doing that all of the time. > >> * Oh well, we can dream of sane hardware when we sleep, no matter how > >> * horrible the crap we have to deal with is when we are awake... > >> */ > >> > >> So, unless we can somehow ignore that comment, I suspect forcing the > >> device to be disabled on driver remove, whether done from pci-core or > >> from x86/pci, is going to cause all sorts of breakage. Are the > >> expectations set by b4b55cda5874 really valid? It seems like something > >> needs to be done to allow the IRQ to be automatically re-established on > >> x86 regardless of the driver doing the right thing when releasing the > >> device. We're still looking at a regression for v4.0 as a result of > >> b4b55cda5874. > > > > In which case we probably should revert commit b4b55cda5874 for the time being. > > > > At least I'd be very nervous about any ad-hoc fixes at this stage of the cycle. > > The comment goes back to the dawn of "git" time ... not sure how much further > back. > > Is this actually still an issue on modern systems? Maybe we need a black list > or white list to separate the good from bad systems? The answer to that is "We don't know" and in my not so humble opinion it is too risky to try to find out at the end of the cycle.
On 2015/3/12 9:17, Rafael J. Wysocki wrote: > On Wednesday, March 11, 2015 10:04:42 PM Luck, Tony wrote: >>>> Unfortunately there's a long standing comment in pci_device_remove(): >>>> >>>> /* >>>> * We would love to complain here if pci_dev->is_enabled is set, that >>>> * the driver should have called pci_disable_device(), but the >>>> * unfortunate fact is there are too many odd BIOS and bridge setups >>>> * that don't like drivers doing that all of the time. >>>> * Oh well, we can dream of sane hardware when we sleep, no matter how >>>> * horrible the crap we have to deal with is when we are awake... >>>> */ >>>> >>>> So, unless we can somehow ignore that comment, I suspect forcing the >>>> device to be disabled on driver remove, whether done from pci-core or >>>> from x86/pci, is going to cause all sorts of breakage. Are the >>>> expectations set by b4b55cda5874 really valid? It seems like something >>>> needs to be done to allow the IRQ to be automatically re-established on >>>> x86 regardless of the driver doing the right thing when releasing the >>>> device. We're still looking at a regression for v4.0 as a result of >>>> b4b55cda5874. >>> >>> In which case we probably should revert commit b4b55cda5874 for the time being. >>> >>> At least I'd be very nervous about any ad-hoc fixes at this stage of the cycle. >> >> The comment goes back to the dawn of "git" time ... not sure how much further >> back. >> >> Is this actually still an issue on modern systems? Maybe we need a black list >> or white list to separate the good from bad systems? > > The answer to that is "We don't know" and in my not so humble opinion it is too > risky to try to find out at the end of the cycle. Hi Rafael and Alex, How about a patch which: 1) gives a warning if PCI device is still enabled when unloading driver 2) release PCI interrupt only if PCI device is disabled. By this way, we could support IOAPIC hot-removal on latest platforms and avoid regressions on old platforms. Thanks! Gerry -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, March 12, 2015 09:41:21 AM Jiang Liu wrote: > On 2015/3/12 9:17, Rafael J. Wysocki wrote: > > On Wednesday, March 11, 2015 10:04:42 PM Luck, Tony wrote: > >>>> Unfortunately there's a long standing comment in pci_device_remove(): > >>>> > >>>> /* > >>>> * We would love to complain here if pci_dev->is_enabled is set, that > >>>> * the driver should have called pci_disable_device(), but the > >>>> * unfortunate fact is there are too many odd BIOS and bridge setups > >>>> * that don't like drivers doing that all of the time. > >>>> * Oh well, we can dream of sane hardware when we sleep, no matter how > >>>> * horrible the crap we have to deal with is when we are awake... > >>>> */ > >>>> > >>>> So, unless we can somehow ignore that comment, I suspect forcing the > >>>> device to be disabled on driver remove, whether done from pci-core or > >>>> from x86/pci, is going to cause all sorts of breakage. Are the > >>>> expectations set by b4b55cda5874 really valid? It seems like something > >>>> needs to be done to allow the IRQ to be automatically re-established on > >>>> x86 regardless of the driver doing the right thing when releasing the > >>>> device. We're still looking at a regression for v4.0 as a result of > >>>> b4b55cda5874. > >>> > >>> In which case we probably should revert commit b4b55cda5874 for the time being. > >>> > >>> At least I'd be very nervous about any ad-hoc fixes at this stage of the cycle. > >> > >> The comment goes back to the dawn of "git" time ... not sure how much further > >> back. > >> > >> Is this actually still an issue on modern systems? Maybe we need a black list > >> or white list to separate the good from bad systems? > > > > The answer to that is "We don't know" and in my not so humble opinion it is too > > risky to try to find out at the end of the cycle. > Hi Rafael and Alex, > How about a patch which: > 1) gives a warning if PCI device is still enabled when unloading driver That may become sort of noisy. I really would prefer to introduce things like that by the beginning of the cycle, not by the end of it. > 2) release PCI interrupt only if PCI device is disabled. > By this way, we could support IOAPIC hot-removal on latest platforms and > avoid regressions on old platforms. Well, please submit a patch for discussion. I would like to know Bjorn's opinion about that too at least. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/3/13 0:08, Rafael J. Wysocki wrote: > On Thursday, March 12, 2015 09:41:21 AM Jiang Liu wrote: >> On 2015/3/12 9:17, Rafael J. Wysocki wrote: >>> On Wednesday, March 11, 2015 10:04:42 PM Luck, Tony wrote: >>>>>> Unfortunately there's a long standing comment in pci_device_remove(): >>>>>> >>>>>> /* >>>>>> * We would love to complain here if pci_dev->is_enabled is set, that >>>>>> * the driver should have called pci_disable_device(), but the >>>>>> * unfortunate fact is there are too many odd BIOS and bridge setups >>>>>> * that don't like drivers doing that all of the time. >>>>>> * Oh well, we can dream of sane hardware when we sleep, no matter how >>>>>> * horrible the crap we have to deal with is when we are awake... >>>>>> */ >>>>>> >>>>>> So, unless we can somehow ignore that comment, I suspect forcing the >>>>>> device to be disabled on driver remove, whether done from pci-core or >>>>>> from x86/pci, is going to cause all sorts of breakage. Are the >>>>>> expectations set by b4b55cda5874 really valid? It seems like something >>>>>> needs to be done to allow the IRQ to be automatically re-established on >>>>>> x86 regardless of the driver doing the right thing when releasing the >>>>>> device. We're still looking at a regression for v4.0 as a result of >>>>>> b4b55cda5874. >>>>> >>>>> In which case we probably should revert commit b4b55cda5874 for the time being. >>>>> >>>>> At least I'd be very nervous about any ad-hoc fixes at this stage of the cycle. >>>> >>>> The comment goes back to the dawn of "git" time ... not sure how much further >>>> back. >>>> >>>> Is this actually still an issue on modern systems? Maybe we need a black list >>>> or white list to separate the good from bad systems? >>> >>> The answer to that is "We don't know" and in my not so humble opinion it is too >>> risky to try to find out at the end of the cycle. >> Hi Rafael and Alex, >> How about a patch which: >> 1) gives a warning if PCI device is still enabled when unloading driver > > That may become sort of noisy. I really would prefer to introduce things like > that by the beginning of the cycle, not by the end of it. Will try this on next merging window. >> 2) release PCI interrupt only if PCI device is disabled. >> By this way, we could support IOAPIC hot-removal on latest platforms and >> avoid regressions on old platforms. > > Well, please submit a patch for discussion. > > I would like to know Bjorn's opinion about that too at least. Still testing the patch, will send it out soon. > > Rafael > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c index 3d2612b..4810194 100644 --- a/arch/x86/pci/common.c +++ b/arch/x86/pci/common.c @@ -527,8 +527,19 @@ static int pci_irq_notifier(struct notifier_block *nb, unsigned long action, if (action != BUS_NOTIFY_UNBOUND_DRIVER) return NOTIFY_DONE; - if (pcibios_disable_irq) + if (pcibios_disable_irq) { + /* + * Broken drivers may allow a device to be .remove()'d while + * still enabled. pci_enable_device() will only re-establish + * dev->irq if the devices is fully disabled. So if we want + * to release the IRQ, we need to make sure the next driver + * can re-establish it using pci_enable_device(). + */ + while (pci_is_enabled(dev)) + pci_disable_device(dev); + pcibios_disable_irq(dev); + } return NOTIFY_OK; }
The IRQ resource for a device is established when pci_enabled_device() is called on a fully disabled device (ie. enable_cnt == 0). With commit b4b55cda5874 ("x86/PCI: Refine the way to release PCI IRQ resources") this same IRQ resource is released when the driver is unbound from the device, regardless of the enable_cnt. This presents the situation that an ill-behaved driver can now make a device unusable to subsequent drivers by an imbalance in their use of pci_enable/disable_device(). It's one thing to break your own device if you're one of these ill-behaved drivers, but it's a serious regression for secondary drivers like vfio-pci, which are innocent of the transgressions of the previous driver. Resolve by pushing the device to a fully disabled state before releasing the IRQ resource. Fixes: b4b55cda5874 ("x86/PCI: Refine the way to release PCI IRQ resources") Signed-off-by: Alex Williamson <alex.williamson@redhat.com> Cc: Jiang Liu <jiang.liu@linux.intel.com> --- arch/x86/pci/common.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html