[PATCH] x86/PCI: Fully disable devices before releasing IRQ resource

[PATCH] x86/PCI: Fully disable devices before releasing IRQ resource

[Alex Williamson]

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);
+	}
 
 	return NOTIFY_OK;
 }

Re: [PATCH] x86/PCI: Fully disable devices before releasing IRQ resource

[Jiang Liu]

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;
>  }
> 

Re: [PATCH] x86/PCI: Fully disable devices before releasing IRQ resource

[Alex Williamson]

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

Re: [PATCH] x86/PCI: Fully disable devices before releasing IRQ resource

[Alex Williamson]

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

Re: [PATCH] x86/PCI: Fully disable devices before releasing IRQ resource

[Rafael J. Wysocki]

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?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH] x86/PCI: Fully disable devices before releasing IRQ resource

[Luck, Tony]

>> 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?

-Tony

RE: [PATCH] x86/PCI: Fully disable devices before releasing IRQ resource

[Luck, Tony]

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

RE: [PATCH] x86/PCI: Fully disable devices before releasing IRQ resource

[Luck, Tony]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1647 bytes --]

>> 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?

-Tony
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] x86/PCI: Fully disable devices before releasing IRQ resource

[Rafael J. Wysocki]

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.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] x86/PCI: Fully disable devices before releasing IRQ resource

[Jiang Liu]

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

Re: [PATCH] x86/PCI: Fully disable devices before releasing IRQ resource

[Rafael J. Wysocki]

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

Re: [PATCH] x86/PCI: Fully disable devices before releasing IRQ resource

[Jiang Liu]

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
> 

[Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding

[Jiang Liu]

To support IOAPIC hot-removal, we need to release PCI interrupt resource
when unbinding PCI device driver. But due to historical reason,
/*
 * 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...
 */
some drivers don't call pci_disable_device() when unloading, which
prevents us from reallocating PCI interrupt resource on reloading
PCI driver and causes regressions.

So release PCI interrupt resource only if PCI device is disabled when
unbinding. By this way, we could support IOAPIC hot-removal on latest
platforms and avoid regressions on old platforms.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 arch/x86/pci/common.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 3d2612b68694..8d792142cb2a 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -527,7 +527,7 @@ 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 (!pci_is_enabled(dev) && pcibios_disable_irq)
 		pcibios_disable_irq(dev);
 
 	return NOTIFY_OK;
-- 
1.7.10.4

Re: [Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding

[Rafael J. Wysocki]

On Friday, March 13, 2015 10:06:43 AM Jiang Liu wrote:
> To support IOAPIC hot-removal, we need to release PCI interrupt resource
> when unbinding PCI device driver. But due to historical reason,
> /*
>  * 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...
>  */
> some drivers don't call pci_disable_device() when unloading, which
> prevents us from reallocating PCI interrupt resource on reloading
> PCI driver and causes regressions.
> 
> So release PCI interrupt resource only if PCI device is disabled when
> unbinding. By this way, we could support IOAPIC hot-removal on latest
> platforms and avoid regressions on old platforms.
> 
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>

OK, I can agree with that.

Bjorn, what do you think?

> ---
>  arch/x86/pci/common.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index 3d2612b68694..8d792142cb2a 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -527,7 +527,7 @@ 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 (!pci_is_enabled(dev) && pcibios_disable_irq)
>  		pcibios_disable_irq(dev);
>  
>  	return NOTIFY_OK;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding

[Jiang Liu]

To support IOAPIC hot-removal, we need to release PCI interrupt resource
when unbinding PCI device driver. But due to historical reason,
/*
 * 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...
 */
some drivers don't call pci_disable_device() when unloading, which
prevents us from reallocating PCI interrupt resource on reloading
PCI driver and causes regressions.

So release PCI interrupt resource only if PCI device is disabled when
unbinding. By this way, we could support IOAPIC hot-removal on latest
platforms and avoid regressions on old platforms.

Please aslo refer to:
https://bugzilla.kernel.org/show_bug.cgi?id=94721

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
Reported-by: Alex Williamson <alex.williamson@redhat.com>
Reported-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Rafael J. Wysocki <rjw@rjwysocki.net>
---
Hi Rafael,
	I have assumed an Reviewed-by from you, is that OK?
Thanks!
Gerry
---
 arch/x86/pci/common.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 3d2612b68694..8d792142cb2a 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -527,7 +527,7 @@ 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 (!pci_is_enabled(dev) && pcibios_disable_irq)
 		pcibios_disable_irq(dev);
 
 	return NOTIFY_OK;
-- 
1.7.10.4

Re: [Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding

[Bjorn Helgaas]

On Tue, Mar 17, 2015 at 03:37:12PM +0800, Jiang Liu wrote:
> To support IOAPIC hot-removal, we need to release PCI interrupt resource
> when unbinding PCI device driver. But due to historical reason,
> /*
>  * 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...
>  */

Quoting the comment here (especially the last two lines) is overkill and
obscures the real point.  The important thing is that some drivers have
legitimate reasons for not calling pci_disable_device().

> some drivers don't call pci_disable_device() when unloading, which
> prevents us from reallocating PCI interrupt resource on reloading
> PCI driver and causes regressions.

This isn't very clear.  I can believe that "drivers not calling
pci_disable_device()" means we don't release IRQ resources, which might
prevent you from hot-removing an IOAPIC.

But "drivers not calling pci_disable_device()" doesn't cause regressions.

> So release PCI interrupt resource only if PCI device is disabled when
> unbinding. By this way, we could support IOAPIC hot-removal on latest
> platforms and avoid regressions on old platforms.

Does this mean you can only hot-remove IOAPICs if all drivers for devices
using the IOAPIC call pci_disable_device()?  If so, it seems sort of
dubious that we have to rely on drivers for that.

What happens if we try to hot-remove an IOAPIC where we haven't released
all the IRQ resources?  Is there a nice error message that will help us
debug problem reports?

This has nothing to do with "latest platforms" and "old platforms."  That
text pretends to convey information, but it doesn't.  To be useful, it
would have to say something specific about how "latest" and "old" platforms
are different.

I haven't even figured out what causes the regressions yet.  I guess maybe
it's the fact that after b4b55cda5874, we always call pcibios_disable_irq(),
while before we only called it if the driver used pci_disable_device()?
The changelog should be clear about this.

> Please aslo refer to:

"also"

> https://bugzilla.kernel.org/show_bug.cgi?id=94721
> 

This apparently fixes something and needs a Fixes: tag to help people who
might backport the broken commit.

> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> Reported-by: Alex Williamson <alex.williamson@redhat.com>
> Reported-by: Thomas Hellstrom <thellstrom@vmware.com>
> Reviewed-by: Rafael J. Wysocki <rjw@rjwysocki.net>
> ---
> Hi Rafael,
> 	I have assumed an Reviewed-by from you, is that OK?
> Thanks!
> Gerry
> ---
>  arch/x86/pci/common.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index 3d2612b68694..8d792142cb2a 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -527,7 +527,7 @@ static int pci_irq_notifier(struct notifier_block *nb, unsigned long action,

I know the notifier was added by b4b55cda5874, not this patch, but I don't
think it's the best mechanism.  I would rather do something like calling
pcibios_disable_irq() directly from pci_device_remove().  That way the call
is more explicit, it's in arch-independent code, and it's more parallel
with how we call pcibios_enable_irq() in the pci_enable_device() path.

This code is all x86-specific.  But other arches use IOAPIC, and there's
nothing obviously x86-specific here.  Won't they still have issues here?

>  	if (action != BUS_NOTIFY_UNBOUND_DRIVER)
>  		return NOTIFY_DONE;
>  
> -	if (pcibios_disable_irq)
> +	if (!pci_is_enabled(dev) && pcibios_disable_irq)
>  		pcibios_disable_irq(dev);
>  
>  	return NOTIFY_OK;
> -- 
> 1.7.10.4
> 

Re: [Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding

[Jiang Liu]

On 2015/3/19 6:11, Bjorn Helgaas wrote:
> On Tue, Mar 17, 2015 at 03:37:12PM +0800, Jiang Liu wrote:
>> To support IOAPIC hot-removal, we need to release PCI interrupt resource
>> when unbinding PCI device driver. But due to historical reason,
>> /*
>>  * 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...
>>  */
> 
> Quoting the comment here (especially the last two lines) is overkill and
> obscures the real point.  The important thing is that some drivers have
> legitimate reasons for not calling pci_disable_device().
Hi Bjorn,
	Thanks for review. I will rewrite the commit message.
>> some drivers don't call pci_disable_device() when unloading, which
>> prevents us from reallocating PCI interrupt resource on reloading
>> PCI driver and causes regressions.
> 
> This isn't very clear.  I can believe that "drivers not calling
> pci_disable_device()" means we don't release IRQ resources, which might
> prevent you from hot-removing an IOAPIC.
> 
> But "drivers not calling pci_disable_device()" doesn't cause regressions.
> 
>> So release PCI interrupt resource only if PCI device is disabled when
>> unbinding. By this way, we could support IOAPIC hot-removal on latest
>> platforms and avoid regressions on old platforms.
> 
> Does this mean you can only hot-remove IOAPICs if all drivers for devices
> using the IOAPIC call pci_disable_device()?  If so, it seems sort of
> dubious that we have to rely on drivers for that.
This is a quickfix for v4.0 merging window. We will try to solve this
issue for next merging window.

> 
> What happens if we try to hot-remove an IOAPIC where we haven't released
> all the IRQ resources?  Is there a nice error message that will help us
> debug problem reports?
> 
> This has nothing to do with "latest platforms" and "old platforms."  That
> text pretends to convey information, but it doesn't.  To be useful, it
> would have to say something specific about how "latest" and "old" platforms
> are different.
> 
> I haven't even figured out what causes the regressions yet.  I guess maybe
> it's the fact that after b4b55cda5874, we always call pcibios_disable_irq(),
> while before we only called it if the driver used pci_disable_device()?
> The changelog should be clear about this.
> 
>> Please aslo refer to:
> 
> "also"
> 
>> https://bugzilla.kernel.org/show_bug.cgi?id=94721
>>
> 
> This apparently fixes something and needs a Fixes: tag to help people who
> might backport the broken commit.
> 
>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>> Reported-by: Alex Williamson <alex.williamson@redhat.com>
>> Reported-by: Thomas Hellstrom <thellstrom@vmware.com>
>> Reviewed-by: Rafael J. Wysocki <rjw@rjwysocki.net>
>> ---
>> Hi Rafael,
>> 	I have assumed an Reviewed-by from you, is that OK?
>> Thanks!
>> Gerry
>> ---
>>  arch/x86/pci/common.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
>> index 3d2612b68694..8d792142cb2a 100644
>> --- a/arch/x86/pci/common.c
>> +++ b/arch/x86/pci/common.c
>> @@ -527,7 +527,7 @@ static int pci_irq_notifier(struct notifier_block *nb, unsigned long action,
> 
> I know the notifier was added by b4b55cda5874, not this patch, but I don't
> think it's the best mechanism.  I would rather do something like calling
> pcibios_disable_irq() directly from pci_device_remove().  That way the call
> is more explicit, it's in arch-independent code, and it's more parallel
> with how we call pcibios_enable_irq() in the pci_enable_device() path.
> 
> This code is all x86-specific.  But other arches use IOAPIC, and there's
> nothing obviously x86-specific here.  Won't they still have issues here?
pcibios_enable_irq() and pcibios_disable_irq() currently are x86
specific, so I tried to keep it x86 specific.
On the other hand, we want to release IOAPIC pin when a PCI device gets
unused instead of getting removed, so just assign IRQ number for PCI
devices in use.

How about this commit message?
------------------------------------------------------------------
x86/PCI: Release PCI IRQ resource only if PCI device is disabled when
unbinding

To support IOAPIC hot-removal, we need to track IOAPIC pin usage, which
is to allocate pin on demand and release pin when unused. According to
the original design should allocate and release IOAPIC pin as below:
pci_enable_device()
    -> pcibios_enable_device() when pci_dev->enable_cnt changing from 0 to 1
        ->pcibios_enable_irq()
            ->allocate IOAPIC pin
pci_disable_device()
    -> pcibios_disable_device() when pci_dev->enable_cnt changing from 1
to 0
        ->pcibios_disable_irq()
            ->release IOAPIC pin

But above design conclicts with PCI PM design. When suspending, PCI
device driver may call pci_disable_device() and eventually release
IOAPIC pin. When resuming, PCI device driver call pci_enable_device()
and reallocating IOAPIC pin. Since v3.19, IOAPIC driver dynamically
allocates IRQ number for IOAPIC pin. So when resuming, a different
IRQ number may assigned, which breaks some PCI drivers' suspend/resume
implementation.

So commit ("x86/PCI: Refine the way to release PCI IRQ resources")
tries to fix PM regressions by releasing IOAPIC pin when unbinding
PCI driver unconditionally, which causes new regressions one some
old platforms because:
1) some PCI device drivers skip calling pci_disable_device() when
   unbinding due to BIOS flaws, which causing non-zero
   pci_dev->enable_cnt after driver unbinding.
2) pci_enable_device() doesn't call pcibios_enable_irq() because
   pci_dev->enable_cnt is not zero when rebinding device driver,
   thus no IOAPIC pin(IRQ number) assigned to PCI device after rebinding.

This patch implements a quick workaround which releases IOAPIC iff
pci_dev->enable_cnt is zero after driver unbinding. A better solution
should be to make IOAPIC allocation/releasing symmetric,
1) calling pcibios_enable_irq() on BUS_NOTIFY_BIND_DRIVER notification
2) calling pcibios_disable_irq() on BUS_NOTIFY_UNBOUND_DRIVER notification
So we could make IOAPIC pin allocation/releasing independent of
pci_dev->enable_cnt. We will try the symmetric solution for next merge
window.

Please also refer to:
https://bugzilla.kernel.org/show_bug.cgi?id=94721

Fixes: b4b55cda5874("x86/PCI: Refine the way to release PCI IRQ resources")
------------------------------------------------------------------
Thanks!
Gerry
> 
>>  	if (action != BUS_NOTIFY_UNBOUND_DRIVER)
>>  		return NOTIFY_DONE;
>>  
>> -	if (pcibios_disable_irq)
>> +	if (!pci_is_enabled(dev) && pcibios_disable_irq)
>>  		pcibios_disable_irq(dev);
>>  
>>  	return NOTIFY_OK;
>> -- 
>> 1.7.10.4
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding

[Rafael J. Wysocki]

On Thursday, March 19, 2015 03:49:33 PM Jiang Liu wrote:
> On 2015/3/19 6:11, Bjorn Helgaas wrote:
> > On Tue, Mar 17, 2015 at 03:37:12PM +0800, Jiang Liu wrote:
> >> To support IOAPIC hot-removal, we need to release PCI interrupt resource
> >> when unbinding PCI device driver. But due to historical reason,
> >> /*
> >>  * 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...
> >>  */
> > 
> > Quoting the comment here (especially the last two lines) is overkill and
> > obscures the real point.  The important thing is that some drivers have
> > legitimate reasons for not calling pci_disable_device().
> Hi Bjorn,
> 	Thanks for review. I will rewrite the commit message.
> >> some drivers don't call pci_disable_device() when unloading, which
> >> prevents us from reallocating PCI interrupt resource on reloading
> >> PCI driver and causes regressions.
> > 
> > This isn't very clear.  I can believe that "drivers not calling
> > pci_disable_device()" means we don't release IRQ resources, which might
> > prevent you from hot-removing an IOAPIC.
> > 
> > But "drivers not calling pci_disable_device()" doesn't cause regressions.
> > 
> >> So release PCI interrupt resource only if PCI device is disabled when
> >> unbinding. By this way, we could support IOAPIC hot-removal on latest
> >> platforms and avoid regressions on old platforms.
> > 
> > Does this mean you can only hot-remove IOAPICs if all drivers for devices
> > using the IOAPIC call pci_disable_device()?  If so, it seems sort of
> > dubious that we have to rely on drivers for that.
> This is a quickfix for v4.0 merging window. We will try to solve this
> issue for next merging window.

If that is the plan, then I'd rather revert the offending commit and try
again in the next cycle.

Bjorn, what do you think?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding

[Bjorn Helgaas]

On Thu, Mar 19, 2015 at 6:29 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, March 19, 2015 03:49:33 PM Jiang Liu wrote:
>> On 2015/3/19 6:11, Bjorn Helgaas wrote:
>> > On Tue, Mar 17, 2015 at 03:37:12PM +0800, Jiang Liu wrote:
>> >> To support IOAPIC hot-removal, we need to release PCI interrupt resource
>> >> when unbinding PCI device driver. But due to historical reason,
>> >> /*
>> >>  * 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...
>> >>  */
>> >
>> > Quoting the comment here (especially the last two lines) is overkill and
>> > obscures the real point.  The important thing is that some drivers have
>> > legitimate reasons for not calling pci_disable_device().
>> Hi Bjorn,
>>       Thanks for review. I will rewrite the commit message.
>> >> some drivers don't call pci_disable_device() when unloading, which
>> >> prevents us from reallocating PCI interrupt resource on reloading
>> >> PCI driver and causes regressions.
>> >
>> > This isn't very clear.  I can believe that "drivers not calling
>> > pci_disable_device()" means we don't release IRQ resources, which might
>> > prevent you from hot-removing an IOAPIC.
>> >
>> > But "drivers not calling pci_disable_device()" doesn't cause regressions.
>> >
>> >> So release PCI interrupt resource only if PCI device is disabled when
>> >> unbinding. By this way, we could support IOAPIC hot-removal on latest
>> >> platforms and avoid regressions on old platforms.
>> >
>> > Does this mean you can only hot-remove IOAPICs if all drivers for devices
>> > using the IOAPIC call pci_disable_device()?  If so, it seems sort of
>> > dubious that we have to rely on drivers for that.
>> This is a quickfix for v4.0 merging window. We will try to solve this
>> issue for next merging window.
>
> If that is the plan, then I'd rather revert the offending commit and try
> again in the next cycle.
>
> Bjorn, what do you think?

I don't know how hard it is to just revert that one commit at this
point, but I would be in favor of doing that if it's feasible.

We're headed toward a real morass of changelogs for a design that
seems destined for overhaul.  That makes it really hard to backport
and rework things later.

>From the revised changelog:

>> When suspending, PCI
>> device driver may call pci_disable_device() and eventually release
>> IOAPIC pin. When resuming, PCI device driver call pci_enable_device()
>> and reallocating IOAPIC pin. Since v3.19, IOAPIC driver dynamically
>> allocates IRQ number for IOAPIC pin. So when resuming, a different
>> IRQ number may assigned, which breaks some PCI drivers' suspend/resume
>> implementation.

It seems like you're really standing on your head to make this
situation work, and I think the result is too complicated and
error-prone.  One test is to see whether you can write a short, simple
description of how driver writers need to manage IRQs with respect to
probe/remove/suspend/remove.

There are two other possibilities I can see:

1) Decide that a driver that captures the IRQ and then calls
pci_enable_device() is just broken, and fix those drivers to
re-capture the IRQ every time they call pci_enable_device().  I assume
you've looked at this already and concluded it's not practical?

2) Configure the IRQ in pci_device_probe().  Then it would be
configured before the driver sees the device, and you could dispose of
it in pci_device_remove() when the driver is unbound.

Does either of those make sense?

Bjorn

Re: [Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding

[Rafael J. Wysocki]

On Thursday, March 19, 2015 09:08:38 AM Bjorn Helgaas wrote:
> On Thu, Mar 19, 2015 at 6:29 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, March 19, 2015 03:49:33 PM Jiang Liu wrote:
> >> On 2015/3/19 6:11, Bjorn Helgaas wrote:
> >> > On Tue, Mar 17, 2015 at 03:37:12PM +0800, Jiang Liu wrote:
> >> >> To support IOAPIC hot-removal, we need to release PCI interrupt resource
> >> >> when unbinding PCI device driver. But due to historical reason,
> >> >> /*
> >> >>  * 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...
> >> >>  */
> >> >
> >> > Quoting the comment here (especially the last two lines) is overkill and
> >> > obscures the real point.  The important thing is that some drivers have
> >> > legitimate reasons for not calling pci_disable_device().
> >> Hi Bjorn,
> >>       Thanks for review. I will rewrite the commit message.
> >> >> some drivers don't call pci_disable_device() when unloading, which
> >> >> prevents us from reallocating PCI interrupt resource on reloading
> >> >> PCI driver and causes regressions.
> >> >
> >> > This isn't very clear.  I can believe that "drivers not calling
> >> > pci_disable_device()" means we don't release IRQ resources, which might
> >> > prevent you from hot-removing an IOAPIC.
> >> >
> >> > But "drivers not calling pci_disable_device()" doesn't cause regressions.
> >> >
> >> >> So release PCI interrupt resource only if PCI device is disabled when
> >> >> unbinding. By this way, we could support IOAPIC hot-removal on latest
> >> >> platforms and avoid regressions on old platforms.
> >> >
> >> > Does this mean you can only hot-remove IOAPICs if all drivers for devices
> >> > using the IOAPIC call pci_disable_device()?  If so, it seems sort of
> >> > dubious that we have to rely on drivers for that.
> >> This is a quickfix for v4.0 merging window. We will try to solve this
> >> issue for next merging window.
> >
> > If that is the plan, then I'd rather revert the offending commit and try
> > again in the next cycle.
> >
> > Bjorn, what do you think?
> 
> I don't know how hard it is to just revert that one commit at this
> point, but I would be in favor of doing that if it's feasible.

The commit reverts cleanly and reverting it won't break anything that used to
work in 3.19 and earlier (Gerry, please let me know if that is not correct).

The only adverse consequence of reverting it I can see would be that the
IOAPIC hotplug won't work in 4.0, but it didn't work before either and
it's supposed to be a new feature in 4.0.

> We're headed toward a real morass of changelogs for a design that
> seems destined for overhaul.  That makes it really hard to backport
> and rework things later.

Precisely.

Rafael

Re: [Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding

[Jiang Liu]

On 2015/3/19 23:57, Rafael J. Wysocki wrote:
> On Thursday, March 19, 2015 09:08:38 AM Bjorn Helgaas wrote:
>> On Thu, Mar 19, 2015 at 6:29 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>> On Thursday, March 19, 2015 03:49:33 PM Jiang Liu wrote:
>>>> On 2015/3/19 6:11, Bjorn Helgaas wrote:
>>>>> On Tue, Mar 17, 2015 at 03:37:12PM +0800, Jiang Liu wrote:
>>>>>> To support IOAPIC hot-removal, we need to release PCI interrupt resource
>>>>>> when unbinding PCI device driver. But due to historical reason,
>>>>>> /*
>>>>>>  * 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...
>>>>>>  */
>>>>>
>>>>> Quoting the comment here (especially the last two lines) is overkill and
>>>>> obscures the real point.  The important thing is that some drivers have
>>>>> legitimate reasons for not calling pci_disable_device().
>>>> Hi Bjorn,
>>>>       Thanks for review. I will rewrite the commit message.
>>>>>> some drivers don't call pci_disable_device() when unloading, which
>>>>>> prevents us from reallocating PCI interrupt resource on reloading
>>>>>> PCI driver and causes regressions.
>>>>>
>>>>> This isn't very clear.  I can believe that "drivers not calling
>>>>> pci_disable_device()" means we don't release IRQ resources, which might
>>>>> prevent you from hot-removing an IOAPIC.
>>>>>
>>>>> But "drivers not calling pci_disable_device()" doesn't cause regressions.
>>>>>
>>>>>> So release PCI interrupt resource only if PCI device is disabled when
>>>>>> unbinding. By this way, we could support IOAPIC hot-removal on latest
>>>>>> platforms and avoid regressions on old platforms.
>>>>>
>>>>> Does this mean you can only hot-remove IOAPICs if all drivers for devices
>>>>> using the IOAPIC call pci_disable_device()?  If so, it seems sort of
>>>>> dubious that we have to rely on drivers for that.
>>>> This is a quickfix for v4.0 merging window. We will try to solve this
>>>> issue for next merging window.
>>>
>>> If that is the plan, then I'd rather revert the offending commit and try
>>> again in the next cycle.
>>>
>>> Bjorn, what do you think?
>>
>> I don't know how hard it is to just revert that one commit at this
>> point, but I would be in favor of doing that if it's feasible.
> 
> The commit reverts cleanly and reverting it won't break anything that used to
> work in 3.19 and earlier (Gerry, please let me know if that is not correct).
Yes, revert should not cause new issues.
Commit b4b55cda5874("Refine the way to release PCI IRQ resources")
is a bugfix for xen-pciback. But the bugfix causes regressions on
other platform. So it would be better to revert it and fix the issue
in another better way in next merging window.

> 
> The only adverse consequence of reverting it I can see would be that the
> IOAPIC hotplug won't work in 4.0, but it didn't work before either and
> it's supposed to be a new feature in 4.0.
IOAPIC hotplug may still work, it only causes regressions to some PCI
drivers.

> 
>> We're headed toward a real morass of changelogs for a design that
>> seems destined for overhaul.  That makes it really hard to backport
>> and rework things later.
> 
> Precisely.
Sorry for the troubles.
When designing IOAPIC hotplug, I found architect has provided suitable
hook points for IOAPIC pin usage track, so I adopted hook points in
pci_enable/disable_device(). But recent regression reports remind
me that's wrong decision, so will rework it in new way.
Thanks!
Gerry
> 
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding

[Rafael J. Wysocki]

On Friday, March 20, 2015 01:40:46 PM Jiang Liu wrote:
> On 2015/3/19 23:57, Rafael J. Wysocki wrote:
> > On Thursday, March 19, 2015 09:08:38 AM Bjorn Helgaas wrote:
> >> On Thu, Mar 19, 2015 at 6:29 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>> On Thursday, March 19, 2015 03:49:33 PM Jiang Liu wrote:
> >>>> On 2015/3/19 6:11, Bjorn Helgaas wrote:
> >>>>> On Tue, Mar 17, 2015 at 03:37:12PM +0800, Jiang Liu wrote:
> >>>>>> To support IOAPIC hot-removal, we need to release PCI interrupt resource
> >>>>>> when unbinding PCI device driver. But due to historical reason,
> >>>>>> /*
> >>>>>>  * 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...
> >>>>>>  */
> >>>>>
> >>>>> Quoting the comment here (especially the last two lines) is overkill and
> >>>>> obscures the real point.  The important thing is that some drivers have
> >>>>> legitimate reasons for not calling pci_disable_device().
> >>>> Hi Bjorn,
> >>>>       Thanks for review. I will rewrite the commit message.
> >>>>>> some drivers don't call pci_disable_device() when unloading, which
> >>>>>> prevents us from reallocating PCI interrupt resource on reloading
> >>>>>> PCI driver and causes regressions.
> >>>>>
> >>>>> This isn't very clear.  I can believe that "drivers not calling
> >>>>> pci_disable_device()" means we don't release IRQ resources, which might
> >>>>> prevent you from hot-removing an IOAPIC.
> >>>>>
> >>>>> But "drivers not calling pci_disable_device()" doesn't cause regressions.
> >>>>>
> >>>>>> So release PCI interrupt resource only if PCI device is disabled when
> >>>>>> unbinding. By this way, we could support IOAPIC hot-removal on latest
> >>>>>> platforms and avoid regressions on old platforms.
> >>>>>
> >>>>> Does this mean you can only hot-remove IOAPICs if all drivers for devices
> >>>>> using the IOAPIC call pci_disable_device()?  If so, it seems sort of
> >>>>> dubious that we have to rely on drivers for that.
> >>>> This is a quickfix for v4.0 merging window. We will try to solve this
> >>>> issue for next merging window.
> >>>
> >>> If that is the plan, then I'd rather revert the offending commit and try
> >>> again in the next cycle.
> >>>
> >>> Bjorn, what do you think?
> >>
> >> I don't know how hard it is to just revert that one commit at this
> >> point, but I would be in favor of doing that if it's feasible.
> > 
> > The commit reverts cleanly and reverting it won't break anything that used to
> > work in 3.19 and earlier (Gerry, please let me know if that is not correct).
> Yes, revert should not cause new issues.
> Commit b4b55cda5874("Refine the way to release PCI IRQ resources")
> is a bugfix for xen-pciback. But the bugfix causes regressions on
> other platform. So it would be better to revert it and fix the issue
> in another better way in next merging window.

OK, I've queued up a revert of b4b55cda5874 and I'm going to push it to Linus
for 4.0-rc5 later today.

Thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding

[Jiang Liu]

On 2015/3/19 22:08, Bjorn Helgaas wrote:
> On Thu, Mar 19, 2015 at 6:29 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Thursday, March 19, 2015 03:49:33 PM Jiang Liu wrote:
>>> On 2015/3/19 6:11, Bjorn Helgaas wrote:
>>>> On Tue, Mar 17, 2015 at 03:37:12PM +0800, Jiang Liu wrote:
>>>>> To support IOAPIC hot-removal, we need to release PCI interrupt resource
>>>>> when unbinding PCI device driver. But due to historical reason,
>>>>> /*
>>>>>  * 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...
>>>>>  */
>>>>
>>>> Quoting the comment here (especially the last two lines) is overkill and
>>>> obscures the real point.  The important thing is that some drivers have
>>>> legitimate reasons for not calling pci_disable_device().
>>> Hi Bjorn,
>>>       Thanks for review. I will rewrite the commit message.
>>>>> some drivers don't call pci_disable_device() when unloading, which
>>>>> prevents us from reallocating PCI interrupt resource on reloading
>>>>> PCI driver and causes regressions.
>>>>
>>>> This isn't very clear.  I can believe that "drivers not calling
>>>> pci_disable_device()" means we don't release IRQ resources, which might
>>>> prevent you from hot-removing an IOAPIC.
>>>>
>>>> But "drivers not calling pci_disable_device()" doesn't cause regressions.
>>>>
>>>>> So release PCI interrupt resource only if PCI device is disabled when
>>>>> unbinding. By this way, we could support IOAPIC hot-removal on latest
>>>>> platforms and avoid regressions on old platforms.
>>>>
>>>> Does this mean you can only hot-remove IOAPICs if all drivers for devices
>>>> using the IOAPIC call pci_disable_device()?  If so, it seems sort of
>>>> dubious that we have to rely on drivers for that.
>>> This is a quickfix for v4.0 merging window. We will try to solve this
>>> issue for next merging window.
>>
>> If that is the plan, then I'd rather revert the offending commit and try
>> again in the next cycle.
>>
>> Bjorn, what do you think?
> 
> I don't know how hard it is to just revert that one commit at this
> point, but I would be in favor of doing that if it's feasible.
I will investigate about reverting.

> 
> We're headed toward a real morass of changelogs for a design that
> seems destined for overhaul.  That makes it really hard to backport
> and rework things later.
> 
> From the revised changelog:
> 
>>> When suspending, PCI
>>> device driver may call pci_disable_device() and eventually release
>>> IOAPIC pin. When resuming, PCI device driver call pci_enable_device()
>>> and reallocating IOAPIC pin. Since v3.19, IOAPIC driver dynamically
>>> allocates IRQ number for IOAPIC pin. So when resuming, a different
>>> IRQ number may assigned, which breaks some PCI drivers' suspend/resume
>>> implementation.
> 
> It seems like you're really standing on your head to make this
> situation work, and I think the result is too complicated and
> error-prone.  One test is to see whether you can write a short, simple
> description of how driver writers need to manage IRQs with respect to
> probe/remove/suspend/remove.
> 
> There are two other possibilities I can see:
> 
> 1) Decide that a driver that captures the IRQ and then calls
> pci_enable_device() is just broken, and fix those drivers to
> re-capture the IRQ every time they call pci_enable_device().  I assume
> you've looked at this already and concluded it's not practical?
> 
> 2) Configure the IRQ in pci_device_probe().  Then it would be
> configured before the driver sees the device, and you could dispose of
> it in pci_device_remove() when the driver is unbound.
Actually I prefer solution 2 above. The key idea is to decouple
IRQ resource allocation from pci_enabe/disable_device(), so irq
resource will be allocated just before driver binding and will
be released after driver unbinding.

One issue left is the way to hook driver binding/unbinding events.
Currently pcibios_enable/disable_irq() are x86 specific, so I use
PCI notification to hook driver binding/unbinding evetns.
If you are OK with introducing two new weak functions
pcibios_enable/disable_irq() into PCI core, that's obviously
a clear solution, easier to maintain and may benefit other platforms
too in future.

So should I introduce pcibios_enable/disable_irq() into PCI core?
Thanks!
Gerry

> 
> Does either of those make sense?
> 
> Bjorn
> 

Re: [Bugfix] x86/PCI: Release PCI IRQ resource only if PCI device is disabled when unbinding

[Bjorn Helgaas]

On Thu, Mar 19, 2015 at 10:09 PM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> On 2015/3/19 22:08, Bjorn Helgaas wrote:

>> There are two other possibilities I can see:
>>
>> 1) Decide that a driver that captures the IRQ and then calls
>> pci_enable_device() is just broken, and fix those drivers to
>> re-capture the IRQ every time they call pci_enable_device().  I assume
>> you've looked at this already and concluded it's not practical?

Did you look at this or not?  I don't have any idea of the scope of the problem.

I think in general we want drivers to start from scratch whenever they
call pci_enable_device().  They should not assume that BARs are the
same, IRQs are the same, etc.  If we ever want to dynamically reassign
resources, e.g., move BARs around to accommodate new hotplugged
devices, a path involving pci_enable_device() seems a likely route of
having the drivers learn about resource changes.

>> 2) Configure the IRQ in pci_device_probe().  Then it would be
>> configured before the driver sees the device, and you could dispose of
>> it in pci_device_remove() when the driver is unbound.
> Actually I prefer solution 2 above. The key idea is to decouple
> IRQ resource allocation from pci_enabe/disable_device(), so irq
> resource will be allocated just before driver binding and will
> be released after driver unbinding.

Solution 2 does have the advantage of making it simpler for driver writers.

One disadvantage is that it *forces* us to do IRQ allocation, even
though it may not be required.  There are drivers that don't need IRQs
because they use MSI or don't need interrupts at all.  If we do IRQ
allocation before binding the driver, and the allocation fails, these
driver will no longer work even though they don't need the IRQs.

> One issue left is the way to hook driver binding/unbinding events.
> Currently pcibios_enable/disable_irq() are x86 specific, so I use
> PCI notification to hook driver binding/unbinding evetns.
> If you are OK with introducing two new weak functions
> pcibios_enable/disable_irq() into PCI core, that's obviously
> a clear solution, easier to maintain and may benefit other platforms
> too in future.
>
> So should I introduce pcibios_enable/disable_irq() into PCI core?

I think it would be better to add new weak interfaces than to use
bus_register_notifier().  New interfaces are much more explicit when
reading the code, and their ordering is very clearly defined.

Bjorn