[Patch v4] x86, irq, PCI: Keep IRQ assignment for runtime power management

[Patch v4] x86, irq, PCI: Keep IRQ assignment for runtime power management

[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 <amanual@openmailbox.org>
Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 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(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index 0aeed5ca356e..478c490f3654 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -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
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 29290f554e79..1e9a921d9701 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -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)
 {
diff --git a/arch/x86/pci/intel_mid_pci.c b/arch/x86/pci/intel_mid_pci.c
index 3865116c51fb..b9958c364075 100644
--- a/arch/x86/pci/intel_mid_pci.c
+++ b/arch/x86/pci/intel_mid_pci.c
@@ -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);
 }
 
diff --git a/arch/x86/pci/irq.c b/arch/x86/pci/irq.c
index bc1a2c341891..eb500c2592ad 100644
--- a/arch/x86/pci/irq.c
+++ b/arch/x86/pci/irq.c
@@ -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;
diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c
index c96887d5289e..6e6b80eb0bba 100644
--- a/drivers/acpi/pci_irq.c
+++ b/drivers/acpi/pci_irq.c
@@ -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)
-- 
1.7.10.4

Re: [Patch v4] x86, irq, PCI: Keep IRQ assignment for runtime power management

[Rafael J. Wysocki]

On Fri, Aug 29, 2014 at 11:26 AM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> 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 <amanual@openmailbox.org>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  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(-)
>
> diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
> index 0aeed5ca356e..478c490f3654 100644
> --- a/arch/x86/include/asm/io_apic.h
> +++ b/arch/x86/include/asm/io_apic.h
> @@ -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
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index 29290f554e79..1e9a921d9701 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -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

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

Please give me some time for looking into this.

Rafael

Re: [Patch v4] x86, irq, PCI: Keep IRQ assignment for runtime power management

[Rafael J. Wysocki]

On Friday, August 29, 2014 04:42:34 PM Rafael J. Wysocki wrote:
> On Fri, Aug 29, 2014 at 11:26 AM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> > 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 <amanual@openmailbox.org>
> > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> > ---
> >  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(-)
> >
> > diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
> > index 0aeed5ca356e..478c490f3654 100644
> > --- a/arch/x86/include/asm/io_apic.h
> > +++ b/arch/x86/include/asm/io_apic.h
> > @@ -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
> > diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> > index 29290f554e79..1e9a921d9701 100644
> > --- a/arch/x86/kernel/apic/io_apic.c
> > +++ b/arch/x86/kernel/apic/io_apic.c
> > @@ -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
> 
> No, you can't do that.  It is racy and incorrect.

Well, maybe not.

> Please give me some time for looking into this.

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

Rafael

Re: [Patch v4] x86, irq, PCI: Keep IRQ assignment for runtime power management

[Jiang Liu]

On 2014/8/30 7:24, Rafael J. Wysocki wrote:
> On Friday, August 29, 2014 04:42:34 PM Rafael J. Wysocki wrote:
>> On Fri, Aug 29, 2014 at 11:26 AM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
>>> 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 <amanual@openmailbox.org>
>>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>>> ---
>>>  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(-)
>>>
>>> diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
>>> index 0aeed5ca356e..478c490f3654 100644
>>> --- a/arch/x86/include/asm/io_apic.h
>>> +++ b/arch/x86/include/asm/io_apic.h
>>> @@ -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
>>> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
>>> index 29290f554e79..1e9a921d9701 100644
>>> --- a/arch/x86/kernel/apic/io_apic.c
>>> +++ b/arch/x86/kernel/apic/io_apic.c
>>> @@ -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
>>
>> No, you can't do that.  It is racy and incorrect.
> 
> Well, maybe not.
> 
>> 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


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

[Patch v4] x86, irq: Fix build error caused by 9eabc99a635a77cbf09

[Jiang Liu]

Commit 9eabc99a635a77cbf09 causes following build error when
IOAPIC is disabled.
   arch/x86/pci/irq.c: In function 'pirq_disable_irq':
>> 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 <jiang.liu@linux.intel.com>
---
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(+)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index 478c490f3654..1733ab49ac5e 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -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)
 {
-- 
1.7.10.4

Re: [Patch v4] x86, irq, PCI: Keep IRQ assignment for runtime power management

[Rafael J. Wysocki]

On Saturday, August 30, 2014 10:44:56 AM Jiang Liu wrote:
> 
> On 2014/8/30 7:24, Rafael J. Wysocki wrote:
> > On Friday, August 29, 2014 04:42:34 PM Rafael J. Wysocki wrote:
> >> On Fri, Aug 29, 2014 at 11:26 AM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> >>> 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 <amanual@openmailbox.org>
> >>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> >>> ---
> >>>  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(-)
> >>>
> >>> diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
> >>> index 0aeed5ca356e..478c490f3654 100644
> >>> --- a/arch/x86/include/asm/io_apic.h
> >>> +++ b/arch/x86/include/asm/io_apic.h
> >>> @@ -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
> >>> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> >>> index 29290f554e79..1e9a921d9701 100644
> >>> --- a/arch/x86/kernel/apic/io_apic.c
> >>> +++ b/arch/x86/kernel/apic/io_apic.c
> >>> @@ -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
> >>
> >> No, you can't do that.  It is racy and incorrect.
> > 
> > Well, maybe not.
> > 
> >> 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.

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?

> 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

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