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

Bjorn Helgaas
2013-10-11T15:08:40Z

On Fri, Oct 11, 2013 at 7:01 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 11 Oct 2013 13:13:47 +0200
> "Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:
>>
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Subject: ACPI / hotplug / PCI: Accept coexistence with native PCIe hotplug
>>
>> Allow ACPIPHP (ACPI-based PCI hotplug) to handle event signaling for
>> devices that have already been claimed by the native PCIe hotplug
>> (pciehp).
>>
>> The ACPI hotplug events are essentially re-scan, remove and eject
>> requests. Re-scan and remove should work regardless, because they
>> may be triggered by user space via sysfs and the ACPI eject (_EJ0)
>> should work if the BIOS wants us to use it. There may be an issue
>> if the BIOS signals ACPI eject and wants us to use the native eject,
>> but that doesn't work without this change anyway.
>>
>> This change prevents the WARN_ON() in acpiphp_enumerate_slots() from
>> triggering unnecessarily for bridges whose parents are managed by
>> pciehp.
>>
>
> Reported-by: Steven Rostedt <rostedt@goodmis.org>
> Tested-by: Steven Rostedt <rostedt@goodmis.org>

I opened https://bugzilla.kernel.org/show_bug.cgi?id=62831 for this
issue because I don't think this question of how to coordinate acpiphp
and pciehp is completely resolved, and I think it might be interesting
to have the complete dmesg, acpidump, and "lspci -vv" output attached
there for future reference.

Steve, I tried to attach the dmesg you mentioned yesterday in IRC, but
the link didn't work for me. Would you mind attaching this info?

Rafael, I assume you'll probably merge this through your tree. Would
you mind adding a reference to this bugzilla in the changelog? I do
have a "convert to dynamic debug" acpiphp patch in my "next" branch
(bd950799), but I suspect you have several more interesting ones in
your tree.

Bjorn

>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>> drivers/pci/hotplug/acpiphp_glue.c | 32 ++++++++++++++++++++++++++------
>> 1 file changed, 26 insertions(+), 6 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
>> @@ -259,6 +259,31 @@ static void acpiphp_dock_release(void *d
>> put_bridge(context->func.parent);
>> }
>>
>> +/**
>> + * slot_should_be_exposed - Check whether or not to expose a slot to userland.
>> + * @bridge: ACPIPHP bridge the slot belongs to.
>> + * @handle: ACPI handle of a device in the slot.
>> + */
>> +static inline bool slot_should_be_exposed(struct acpiphp_bridge *bridge,
>> + acpi_handle handle)
>> +{
>> + struct pci_bus *pbus = bridge->pci_bus;
>> + struct pci_dev *pdev = bridge->pci_dev;
>> +
>> + /*
>> + * Do not expose slots whose bridges are managed by pciehp, because they
>> + * will be exposed to user space by the pciehp driver.
>> + */
>> + if (pdev && device_is_managed_by_native_pciehp(pdev))
>> + return false;
>> +
>> + /*
>> + * Expose slots for devices with either _EJ0 or _RMV and for devices
>> + * on docking stations.
>> + */
>> + return acpi_pci_check_ejectable(pbus, handle) || is_dock_device(handle);
>> +}
>> +
>> /* callback routine to register each ACPI PCI slot object */
>> static acpi_status register_slot(acpi_handle handle, u32 lvl, void *data,
>> void **rv)
>> @@ -271,12 +296,8 @@ static acpi_status register_slot(acpi_ha
>> unsigned long long adr;
>> int device, function;
>> struct pci_bus *pbus = bridge->pci_bus;
>> - struct pci_dev *pdev = bridge->pci_dev;
>> u32 val;
>>
>> - if (pdev && device_is_managed_by_native_pciehp(pdev))
>> - return AE_OK;
>> -
>> status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
>> if (ACPI_FAILURE(status)) {
>> acpi_handle_warn(handle, "can't evaluate _ADR (%#x)\n", status);
>> @@ -325,8 +346,7 @@ static acpi_status register_slot(acpi_ha
>>
>> list_add_tail(&slot->node, &bridge->slots);
>>
>> - /* Register slots for ejectable funtions only. */
>> - if (acpi_pci_check_ejectable(pbus, handle) || is_dock_device(handle)) {
>> + if (slot_should_be_exposed(bridge, handle)) {
>> unsigned long long sun;
>> int retval;
>>
>