pcibios_free_irq()"
Please apply these reverts to the v4.4 stable kernel.
We reverted the following changes from v4.5 to fix a regression:
8affb487d4a4 ("x86/PCI: Don't alloc pcibios-irq when MSI is enabled")
811a4e6fce09 ("PCI: Add helpers to manage pci_dev->irq and pci_dev->irq_managed")
991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()")
but I forgot to mark those reverts for stable. This series contains the
backports for v4.4.
For reference, the v4.5 reverts are:
fe25d078874f ("Revert "x86/PCI: Don't alloc pcibios-irq when MSI is enabled"")
67b4eab91caf ("Revert "PCI: Add helpers to manage pci_dev->irq and pci_dev->irq_managed"")
6c777e8799a9 ("Revert "PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()"")
---
Bjorn Helgaas (3):
Revert "x86/PCI: Don't alloc pcibios-irq when MSI is enabled"
Revert "PCI: Add helpers to manage pci_dev->irq and pci_dev->irq_managed"
Revert "PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()"
arch/x86/include/asm/pci_x86.h | 2 ++
arch/x86/pci/common.c | 26 ++++++++++----------------
arch/x86/pci/intel_mid_pci.c | 9 +++------
arch/x86/pci/irq.c | 23 +++++++++++++++++++----
drivers/acpi/pci_irq.c | 17 +++++++++++++----
include/linux/pci.h | 17 -----------------
6 files changed, 47 insertions(+), 47 deletions(-)
This reverts commit 8affb487d4a4e223d961d7034cb41cd31982b618.
Revert 8affb487d4a4 ("x86/PCI: Don't alloc pcibios-irq when MSI is
enabled").
This is part of reverting 991de2e59090 ("PCI, x86: Implement
pcibios_alloc_irq() and pcibios_free_irq()") to fix regressions it
introduced.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=111211
Fixes: 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()")
Signed-off-by: Bjorn Helgaas <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
CC: [email protected] # v4.4
CC: Jiang Liu <[email protected]>
CC: Joerg Roedel <[email protected]>
---
arch/x86/pci/common.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index eccd4d9..dc78a4a 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -675,14 +675,6 @@ int pcibios_add_device(struct pci_dev *dev)
int pcibios_alloc_irq(struct pci_dev *dev)
{
- /*
- * If the PCI device was already claimed by core code and has
- * MSI enabled, probing of the pcibios IRQ will overwrite
- * dev->irq. So bail out if MSI is already enabled.
- */
- if (pci_dev_msi_enabled(dev))
- return -EBUSY;
-
return pcibios_enable_irq(dev);
}
This reverts commit 991de2e59090e55c65a7f59a049142e3c480f7bd.
Revert "PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()"
991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and
pcibios_free_irq()") appeared in v4.3 and helps support IOAPIC hotplug.
Олег reported that the Elcus-1553 TA1-PCI driver worked in v4.2 but not
v4.3 and bisected it to 991de2e59090. Sunjin reported that the RocketRAID
272x driver worked in v4.2 but not v4.3. In both cases booting with
"pci=routirq" is a workaround.
I think the problem is that after 991de2e59090, we no longer call
pcibios_enable_irq() for upstream bridges. Prior to 991de2e59090, when a
driver called pci_enable_device(), we recursively called
pcibios_enable_irq() for upstream bridges via pci_enable_bridge().
After 991de2e59090, we call pcibios_enable_irq() from pci_device_probe()
instead of the pci_enable_device() path, which does *not* call
pcibios_enable_irq() for upstream bridges.
Revert 991de2e59090 to fix these driver regressions.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=111211
Fixes: 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()")
Reported-and-tested-by: Олег Мороз <[email protected]>
Reported-by: Sunjin Yang <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
CC: [email protected] # v4.4
CC: Jiang Liu <[email protected]>
---
arch/x86/include/asm/pci_x86.h | 2 ++
arch/x86/pci/common.c | 20 +++++++++++---------
arch/x86/pci/intel_mid_pci.c | 7 ++-----
arch/x86/pci/irq.c | 15 ++++++++++++++-
drivers/acpi/pci_irq.c | 9 ++++++++-
5 files changed, 37 insertions(+), 16 deletions(-)
diff --git a/arch/x86/include/asm/pci_x86.h b/arch/x86/include/asm/pci_x86.h
index fa1195d..164e3f8 100644
--- a/arch/x86/include/asm/pci_x86.h
+++ b/arch/x86/include/asm/pci_x86.h
@@ -93,6 +93,8 @@ extern raw_spinlock_t pci_config_lock;
extern int (*pcibios_enable_irq)(struct pci_dev *dev);
extern void (*pcibios_disable_irq)(struct pci_dev *dev);
+extern bool mp_should_keep_irq(struct device *dev);
+
struct pci_raw_ops {
int (*read)(unsigned int domain, unsigned int bus, unsigned int devfn,
int reg, int len, u32 *val);
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index dc78a4a..8fd6f44 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -673,20 +673,22 @@ int pcibios_add_device(struct pci_dev *dev)
return 0;
}
-int pcibios_alloc_irq(struct pci_dev *dev)
+int pcibios_enable_device(struct pci_dev *dev, int mask)
{
- return pcibios_enable_irq(dev);
-}
+ int err;
-void pcibios_free_irq(struct pci_dev *dev)
-{
- if (pcibios_disable_irq)
- pcibios_disable_irq(dev);
+ if ((err = pci_enable_resources(dev, mask)) < 0)
+ return err;
+
+ if (!pci_dev_msi_enabled(dev))
+ return pcibios_enable_irq(dev);
+ return 0;
}
-int pcibios_enable_device(struct pci_dev *dev, int mask)
+void pcibios_disable_device (struct pci_dev *dev)
{
- return pci_enable_resources(dev, mask);
+ if (!pci_dev_msi_enabled(dev) && pcibios_disable_irq)
+ pcibios_disable_irq(dev);
}
int pci_ext_cfg_avail(void)
diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
index 8826ff5..8b93e63 100644
--- a/arch/x86/pci/intel_mid_pci.c
+++ b/arch/x86/pci/intel_mid_pci.c
@@ -256,13 +256,10 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
static void intel_mid_pci_irq_disable(struct pci_dev *dev)
{
- if (dev->irq_managed && dev->irq > 0) {
+ if (!mp_should_keep_irq(&dev->dev) && dev->irq_managed &&
+ dev->irq > 0) {
mp_unmap_irq(dev->irq);
dev->irq_managed = 0;
- /*
- * Don't reset dev->irq here, otherwise
- * intel_mid_pci_irq_enable() will fail on next call.
- */
}
}
diff --git a/arch/x86/pci/irq.c b/arch/x86/pci/irq.c
index 72108f0..9bd1154 100644
--- a/arch/x86/pci/irq.c
+++ b/arch/x86/pci/irq.c
@@ -1257,9 +1257,22 @@ static int pirq_enable_irq(struct pci_dev *dev)
return 0;
}
+bool mp_should_keep_irq(struct device *dev)
+{
+ if (dev->power.is_prepared)
+ return true;
+#ifdef CONFIG_PM
+ if (dev->power.runtime_status == RPM_SUSPENDING)
+ return true;
+#endif
+
+ return false;
+}
+
static void pirq_disable_irq(struct pci_dev *dev)
{
- if (io_apic_assign_pci_irqs && dev->irq_managed && dev->irq) {
+ if (io_apic_assign_pci_irqs && !mp_should_keep_irq(&dev->dev) &&
+ dev->irq_managed && dev->irq) {
mp_unmap_irq(dev->irq);
dev->irq = 0;
dev->irq_managed = 0;
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index 172b74d..8a10a7a 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -481,6 +481,14 @@ void acpi_pci_irq_disable(struct pci_dev *dev)
if (!pin || !dev->irq_managed || dev->irq <= 0)
return;
+ /* Keep IOAPIC pin configuration when suspending */
+ if (dev->dev.power.is_prepared)
+ return;
+#ifdef CONFIG_PM
+ if (dev->dev.power.runtime_status == RPM_SUSPENDING)
+ return;
+#endif
+
entry = acpi_pci_irq_lookup(dev, pin);
if (!entry)
return;
@@ -501,6 +509,5 @@ void acpi_pci_irq_disable(struct pci_dev *dev)
if (gsi >= 0) {
acpi_unregister_gsi(gsi);
dev->irq_managed = 0;
- dev->irq = 0;
}
}
This reverts commit 811a4e6fce09bc9239c664c6a1a53645a678c303.
Revert 811a4e6fce09 ("PCI: Add helpers to manage pci_dev->irq and
pci_dev->irq_managed").
This is part of reverting 991de2e59090 ("PCI, x86: Implement
pcibios_alloc_irq() and pcibios_free_irq()") to fix regressions it
introduced.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=111211
Fixes: 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()")
Signed-off-by: Bjorn Helgaas <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
CC: [email protected] # v4.4
CC: Jiang Liu <[email protected]>
---
arch/x86/pci/intel_mid_pci.c | 4 ++--
arch/x86/pci/irq.c | 10 ++++++----
drivers/acpi/pci_irq.c | 10 ++++++----
include/linux/pci.h | 17 -----------------
4 files changed, 14 insertions(+), 27 deletions(-)
diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
index 0d24e7c..8826ff5 100644
--- a/arch/x86/pci/intel_mid_pci.c
+++ b/arch/x86/pci/intel_mid_pci.c
@@ -215,7 +215,7 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
int polarity;
int ret;
- if (pci_has_managed_irq(dev))
+ if (dev->irq_managed && dev->irq > 0)
return 0;
switch (intel_mid_identify_cpu()) {
@@ -256,7 +256,7 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
static void intel_mid_pci_irq_disable(struct pci_dev *dev)
{
- if (pci_has_managed_irq(dev)) {
+ if (dev->irq_managed && dev->irq > 0) {
mp_unmap_irq(dev->irq);
dev->irq_managed = 0;
/*
diff --git a/arch/x86/pci/irq.c b/arch/x86/pci/irq.c
index 32e7034..72108f0 100644
--- a/arch/x86/pci/irq.c
+++ b/arch/x86/pci/irq.c
@@ -1202,7 +1202,7 @@ static int pirq_enable_irq(struct pci_dev *dev)
struct pci_dev *temp_dev;
int irq;
- if (pci_has_managed_irq(dev))
+ if (dev->irq_managed && dev->irq > 0)
return 0;
irq = IO_APIC_get_PCI_irq_vector(dev->bus->number,
@@ -1230,7 +1230,8 @@ static int pirq_enable_irq(struct pci_dev *dev)
}
dev = temp_dev;
if (irq >= 0) {
- pci_set_managed_irq(dev, irq);
+ dev->irq_managed = 1;
+ dev->irq = irq;
dev_info(&dev->dev, "PCI->APIC IRQ transform: "
"INT %c -> IRQ %d\n", 'A' + pin - 1, irq);
return 0;
@@ -1258,8 +1259,9 @@ static int pirq_enable_irq(struct pci_dev *dev)
static void pirq_disable_irq(struct pci_dev *dev)
{
- if (io_apic_assign_pci_irqs && pci_has_managed_irq(dev)) {
+ if (io_apic_assign_pci_irqs && dev->irq_managed && dev->irq) {
mp_unmap_irq(dev->irq);
- pci_reset_managed_irq(dev);
+ dev->irq = 0;
+ dev->irq_managed = 0;
}
}
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index c933675..172b74d 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -409,7 +409,7 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
return 0;
}
- if (pci_has_managed_irq(dev))
+ if (dev->irq_managed && dev->irq > 0)
return 0;
entry = acpi_pci_irq_lookup(dev, pin);
@@ -454,7 +454,8 @@ int acpi_pci_irq_enable(struct pci_dev *dev)
kfree(entry);
return rc;
}
- pci_set_managed_irq(dev, rc);
+ dev->irq = rc;
+ dev->irq_managed = 1;
if (link)
snprintf(link_desc, sizeof(link_desc), " -> Link[%s]", link);
@@ -477,7 +478,7 @@ void acpi_pci_irq_disable(struct pci_dev *dev)
u8 pin;
pin = dev->pin;
- if (!pin || !pci_has_managed_irq(dev))
+ if (!pin || !dev->irq_managed || dev->irq <= 0)
return;
entry = acpi_pci_irq_lookup(dev, pin);
@@ -499,6 +500,7 @@ void acpi_pci_irq_disable(struct pci_dev *dev)
dev_dbg(&dev->dev, "PCI INT %c disabled\n", pin_name(pin));
if (gsi >= 0) {
acpi_unregister_gsi(gsi);
- pci_reset_managed_irq(dev);
+ dev->irq_managed = 0;
+ dev->irq = 0;
}
}
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6ae25aa..1cca5d8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -988,23 +988,6 @@ static inline int pci_is_managed(struct pci_dev *pdev)
return pdev->is_managed;
}
-static inline void pci_set_managed_irq(struct pci_dev *pdev, unsigned int irq)
-{
- pdev->irq = irq;
- pdev->irq_managed = 1;
-}
-
-static inline void pci_reset_managed_irq(struct pci_dev *pdev)
-{
- pdev->irq = 0;
- pdev->irq_managed = 0;
-}
-
-static inline bool pci_has_managed_irq(struct pci_dev *pdev)
-{
- return pdev->irq_managed && pdev->irq > 0;
-}
-
void pci_disable_device(struct pci_dev *dev);
extern unsigned int pcibios_max_latency;
Hi Bjorn,
On Wed, Apr 13, 2016 at 12:56:59AM -0500, Bjorn Helgaas wrote:
> We reverted the following changes from v4.5 to fix a regression:
>
> 8affb487d4a4 ("x86/PCI: Don't alloc pcibios-irq when MSI is enabled")
> 811a4e6fce09 ("PCI: Add helpers to manage pci_dev->irq and pci_dev->irq_managed")
> 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()")
Do you have a link to the thread about these issues? I'd like to have a
look at what has been tried to solve the regressions before the revert.
I had a look at commit 991de2e59090 and noted that the main difference
it introduces is that the pcibios-irq is allocated earlier (probe time,
before the commit it was pci_enable_device() time). In fact, it is now
allocated before pci_enable_resources() has been called on the device
(as far as I can see). I wonder if the regression can be fixed by
also moving pci_enable_resources() to probe time.
Regards,
Joerg
On Thu, Apr 14, 2016 at 05:50:44PM +0200, Joerg Roedel wrote:
> Hi Bjorn,
>
> On Wed, Apr 13, 2016 at 12:56:59AM -0500, Bjorn Helgaas wrote:
> > We reverted the following changes from v4.5 to fix a regression:
> >
> > 8affb487d4a4 ("x86/PCI: Don't alloc pcibios-irq when MSI is enabled")
> > 811a4e6fce09 ("PCI: Add helpers to manage pci_dev->irq and pci_dev->irq_managed")
> > 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()")
>
> Do you have a link to the thread about these issues? I'd like to have a
> look at what has been tried to solve the regressions before the revert.
The revert was 6c777e8799a9 ("Revert "PCI, x86: Implement
pcibios_alloc_irq() and pcibios_free_irq()"").
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=111211
Email discussion: http://lkml.kernel.org/r/[email protected]
> I had a look at commit 991de2e59090 and noted that the main difference
> it introduces is that the pcibios-irq is allocated earlier (probe time,
> before the commit it was pci_enable_device() time). In fact, it is now
> allocated before pci_enable_resources() has been called on the device
> (as far as I can see). I wonder if the regression can be fixed by
> also moving pci_enable_resources() to probe time.
I assume you're thinking about doing pci_enable_resources() before
the core calls the driver's probe method? One question there is how
we would deal with pci_enable_device_mem(). If the core calls
pci_enable_resources(), it has to assume the driver requires all BARs,
and there are quite a few drivers that don't need the I/O BARs.
I'd be very glad if you poked at this a little more. Jiang did a lot
of nice work on IOAPIC hotplug, and I feel bad reverting this piece of
it. It's just that nobody so far has had the time or interest to work
out a better fix.
Bjorn
On Wed, Apr 13, 2016 at 12:56:59AM -0500, Bjorn Helgaas wrote:
> pcibios_free_irq()"
>
> Please apply these reverts to the v4.4 stable kernel.
>
> We reverted the following changes from v4.5 to fix a regression:
>
> 8affb487d4a4 ("x86/PCI: Don't alloc pcibios-irq when MSI is enabled")
> 811a4e6fce09 ("PCI: Add helpers to manage pci_dev->irq and pci_dev->irq_managed")
> 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()")
>
> but I forgot to mark those reverts for stable. This series contains the
> backports for v4.4.
>
> For reference, the v4.5 reverts are:
>
> fe25d078874f ("Revert "x86/PCI: Don't alloc pcibios-irq when MSI is enabled"")
> 67b4eab91caf ("Revert "PCI: Add helpers to manage pci_dev->irq and pci_dev->irq_managed"")
> 6c777e8799a9 ("Revert "PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()"")
All now applied, thanks for these.
greg k-h
Hi Bjorn,
On Fri, Apr 15, 2016 at 10:08:21AM -0500, Bjorn Helgaas wrote:
> I assume you're thinking about doing pci_enable_resources() before
> the core calls the driver's probe method? One question there is how
> we would deal with pci_enable_device_mem(). If the core calls
> pci_enable_resources(), it has to assume the driver requires all BARs,
> and there are quite a few drivers that don't need the I/O BARs.
Yes, I think that the problem might be fixed when the resources are
enabled during the pcibios-call.
What do you think of enabling the the resources at probe time for the
pcibios-call and disable them afterwards? Then the driver can re-enable
whatever it needs and keep the rest disabled.
Joerg
On Mon, Apr 18, 2016 at 01:50:15PM +0200, Joerg Roedel wrote:
> Hi Bjorn,
>
> On Fri, Apr 15, 2016 at 10:08:21AM -0500, Bjorn Helgaas wrote:
> > I assume you're thinking about doing pci_enable_resources() before
> > the core calls the driver's probe method? One question there is how
> > we would deal with pci_enable_device_mem(). If the core calls
> > pci_enable_resources(), it has to assume the driver requires all BARs,
> > and there are quite a few drivers that don't need the I/O BARs.
>
> Yes, I think that the problem might be fixed when the resources are
> enabled during the pcibios-call.
>
> What do you think of enabling the the resources at probe time for the
> pcibios-call and disable them afterwards? Then the driver can re-enable
> whatever it needs and keep the rest disabled.
That might work, but the problem seems to be that we aren't enabling
IRQs correctly, so I'd rather have a fix that explicitly addresses
IRQs than one that relies on some non-obvious connection between
enabling BARs and IRQs.
Bjorn
On Mon, Apr 18, 2016 at 09:43:36AM -0500, Bjorn Helgaas wrote:
> That might work, but the problem seems to be that we aren't enabling
> IRQs correctly, so I'd rather have a fix that explicitly addresses
> IRQs than one that relies on some non-obvious connection between
> enabling BARs and IRQs.
Yeah, either we or the BIOS does something wrong. The bugzilla entry
states that it works with pci=routeirq. The dmesg from kernel 4.3 shows
that per default ACPI routing is used for IRQs.
The difference this makes in the code is the pcibios_enable_irq function
pointer. For ACPI it points to acpi_pci_irq_enable and in the
pci=routeirq case to pirq_enable_irq.
So the problem appears when acpi_pci_irq_enable is used with device
resources disabled. This might very well also be a bios issue that we
have to work around, no?
Joerg