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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> > >>> Signed-off-by: Jiang Liu <jiang.liu@xxxxxxxxxxxxxxx> > >>> --- > >>> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
