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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <4543591.GHid1Qx4Vl@vostro.rjw.lan>
Date:	Mon, 01 Sep 2014 15:52:06 +0200
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Jiang Liu <jiang.liu@...ux.intel.com>
Cc:	EmanueL Czirai <amanual@...nmailbox.org>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Randy Dunlap <rdunlap@...radead.org>,
	Yinghai Lu <yinghai@...nel.org>,
	Borislav Petkov <bp@...en8.de>,
	Grant Likely <grant.likely@...aro.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Tony Luck <tony.luck@...el.com>,
	Joerg Roedel <joro@...tes.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	x86@...nel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linux PCI <linux-pci@...r.kernel.org>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>
Subject: Re: [Patch v4] x86, irq, PCI: Keep IRQ assignment for runtime power management

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