Re: [PATCH] ACPI / hotplug / PCI: Check presence of slot itself in get_slot_status()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12 February 2018 at 15:21, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> On Mon, Feb 12, 2018 at 11:08:55AM +0000, Mike Lothian wrote:
>> On 12 February 2018 at 10:55, Mika Westerberg
>> <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
>> > Mike Lothian reported that plugging in a USB-C device does not work
>> > properly in his Dell Alienware system. This system has Intel Alpine
>> > Ridge Thunderbolt controller providing USB-C functionality. In these
>> > systems the USB controller (xHCI) is hotplugged whenever a device is
>> > connected to the port using ACPI based hotplug.
>> >
>> > The ACPI description of the root port in question looks as follows:
>> >
>> > Device (RP01)
>> > {
>> >     Name (_ADR, 0x001C0000)
>> >
>> >     Device (PXSX)
>> >     {
>> >         Name (_ADR, 0x02)
>> >
>> >         Method (_RMV, 0, NotSerialized)
>> >         {
>> >             // ...
>> >         }
>> >     }
>> >
>> > Here _ADR 0x02 means device 0, function 2 on the bus under root port
>> > (RP1) but that seems to be incorrect because device 0 is the upstream
>> > port of the Alpine Ridge PCIe switch and it does not have other
>> > functions than 0 (the bridge itself). When we get ACPI Notify() to the
>> > root port resulting from connecting a USB-C device, Linux tries to read
>> > PCI_VENDOR_ID from device 0, function 2 which of course always returns
>> > 0xffffffff because there is no such function and we never find the
>> > device.
>> >
>> > In Windows this works fine.
>> >
>> > Now, since we get ACPI Notify() to the root port and not to the PXSX
>> > device we should actually start our scan from there as well and not from
>> > the non-existent PXSX device so fix this by checking presence of the
>> > slot itself (function 0) if we fail to do that otherwise.
>> >
>> > While there use pci_bus_read_dev_vendor_id() in get_slot_status() which
>> > is the recommended way to read device and vendor ID of device on PCI bus.
>> >
>> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=198557
>> > Reported-by: Mike Lothian <mike@xxxxxxxxxxxxxx>
>> > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
>> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> > Cc: stable@xxxxxxxxxxxxxxx
>> > ---
>> >  drivers/pci/hotplug/acpiphp_glue.c | 23 ++++++++++++++++-------
>> >  1 file changed, 16 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
>> > index e2198a2feeca..b45b375c0e6c 100644
>> > --- a/drivers/pci/hotplug/acpiphp_glue.c
>> > +++ b/drivers/pci/hotplug/acpiphp_glue.c
>> > @@ -541,6 +541,7 @@ static unsigned int get_slot_status(struct acpiphp_slot *slot)
>> >  {
>> >         unsigned long long sta = 0;
>> >         struct acpiphp_func *func;
>> > +       u32 dvid;
>> >
>> >         list_for_each_entry(func, &slot->funcs, sibling) {
>> >                 if (func->flags & FUNC_HAS_STA) {
>> > @@ -551,19 +552,27 @@ static unsigned int get_slot_status(struct acpiphp_slot *slot)
>> >                         if (ACPI_SUCCESS(status) && sta)
>> >                                 break;
>> >                 } else {
>> > -                       u32 dvid;
>> > -
>> > -                       pci_bus_read_config_dword(slot->bus,
>> > -                                                 PCI_DEVFN(slot->device,
>> > -                                                           func->function),
>> > -                                                 PCI_VENDOR_ID, &dvid);
>> > -                       if (dvid != 0xffffffff) {
>> > +                       if (pci_bus_read_dev_vendor_id(slot->bus,
>> > +                                       PCI_DEVFN(slot->device, func->function),
>> > +                                       &dvid, 0)) {
>> >                                 sta = ACPI_STA_ALL;
>> >                                 break;
>> >                         }
>> >                 }
>> >         }
>> >
>> > +       if (!sta) {
>> > +               /*
>> > +                * Check for the slot itself since it may be that the
>> > +                * ACPI slot is a device below PCIe upstream port so in
>> > +                * that case it may not even be reachable yet.
>> > +                */
>> > +               if (pci_bus_read_dev_vendor_id(slot->bus,
>> > +                               PCI_DEVFN(slot->device, 0), &dvid, 0)) {
>> > +                       sta = ACPI_STA_ALL;
>> > +               }
>> > +       }
>> > +
>> >         return (unsigned int)sta;
>> >  }
>> >
>> > --
>> > 2.15.1
>> >
>>
>> Thanks for this, it fixes a few other issues on my machine too
>
> Thanks for testing this.  Can you elaborate on what other issues this
> fixes?  I don't know what they are, but they might be worth mentioning
> in the changelog do make this fix more discoverable.
>
>> I'm going to do some searches around different bugzillas for the USB-C
>> non-detection and the NVMe drive disappearing after suspend issues and
>> report that they'll be getting a fix soon
>
> If you find more bugzillas that are fixed by this, please include the
> URLs so we can include them in the changelog.  (I can do this; no need
> to respin the patch just for this.)
>
>> Tested-by: Mike Lothian <mike@xxxxxxxxxxxxxx>
>> --
>> 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

Hi

The main issue was the NVMe drive disappearing after suspend/resume
https://bugzilla.kernel.org/show_bug.cgi?id=112121

The workaround was to disable hotplugging - most of the forum posts I
found relating to this issue have the solution acpiphp.disable=1

Through my bug I also realised that CONFIG_ACPI_PCI_SLOT also didn't
work and prevented my machine from booting - that's fixed now too

I've went round as many of the distro sites as possible posting about
this fix so people can avoid the work around and get working USB-C
ports

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1655100
https://bbs.archlinux.org/viewtopic.php?id=216520
https://askubuntu.com/questions/981657/cannot-suspend-with-nvme-m-2-ssd
https://forums.linuxmint.com/viewtopic.php?t=234368

Hope that helps

Mike



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux