Re: [BUG] WARN_ON(!context) in drivers/pci/hotplug/acpiphp_glue.c

[Rafael J. Wysocki]

On Friday, October 11, 2013 11:58:48 PM Rafael J. Wysocki wrote:
> On Friday, October 11, 2013 10:21:35 AM Linus Torvalds wrote:
> > On Fri, Oct 11, 2013 at 4:13 AM, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:

[...]

> > Or am I misreading the code? It's more readable, and no longer makes
> > me homicidal, but I don't actually know the code itself.
> 
> I think you're reading it correctly, it really makes acpiphp see all slots
> even if pciehp sees them too.  So the change is somewhat risky.
> 
> That said the risk doesn't seem to be huge and there seem to be cases in
> which it actually would be useful to have both acpiphp and pciehp signaling
> available for the same device.  For example, even if the BIOS told us that
> we could use the native mechanism (pciehp), it may not actually work.  That is,
> we may not get any hotplug interrupts from PCIe ports due to platform bugs of
> some sort and we may get ACPI notifications instead (because the platform
> designer knew about those bugs and thought it would be smart to use ACPI to
> work around them).
> 
> There are bug reports indicating thinks like that, so we were going to allow
> acpiphp and pciehp to handle the same devices anyway at one point.  I thought
> we might as well try to do it now and see how it goes.  Still, if you think
> it's too risky for this stage of the cycle, I'll just send a patch removing
> the WARN_ON() and we'll revisit that thing in 3.13.

Having reconsidered this I think it's best to just drop the WARN_ON() for now
after all and sort out the coexistence between acpiphp and pciehp later, so
that we don't run into a weird corner case late in the cycle.

So the one below is what I'm going to do for 3.12.

Rafael

---
From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
Subject: ACPI / hotplug / PCI: Drop WARN_ON() from acpiphp_enumerate_slots()

The WARN_ON() in acpiphp_enumerate_slots() triggers unnecessarily for
devices whose bridges are going to be handled by native PCIe hotplug
(pciehp) and the simplest way to prevent that from happening is to
drop the WARN_ON().

References: https://bugzilla.kernel.org/show_bug.cgi?id=62831
Reported-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
---
 drivers/pci/hotplug/acpiphp_glue.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
@@ -999,12 +999,13 @@ void acpiphp_enumerate_slots(struct pci_
 
 		/*
 		 * This bridge should have been registered as a hotplug function
-		 * under its parent, so the context has to be there.  If not, we
-		 * are in deep goo.
+		 * under its parent, so the context should be there, unless the
+		 * parent is going to be handled by pciehp, in which case this
+		 * bridge is not interesting to us either.
 		 */
 		mutex_lock(&acpiphp_context_lock);
 		context = acpiphp_get_context(handle);
-		if (WARN_ON(!context)) {
+		if (!context) {
 			mutex_unlock(&acpiphp_context_lock);
 			put_device(&bus->dev);
 			kfree(bridge);

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