Skip to contentSkip to conversation

x86, irq, PCI: Keep IRQ assignment for runtime power management

PATCH In review Jiang Liu 4 participants 10 messages Aug 29, 2014 – Sep 1, 2014

Subsystems (3)
x86 ACPI PCI
lore
Share & tools
lore

Jiang Liu

author
#0 lore raw

PATCH v4 1/1 x86, irq, PCI: Keep IRQ assignment for runtime power management

Now IOAPIC driver dynamically allocates IRQ numbers for IOAPIC pins. We need to keep IRQ assignment for PCI devices during runtime power management, otherwise it may cause failure of device wakeups.

Commit 3eec595235c17a7 "x86, irq, PCI: Keep IRQ assignment for PCI devices during suspend/hibernation" has fixed the issue for suspend/ hibernation, we also need the same fix for runtime device sleep too.

Fix: https://bugzilla.kernel.org/show_bug.cgi?id=83271

Reported-and-Tested-by EmanueL Czirai <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
---
 arch/x86/include/asm/io_apic.h |    2 ++
 arch/x86/kernel/apic/io_apic.c |   12 ++++++++++++
 arch/x86/pci/intel_mid_pci.c   |    2 +-
 arch/x86/pci/irq.c             |    2 +-
 drivers/acpi/pci_irq.c         |    4 ++++
 5 files changed, 20 insertions(+), 2 deletions(-)
arch/x86/include/asm/io_apic.h +2 -0
index 0aeed5ca356e..478c490f3654 100644
@@ -227,6 +227,8 @@ static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned
 
 extern void io_apic_eoi(unsigned int apic, unsigned int vector);
 
+extern bool mp_should_keep_irq(struct device *dev);
+
 #else  /* !CONFIG_X86_IO_APIC */
 
 #define io_apic_assign_pci_irqs 0
arch/x86/kernel/apic/io_apic.c +12 -0
index 29290f554e79..1e9a921d9701 100644
@@ -3946,6 +3946,18 @@ int mp_set_gsi_attr(u32 gsi, int trigger, int polarity, int node)
 	return ret;
 }
 
+bool mp_should_keep_irq(struct device *dev)
+{
+	if (dev->power.is_prepared)
+		return true;
+#ifdef	CONFIG_PM_RUNTIME
+	if (dev->power.runtime_status == RPM_SUSPENDING)
+		return true;
+#endif
+
+	return false;
+}
+
 /* Enable IOAPIC early just for system timer */
 void __init pre_init_apic_IRQ0(void)
 {
arch/x86/pci/intel_mid_pci.c +1 -1
index 3865116c51fb..b9958c364075 100644
@@ -229,7 +229,7 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
 
 static void intel_mid_pci_irq_disable(struct pci_dev *dev)
 {
-	if (!dev->dev.power.is_prepared && dev->irq > 0)
+	if (!mp_should_keep_irq(&dev->dev) && dev->irq > 0)
 		mp_unmap_irq(dev->irq);
 }
arch/x86/pci/irq.c +1 -1
index bc1a2c341891..eb500c2592ad 100644
@@ -1256,7 +1256,7 @@ static int pirq_enable_irq(struct pci_dev *dev)
 
 static void pirq_disable_irq(struct pci_dev *dev)
 {
-	if (io_apic_assign_pci_irqs && !dev->dev.power.is_prepared &&
+	if (io_apic_assign_pci_irqs && !mp_should_keep_irq(&dev->dev) &&
 	    dev->irq) {
 		mp_unmap_irq(dev->irq);
 		dev->irq = 0;
drivers/acpi/pci_irq.c +4 -0
index c96887d5289e..6e6b80eb0bba 100644
@@ -484,6 +484,10 @@ void acpi_pci_irq_disable(struct pci_dev *dev)
 	/* Keep IOAPIC pin configuration when suspending */
 	if (dev->dev.power.is_prepared)
 		return;
+#ifdef	CONFIG_PM_RUNTIME
+	if (dev->dev.power.runtime_status == RPM_SUSPENDING)
+		return;
+#endif
 
 	entry = acpi_pci_irq_lookup(dev, pin);
 	if (!entry)
Signature · 1 line
1.7.10.4
merge bot
#1 lore raw
x86, irq, PCI: Keep IRQ assignment for runtime power management
Commit-ID:  9eabc99a635a77cbf0948ce17d3cbc2b51680d4a
Gitweb:     http://git.kernel.org/tip/9eabc99a635a77cbf0948ce17d3cbc2b51680d4a
Author:     Jiang Liu <[email protected]>
AuthorDate: Fri, 29 Aug 2014 17:26:23 +0800
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Fri, 29 Aug 2014 13:38:00 +0200

x86, irq, PCI: Keep IRQ assignment for runtime power management

Now IOAPIC driver dynamically allocates IRQ numbers for IOAPIC pins. We need to keep IRQ assignment for PCI devices during runtime power management, otherwise it may cause failure of device wakeups.

Commit 3eec595235c17a7 "x86, irq, PCI: Keep IRQ assignment for PCI devices during suspend/hibernation" has fixed the issue for suspend/ hibernation, we also need the same fix for runtime device sleep too.

Fix: https://bugzilla.kernel.org/show_bug.cgi?id=83271

Reported-and-Tested-by EmanueL Czirai <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Tony Luck <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc EmanueL Czirai <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Randy Dunlap <[email protected]>
Cc Yinghai Lu <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Grant Likely <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Thomas Gleixner <[email protected]>
---
 arch/x86/include/asm/io_apic.h |  2 ++
 arch/x86/kernel/apic/io_apic.c | 12 ++++++++++++
 arch/x86/pci/intel_mid_pci.c   |  2 +-
 arch/x86/pci/irq.c             |  2 +-
 drivers/acpi/pci_irq.c         |  4 ++++
 5 files changed, 20 insertions(+), 2 deletions(-)
arch/x86/include/asm/io_apic.h +2 -0
index 0aeed5c..478c490 100644
@@ -227,6 +227,8 @@ static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned
 
 extern void io_apic_eoi(unsigned int apic, unsigned int vector);
 
+extern bool mp_should_keep_irq(struct device *dev);
+
 #else  /* !CONFIG_X86_IO_APIC */
 
 #define io_apic_assign_pci_irqs 0
arch/x86/kernel/apic/io_apic.c +12 -0
index 40a4aa3..337ce5a 100644
@@ -3959,6 +3959,18 @@ int mp_set_gsi_attr(u32 gsi, int trigger, int polarity, int node)
 	return ret;
 }
 
+bool mp_should_keep_irq(struct device *dev)
+{
+	if (dev->power.is_prepared)
+		return true;
+#ifdef	CONFIG_PM_RUNTIME
+	if (dev->power.runtime_status == RPM_SUSPENDING)
+		return true;
+#endif
+
+	return false;
+}
+
 /* Enable IOAPIC early just for system timer */
 void __init pre_init_apic_IRQ0(void)
 {
arch/x86/pci/intel_mid_pci.c +1 -1
index 3865116..b9958c3 100644
@@ -229,7 +229,7 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
 
 static void intel_mid_pci_irq_disable(struct pci_dev *dev)
 {
-	if (!dev->dev.power.is_prepared && dev->irq > 0)
+	if (!mp_should_keep_irq(&dev->dev) && dev->irq > 0)
 		mp_unmap_irq(dev->irq);
 }
arch/x86/pci/irq.c +1 -1
index bc1a2c3..eb500c2 100644
@@ -1256,7 +1256,7 @@ static int pirq_enable_irq(struct pci_dev *dev)
 
 static void pirq_disable_irq(struct pci_dev *dev)
 {
-	if (io_apic_assign_pci_irqs && !dev->dev.power.is_prepared &&
+	if (io_apic_assign_pci_irqs && !mp_should_keep_irq(&dev->dev) &&
 	    dev->irq) {
 		mp_unmap_irq(dev->irq);
 		dev->irq = 0;
drivers/acpi/pci_irq.c +4 -0
index c96887d..6e6b80e 100644
@@ -484,6 +484,10 @@ void acpi_pci_irq_disable(struct pci_dev *dev)
 	/* Keep IOAPIC pin configuration when suspending */
 	if (dev->dev.power.is_prepared)
 		return;
+#ifdef	CONFIG_PM_RUNTIME
+	if (dev->dev.power.runtime_status == RPM_SUSPENDING)
+		return;
+#endif
 
 	entry = acpi_pci_irq_lookup(dev, pin);
 	if (!entry)
#2 lore raw
Quoted 18 lines · tip-bot for Jiang Liu
Commit-ID:  9eabc99a635a77cbf0948ce17d3cbc2b51680d4a
Gitweb:     http://git.kernel.org/tip/9eabc99a635a77cbf0948ce17d3cbc2b51680d4a
Author:     Jiang Liu <[email protected]>
AuthorDate: Fri, 29 Aug 2014 17:26:23 +0800
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Fri, 29 Aug 2014 13:38:00 +0200

x86, irq, PCI: Keep IRQ assignment for runtime power management

Now IOAPIC driver dynamically allocates IRQ numbers for IOAPIC pins. We need to keep IRQ assignment for PCI devices during runtime power management, otherwise it may cause failure of device wakeups.

Commit 3eec595235c17a7 "x86, irq, PCI: Keep IRQ assignment for PCI devices during suspend/hibernation" has fixed the issue for suspend/ hibernation, we also need the same fix for runtime device sleep too.

Fix: https://bugzilla.kernel.org/show_bug.cgi?id=83271

Quoted 16 lines
Reported-and-Tested-by EmanueL Czirai <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Tony Luck <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc EmanueL Czirai <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Randy Dunlap <[email protected]>
Cc Yinghai Lu <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Grant Likely <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Thomas Gleixner <[email protected]>

This patch is incorrect, can you please drop it/revert it?

Rafael

Quoted 85 lines
---
 arch/x86/include/asm/io_apic.h |  2 ++
 arch/x86/kernel/apic/io_apic.c | 12 ++++++++++++
 arch/x86/pci/intel_mid_pci.c   |  2 +-
 arch/x86/pci/irq.c             |  2 +-
 drivers/acpi/pci_irq.c         |  4 ++++
 5 files changed, 20 insertions(+), 2 deletions(-)
arch/x86/include/asm/io_apic.h +2 -0
index 0aeed5c..478c490 100644
@@ -227,6 +227,8 @@ static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned
 
 extern void io_apic_eoi(unsigned int apic, unsigned int vector);
 
+extern bool mp_should_keep_irq(struct device *dev);
+
 #else  /* !CONFIG_X86_IO_APIC */
 
 #define io_apic_assign_pci_irqs 0
arch/x86/kernel/apic/io_apic.c +12 -0
index 40a4aa3..337ce5a 100644
@@ -3959,6 +3959,18 @@ int mp_set_gsi_attr(u32 gsi, int trigger, int polarity, int node)
 	return ret;
 }
 
+bool mp_should_keep_irq(struct device *dev)
+{
+	if (dev->power.is_prepared)
+		return true;
+#ifdef	CONFIG_PM_RUNTIME
+	if (dev->power.runtime_status == RPM_SUSPENDING)
+		return true;
+#endif
+
+	return false;
+}
+
 /* Enable IOAPIC early just for system timer */
 void __init pre_init_apic_IRQ0(void)
 {
arch/x86/pci/intel_mid_pci.c +1 -1
index 3865116..b9958c3 100644
@@ -229,7 +229,7 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
 
 static void intel_mid_pci_irq_disable(struct pci_dev *dev)
 {
-	if (!dev->dev.power.is_prepared && dev->irq > 0)
+	if (!mp_should_keep_irq(&dev->dev) && dev->irq > 0)
 		mp_unmap_irq(dev->irq);
 }
arch/x86/pci/irq.c +1 -1
index bc1a2c3..eb500c2 100644
@@ -1256,7 +1256,7 @@ static int pirq_enable_irq(struct pci_dev *dev)
 
 static void pirq_disable_irq(struct pci_dev *dev)
 {
-	if (io_apic_assign_pci_irqs && !dev->dev.power.is_prepared &&
+	if (io_apic_assign_pci_irqs && !mp_should_keep_irq(&dev->dev) &&
 	    dev->irq) {
 		mp_unmap_irq(dev->irq);
 		dev->irq = 0;
drivers/acpi/pci_irq.c +4 -0
index c96887d..6e6b80e 100644
@@ -484,6 +484,10 @@ void acpi_pci_irq_disable(struct pci_dev *dev)
 	/* Keep IOAPIC pin configuration when suspending */
 	if (dev->dev.power.is_prepared)
 		return;
+#ifdef	CONFIG_PM_RUNTIME
+	if (dev->dev.power.runtime_status == RPM_SUSPENDING)
+		return;
+#endif
 
 	entry = acpi_pci_irq_lookup(dev, pin);
 	if (!entry)
#3 lore raw
Quoted 36 lines · Rafael J. Wysocki
Quoted 18 lines · tip-bot for Jiang Liu
Commit-ID:  9eabc99a635a77cbf0948ce17d3cbc2b51680d4a
Gitweb:     http://git.kernel.org/tip/9eabc99a635a77cbf0948ce17d3cbc2b51680d4a
Author:     Jiang Liu <[email protected]>
AuthorDate: Fri, 29 Aug 2014 17:26:23 +0800
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Fri, 29 Aug 2014 13:38:00 +0200

x86, irq, PCI: Keep IRQ assignment for runtime power management

Now IOAPIC driver dynamically allocates IRQ numbers for IOAPIC pins. We need to keep IRQ assignment for PCI devices during runtime power management, otherwise it may cause failure of device wakeups.

Commit 3eec595235c17a7 "x86, irq, PCI: Keep IRQ assignment for PCI devices during suspend/hibernation" has fixed the issue for suspend/ hibernation, we also need the same fix for runtime device sleep too.

Fix: https://bugzilla.kernel.org/show_bug.cgi?id=83271

Quoted 16 lines
Reported-and-Tested-by EmanueL Czirai <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Tony Luck <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc EmanueL Czirai <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Randy Dunlap <[email protected]>
Cc Yinghai Lu <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Grant Likely <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Thomas Gleixner <[email protected]>
Quoted 1 line

This patch is incorrect, can you please drop it/revert it?

Well, I'm actually not sure that it is correct, but also it is not as outright incorrect as I thought.

The problem is that dev->power.runtime_status generally cannot be relied on outside of dev.power->lock, but if mp_should_keep_irq() is guaranteed to be executed from a callback run by rpm_suspend() for dev, then the value will be RPM_SUSPENDING.

I'm not quite sure that this always is the case, however.

Quoted 91 lines
Quoted 75 lines
---
 arch/x86/include/asm/io_apic.h |  2 ++
 arch/x86/kernel/apic/io_apic.c | 12 ++++++++++++
 arch/x86/pci/intel_mid_pci.c   |  2 +-
 arch/x86/pci/irq.c             |  2 +-
 drivers/acpi/pci_irq.c         |  4 ++++
 5 files changed, 20 insertions(+), 2 deletions(-)
arch/x86/include/asm/io_apic.h +2 -0
index 0aeed5c..478c490 100644
@@ -227,6 +227,8 @@ static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned
 
 extern void io_apic_eoi(unsigned int apic, unsigned int vector);
 
+extern bool mp_should_keep_irq(struct device *dev);
+
 #else  /* !CONFIG_X86_IO_APIC */
 
 #define io_apic_assign_pci_irqs 0
arch/x86/kernel/apic/io_apic.c +12 -0
index 40a4aa3..337ce5a 100644
@@ -3959,6 +3959,18 @@ int mp_set_gsi_attr(u32 gsi, int trigger, int polarity, int node)
 	return ret;
 }
 
+bool mp_should_keep_irq(struct device *dev)
+{
+	if (dev->power.is_prepared)
+		return true;
+#ifdef	CONFIG_PM_RUNTIME
+	if (dev->power.runtime_status == RPM_SUSPENDING)
+		return true;
+#endif
+
+	return false;
+}
+
 /* Enable IOAPIC early just for system timer */
 void __init pre_init_apic_IRQ0(void)
 {
arch/x86/pci/intel_mid_pci.c +1 -1
index 3865116..b9958c3 100644
@@ -229,7 +229,7 @@ static int intel_mid_pci_irq_enable(struct pci_dev *dev)
 
 static void intel_mid_pci_irq_disable(struct pci_dev *dev)
 {
-	if (!dev->dev.power.is_prepared && dev->irq > 0)
+	if (!mp_should_keep_irq(&dev->dev) && dev->irq > 0)
 		mp_unmap_irq(dev->irq);
 }
arch/x86/pci/irq.c +1 -1
index bc1a2c3..eb500c2 100644
@@ -1256,7 +1256,7 @@ static int pirq_enable_irq(struct pci_dev *dev)
 
 static void pirq_disable_irq(struct pci_dev *dev)
 {
-	if (io_apic_assign_pci_irqs && !dev->dev.power.is_prepared &&
+	if (io_apic_assign_pci_irqs && !mp_should_keep_irq(&dev->dev) &&
 	    dev->irq) {
 		mp_unmap_irq(dev->irq);
 		dev->irq = 0;
drivers/acpi/pci_irq.c +0 -0
index c96887d..6e6b80e 100644

drivers/acpi/pci_irq.c void acpi_pci_irq_disable(struct pci_dev *dev) · line 493

Quoted 10 lines
drivers/acpi/pci_irq.c +4 -0
@@ -484,6 +484,10 @@ void acpi_pci_irq_disable(struct pci_dev *dev)
 	/* Keep IOAPIC pin configuration when suspending */
 	if (dev->dev.power.is_prepared)
 		return;
+#ifdef	CONFIG_PM_RUNTIME
+	if (dev->dev.power.runtime_status == RPM_SUSPENDING)
+		return;
+#endif
 
 	entry = acpi_pci_irq_lookup(dev, pin);
 	if (!entry)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
maintainer
#4 lore raw
In reply to Jiang Liu
Quoted 38 lines · Jiang Liu

Now IOAPIC driver dynamically allocates IRQ numbers for IOAPIC pins. We need to keep IRQ assignment for PCI devices during runtime power management, otherwise it may cause failure of device wakeups.

Commit 3eec595235c17a7 "x86, irq, PCI: Keep IRQ assignment for PCI devices during suspend/hibernation" has fixed the issue for suspend/ hibernation, we also need the same fix for runtime device sleep too.

Fix: https://bugzilla.kernel.org/show_bug.cgi?id=83271

Reported-and-Tested-by EmanueL Czirai <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
---
 arch/x86/include/asm/io_apic.h |    2 ++
 arch/x86/kernel/apic/io_apic.c |   12 ++++++++++++
 arch/x86/pci/intel_mid_pci.c   |    2 +-
 arch/x86/pci/irq.c             |    2 +-
 drivers/acpi/pci_irq.c         |    4 ++++
 5 files changed, 20 insertions(+), 2 deletions(-)
arch/x86/include/asm/io_apic.h +2 -0
index 0aeed5ca356e..478c490f3654 100644
@@ -227,6 +227,8 @@ static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned
 
 extern void io_apic_eoi(unsigned int apic, unsigned int vector);
 
+extern bool mp_should_keep_irq(struct device *dev);
+
 #else  /* !CONFIG_X86_IO_APIC */
 
 #define io_apic_assign_pci_irqs 0
arch/x86/kernel/apic/io_apic.c +0 -0
index 29290f554e79..1e9a921d9701 100644
@@ -3946,6 +3946,18 @@ int mp_set_gsi_attr(u32 gsi, int trigger, int polarity, int node)
        return ret;

arch/x86/kernel/apic/io_apic.c int mp_set_gsi_attr(u32 gsi, int trigger, int polarity, int node) · line 3956

Quoted 10 lines
arch/x86/kernel/apic/io_apic.c +8 -0
 }
 
+bool mp_should_keep_irq(struct device *dev)
+{
+       if (dev->power.is_prepared)
+               return true;
+#ifdef CONFIG_PM_RUNTIME
+       if (dev->power.runtime_status == RPM_SUSPENDING)
+               return true;
+#endif

No, you can't do that. It is racy and incorrect.

Please give me some time for looking into this.

Rafael

#5 lore raw
Quoted 41 lines · Rafael J. Wysocki
Quoted 40 lines · Jiang Liu

Now IOAPIC driver dynamically allocates IRQ numbers for IOAPIC pins. We need to keep IRQ assignment for PCI devices during runtime power management, otherwise it may cause failure of device wakeups.

Commit 3eec595235c17a7 "x86, irq, PCI: Keep IRQ assignment for PCI devices during suspend/hibernation" has fixed the issue for suspend/ hibernation, we also need the same fix for runtime device sleep too.

Fix: https://bugzilla.kernel.org/show_bug.cgi?id=83271

Reported-and-Tested-by EmanueL Czirai <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
---
 arch/x86/include/asm/io_apic.h |    2 ++
 arch/x86/kernel/apic/io_apic.c |   12 ++++++++++++
 arch/x86/pci/intel_mid_pci.c   |    2 +-
 arch/x86/pci/irq.c             |    2 +-
 drivers/acpi/pci_irq.c         |    4 ++++
 5 files changed, 20 insertions(+), 2 deletions(-)
arch/x86/include/asm/io_apic.h +2 -0
index 0aeed5ca356e..478c490f3654 100644
@@ -227,6 +227,8 @@ static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned
 
 extern void io_apic_eoi(unsigned int apic, unsigned int vector);
 
+extern bool mp_should_keep_irq(struct device *dev);
+
 #else  /* !CONFIG_X86_IO_APIC */
 
 #define io_apic_assign_pci_irqs 0
arch/x86/kernel/apic/io_apic.c +0 -0
index 29290f554e79..1e9a921d9701 100644
@@ -3946,6 +3946,18 @@ int mp_set_gsi_attr(u32 gsi, int trigger, int polarity, int node)
        return ret;
 }
Quoted 10 lines

arch/x86/kernel/apic/io_apic.c int mp_set_gsi_attr(u32 gsi, int trigger, int polarity, int node) · line 3956

Quoted 8 lines
arch/x86/kernel/apic/io_apic.c +8 -0
+bool mp_should_keep_irq(struct device *dev)
+{
+       if (dev->power.is_prepared)
+               return true;
+#ifdef CONFIG_PM_RUNTIME
+       if (dev->power.runtime_status == RPM_SUSPENDING)
+               return true;
+#endif

No, you can't do that. It is racy and incorrect.

Well, maybe not.

Quoted 1 line

Please give me some time for looking into this.

So I guess the intended check is "Am I running in a suspend callback"?

Rafael

author
#6 lore raw
Quoted 53 lines · Rafael J. Wysocki
Quoted 41 lines · Rafael J. Wysocki
Quoted 40 lines · Jiang Liu

Now IOAPIC driver dynamically allocates IRQ numbers for IOAPIC pins. We need to keep IRQ assignment for PCI devices during runtime power management, otherwise it may cause failure of device wakeups.

Commit 3eec595235c17a7 "x86, irq, PCI: Keep IRQ assignment for PCI devices during suspend/hibernation" has fixed the issue for suspend/ hibernation, we also need the same fix for runtime device sleep too.

Fix: https://bugzilla.kernel.org/show_bug.cgi?id=83271

Reported-and-Tested-by EmanueL Czirai <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
---
 arch/x86/include/asm/io_apic.h |    2 ++
 arch/x86/kernel/apic/io_apic.c |   12 ++++++++++++
 arch/x86/pci/intel_mid_pci.c   |    2 +-
 arch/x86/pci/irq.c             |    2 +-
 drivers/acpi/pci_irq.c         |    4 ++++
 5 files changed, 20 insertions(+), 2 deletions(-)
arch/x86/include/asm/io_apic.h +2 -0
index 0aeed5ca356e..478c490f3654 100644
@@ -227,6 +227,8 @@ static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned
 
 extern void io_apic_eoi(unsigned int apic, unsigned int vector);
 
+extern bool mp_should_keep_irq(struct device *dev);
+
 #else  /* !CONFIG_X86_IO_APIC */
 
 #define io_apic_assign_pci_irqs 0
arch/x86/kernel/apic/io_apic.c +0 -0
index 29290f554e79..1e9a921d9701 100644
@@ -3946,6 +3946,18 @@ int mp_set_gsi_attr(u32 gsi, int trigger, int polarity, int node)
        return ret;
 }
Quoted 10 lines

arch/x86/kernel/apic/io_apic.c int mp_set_gsi_attr(u32 gsi, int trigger, int polarity, int node) · line 3956

Quoted 8 lines
arch/x86/kernel/apic/io_apic.c +8 -0
+bool mp_should_keep_irq(struct device *dev)
+{
+       if (dev->power.is_prepared)
+               return true;
+#ifdef CONFIG_PM_RUNTIME
+       if (dev->power.runtime_status == RPM_SUSPENDING)
+               return true;
+#endif

No, you can't do that. It is racy and incorrect.

Quoted 5 lines

Well, maybe not.

Quoted 1 line

Please give me some time for looking into this.

So I guess the intended check is "Am I running in a suspend callback"?

Hi Rafael,
Yes, we are trying to check whether it's called for suspend.
Function pci_disable_device() may be called when:
1) unbinding PCI driver
2) destroying PCI device
3) suspending for runtime power management or sleep
For case 1 and 2, we should release IRQ assigned. But we need to keep assigned IRQ for case 3.

So is it safe to check runtime_status and is_prepared flags to detect case 3? Any better ways?

Regards!
Gerry

Quoted 9 lines

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

#7 lore raw
Quoted 61 lines · Jiang Liu
Quoted 53 lines · Rafael J. Wysocki
Quoted 41 lines · Rafael J. Wysocki
Quoted 40 lines · Jiang Liu

Now IOAPIC driver dynamically allocates IRQ numbers for IOAPIC pins. We need to keep IRQ assignment for PCI devices during runtime power management, otherwise it may cause failure of device wakeups.

Commit 3eec595235c17a7 "x86, irq, PCI: Keep IRQ assignment for PCI devices during suspend/hibernation" has fixed the issue for suspend/ hibernation, we also need the same fix for runtime device sleep too.

Fix: https://bugzilla.kernel.org/show_bug.cgi?id=83271

Reported-and-Tested-by EmanueL Czirai <[email protected]>
Signed-off-by Jiang Liu <[email protected]>
---
 arch/x86/include/asm/io_apic.h |    2 ++
 arch/x86/kernel/apic/io_apic.c |   12 ++++++++++++
 arch/x86/pci/intel_mid_pci.c   |    2 +-
 arch/x86/pci/irq.c             |    2 +-
 drivers/acpi/pci_irq.c         |    4 ++++
 5 files changed, 20 insertions(+), 2 deletions(-)
arch/x86/include/asm/io_apic.h +2 -0
index 0aeed5ca356e..478c490f3654 100644
@@ -227,6 +227,8 @@ static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned
 
 extern void io_apic_eoi(unsigned int apic, unsigned int vector);
 
+extern bool mp_should_keep_irq(struct device *dev);
+
 #else  /* !CONFIG_X86_IO_APIC */
 
 #define io_apic_assign_pci_irqs 0
arch/x86/kernel/apic/io_apic.c +0 -0
index 29290f554e79..1e9a921d9701 100644
@@ -3946,6 +3946,18 @@ int mp_set_gsi_attr(u32 gsi, int trigger, int polarity, int node)
        return ret;
 }
Quoted 10 lines

arch/x86/kernel/apic/io_apic.c int mp_set_gsi_attr(u32 gsi, int trigger, int polarity, int node) · line 3956

Quoted 8 lines
arch/x86/kernel/apic/io_apic.c +8 -0
+bool mp_should_keep_irq(struct device *dev)
+{
+       if (dev->power.is_prepared)
+               return true;
+#ifdef CONFIG_PM_RUNTIME
+       if (dev->power.runtime_status == RPM_SUSPENDING)
+               return true;
+#endif

No, you can't do that. It is racy and incorrect.

Quoted 5 lines

Well, maybe not.

Quoted 1 line

Please give me some time for looking into this.

So I guess the intended check is "Am I running in a suspend callback"?

Quoted 8 lines

Hi Rafael,
Yes, we are trying to check whether it's called for suspend.
Function pci_disable_device() may be called when:
1) unbinding PCI driver
2) destroying PCI device
3) suspending for runtime power management or sleep
For case 1 and 2, we should release IRQ assigned. But we need to keep assigned IRQ for case 3.

Why don't we add an extra arg to that funtion, then, instead of trying to play with the PM core's internals in generally unreliable ways?

Quoted 2 lines

So is it safe to check runtime_status and is_prepared flags to detect case 3? Any better ways?

Above?

Both runtime_status and is_prepared are internal to the PM core. In fact, there's no guarantee that is_prepared will be needed in the future even, so you've made quite a bit of a mess by using it outside of the PM core.

Moreover, what you've done is obviously racy, but question is whether or not the race really matters. Like, for example, what happens if PCI device removal is triggered from user space via sysfs in parallel with a runtime suspend?

Plus the $subject patch introduced mp_should_keep_irq() and then duplicated the exact same code in acpi_pci_irq_disable(), so it doesn't look like it actually received enough consideration which is worrisome.

Moreover, the original patchset this is "fixing" apparently disregarded power management entirely which is even more concerning.

KR,
Rafael

Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
author
#8 lore raw

PATCH v4 1/1 x86, irq: Fix build error caused by 9eabc99a635a77cbf09

In reply to Jiang Liu

Commit 9eabc99a635a77cbf09 causes following build error when IOAPIC is disabled.
arch/x86/pci/irq.c: In function 'pirq_disable_irq':

Quoted 1 line
Quoted 1 line

arch/x86/pci/irq.c:1259:2: error: implicit declaration of function 'mp_should_keep_irq' [-Werror=implicit-function-declaration]

     if (io_apic_assign_pci_irqs && !mp_should_keep_irq(&dev->dev) &&
     ^

cc1: some warnings being treated as errors

Signed-off-by Jiang Liu <[email protected]>

---
Hi Thomas and HPA,
Really sorry about this. I only tested CONFIG_PM_RUNTIME,
but forgot CONFIG_X86_IO_APIC. Could you please help to merge or fold this patch?
Regards!
Gerry

---
 arch/x86/include/asm/io_apic.h |    1 +
 1 file changed, 1 insertion(+)
arch/x86/include/asm/io_apic.h +1 -0
index 478c490f3654..1733ab49ac5e 100644
@@ -239,6 +239,7 @@ static inline int mp_find_ioapic(u32 gsi) { return 0; }
 static inline u32 mp_pin_to_gsi(int ioapic, int pin) { return UINT_MAX; }
 static inline int mp_map_gsi_to_irq(u32 gsi, unsigned int flags) { return gsi; }
 static inline void mp_unmap_irq(int irq) { }
+static inline bool mp_should_keep_irq(struct device *dev) { return 1; }
 
 static inline int save_ioapic_entries(void)
 {
Signature · 1 line
1.7.10.4
merge bot
#9 lore raw

[tip:x86/urgent] x86, irq: Fix build error caused by 9eabc99a635a77cbf09

x86, irq: Fix build error caused by 9eabc99a635a77cbf09
Commit-ID:  f3761db1646d3ccb9ada2df8e27553a73ef43826
Gitweb:     http://git.kernel.org/tip/f3761db1646d3ccb9ada2df8e27553a73ef43826
Author:     Jiang Liu <[email protected]>
AuthorDate: Sat, 30 Aug 2014 15:15:16 +0800
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Mon, 1 Sep 2014 10:12:03 +0200

x86, irq: Fix build error caused by 9eabc99a635a77cbf09

Commit 9eabc99a635a77cbf09 causes following build error when IOAPIC is disabled.
arch/x86/pci/irq.c: In function 'pirq_disable_irq':

Quoted 1 line
Quoted 1 line

arch/x86/pci/irq.c:1259:2: error: implicit declaration of function 'mp_should_keep_irq' [-Werror=implicit-function-declaration]

     if (io_apic_assign_pci_irqs && !mp_should_keep_irq(&dev->dev) &&
     ^

cc1: some warnings being treated as errors

Signed-off-by Jiang Liu <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc Tony Luck <[email protected]>
Cc Joerg Roedel <[email protected]>
Cc Greg Kroah-Hartman <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
Cc Rafael J. Wysocki <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Randy Dunlap <[email protected]>
Cc Yinghai Lu <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Grant Likely <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Thomas Gleixner <[email protected]>
---
 arch/x86/include/asm/io_apic.h | 1 +
 1 file changed, 1 insertion(+)
arch/x86/include/asm/io_apic.h +1 -0
index 478c490..1733ab4 100644
@@ -239,6 +239,7 @@ static inline int mp_find_ioapic(u32 gsi) { return 0; }
 static inline u32 mp_pin_to_gsi(int ioapic, int pin) { return UINT_MAX; }
 static inline int mp_map_gsi_to_irq(u32 gsi, unsigned int flags) { return gsi; }
 static inline void mp_unmap_irq(int irq) { }
+static inline bool mp_should_keep_irq(struct device *dev) { return 1; }
 
 static inline int save_ioapic_entries(void)
 {