diff mbox

ACPI / hotplug / PCI: Check presence of slot itself in get_slot_status()

Message ID 20180212105523.15984-1-mika.westerberg@linux.intel.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Mika Westerberg Feb. 12, 2018, 10:55 a.m. UTC
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@fireburn.co.uk>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: stable@vger.kernel.org
---
 drivers/pci/hotplug/acpiphp_glue.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Comments

Mike Lothian Feb. 12, 2018, 11:08 a.m. UTC | #1
On 12 February 2018 at 10:55, Mika Westerberg
<mika.westerberg@linux.intel.com> 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@fireburn.co.uk>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: stable@vger.kernel.org
> ---
>  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

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

Tested-by: Mike Lothian <mike@fireburn.co.uk>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Feb. 12, 2018, 12:05 p.m. UTC | #2
On Mon, Feb 12, 2018 at 11:55 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> 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@fireburn.co.uk>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: stable@vger.kernel.org

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Feb. 12, 2018, 3:21 p.m. UTC | #3
On Mon, Feb 12, 2018 at 11:08:55AM +0000, Mike Lothian wrote:
> On 12 February 2018 at 10:55, Mika Westerberg
> <mika.westerberg@linux.intel.com> 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@fireburn.co.uk>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: stable@vger.kernel.org
> > ---
> >  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@fireburn.co.uk>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Lothian Feb. 12, 2018, 3:42 p.m. UTC | #4
On 12 February 2018 at 15:21, Bjorn Helgaas <helgaas@kernel.org> 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@linux.intel.com> 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@fireburn.co.uk>
>> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > Cc: stable@vger.kernel.org
>> > ---
>> >  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@fireburn.co.uk>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Lothian Feb. 22, 2018, 9:19 a.m. UTC | #5
On 12 February 2018 at 15:21, Bjorn Helgaas <helgaas@kernel.org> 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@linux.intel.com> 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@fireburn.co.uk>
>> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > Cc: stable@vger.kernel.org
>> > ---
>> >  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@fireburn.co.uk>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi

Is this going in via this tree:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/

Is there anything else that needs to be done before it can go into rc3?

Cheers

Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Feb. 22, 2018, 1:13 p.m. UTC | #6
On Thu, Feb 22, 2018 at 09:19:24AM +0000, Mike Lothian wrote:
> On 12 February 2018 at 15:21, Bjorn Helgaas <helgaas@kernel.org> 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@linux.intel.com> 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@fireburn.co.uk>
> >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> > Cc: stable@vger.kernel.org
> >> > ---
> >> >  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@fireburn.co.uk>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Hi
> 
> Is this going in via this tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/
> 
> Is there anything else that needs to be done before it can go into rc3?

Not on your end.  But thanks fo the reminder, and also for the hint
that you'd like it in -rc3.  It's not likely to make -rc3 because I
like things to at least be in -next for a few days before I ask Linus
to pull them.

But more importantly, what's the justification for adding this after
the merge window?  It doesn't look like a regression or a fix for
something we broke in the merge window.  It certainly *could* still be
merged for v4.16, but the default target would be v4.17.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Lothian Feb. 23, 2018, 10:14 a.m. UTC | #7
On 22 February 2018 at 13:13, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> Not on your end.  But thanks fo the reminder, and also for the hint
> that you'd like it in -rc3.  It's not likely to make -rc3 because I
> like things to at least be in -next for a few days before I ask Linus
> to pull them.
>
> But more importantly, what's the justification for adding this after
> the merge window?  It doesn't look like a regression or a fix for
> something we broke in the merge window.  It certainly *could* still be
> merged for v4.16, but the default target would be v4.17.
>
> Bjorn

Hi Bjorn

Apologies, I thought all fixes got into the release candidates rather
than just regressions

I was hoping the original fix was going to make it into the merge
window that just passed

I just wanted to make sure the patch doesn't fall through the cracks

I'm carrying this patch locally so it doesn't make too much difference
to me which route it takes

Thanks again

Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Lothian March 23, 2018, 10:10 a.m. UTC | #8
Hi

This isn't in linux-next yet, does it still have enough time to sit in
linux-next to be included in the next merge window

Regards

Mike

On 14 March 2018 at 01:18, Mike Lothian <mike@fireburn.co.uk> wrote:
> Hi
>
> Just another reminder / hint that this hasn't landed in linux-next yet
>
> Regards
>
> Mike
>
> On Fri, 23 Feb 2018 at 10:14 Mike Lothian <mike@fireburn.co.uk> wrote:
>>
>> On 22 February 2018 at 13:13, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> >
>> > Not on your end.  But thanks fo the reminder, and also for the hint
>> > that you'd like it in -rc3.  It's not likely to make -rc3 because I
>> > like things to at least be in -next for a few days before I ask Linus
>> > to pull them.
>> >
>> > But more importantly, what's the justification for adding this after
>> > the merge window?  It doesn't look like a regression or a fix for
>> > something we broke in the merge window.  It certainly *could* still be
>> > merged for v4.16, but the default target would be v4.17.
>> >
>> > Bjorn
>>
>> Hi Bjorn
>>
>> Apologies, I thought all fixes got into the release candidates rather
>> than just regressions
>>
>> I was hoping the original fix was going to make it into the merge
>> window that just passed
>>
>> I just wanted to make sure the patch doesn't fall through the cracks
>>
>> I'm carrying this patch locally so it doesn't make too much difference
>> to me which route it takes
>>
>> Thanks again
>>
>> Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas March 23, 2018, 9:18 p.m. UTC | #9
On Mon, Feb 12, 2018 at 01:55:23PM +0300, Mika Westerberg 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@fireburn.co.uk>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: stable@vger.kernel.org

I (finally) applied this to pci/hotplug with Rafael's ack for v4.17,
thanks!

> ---
>  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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Lothian April 1, 2018, 10:36 p.m. UTC | #10
On 23 March 2018 at 21:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> I (finally) applied this to pci/hotplug with Rafael's ack for v4.17,
> thanks!
>

Sorry for sounding like a broken record but this still hasn't landed
in linux-next (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/drivers/pci/hotplug/acpiphp_glue.c)

Will it still be eligible for the recently opened merge window?
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas April 2, 2018, 12:42 a.m. UTC | #11
On Sun, Apr 01, 2018 at 11:36:02PM +0100, Mike Lothian wrote:
> On 23 March 2018 at 21:18, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > I (finally) applied this to pci/hotplug with Rafael's ack for v4.17,
> > thanks!
> >
> 
> Sorry for sounding like a broken record but this still hasn't landed
> in linux-next (https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/drivers/pci/hotplug/acpiphp_glue.c)

It's in my "next" branch pushed yesterday, so it should appear in
the next cut of linux-next.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 }