From: Rafael J. Wysocki <[email protected]>
pcibios_enable_device() and pcibios_disable_device() don't handle
IRQs for devices that have MSI enabled and it should tread the
devices with MSI-X enabled in the same way.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
arch/x86/pci/common.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Index: linux-2.6/arch/x86/pci/common.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/common.c
+++ linux-2.6/arch/x86/pci/common.c
@@ -551,14 +551,14 @@ int pcibios_enable_device(struct pci_dev
if ((err = pci_enable_resources(dev, mask)) < 0)
return err;
- if (!dev->msi_enabled)
+ if (!dev->msi_enabled && !dev->msix_enabled)
return pcibios_enable_irq(dev);
return 0;
}
void pcibios_disable_device (struct pci_dev *dev)
{
- if (!dev->msi_enabled && pcibios_disable_irq)
+ if (!dev->msi_enabled && !dev->msix_enabled && pcibios_disable_irq)
pcibios_disable_irq(dev);
}
* Rafael J. Wysocki <[email protected]> wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> pcibios_enable_device() and pcibios_disable_device() don't handle
> IRQs for devices that have MSI enabled and it should tread the
s/tread/treat
> devices with MSI-X enabled in the same way.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> arch/x86/pci/common.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
looks good - Jesse, what do you think?
Rafael, i'm curious is this in response to some regression/bug? Did some
box or driver get confused by us enabling/disabling the GSI? Some IRQ
flood perhaps?
btw., there's a small observation:
> + if (!dev->msi_enabled && !dev->msix_enabled)
maybe a "pci_has_gsi_irq()" wrapper would make these checks cleaner and
would make things more robust, should there be any new IRQ delivery method
be introduced in the future?
Ingo
On Monday 05 January 2009, Ingo Molnar wrote:
>
> * Rafael J. Wysocki <[email protected]> wrote:
>
> > From: Rafael J. Wysocki <[email protected]>
> >
> > pcibios_enable_device() and pcibios_disable_device() don't handle
> > IRQs for devices that have MSI enabled and it should tread the
>
> s/tread/treat
Ah, thanks.
> > devices with MSI-X enabled in the same way.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > arch/x86/pci/common.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
>
> looks good - Jesse, what do you think?
>
> Rafael, i'm curious is this in response to some regression/bug? Did some
> box or driver get confused by us enabling/disabling the GSI? Some IRQ
> flood perhaps?
Well, I don't have any MSI-X capable boxes around. :-)
I was just reviewing the code and spotted this.
> btw., there's a small observation:
>
> > + if (!dev->msi_enabled && !dev->msix_enabled)
>
> maybe a "pci_has_gsi_irq()" wrapper would make these checks cleaner and
> would make things more robust, should there be any new IRQ delivery method
> be introduced in the future?
Well, perhaps something like the patch below?
Thanks,
Rafael
---
Subject: x86 PCI: Do not use interrupt links for devices using MSI-X (rev. 2)
From: Rafael J. Wysocki <[email protected]>
pcibios_enable_device() and pcibios_disable_device() don't handle
IRQs for devices that have MSI enabled and it should treat the
devices with MSI-X enabled in the same way.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
arch/x86/pci/common.c | 4 ++--
include/linux/pci.h | 9 +++++++++
2 files changed, 11 insertions(+), 2 deletions(-)
Index: linux-2.6/arch/x86/pci/common.c
===================================================================
--- linux-2.6.orig/arch/x86/pci/common.c
+++ linux-2.6/arch/x86/pci/common.c
@@ -551,14 +551,14 @@ int pcibios_enable_device(struct pci_dev
if ((err = pci_enable_resources(dev, mask)) < 0)
return err;
- if (!dev->msi_enabled)
+ if (!pci_msi_enabled(dev))
return pcibios_enable_irq(dev);
return 0;
}
void pcibios_disable_device (struct pci_dev *dev)
{
- if (!dev->msi_enabled && pcibios_disable_irq)
+ if (!pci_msi_enabled(dev) && pcibios_disable_irq)
pcibios_disable_irq(dev);
}
Index: linux-2.6/include/linux/pci.h
===================================================================
--- linux-2.6.orig/include/linux/pci.h
+++ linux-2.6/include/linux/pci.h
@@ -274,6 +274,15 @@ static inline void pci_add_saved_cap(str
hlist_add_head(&new_cap->next, &pci_dev->saved_cap_space);
}
+#ifdef CONFIG_PCI_MSI
+static inline bool pci_msi_enabled(struct pci_dev *pci_dev)
+{
+ return dev->msi_enabled || dev->msix_enabled;
+}
+#else
+static inline bool pci_msi_enabled(struct pci_dev *pci_dev) { return false; }
+#endif
+
/*
* For PCI devices, the region numbers are assigned this way:
*
On Monday, January 5, 2009 5:04 am Ingo Molnar wrote:
> * Rafael J. Wysocki <[email protected]> wrote:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > pcibios_enable_device() and pcibios_disable_device() don't handle
> > IRQs for devices that have MSI enabled and it should tread the
>
> s/tread/treat
>
> > devices with MSI-X enabled in the same way.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> > arch/x86/pci/common.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
>
> looks good - Jesse, what do you think?
Yeah, seems obviously correct, I'll queue it up.
> Rafael, i'm curious is this in response to some regression/bug? Did some
> box or driver get confused by us enabling/disabling the GSI? Some IRQ
> flood perhaps?
>
> btw., there's a small observation:
> > + if (!dev->msi_enabled && !dev->msix_enabled)
>
> maybe a "pci_has_gsi_irq()" wrapper would make these checks cleaner and
> would make things more robust, should there be any new IRQ delivery method
> be introduced in the future?
pci_has_msi_irq surely? Otherwise we'll catch pretty much everything? Or did
you mean !pci_has_gsi_irq() here instead?
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
On Monday, January 5, 2009 5:50 am Rafael J. Wysocki wrote:
> On Monday 05 January 2009, Ingo Molnar wrote:
> > * Rafael J. Wysocki <[email protected]> wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > pcibios_enable_device() and pcibios_disable_device() don't handle
> > > IRQs for devices that have MSI enabled and it should tread the
> >
> > s/tread/treat
>
> Ah, thanks.
>
> > > devices with MSI-X enabled in the same way.
> > >
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > ---
> > > arch/x86/pci/common.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > looks good - Jesse, what do you think?
> >
> > Rafael, i'm curious is this in response to some regression/bug? Did some
> > box or driver get confused by us enabling/disabling the GSI? Some IRQ
> > flood perhaps?
>
> Well, I don't have any MSI-X capable boxes around. :-)
>
> I was just reviewing the code and spotted this.
>
> > btw., there's a small observation:
> > > + if (!dev->msi_enabled && !dev->msix_enabled)
> >
> > maybe a "pci_has_gsi_irq()" wrapper would make these checks cleaner and
> > would make things more robust, should there be any new IRQ delivery
> > method be introduced in the future?
>
> Well, perhaps something like the patch below?
>
> Thanks,
> Rafael
>
> ---
> Subject: x86 PCI: Do not use interrupt links for devices using MSI-X (rev.
> 2) From: Rafael J. Wysocki <[email protected]>
>
> pcibios_enable_device() and pcibios_disable_device() don't handle
> IRQs for devices that have MSI enabled and it should treat the
> devices with MSI-X enabled in the same way.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
Looks good, applied this one.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
On Sunday 04 January 2009 03:08:42 pm Rafael J. Wysocki wrote:
> pcibios_enable_device() and pcibios_disable_device() don't handle
> IRQs for devices that have MSI enabled and it should tread the
> devices with MSI-X enabled in the same way.
There are other places that need similar fixes, too, aren't there?
I see cris, frv, ia64, and a driver or two testing dev->msi_enabled.
> --- linux-2.6.orig/arch/x86/pci/common.c
> +++ linux-2.6/arch/x86/pci/common.c
> @@ -551,14 +551,14 @@ int pcibios_enable_device(struct pci_dev
> if ((err = pci_enable_resources(dev, mask)) < 0)
> return err;
>
> - if (!dev->msi_enabled)
> + if (!dev->msi_enabled && !dev->msix_enabled)
> return pcibios_enable_irq(dev);
> return 0;
> }
>
> void pcibios_disable_device (struct pci_dev *dev)
> {
> - if (!dev->msi_enabled && pcibios_disable_irq)
> + if (!dev->msi_enabled && !dev->msix_enabled && pcibios_disable_irq)
> pcibios_disable_irq(dev);
> }
On Tuesday 06 January 2009, Bjorn Helgaas wrote:
> On Sunday 04 January 2009 03:08:42 pm Rafael J. Wysocki wrote:
> > pcibios_enable_device() and pcibios_disable_device() don't handle
> > IRQs for devices that have MSI enabled and it should tread the
> > devices with MSI-X enabled in the same way.
>
> There are other places that need similar fixes, too, aren't there?
> I see cris, frv, ia64, and a driver or two testing dev->msi_enabled.
Well, I didn't look at the other places, just found this one while reviewing
the code.
I'll check them later this week.
> > --- linux-2.6.orig/arch/x86/pci/common.c
> > +++ linux-2.6/arch/x86/pci/common.c
> > @@ -551,14 +551,14 @@ int pcibios_enable_device(struct pci_dev
> > if ((err = pci_enable_resources(dev, mask)) < 0)
> > return err;
> >
> > - if (!dev->msi_enabled)
> > + if (!dev->msi_enabled && !dev->msix_enabled)
> > return pcibios_enable_irq(dev);
> > return 0;
> > }
> >
> > void pcibios_disable_device (struct pci_dev *dev)
> > {
> > - if (!dev->msi_enabled && pcibios_disable_irq)
> > + if (!dev->msi_enabled && !dev->msix_enabled && pcibios_disable_irq)
> > pcibios_disable_irq(dev);
> > }
* Jesse Barnes <[email protected]> wrote:
> On Monday, January 5, 2009 5:04 am Ingo Molnar wrote:
> > * Rafael J. Wysocki <[email protected]> wrote:
> > > From: Rafael J. Wysocki <[email protected]>
> > >
> > > pcibios_enable_device() and pcibios_disable_device() don't handle
> > > IRQs for devices that have MSI enabled and it should tread the
> >
> > s/tread/treat
> >
> > > devices with MSI-X enabled in the same way.
> > >
> > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > ---
> > > arch/x86/pci/common.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > looks good - Jesse, what do you think?
>
> Yeah, seems obviously correct, I'll queue it up.
>
> > Rafael, i'm curious is this in response to some regression/bug? Did some
> > box or driver get confused by us enabling/disabling the GSI? Some IRQ
> > flood perhaps?
> >
> > btw., there's a small observation:
> > > + if (!dev->msi_enabled && !dev->msix_enabled)
> >
> > maybe a "pci_has_gsi_irq()" wrapper would make these checks cleaner and
> > would make things more robust, should there be any new IRQ delivery method
> > be introduced in the future?
>
> pci_has_msi_irq surely? Otherwise we'll catch pretty much everything? Or did
> you mean !pci_has_gsi_irq() here instead?
Well - here the check is: "if (not MSI or MSIX)" in essence. I thought
that it might be confusing to call it _msi() as well, so we could approach
it via the inverse space: general system interrupts (GSIs) - which are
device irqs that are neither MSI nor MSIX.
But if pci_has_msi_irq() can cleanly include the MSIX portion too, that's
fine too. (MSI-X is really MSI with wider eventing capabilities but
otherwise non-GSI just as much - and we dont want to enable (or even
touch) the legacy IRQ line registers for any of them, even if they happen
to be enumerated)
Right?
Ingo
On Wednesday, January 7, 2009 5:13 am Ingo Molnar wrote:
> * Jesse Barnes <[email protected]> wrote:
> > On Monday, January 5, 2009 5:04 am Ingo Molnar wrote:
> > > * Rafael J. Wysocki <[email protected]> wrote:
> > > > From: Rafael J. Wysocki <[email protected]>
> > > >
> > > > pcibios_enable_device() and pcibios_disable_device() don't handle
> > > > IRQs for devices that have MSI enabled and it should tread the
> > >
> > > s/tread/treat
> > >
> > > > devices with MSI-X enabled in the same way.
> > > >
> > > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > > ---
> > > > arch/x86/pci/common.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > looks good - Jesse, what do you think?
> >
> > Yeah, seems obviously correct, I'll queue it up.
> >
> > > Rafael, i'm curious is this in response to some regression/bug? Did
> > > some box or driver get confused by us enabling/disabling the GSI? Some
> > > IRQ flood perhaps?
> > >
> > > btw., there's a small observation:
> > > > + if (!dev->msi_enabled && !dev->msix_enabled)
> > >
> > > maybe a "pci_has_gsi_irq()" wrapper would make these checks cleaner and
> > > would make things more robust, should there be any new IRQ delivery
> > > method be introduced in the future?
> >
> > pci_has_msi_irq surely? Otherwise we'll catch pretty much everything?
> > Or did you mean !pci_has_gsi_irq() here instead?
>
> Well - here the check is: "if (not MSI or MSIX)" in essence. I thought
> that it might be confusing to call it _msi() as well, so we could approach
> it via the inverse space: general system interrupts (GSIs) - which are
> device irqs that are neither MSI nor MSIX.
>
> But if pci_has_msi_irq() can cleanly include the MSIX portion too, that's
> fine too. (MSI-X is really MSI with wider eventing capabilities but
> otherwise non-GSI just as much - and we dont want to enable (or even
> touch) the legacy IRQ line registers for any of them, even if they happen
> to be enumerated)
>
> Right?
Right, I see where you're coming from. However, I've queued up Rafael's last
patch with some fixes for dev vs. pci_dev and a name collision
(pci_msi_enabled -> pci_dev_msi_enabled). Bjorn caught the fact that some
other arches may want similar treatment too, I think Rafael is checking that
out.
--
Jesse Barnes, Intel Open Source Technology Center
* Jesse Barnes <[email protected]> wrote:
> On Wednesday, January 7, 2009 5:13 am Ingo Molnar wrote:
> > * Jesse Barnes <[email protected]> wrote:
> > > On Monday, January 5, 2009 5:04 am Ingo Molnar wrote:
> > > > * Rafael J. Wysocki <[email protected]> wrote:
> > > > > From: Rafael J. Wysocki <[email protected]>
> > > > >
> > > > > pcibios_enable_device() and pcibios_disable_device() don't handle
> > > > > IRQs for devices that have MSI enabled and it should tread the
> > > >
> > > > s/tread/treat
> > > >
> > > > > devices with MSI-X enabled in the same way.
> > > > >
> > > > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > > > ---
> > > > > arch/x86/pci/common.c | 4 ++--
> > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > looks good - Jesse, what do you think?
> > >
> > > Yeah, seems obviously correct, I'll queue it up.
> > >
> > > > Rafael, i'm curious is this in response to some regression/bug? Did
> > > > some box or driver get confused by us enabling/disabling the GSI? Some
> > > > IRQ flood perhaps?
> > > >
> > > > btw., there's a small observation:
> > > > > + if (!dev->msi_enabled && !dev->msix_enabled)
> > > >
> > > > maybe a "pci_has_gsi_irq()" wrapper would make these checks cleaner and
> > > > would make things more robust, should there be any new IRQ delivery
> > > > method be introduced in the future?
> > >
> > > pci_has_msi_irq surely? Otherwise we'll catch pretty much everything?
> > > Or did you mean !pci_has_gsi_irq() here instead?
> >
> > Well - here the check is: "if (not MSI or MSIX)" in essence. I thought
> > that it might be confusing to call it _msi() as well, so we could approach
> > it via the inverse space: general system interrupts (GSIs) - which are
> > device irqs that are neither MSI nor MSIX.
> >
> > But if pci_has_msi_irq() can cleanly include the MSIX portion too, that's
> > fine too. (MSI-X is really MSI with wider eventing capabilities but
> > otherwise non-GSI just as much - and we dont want to enable (or even
> > touch) the legacy IRQ line registers for any of them, even if they happen
> > to be enumerated)
> >
> > Right?
>
> Right, I see where you're coming from. However, I've queued up Rafael's
> last patch with some fixes for dev vs. pci_dev and a name collision
> (pci_msi_enabled -> pci_dev_msi_enabled). Bjorn caught the fact that
> some other arches may want similar treatment too, I think Rafael is
> checking that out.
Sure - that sounds good too!
Ingo
On Wednesday 07 January 2009, Jesse Barnes wrote:
> On Wednesday, January 7, 2009 5:13 am Ingo Molnar wrote:
> > * Jesse Barnes <[email protected]> wrote:
> > > On Monday, January 5, 2009 5:04 am Ingo Molnar wrote:
> > > > * Rafael J. Wysocki <[email protected]> wrote:
> > > > > From: Rafael J. Wysocki <[email protected]>
> > > > >
> > > > > pcibios_enable_device() and pcibios_disable_device() don't handle
> > > > > IRQs for devices that have MSI enabled and it should tread the
> > > >
> > > > s/tread/treat
> > > >
> > > > > devices with MSI-X enabled in the same way.
> > > > >
> > > > > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > > > > ---
> > > > > arch/x86/pci/common.c | 4 ++--
> > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > looks good - Jesse, what do you think?
> > >
> > > Yeah, seems obviously correct, I'll queue it up.
> > >
> > > > Rafael, i'm curious is this in response to some regression/bug? Did
> > > > some box or driver get confused by us enabling/disabling the GSI? Some
> > > > IRQ flood perhaps?
> > > >
> > > > btw., there's a small observation:
> > > > > + if (!dev->msi_enabled && !dev->msix_enabled)
> > > >
> > > > maybe a "pci_has_gsi_irq()" wrapper would make these checks cleaner and
> > > > would make things more robust, should there be any new IRQ delivery
> > > > method be introduced in the future?
> > >
> > > pci_has_msi_irq surely? Otherwise we'll catch pretty much everything?
> > > Or did you mean !pci_has_gsi_irq() here instead?
> >
> > Well - here the check is: "if (not MSI or MSIX)" in essence. I thought
> > that it might be confusing to call it _msi() as well, so we could approach
> > it via the inverse space: general system interrupts (GSIs) - which are
> > device irqs that are neither MSI nor MSIX.
> >
> > But if pci_has_msi_irq() can cleanly include the MSIX portion too, that's
> > fine too. (MSI-X is really MSI with wider eventing capabilities but
> > otherwise non-GSI just as much - and we dont want to enable (or even
> > touch) the legacy IRQ line registers for any of them, even if they happen
> > to be enumerated)
> >
> > Right?
>
> Right, I see where you're coming from. However, I've queued up Rafael's last
> patch with some fixes for dev vs. pci_dev and a name collision
> (pci_msi_enabled -> pci_dev_msi_enabled). Bjorn caught the fact that some
> other arches may want similar treatment too, I think Rafael is checking that
> out.
Yes, I am.
Thanks,
Rafael