[PATCH] pci/hotplug: work-around for missing _RMV on HP ZBook G2

[PATCH] pci/hotplug: work-around for missing _RMV on HP ZBook G2

[Jarod Wilson]

The HP ZBook 15 and 17 Mobile Workstations, generation 2, up to and
including at least BIOS revision 01.07, do not have an ACPI _RMV object
associated with their expresscard slots, so acpi-based hotplug-capable
slot detection fails. If we fall back to pcie-based detection, the systems
work just fine, so this uses dmi matching to do that. With luck, a future
BIOS will remedy this (I've let someone at HP know about the problem),
but for now, just use this for all existing versions.

Note: they *do* have a proper _RMV object for what I believe is their
thunderbolt ports.

Tested successfully on an HP ZBook 17 G2 and HP ZBook 15 G2.

CC: Len Brown <lenb@kernel.org>
CC: "Rafael J. Wysocki" <rjw@sisk.pl>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: linux-acpi@vger.kernel.org
CC: linux-pci@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
---
 drivers/pci/hotplug/pciehp_acpi.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_acpi.c b/drivers/pci/hotplug/pciehp_acpi.c
index 93cc926..db38fb5 100644
--- a/drivers/pci/hotplug/pciehp_acpi.c
+++ b/drivers/pci/hotplug/pciehp_acpi.c
@@ -28,6 +28,7 @@
 #include <linux/pci_hotplug.h>
 #include <linux/slab.h>
 #include <linux/module.h>
+#include <linux/dmi.h>
 #include "pciehp.h"
 
 #define PCIEHP_DETECT_PCIE	(0)
@@ -109,10 +110,40 @@ static struct pcie_port_service_driver __initdata dummy_driver = {
 	.probe		= dummy_probe,
 };
 
+static int __init set_slot_detection_mode_pcie(const struct dmi_system_id *d)
+{
+	info("%s lacks ACPI _RMV object for expresscard\n", d->ident);
+	return 1;
+}
+
+static struct dmi_system_id __initdata missing_acpi_rmv[] = {
+	/* ZBook 17 through at least bios v01.07 */
+	{
+	 .callback = set_slot_detection_mode_pcie,
+	 .ident = "HP ZBook 17 G2 Mobile Workstation",
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "HP ZBook 17 G2"),
+		},
+	},
+	/* ZBook 15 through at least bios v01.07 */
+	{
+	 .callback = set_slot_detection_mode_pcie,
+	 .ident = "HP ZBook 15 G2 Mobile Workstation",
+	 .matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "HP ZBook 15 G2"),
+		},
+	},
+	{ .ident = NULL }
+};
+
 static int __init select_detection_mode(void)
 {
 	struct dummy_slot *slot, *tmp;
 
+	if (dmi_check_system(missing_acpi_rmv))
+		return PCIEHP_DETECT_PCIE;
 	if (pcie_port_service_register(&dummy_driver))
 		return PCIEHP_DETECT_ACPI;
 	pcie_port_service_unregister(&dummy_driver);
@@ -134,4 +165,6 @@ void __init pciehp_acpi_slot_detection_init(void)
 out:
 	if (slot_detection_mode == PCIEHP_DETECT_ACPI)
 		info("Using ACPI for slot detection.\n");
+	else if (slot_detection_mode == PCIEHP_DETECT_PCIE)
+		info("Using PCIE-based slot detection.\n");
 }
-- 
1.8.3.1

Re: [PATCH] pci/hotplug: work-around for missing _RMV on HP ZBook G2

[Bjorn Helgaas]

Hi Jarod,

On Thu, May 14, 2015 at 03:33:58PM -0400, Jarod Wilson wrote:
> The HP ZBook 15 and 17 Mobile Workstations, generation 2, up to and
> including at least BIOS revision 01.07, do not have an ACPI _RMV object
> associated with their expresscard slots, so acpi-based hotplug-capable
> slot detection fails. If we fall back to pcie-based detection, the systems
> work just fine, so this uses dmi matching to do that. With luck, a future
> BIOS will remedy this (I've let someone at HP know about the problem),
> but for now, just use this for all existing versions.
> 
> Note: they *do* have a proper _RMV object for what I believe is their
> thunderbolt ports.
> 
> Tested successfully on an HP ZBook 17 G2 and HP ZBook 15 G2.
> 
> CC: Len Brown <lenb@kernel.org>
> CC: "Rafael J. Wysocki" <rjw@sisk.pl>
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: linux-acpi@vger.kernel.org
> CC: linux-pci@vger.kernel.org
> Signed-off-by: Jarod Wilson <jarod@redhat.com>
> ---
>  drivers/pci/hotplug/pciehp_acpi.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/pci/hotplug/pciehp_acpi.c b/drivers/pci/hotplug/pciehp_acpi.c
> index 93cc926..db38fb5 100644
> --- a/drivers/pci/hotplug/pciehp_acpi.c
> +++ b/drivers/pci/hotplug/pciehp_acpi.c
> @@ -28,6 +28,7 @@
>  #include <linux/pci_hotplug.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> +#include <linux/dmi.h>
>  #include "pciehp.h"
>  
>  #define PCIEHP_DETECT_PCIE	(0)
> @@ -109,10 +110,40 @@ static struct pcie_port_service_driver __initdata dummy_driver = {
>  	.probe		= dummy_probe,
>  };
>  
> +static int __init set_slot_detection_mode_pcie(const struct dmi_system_id *d)
> +{
> +	info("%s lacks ACPI _RMV object for expresscard\n", d->ident);
> +	return 1;
> +}
> +
> +static struct dmi_system_id __initdata missing_acpi_rmv[] = {
> +	/* ZBook 17 through at least bios v01.07 */
> +	{
> +	 .callback = set_slot_detection_mode_pcie,
> +	 .ident = "HP ZBook 17 G2 Mobile Workstation",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "HP ZBook 17 G2"),
> +		},
> +	},
> +	/* ZBook 15 through at least bios v01.07 */
> +	{
> +	 .callback = set_slot_detection_mode_pcie,
> +	 .ident = "HP ZBook 15 G2 Mobile Workstation",
> +	 .matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "HP ZBook 15 G2"),
> +		},
> +	},
> +	{ .ident = NULL }
> +};
> +
>  static int __init select_detection_mode(void)
>  {
>  	struct dummy_slot *slot, *tmp;
>  
> +	if (dmi_check_system(missing_acpi_rmv))
> +		return PCIEHP_DETECT_PCIE;

Oh, my goodness.  I forgot how terrible this path is.  Can anyone write a
simple explanation of how we choose to use acpiphp or pciehp?  Module
parameters?  A dummy driver that looks for duplicate slot numbers?  Looking
for _ADR, _EJ0, _RMV?  This is just nuts.

I can't really believe that we're doing this correctly.

If I understand correctly, the ZBooks don't have _RMV, but we try to use
acpiphp anyway, and acpiphp doesn't work?  That sounds more like a problem
with our acpiphp/pciehp selection "algorithm" than a BIOS bug.

Jarod, can you open a report at http://bugzilla.kernel.org and attach a
complete dmesg log, "lspci -vv" output, and an acpidump?  I'm particularly
interested in whether the BIOS granted us control over PCIe native hotplug.
If it did, I wonder why we would even attempt to use acpiphp.

Bjorn

>  	if (pcie_port_service_register(&dummy_driver))
>  		return PCIEHP_DETECT_ACPI;
>  	pcie_port_service_unregister(&dummy_driver);
> @@ -134,4 +165,6 @@ void __init pciehp_acpi_slot_detection_init(void)
>  out:
>  	if (slot_detection_mode == PCIEHP_DETECT_ACPI)
>  		info("Using ACPI for slot detection.\n");
> +	else if (slot_detection_mode == PCIEHP_DETECT_PCIE)
> +		info("Using PCIE-based slot detection.\n");
>  }
> -- 
> 1.8.3.1
> 

Re: [PATCH] pci/hotplug: work-around for missing _RMV on HP ZBook G2

[Bjorn Helgaas]

[fix Rafael's email address]

On Sat, May 16, 2015 at 09:37:50AM -0500, Bjorn Helgaas wrote:
> Hi Jarod,
> 
> On Thu, May 14, 2015 at 03:33:58PM -0400, Jarod Wilson wrote:
> > The HP ZBook 15 and 17 Mobile Workstations, generation 2, up to and
> > including at least BIOS revision 01.07, do not have an ACPI _RMV object
> > associated with their expresscard slots, so acpi-based hotplug-capable
> > slot detection fails. If we fall back to pcie-based detection, the systems
> > work just fine, so this uses dmi matching to do that. With luck, a future
> > BIOS will remedy this (I've let someone at HP know about the problem),
> > but for now, just use this for all existing versions.
> > 
> > Note: they *do* have a proper _RMV object for what I believe is their
> > thunderbolt ports.
> > 
> > Tested successfully on an HP ZBook 17 G2 and HP ZBook 15 G2.
> > 
> > CC: Len Brown <lenb@kernel.org>
> > CC: "Rafael J. Wysocki" <rjw@sisk.pl>
> > CC: Bjorn Helgaas <bhelgaas@google.com>
> > CC: linux-acpi@vger.kernel.org
> > CC: linux-pci@vger.kernel.org
> > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> > ---
> >  drivers/pci/hotplug/pciehp_acpi.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> > 
> > diff --git a/drivers/pci/hotplug/pciehp_acpi.c b/drivers/pci/hotplug/pciehp_acpi.c
> > index 93cc926..db38fb5 100644
> > --- a/drivers/pci/hotplug/pciehp_acpi.c
> > +++ b/drivers/pci/hotplug/pciehp_acpi.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/pci_hotplug.h>
> >  #include <linux/slab.h>
> >  #include <linux/module.h>
> > +#include <linux/dmi.h>
> >  #include "pciehp.h"
> >  
> >  #define PCIEHP_DETECT_PCIE	(0)
> > @@ -109,10 +110,40 @@ static struct pcie_port_service_driver __initdata dummy_driver = {
> >  	.probe		= dummy_probe,
> >  };
> >  
> > +static int __init set_slot_detection_mode_pcie(const struct dmi_system_id *d)
> > +{
> > +	info("%s lacks ACPI _RMV object for expresscard\n", d->ident);
> > +	return 1;
> > +}
> > +
> > +static struct dmi_system_id __initdata missing_acpi_rmv[] = {
> > +	/* ZBook 17 through at least bios v01.07 */
> > +	{
> > +	 .callback = set_slot_detection_mode_pcie,
> > +	 .ident = "HP ZBook 17 G2 Mobile Workstation",
> > +	 .matches = {
> > +		DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
> > +		DMI_MATCH(DMI_PRODUCT_NAME, "HP ZBook 17 G2"),
> > +		},
> > +	},
> > +	/* ZBook 15 through at least bios v01.07 */
> > +	{
> > +	 .callback = set_slot_detection_mode_pcie,
> > +	 .ident = "HP ZBook 15 G2 Mobile Workstation",
> > +	 .matches = {
> > +		DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
> > +		DMI_MATCH(DMI_PRODUCT_NAME, "HP ZBook 15 G2"),
> > +		},
> > +	},
> > +	{ .ident = NULL }
> > +};
> > +
> >  static int __init select_detection_mode(void)
> >  {
> >  	struct dummy_slot *slot, *tmp;
> >  
> > +	if (dmi_check_system(missing_acpi_rmv))
> > +		return PCIEHP_DETECT_PCIE;
> 
> Oh, my goodness.  I forgot how terrible this path is.  Can anyone write a
> simple explanation of how we choose to use acpiphp or pciehp?  Module
> parameters?  A dummy driver that looks for duplicate slot numbers?  Looking
> for _ADR, _EJ0, _RMV?  This is just nuts.
> 
> I can't really believe that we're doing this correctly.
> 
> If I understand correctly, the ZBooks don't have _RMV, but we try to use
> acpiphp anyway, and acpiphp doesn't work?  That sounds more like a problem
> with our acpiphp/pciehp selection "algorithm" than a BIOS bug.
> 
> Jarod, can you open a report at http://bugzilla.kernel.org and attach a
> complete dmesg log, "lspci -vv" output, and an acpidump?  I'm particularly
> interested in whether the BIOS granted us control over PCIe native hotplug.
> If it did, I wonder why we would even attempt to use acpiphp.
> 
> Bjorn
> 
> >  	if (pcie_port_service_register(&dummy_driver))
> >  		return PCIEHP_DETECT_ACPI;
> >  	pcie_port_service_unregister(&dummy_driver);
> > @@ -134,4 +165,6 @@ void __init pciehp_acpi_slot_detection_init(void)
> >  out:
> >  	if (slot_detection_mode == PCIEHP_DETECT_ACPI)
> >  		info("Using ACPI for slot detection.\n");
> > +	else if (slot_detection_mode == PCIEHP_DETECT_PCIE)
> > +		info("Using PCIE-based slot detection.\n");
> >  }
> > -- 
> > 1.8.3.1
> > 

Re: [PATCH] pci/hotplug: work-around for missing _RMV on HP ZBook G2

[Rafael J. Wysocki]

On Saturday, May 16, 2015 09:41:55 AM Bjorn Helgaas wrote:
> [fix Rafael's email address]

Thanks. :-)

> On Sat, May 16, 2015 at 09:37:50AM -0500, Bjorn Helgaas wrote:
> > Hi Jarod,
> > 
> > On Thu, May 14, 2015 at 03:33:58PM -0400, Jarod Wilson wrote:
> > > The HP ZBook 15 and 17 Mobile Workstations, generation 2, up to and
> > > including at least BIOS revision 01.07, do not have an ACPI _RMV object
> > > associated with their expresscard slots, so acpi-based hotplug-capable
> > > slot detection fails. If we fall back to pcie-based detection, the systems
> > > work just fine, so this uses dmi matching to do that. With luck, a future
> > > BIOS will remedy this (I've let someone at HP know about the problem),
> > > but for now, just use this for all existing versions.
> > > 
> > > Note: they *do* have a proper _RMV object for what I believe is their
> > > thunderbolt ports.
> > > 
> > > Tested successfully on an HP ZBook 17 G2 and HP ZBook 15 G2.
> > > 
> > > CC: Len Brown <lenb@kernel.org>
> > > CC: "Rafael J. Wysocki" <rjw@sisk.pl>
> > > CC: Bjorn Helgaas <bhelgaas@google.com>
> > > CC: linux-acpi@vger.kernel.org
> > > CC: linux-pci@vger.kernel.org
> > > Signed-off-by: Jarod Wilson <jarod@redhat.com>
> > > ---
> > >  drivers/pci/hotplug/pciehp_acpi.c | 33 +++++++++++++++++++++++++++++++++
> > >  1 file changed, 33 insertions(+)
> > > 
> > > diff --git a/drivers/pci/hotplug/pciehp_acpi.c b/drivers/pci/hotplug/pciehp_acpi.c
> > > index 93cc926..db38fb5 100644
> > > --- a/drivers/pci/hotplug/pciehp_acpi.c
> > > +++ b/drivers/pci/hotplug/pciehp_acpi.c
> > > @@ -28,6 +28,7 @@
> > >  #include <linux/pci_hotplug.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/module.h>
> > > +#include <linux/dmi.h>
> > >  #include "pciehp.h"
> > >  
> > >  #define PCIEHP_DETECT_PCIE	(0)
> > > @@ -109,10 +110,40 @@ static struct pcie_port_service_driver __initdata dummy_driver = {
> > >  	.probe		= dummy_probe,
> > >  };
> > >  
> > > +static int __init set_slot_detection_mode_pcie(const struct dmi_system_id *d)
> > > +{
> > > +	info("%s lacks ACPI _RMV object for expresscard\n", d->ident);
> > > +	return 1;
> > > +}
> > > +
> > > +static struct dmi_system_id __initdata missing_acpi_rmv[] = {
> > > +	/* ZBook 17 through at least bios v01.07 */
> > > +	{
> > > +	 .callback = set_slot_detection_mode_pcie,
> > > +	 .ident = "HP ZBook 17 G2 Mobile Workstation",
> > > +	 .matches = {
> > > +		DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
> > > +		DMI_MATCH(DMI_PRODUCT_NAME, "HP ZBook 17 G2"),
> > > +		},
> > > +	},
> > > +	/* ZBook 15 through at least bios v01.07 */
> > > +	{
> > > +	 .callback = set_slot_detection_mode_pcie,
> > > +	 .ident = "HP ZBook 15 G2 Mobile Workstation",
> > > +	 .matches = {
> > > +		DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
> > > +		DMI_MATCH(DMI_PRODUCT_NAME, "HP ZBook 15 G2"),
> > > +		},
> > > +	},
> > > +	{ .ident = NULL }
> > > +};
> > > +
> > >  static int __init select_detection_mode(void)
> > >  {
> > >  	struct dummy_slot *slot, *tmp;
> > >  
> > > +	if (dmi_check_system(missing_acpi_rmv))
> > > +		return PCIEHP_DETECT_PCIE;
> > 
> > Oh, my goodness.  I forgot how terrible this path is.  Can anyone write a
> > simple explanation of how we choose to use acpiphp or pciehp?

In theory, that should depend on the _OSC handshake in acpi_pci_root_add().

If the firmware doesn't give us control of the PCIe features, we'll not use
pciehp (or at least that's the idea).

acpiphp is used if pciehp doesn't claim the device, AFAICS.

Rafael

Re: [PATCH] pci/hotplug: work-around for missing _RMV on HP ZBook G2

[Jarod Wilson]

On 5/16/2015 10:41 AM, Bjorn Helgaas wrote:
> [fix Rafael's email address]
>
> On Sat, May 16, 2015 at 09:37:50AM -0500, Bjorn Helgaas wrote:
>> Hi Jarod,
>>
>> On Thu, May 14, 2015 at 03:33:58PM -0400, Jarod Wilson wrote:
>>> The HP ZBook 15 and 17 Mobile Workstations, generation 2, up to and
>>> including at least BIOS revision 01.07, do not have an ACPI _RMV object
>>> associated with their expresscard slots, so acpi-based hotplug-capable
>>> slot detection fails. If we fall back to pcie-based detection, the systems
>>> work just fine, so this uses dmi matching to do that. With luck, a future
>>> BIOS will remedy this (I've let someone at HP know about the problem),
>>> but for now, just use this for all existing versions.
>>>
>>> Note: they *do* have a proper _RMV object for what I believe is their
>>> thunderbolt ports.
>>>
>>> Tested successfully on an HP ZBook 17 G2 and HP ZBook 15 G2.
>>>
>>> CC: Len Brown <lenb@kernel.org>
>>> CC: "Rafael J. Wysocki" <rjw@sisk.pl>
>>> CC: Bjorn Helgaas <bhelgaas@google.com>
>>> CC: linux-acpi@vger.kernel.org
>>> CC: linux-pci@vger.kernel.org
>>> Signed-off-by: Jarod Wilson <jarod@redhat.com>
>>> ---
>>>   drivers/pci/hotplug/pciehp_acpi.c | 33 +++++++++++++++++++++++++++++++++
>>>   1 file changed, 33 insertions(+)
>>>
>>> diff --git a/drivers/pci/hotplug/pciehp_acpi.c b/drivers/pci/hotplug/pciehp_acpi.c
>>> index 93cc926..db38fb5 100644
>>> --- a/drivers/pci/hotplug/pciehp_acpi.c
>>> +++ b/drivers/pci/hotplug/pciehp_acpi.c
>>> @@ -28,6 +28,7 @@
>>>   #include <linux/pci_hotplug.h>
>>>   #include <linux/slab.h>
>>>   #include <linux/module.h>
>>> +#include <linux/dmi.h>
>>>   #include "pciehp.h"
>>>
>>>   #define PCIEHP_DETECT_PCIE	(0)
>>> @@ -109,10 +110,40 @@ static struct pcie_port_service_driver __initdata dummy_driver = {
>>>   	.probe		= dummy_probe,
>>>   };
>>>
>>> +static int __init set_slot_detection_mode_pcie(const struct dmi_system_id *d)
>>> +{
>>> +	info("%s lacks ACPI _RMV object for expresscard\n", d->ident);
>>> +	return 1;
>>> +}
>>> +
>>> +static struct dmi_system_id __initdata missing_acpi_rmv[] = {
>>> +	/* ZBook 17 through at least bios v01.07 */
>>> +	{
>>> +	 .callback = set_slot_detection_mode_pcie,
>>> +	 .ident = "HP ZBook 17 G2 Mobile Workstation",
>>> +	 .matches = {
>>> +		DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
>>> +		DMI_MATCH(DMI_PRODUCT_NAME, "HP ZBook 17 G2"),
>>> +		},
>>> +	},
>>> +	/* ZBook 15 through at least bios v01.07 */
>>> +	{
>>> +	 .callback = set_slot_detection_mode_pcie,
>>> +	 .ident = "HP ZBook 15 G2 Mobile Workstation",
>>> +	 .matches = {
>>> +		DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
>>> +		DMI_MATCH(DMI_PRODUCT_NAME, "HP ZBook 15 G2"),
>>> +		},
>>> +	},
>>> +	{ .ident = NULL }
>>> +};
>>> +
>>>   static int __init select_detection_mode(void)
>>>   {
>>>   	struct dummy_slot *slot, *tmp;
>>>
>>> +	if (dmi_check_system(missing_acpi_rmv))
>>> +		return PCIEHP_DETECT_PCIE;
>>
>> Oh, my goodness.  I forgot how terrible this path is.  Can anyone write a
>> simple explanation of how we choose to use acpiphp or pciehp?  Module
>> parameters?  A dummy driver that looks for duplicate slot numbers?  Looking
>> for _ADR, _EJ0, _RMV?  This is just nuts.
>>
>> I can't really believe that we're doing this correctly.
>>
>> If I understand correctly, the ZBooks don't have _RMV, but we try to use
>> acpiphp anyway, and acpiphp doesn't work?

They do have an _RMV entry for another device, whatever is on 
0000:00:1c.0, which appears to be the thunderbolt port, but I have yet 
to verify that (no thunderbolt devices to play with yet). The 
expresscard slot is 0000:3c:02.0.

>> That sounds more like a problem
>> with our acpiphp/pciehp selection "algorithm" than a BIOS bug.
>>
>> Jarod, can you open a report at http://bugzilla.kernel.org and attach a
>> complete dmesg log, "lspci -vv" output, and an acpidump?  I'm particularly
>> interested in whether the BIOS granted us control over PCIe native hotplug.
>> If it did, I wonder why we would even attempt to use acpiphp.

Done:

https://bugzilla.kernel.org/show_bug.cgi?id=98581

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] pci/hotplug: work-around for missing _RMV on HP ZBook G2

[Jarod Wilson]

On 5/17/2015 8:26 PM, Rafael J. Wysocki wrote:
> On Saturday, May 16, 2015 09:41:55 AM Bjorn Helgaas wrote:
>> On Sat, May 16, 2015 at 09:37:50AM -0500, Bjorn Helgaas wrote:
>>> Hi Jarod,
>>>
>>> On Thu, May 14, 2015 at 03:33:58PM -0400, Jarod Wilson wrote:
>>>> The HP ZBook 15 and 17 Mobile Workstations, generation 2, up to and
>>>> including at least BIOS revision 01.07, do not have an ACPI _RMV object
>>>> associated with their expresscard slots, so acpi-based hotplug-capable
>>>> slot detection fails. If we fall back to pcie-based detection, the systems
>>>> work just fine, so this uses dmi matching to do that. With luck, a future
>>>> BIOS will remedy this (I've let someone at HP know about the problem),
>>>> but for now, just use this for all existing versions.
...
>>> Oh, my goodness.  I forgot how terrible this path is.  Can anyone write a
>>> simple explanation of how we choose to use acpiphp or pciehp?
>
> In theory, that should depend on the _OSC handshake in acpi_pci_root_add().
>
> If the firmware doesn't give us control of the PCIe features, we'll not use
> pciehp (or at least that's the idea).
>
> acpiphp is used if pciehp doesn't claim the device, AFAICS.

[    4.013326] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM 
ClockPM Segments MSI]
[    4.015860] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME 
AER PCIeCapability]

So at a glance, it would appear that pciehp *should* be claiming it, 
right? Something I noted in the bug I filed is that the device ID 
reported there is PNP0A08, and the root_device_id table that associates 
with acpi_pci_root_add() only includes PNP0A03 in it. Is that correct, 
or should 08 also be in there, which might remedy this? (I can test this 
out easily enough).

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] pci/hotplug: work-around for missing _RMV on HP ZBook G2

[Jarod Wilson]

On 5/18/2015 10:33 AM, Jarod Wilson wrote:
> On 5/17/2015 8:26 PM, Rafael J. Wysocki wrote:
>> On Saturday, May 16, 2015 09:41:55 AM Bjorn Helgaas wrote:
>>> On Sat, May 16, 2015 at 09:37:50AM -0500, Bjorn Helgaas wrote:
>>>> Hi Jarod,
>>>>
>>>> On Thu, May 14, 2015 at 03:33:58PM -0400, Jarod Wilson wrote:
>>>>> The HP ZBook 15 and 17 Mobile Workstations, generation 2, up to and
>>>>> including at least BIOS revision 01.07, do not have an ACPI _RMV
>>>>> object
>>>>> associated with their expresscard slots, so acpi-based hotplug-capable
>>>>> slot detection fails. If we fall back to pcie-based detection, the
>>>>> systems
>>>>> work just fine, so this uses dmi matching to do that. With luck, a
>>>>> future
>>>>> BIOS will remedy this (I've let someone at HP know about the problem),
>>>>> but for now, just use this for all existing versions.
> ...
>>>> Oh, my goodness.  I forgot how terrible this path is.  Can anyone
>>>> write a
>>>> simple explanation of how we choose to use acpiphp or pciehp?
>>
>> In theory, that should depend on the _OSC handshake in
>> acpi_pci_root_add().
>>
>> If the firmware doesn't give us control of the PCIe features, we'll
>> not use
>> pciehp (or at least that's the idea).
>>
>> acpiphp is used if pciehp doesn't claim the device, AFAICS.
>
> [    4.013326] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
> ClockPM Segments MSI]
> [    4.015860] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME
> AER PCIeCapability]
>
> So at a glance, it would appear that pciehp *should* be claiming it,
> right? Something I noted in the bug I filed is that the device ID
> reported there is PNP0A08, and the root_device_id table that associates
> with acpi_pci_root_add() only includes PNP0A03 in it. Is that correct,
> or should 08 also be in there, which might remedy this? (I can test this
> out easily enough).

Nope, makes no difference, seems those are just two different references 
to the same bus, based on a peek at the extracted dsdt:

Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */)  // _HID: Hardware ID
Name (_CID, EisaId ("PNP0A03") /* PCI Bus */)  // _CID: Compatible ID

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] pci/hotplug: work-around for missing _RMV on HP ZBook G2

[Jarod Wilson]

On 5/18/2015 12:17 PM, Jarod Wilson wrote:
> On 5/18/2015 10:33 AM, Jarod Wilson wrote:
>> On 5/17/2015 8:26 PM, Rafael J. Wysocki wrote:
>>> On Saturday, May 16, 2015 09:41:55 AM Bjorn Helgaas wrote:
>>>> On Sat, May 16, 2015 at 09:37:50AM -0500, Bjorn Helgaas wrote:
>>>>> Hi Jarod,
>>>>>
>>>>> On Thu, May 14, 2015 at 03:33:58PM -0400, Jarod Wilson wrote:
>>>>>> The HP ZBook 15 and 17 Mobile Workstations, generation 2, up to and
>>>>>> including at least BIOS revision 01.07, do not have an ACPI _RMV
>>>>>> object
>>>>>> associated with their expresscard slots, so acpi-based
>>>>>> hotplug-capable
>>>>>> slot detection fails. If we fall back to pcie-based detection, the
>>>>>> systems
>>>>>> work just fine, so this uses dmi matching to do that. With luck, a
>>>>>> future
>>>>>> BIOS will remedy this (I've let someone at HP know about the
>>>>>> problem),
>>>>>> but for now, just use this for all existing versions.
>> ...
>>>>> Oh, my goodness.  I forgot how terrible this path is.  Can anyone
>>>>> write a
>>>>> simple explanation of how we choose to use acpiphp or pciehp?
>>>
>>> In theory, that should depend on the _OSC handshake in
>>> acpi_pci_root_add().
>>>
>>> If the firmware doesn't give us control of the PCIe features, we'll
>>> not use
>>> pciehp (or at least that's the idea).
>>>
>>> acpiphp is used if pciehp doesn't claim the device, AFAICS.
>>
>> [    4.013326] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
>> ClockPM Segments MSI]
>> [    4.015860] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME
>> AER PCIeCapability]
>>
>> So at a glance, it would appear that pciehp *should* be claiming it,
>> right? Something I noted in the bug I filed is that the device ID
>> reported there is PNP0A08, and the root_device_id table that associates
>> with acpi_pci_root_add() only includes PNP0A03 in it. Is that correct,
>> or should 08 also be in there, which might remedy this? (I can test this
>> out easily enough).
>
> Nope, makes no difference, seems those are just two different references
> to the same bus, based on a peek at the extracted dsdt:
>
> Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */)  // _HID: Hardware ID
> Name (_CID, EisaId ("PNP0A03") /* PCI Bus */)  // _CID: Compatible ID

Ah, I forgot some additional details. pciehp_probe() in 
drivers/pci/hotplug/pciehp_core.c fails on the 
pciehp_acpi_slot_detection_check() call for the expresscard slot, which 
is why the base pciehp doesn't bind. DEVICE_ACPI_HANDLE(&dev->dev) in 
the slot detection check is winding up with a NULL acpi device.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] pci/hotplug: work-around for missing _RMV on HP ZBook G2

[Rafael J. Wysocki]

On Monday, May 18, 2015 10:33:08 AM Jarod Wilson wrote:
> On 5/17/2015 8:26 PM, Rafael J. Wysocki wrote:
> > On Saturday, May 16, 2015 09:41:55 AM Bjorn Helgaas wrote:
> >> On Sat, May 16, 2015 at 09:37:50AM -0500, Bjorn Helgaas wrote:
> >>> Hi Jarod,
> >>>
> >>> On Thu, May 14, 2015 at 03:33:58PM -0400, Jarod Wilson wrote:
> >>>> The HP ZBook 15 and 17 Mobile Workstations, generation 2, up to and
> >>>> including at least BIOS revision 01.07, do not have an ACPI _RMV object
> >>>> associated with their expresscard slots, so acpi-based hotplug-capable
> >>>> slot detection fails. If we fall back to pcie-based detection, the systems
> >>>> work just fine, so this uses dmi matching to do that. With luck, a future
> >>>> BIOS will remedy this (I've let someone at HP know about the problem),
> >>>> but for now, just use this for all existing versions.
> ...
> >>> Oh, my goodness.  I forgot how terrible this path is.  Can anyone write a
> >>> simple explanation of how we choose to use acpiphp or pciehp?
> >
> > In theory, that should depend on the _OSC handshake in acpi_pci_root_add().
> >
> > If the firmware doesn't give us control of the PCIe features, we'll not use
> > pciehp (or at least that's the idea).
> >
> > acpiphp is used if pciehp doesn't claim the device, AFAICS.

That wasn't precise enough, sorry.  ACPIPHP will handle device check and bus
check notifications from ACPI even for devices that have been claimed by
pciehp.  It won't call pci_hp_register() for them, though.

> [    4.013326] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM 
> ClockPM Segments MSI]
> [    4.015860] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME 
> AER PCIeCapability]
> 
> So at a glance, it would appear that pciehp *should* be claiming it, 
> right?

Yes, it should.

> Something I noted in the bug I filed is that the device ID 
> reported there is PNP0A08, and the root_device_id table that associates 
> with acpi_pci_root_add() only includes PNP0A03 in it. Is that correct, 
> or should 08 also be in there, which might remedy this? (I can test this 
> out easily enough).

It is correct (or at least not obviously incorrect).  The _OSC is for the host
bridge and determines the behavior for all devices below root ports.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] pci/hotplug: work-around for missing _RMV on HP ZBook G2

[Rafael J. Wysocki]

On Monday, May 18, 2015 04:45:28 PM Jarod Wilson wrote:
> On 5/18/2015 12:17 PM, Jarod Wilson wrote:
> > On 5/18/2015 10:33 AM, Jarod Wilson wrote:
> >> On 5/17/2015 8:26 PM, Rafael J. Wysocki wrote:
> >>> On Saturday, May 16, 2015 09:41:55 AM Bjorn Helgaas wrote:
> >>>> On Sat, May 16, 2015 at 09:37:50AM -0500, Bjorn Helgaas wrote:
> >>>>> Hi Jarod,
> >>>>>
> >>>>> On Thu, May 14, 2015 at 03:33:58PM -0400, Jarod Wilson wrote:
> >>>>>> The HP ZBook 15 and 17 Mobile Workstations, generation 2, up to and
> >>>>>> including at least BIOS revision 01.07, do not have an ACPI _RMV
> >>>>>> object
> >>>>>> associated with their expresscard slots, so acpi-based
> >>>>>> hotplug-capable
> >>>>>> slot detection fails. If we fall back to pcie-based detection, the
> >>>>>> systems
> >>>>>> work just fine, so this uses dmi matching to do that. With luck, a
> >>>>>> future
> >>>>>> BIOS will remedy this (I've let someone at HP know about the
> >>>>>> problem),
> >>>>>> but for now, just use this for all existing versions.
> >> ...
> >>>>> Oh, my goodness.  I forgot how terrible this path is.  Can anyone
> >>>>> write a
> >>>>> simple explanation of how we choose to use acpiphp or pciehp?
> >>>
> >>> In theory, that should depend on the _OSC handshake in
> >>> acpi_pci_root_add().
> >>>
> >>> If the firmware doesn't give us control of the PCIe features, we'll
> >>> not use
> >>> pciehp (or at least that's the idea).
> >>>
> >>> acpiphp is used if pciehp doesn't claim the device, AFAICS.
> >>
> >> [    4.013326] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
> >> ClockPM Segments MSI]
> >> [    4.015860] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME
> >> AER PCIeCapability]
> >>
> >> So at a glance, it would appear that pciehp *should* be claiming it,
> >> right? Something I noted in the bug I filed is that the device ID
> >> reported there is PNP0A08, and the root_device_id table that associates
> >> with acpi_pci_root_add() only includes PNP0A03 in it. Is that correct,
> >> or should 08 also be in there, which might remedy this? (I can test this
> >> out easily enough).
> >
> > Nope, makes no difference, seems those are just two different references
> > to the same bus, based on a peek at the extracted dsdt:
> >
> > Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */)  // _HID: Hardware ID
> > Name (_CID, EisaId ("PNP0A03") /* PCI Bus */)  // _CID: Compatible ID
> 
> Ah, I forgot some additional details. pciehp_probe() in 
> drivers/pci/hotplug/pciehp_core.c fails on the 
> pciehp_acpi_slot_detection_check() call for the expresscard slot, which 
> is why the base pciehp doesn't bind. DEVICE_ACPI_HANDLE(&dev->dev) in 
> the slot detection check is winding up with a NULL acpi device.

So IMO the bug is that select_detection_mode() assumes that ACPI should be
used as the PCIe hotplug detection method if it has found at least one
device that looks like an "ACPI hotplug slot" (Thuderbolt breaks that "logic").

To be honest, I'm not sure why we need the pciehp_acpi_slot_detection_check()
in pciehp_probe() at all.  It doesn't add any value as far as I can say.

If pciehp_probe() is called at all, we have registered a PCIe port service
and if this is a "hotplug" service, we wouldn't have registered it if the
_OSC handshake had not given us contol over native hotplug.

So I wonder if the patch below makes any difference.

---
 drivers/pci/hotplug/pciehp_core.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/pci/hotplug/pciehp_core.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/pciehp_core.c
+++ linux-pm/drivers/pci/hotplug/pciehp_core.c
@@ -248,12 +248,9 @@ static int pciehp_probe(struct pcie_devi
 	struct slot *slot;
 	u8 occupied, poweron;
 
-	if (pciehp_force)
-		dev_info(&dev->device,
-			 "Bypassing BIOS check for pciehp use on %s\n",
-			 pci_name(dev->port));
-	else if (pciehp_acpi_slot_detection_check(dev->port))
-		goto err_out_none;
+	/* If this is not a "hotplug" service, we have no business here. */
+	if (dev->service != PCIE_PORT_SERVICE_HP)
+		return -ENODEV;
 
 	if (!dev->port->subordinate) {
 		/* Can happen if we run out of bus numbers during probe */

Re: [PATCH] pci/hotplug: work-around for missing _RMV on HP ZBook G2

[Jarod Wilson]

On 5/18/2015 7:06 PM, Rafael J. Wysocki wrote:
> On Monday, May 18, 2015 04:45:28 PM Jarod Wilson wrote:
...
>>>>>>> On Thu, May 14, 2015 at 03:33:58PM -0400, Jarod Wilson wrote:
>>>>>>>> The HP ZBook 15 and 17 Mobile Workstations, generation 2, up to and
>>>>>>>> including at least BIOS revision 01.07, do not have an ACPI _RMV
>>>>>>>> object
>>>>>>>> associated with their expresscard slots, so acpi-based
>>>>>>>> hotplug-capable
>>>>>>>> slot detection fails. If we fall back to pcie-based detection, the
>>>>>>>> systems
>>>>>>>> work just fine
...
>> Ah, I forgot some additional details. pciehp_probe() in
>> drivers/pci/hotplug/pciehp_core.c fails on the
>> pciehp_acpi_slot_detection_check() call for the expresscard slot, which
>> is why the base pciehp doesn't bind. DEVICE_ACPI_HANDLE(&dev->dev) in
>> the slot detection check is winding up with a NULL acpi device.
>
> So IMO the bug is that select_detection_mode() assumes that ACPI should be
> used as the PCIe hotplug detection method if it has found at least one
> device that looks like an "ACPI hotplug slot" (Thuderbolt breaks that "logic").
>
> To be honest, I'm not sure why we need the pciehp_acpi_slot_detection_check()
> in pciehp_probe() at all.  It doesn't add any value as far as I can say.
>
> If pciehp_probe() is called at all, we have registered a PCIe port service
> and if this is a "hotplug" service, we wouldn't have registered it if the
> _OSC handshake had not given us contol over native hotplug.
>
> So I wonder if the patch below makes any difference.

Yeah, that also works, for the most part. You still get spew from 
pciehp_acpi_slot_detection_init() saying "Using ACPI for slot 
detection.", but in re-reading pciehp_acpi.c in its entirety... I can't 
see anything productive that it actually does. I'm of the mind that the 
entire file should just be nuked, the path from pciehp_core.c that your 
patch alters was the only one that called 
pciehp_acpi_slot_detection_check(), and everything else is basically the 
dummy probe and fluff, nothing meaningful actually happens. I can whip 
up a follow-up patch that neuters that file entirely in the morning. At 
the very least, the "Using ACPI" bit needs to be beaten into submission, 
since its not going to be accurate.

-- 
Jarod Wilson
jarod@redhat.com

Re: [PATCH] pci/hotplug: work-around for missing _RMV on HP ZBook G2

[Rafael J. Wysocki]

On Monday, May 18, 2015 11:06:53 PM Jarod Wilson wrote:
> On 5/18/2015 7:06 PM, Rafael J. Wysocki wrote:
> > On Monday, May 18, 2015 04:45:28 PM Jarod Wilson wrote:
> ...
> >>>>>>> On Thu, May 14, 2015 at 03:33:58PM -0400, Jarod Wilson wrote:
> >>>>>>>> The HP ZBook 15 and 17 Mobile Workstations, generation 2, up to and
> >>>>>>>> including at least BIOS revision 01.07, do not have an ACPI _RMV
> >>>>>>>> object
> >>>>>>>> associated with their expresscard slots, so acpi-based
> >>>>>>>> hotplug-capable
> >>>>>>>> slot detection fails. If we fall back to pcie-based detection, the
> >>>>>>>> systems
> >>>>>>>> work just fine
> ...
> >> Ah, I forgot some additional details. pciehp_probe() in
> >> drivers/pci/hotplug/pciehp_core.c fails on the
> >> pciehp_acpi_slot_detection_check() call for the expresscard slot, which
> >> is why the base pciehp doesn't bind. DEVICE_ACPI_HANDLE(&dev->dev) in
> >> the slot detection check is winding up with a NULL acpi device.
> >
> > So IMO the bug is that select_detection_mode() assumes that ACPI should be
> > used as the PCIe hotplug detection method if it has found at least one
> > device that looks like an "ACPI hotplug slot" (Thuderbolt breaks that "logic").
> >
> > To be honest, I'm not sure why we need the pciehp_acpi_slot_detection_check()
> > in pciehp_probe() at all.  It doesn't add any value as far as I can say.
> >
> > If pciehp_probe() is called at all, we have registered a PCIe port service
> > and if this is a "hotplug" service, we wouldn't have registered it if the
> > _OSC handshake had not given us contol over native hotplug.
> >
> > So I wonder if the patch below makes any difference.
> 
> Yeah, that also works, for the most part. You still get spew from 
> pciehp_acpi_slot_detection_init() saying "Using ACPI for slot 
> detection.", but in re-reading pciehp_acpi.c in its entirety... I can't 
> see anything productive that it actually does. I'm of the mind that the 
> entire file should just be nuked, the path from pciehp_core.c that your 
> patch alters was the only one that called 
> pciehp_acpi_slot_detection_check(), and everything else is basically the 
> dummy probe and fluff, nothing meaningful actually happens.

Right, all that is horrible horrible garbage.

> I can whip up a follow-up patch that neuters that file entirely in the morning.

Well, let me have that pleasure. :-)

Anyway, code that is only used in one place should be dropped along with its
only user.

> At the very least, the "Using ACPI" bit needs to be beaten into submission, 
> since its not going to be accurate.

An updated patch will follow this message.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH] PCIe / hotplug: Drop pointless ACPI-based "slot detection" check

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Jarod Wilson reports that the expresscard hotplug setup doesn't work
on HP ZBook G2.  The problem turns out to be the ACPI-based "slot
detection" code called from pciehp_probe() which tries to use some
questionable heuristics based on what ACPI objects are present for
the PCIe port device at hand to figure out whether or not to register
a hotplug slot for that port.

That code is used if there is at least one PCIe port having an ACPI
device configuration object related to hotplug (such as _EJ0 or _RMV)
and the Thunderbolt port on the affected machine has _RMV.  Of course,
Thunderbolt and PCIe native hotplug need not be mutually exclusive
(as they aren't on the machine in question), so that rule is simply
incorrect.

Moreover, the ACPI-based "slot detection" check does not add any
value if pciehp_probe() is called at all and the service type of the
device object it has been called for is PCIE_PORT_SERVICE_HP, because
PCIe hotplug services are only registered if the _OSC handshake in
acpi_pci_root_add() allows the kernel to control the PCIe native
hotplug feature.  No more checks need to be carried out to decide
whether or not to register a native PCIe hotlug slot in that case.

For the above reasons, make pciehp_probe() check if it has been
called for the right service type and drop the pointless ACPI-based
"slot detection" check from it.  Also remove the entire code whose
only user is that check (the entire pciehp_acpi.c file goes away
as a result) and drop function headers related to it from the
internal PCIeHP header file.

Link: http://marc.info/?t=143163219300002&r=1&w=2
Link: https://bugzilla.kernel.org/show_bug.cgi?id=98581
Reported-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Bjorn, that's -stable material I think.  It should be applicable at least
since commit 5ba113f7c4fb (PCI: acpiphp: Handle PCIe ports without native
hotplug capability) that was shipped in 3.10.

Thanks!

---
 drivers/pci/hotplug/pciehp.h      |   17 ----
 drivers/pci/hotplug/pciehp_acpi.c |  137 --------------------------------------
 drivers/pci/hotplug/pciehp_core.c |   10 --
 3 files changed, 3 insertions(+), 161 deletions(-)

Index: linux-pm/drivers/pci/hotplug/pciehp_core.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/pciehp_core.c
+++ linux-pm/drivers/pci/hotplug/pciehp_core.c
@@ -248,12 +248,9 @@ static int pciehp_probe(struct pcie_devi
 	struct slot *slot;
 	u8 occupied, poweron;
 
-	if (pciehp_force)
-		dev_info(&dev->device,
-			 "Bypassing BIOS check for pciehp use on %s\n",
-			 pci_name(dev->port));
-	else if (pciehp_acpi_slot_detection_check(dev->port))
-		goto err_out_none;
+	/* If this is not a "hotplug" service, we have no business here. */
+	if (dev->service != PCIE_PORT_SERVICE_HP)
+		return -ENODEV;
 
 	if (!dev->port->subordinate) {
 		/* Can happen if we run out of bus numbers during probe */
@@ -366,7 +363,6 @@ static int __init pcied_init(void)
 {
 	int retval = 0;
 
-	pciehp_firmware_init();
 	retval = pcie_port_service_register(&hpdriver_portdrv);
 	dbg("pcie_port_service_register = %d\n", retval);
 	info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
Index: linux-pm/drivers/pci/hotplug/pciehp_acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/pciehp_acpi.c
+++ /dev/null
@@ -1,137 +0,0 @@
-/*
- * ACPI related functions for PCI Express Hot Plug driver.
- *
- * Copyright (C) 2008 Kenji Kaneshige
- * Copyright (C) 2008 Fujitsu Limited.
- *
- * All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or (at
- * your option) any later version.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
- * NON INFRINGEMENT.  See the GNU General Public License for more
- * details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
- *
- */
-
-#include <linux/acpi.h>
-#include <linux/pci.h>
-#include <linux/pci_hotplug.h>
-#include <linux/slab.h>
-#include <linux/module.h>
-#include "pciehp.h"
-
-#define PCIEHP_DETECT_PCIE	(0)
-#define PCIEHP_DETECT_ACPI	(1)
-#define PCIEHP_DETECT_AUTO	(2)
-#define PCIEHP_DETECT_DEFAULT	PCIEHP_DETECT_AUTO
-
-struct dummy_slot {
-	u32 number;
-	struct list_head list;
-};
-
-static int slot_detection_mode;
-static char *pciehp_detect_mode;
-module_param(pciehp_detect_mode, charp, 0444);
-MODULE_PARM_DESC(pciehp_detect_mode,
-	 "Slot detection mode: pcie, acpi, auto\n"
-	 "  pcie          - Use PCIe based slot detection\n"
-	 "  acpi          - Use ACPI for slot detection\n"
-	 "  auto(default) - Auto select mode. Use acpi option if duplicate\n"
-	 "                  slot ids are found. Otherwise, use pcie option\n");
-
-int pciehp_acpi_slot_detection_check(struct pci_dev *dev)
-{
-	if (slot_detection_mode != PCIEHP_DETECT_ACPI)
-		return 0;
-	if (acpi_pci_detect_ejectable(ACPI_HANDLE(&dev->dev)))
-		return 0;
-	return -ENODEV;
-}
-
-static int __init parse_detect_mode(void)
-{
-	if (!pciehp_detect_mode)
-		return PCIEHP_DETECT_DEFAULT;
-	if (!strcmp(pciehp_detect_mode, "pcie"))
-		return PCIEHP_DETECT_PCIE;
-	if (!strcmp(pciehp_detect_mode, "acpi"))
-		return PCIEHP_DETECT_ACPI;
-	if (!strcmp(pciehp_detect_mode, "auto"))
-		return PCIEHP_DETECT_AUTO;
-	warn("bad specifier '%s' for pciehp_detect_mode. Use default\n",
-	     pciehp_detect_mode);
-	return PCIEHP_DETECT_DEFAULT;
-}
-
-static int __initdata dup_slot_id;
-static int __initdata acpi_slot_detected;
-static struct list_head __initdata dummy_slots = LIST_HEAD_INIT(dummy_slots);
-
-/* Dummy driver for duplicate name detection */
-static int __init dummy_probe(struct pcie_device *dev)
-{
-	u32 slot_cap;
-	acpi_handle handle;
-	struct dummy_slot *slot, *tmp;
-	struct pci_dev *pdev = dev->port;
-
-	pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &slot_cap);
-	slot = kzalloc(sizeof(*slot), GFP_KERNEL);
-	if (!slot)
-		return -ENOMEM;
-	slot->number = (slot_cap & PCI_EXP_SLTCAP_PSN) >> 19;
-	list_for_each_entry(tmp, &dummy_slots, list) {
-		if (tmp->number == slot->number)
-			dup_slot_id++;
-	}
-	list_add_tail(&slot->list, &dummy_slots);
-	handle = ACPI_HANDLE(&pdev->dev);
-	if (!acpi_slot_detected && acpi_pci_detect_ejectable(handle))
-		acpi_slot_detected = 1;
-	return -ENODEV;         /* dummy driver always returns error */
-}
-
-static struct pcie_port_service_driver __initdata dummy_driver = {
-	.name		= "pciehp_dummy",
-	.port_type	= PCIE_ANY_PORT,
-	.service	= PCIE_PORT_SERVICE_HP,
-	.probe		= dummy_probe,
-};
-
-static int __init select_detection_mode(void)
-{
-	struct dummy_slot *slot, *tmp;
-
-	if (pcie_port_service_register(&dummy_driver))
-		return PCIEHP_DETECT_ACPI;
-	pcie_port_service_unregister(&dummy_driver);
-	list_for_each_entry_safe(slot, tmp, &dummy_slots, list) {
-		list_del(&slot->list);
-		kfree(slot);
-	}
-	if (acpi_slot_detected && dup_slot_id)
-		return PCIEHP_DETECT_ACPI;
-	return PCIEHP_DETECT_PCIE;
-}
-
-void __init pciehp_acpi_slot_detection_init(void)
-{
-	slot_detection_mode = parse_detect_mode();
-	if (slot_detection_mode != PCIEHP_DETECT_AUTO)
-		goto out;
-	slot_detection_mode = select_detection_mode();
-out:
-	if (slot_detection_mode == PCIEHP_DETECT_ACPI)
-		info("Using ACPI for slot detection.\n");
-}
Index: linux-pm/drivers/pci/hotplug/pciehp.h
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/pciehp.h
+++ linux-pm/drivers/pci/hotplug/pciehp.h
@@ -167,21 +167,4 @@ static inline const char *slot_name(stru
 	return hotplug_slot_name(slot->hotplug_slot);
 }
 
-#ifdef CONFIG_ACPI
-#include <linux/pci-acpi.h>
-
-void __init pciehp_acpi_slot_detection_init(void);
-int pciehp_acpi_slot_detection_check(struct pci_dev *dev);
-
-static inline void pciehp_firmware_init(void)
-{
-	pciehp_acpi_slot_detection_init();
-}
-#else
-#define pciehp_firmware_init()				do {} while (0)
-static inline int pciehp_acpi_slot_detection_check(struct pci_dev *dev)
-{
-	return 0;
-}
-#endif				/* CONFIG_ACPI */
 #endif				/* _PCIEHP_H */

Re: [PATCH] PCIe / hotplug: Drop pointless ACPI-based "slot detection" check

[Jarod Wilson]

On 5/19/2015 7:43 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Jarod Wilson reports that the expresscard hotplug setup doesn't work
> on HP ZBook G2.  The problem turns out to be the ACPI-based "slot
> detection" code called from pciehp_probe() which tries to use some
> questionable heuristics based on what ACPI objects are present for
> the PCIe port device at hand to figure out whether or not to register
> a hotplug slot for that port.
>
> That code is used if there is at least one PCIe port having an ACPI
> device configuration object related to hotplug (such as _EJ0 or _RMV)
> and the Thunderbolt port on the affected machine has _RMV.  Of course,
> Thunderbolt and PCIe native hotplug need not be mutually exclusive
> (as they aren't on the machine in question), so that rule is simply
> incorrect.
>
> Moreover, the ACPI-based "slot detection" check does not add any
> value if pciehp_probe() is called at all and the service type of the
> device object it has been called for is PCIE_PORT_SERVICE_HP, because
> PCIe hotplug services are only registered if the _OSC handshake in
> acpi_pci_root_add() allows the kernel to control the PCIe native
> hotplug feature.  No more checks need to be carried out to decide
> whether or not to register a native PCIe hotlug slot in that case.
>
> For the above reasons, make pciehp_probe() check if it has been
> called for the right service type and drop the pointless ACPI-based
> "slot detection" check from it.  Also remove the entire code whose
> only user is that check (the entire pciehp_acpi.c file goes away
> as a result) and drop function headers related to it from the
> internal PCIeHP header file.
>
> Link: http://marc.info/?t=143163219300002&r=1&w=2
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=98581
> Reported-by: Jarod Wilson <jarod@redhat.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> Bjorn, that's -stable material I think.  It should be applicable at least
> since commit 5ba113f7c4fb (PCI: acpiphp: Handle PCIe ports without native
> hotplug capability) that was shipped in 3.10.
>
> Thanks!

I believe you also want to rip the pciehp_acpi.o hunk out of 
drivers/pci/hotplug/Makefile, but I made more or less the exact same 
changes here, and am about to give them a spin on the ZBook 17.

-- 
Jarod Wilson
jarod@redhat.com

[Update][PATCH] PCIe / hotplug: Drop pointless ACPI-based "slot detection" check

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Jarod Wilson reports that the expresscard hotplug setup doesn't work
on HP ZBook G2.  The problem turns out to be the ACPI-based "slot
detection" code called from pciehp_probe() which tries to use some
questionable heuristics based on what ACPI objects are present for
the PCIe port device at hand to figure out whether or not to register
a hotplug slot for that port.

That code is used if there is at least one PCIe port having an ACPI
device configuration object related to hotplug (such as _EJ0 or _RMV)
and the Thunderbolt port on the affected machine has _RMV.  Of course,
Thunderbolt and PCIe native hotplug need not be mutually exclusive
(as they aren't on the machine in question), so that rule is simply
incorrect.

Moreover, the ACPI-based "slot detection" check does not add any
value if pciehp_probe() is called at all and the service type of the
device object it has been called for is PCIE_PORT_SERVICE_HP, because
PCIe hotplug services are only registered if the _OSC handshake in
acpi_pci_root_add() allows the kernel to control the PCIe native
hotplug feature.  No more checks need to be carried out to decide
whether or not to register a native PCIe hotlug slot in that case.

For the above reasons, make pciehp_probe() check if it has been
called for the right service type and drop the pointless ACPI-based
"slot detection" check from it.  Also remove the entire code whose
only user is that check (the entire pciehp_acpi.c file goes away
as a result) and drop function headers related to it from the
internal PCIeHP header file.

Link: http://marc.info/?t=143163219300002&r=1&w=2
Link: https://bugzilla.kernel.org/show_bug.cgi?id=98581
Reported-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

Changes in Makefile were missing from the previous version.

Bjorn, that's -stable material I think.  It should be applicable at least
since commit 5ba113f7c4fb (PCI: acpiphp: Handle PCIe ports without native
hotplug capability) that was shipped in 3.10.

Thanks!

---
 drivers/pci/hotplug/Makefile      |    3 
 drivers/pci/hotplug/pciehp.h      |   17 ----
 drivers/pci/hotplug/pciehp_acpi.c |  137 --------------------------------------
 drivers/pci/hotplug/pciehp_core.c |   10 --
 4 files changed, 3 insertions(+), 164 deletions(-)

Index: linux-pm/drivers/pci/hotplug/pciehp_core.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/pciehp_core.c
+++ linux-pm/drivers/pci/hotplug/pciehp_core.c
@@ -248,12 +248,9 @@ static int pciehp_probe(struct pcie_devi
 	struct slot *slot;
 	u8 occupied, poweron;
 
-	if (pciehp_force)
-		dev_info(&dev->device,
-			 "Bypassing BIOS check for pciehp use on %s\n",
-			 pci_name(dev->port));
-	else if (pciehp_acpi_slot_detection_check(dev->port))
-		goto err_out_none;
+	/* If this is not a "hotplug" service, we have no business here. */
+	if (dev->service != PCIE_PORT_SERVICE_HP)
+		return -ENODEV;
 
 	if (!dev->port->subordinate) {
 		/* Can happen if we run out of bus numbers during probe */
@@ -366,7 +363,6 @@ static int __init pcied_init(void)
 {
 	int retval = 0;
 
-	pciehp_firmware_init();
 	retval = pcie_port_service_register(&hpdriver_portdrv);
 	dbg("pcie_port_service_register = %d\n", retval);
 	info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
Index: linux-pm/drivers/pci/hotplug/pciehp_acpi.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/pciehp_acpi.c
+++ /dev/null
@@ -1,137 +0,0 @@
-/*
- * ACPI related functions for PCI Express Hot Plug driver.
- *
- * Copyright (C) 2008 Kenji Kaneshige
- * Copyright (C) 2008 Fujitsu Limited.
- *
- * All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or (at
- * your option) any later version.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
- * NON INFRINGEMENT.  See the GNU General Public License for more
- * details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
- *
- */
-
-#include <linux/acpi.h>
-#include <linux/pci.h>
-#include <linux/pci_hotplug.h>
-#include <linux/slab.h>
-#include <linux/module.h>
-#include "pciehp.h"
-
-#define PCIEHP_DETECT_PCIE	(0)
-#define PCIEHP_DETECT_ACPI	(1)
-#define PCIEHP_DETECT_AUTO	(2)
-#define PCIEHP_DETECT_DEFAULT	PCIEHP_DETECT_AUTO
-
-struct dummy_slot {
-	u32 number;
-	struct list_head list;
-};
-
-static int slot_detection_mode;
-static char *pciehp_detect_mode;
-module_param(pciehp_detect_mode, charp, 0444);
-MODULE_PARM_DESC(pciehp_detect_mode,
-	 "Slot detection mode: pcie, acpi, auto\n"
-	 "  pcie          - Use PCIe based slot detection\n"
-	 "  acpi          - Use ACPI for slot detection\n"
-	 "  auto(default) - Auto select mode. Use acpi option if duplicate\n"
-	 "                  slot ids are found. Otherwise, use pcie option\n");
-
-int pciehp_acpi_slot_detection_check(struct pci_dev *dev)
-{
-	if (slot_detection_mode != PCIEHP_DETECT_ACPI)
-		return 0;
-	if (acpi_pci_detect_ejectable(ACPI_HANDLE(&dev->dev)))
-		return 0;
-	return -ENODEV;
-}
-
-static int __init parse_detect_mode(void)
-{
-	if (!pciehp_detect_mode)
-		return PCIEHP_DETECT_DEFAULT;
-	if (!strcmp(pciehp_detect_mode, "pcie"))
-		return PCIEHP_DETECT_PCIE;
-	if (!strcmp(pciehp_detect_mode, "acpi"))
-		return PCIEHP_DETECT_ACPI;
-	if (!strcmp(pciehp_detect_mode, "auto"))
-		return PCIEHP_DETECT_AUTO;
-	warn("bad specifier '%s' for pciehp_detect_mode. Use default\n",
-	     pciehp_detect_mode);
-	return PCIEHP_DETECT_DEFAULT;
-}
-
-static int __initdata dup_slot_id;
-static int __initdata acpi_slot_detected;
-static struct list_head __initdata dummy_slots = LIST_HEAD_INIT(dummy_slots);
-
-/* Dummy driver for duplicate name detection */
-static int __init dummy_probe(struct pcie_device *dev)
-{
-	u32 slot_cap;
-	acpi_handle handle;
-	struct dummy_slot *slot, *tmp;
-	struct pci_dev *pdev = dev->port;
-
-	pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &slot_cap);
-	slot = kzalloc(sizeof(*slot), GFP_KERNEL);
-	if (!slot)
-		return -ENOMEM;
-	slot->number = (slot_cap & PCI_EXP_SLTCAP_PSN) >> 19;
-	list_for_each_entry(tmp, &dummy_slots, list) {
-		if (tmp->number == slot->number)
-			dup_slot_id++;
-	}
-	list_add_tail(&slot->list, &dummy_slots);
-	handle = ACPI_HANDLE(&pdev->dev);
-	if (!acpi_slot_detected && acpi_pci_detect_ejectable(handle))
-		acpi_slot_detected = 1;
-	return -ENODEV;         /* dummy driver always returns error */
-}
-
-static struct pcie_port_service_driver __initdata dummy_driver = {
-	.name		= "pciehp_dummy",
-	.port_type	= PCIE_ANY_PORT,
-	.service	= PCIE_PORT_SERVICE_HP,
-	.probe		= dummy_probe,
-};
-
-static int __init select_detection_mode(void)
-{
-	struct dummy_slot *slot, *tmp;
-
-	if (pcie_port_service_register(&dummy_driver))
-		return PCIEHP_DETECT_ACPI;
-	pcie_port_service_unregister(&dummy_driver);
-	list_for_each_entry_safe(slot, tmp, &dummy_slots, list) {
-		list_del(&slot->list);
-		kfree(slot);
-	}
-	if (acpi_slot_detected && dup_slot_id)
-		return PCIEHP_DETECT_ACPI;
-	return PCIEHP_DETECT_PCIE;
-}
-
-void __init pciehp_acpi_slot_detection_init(void)
-{
-	slot_detection_mode = parse_detect_mode();
-	if (slot_detection_mode != PCIEHP_DETECT_AUTO)
-		goto out;
-	slot_detection_mode = select_detection_mode();
-out:
-	if (slot_detection_mode == PCIEHP_DETECT_ACPI)
-		info("Using ACPI for slot detection.\n");
-}
Index: linux-pm/drivers/pci/hotplug/pciehp.h
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/pciehp.h
+++ linux-pm/drivers/pci/hotplug/pciehp.h
@@ -167,21 +167,4 @@ static inline const char *slot_name(stru
 	return hotplug_slot_name(slot->hotplug_slot);
 }
 
-#ifdef CONFIG_ACPI
-#include <linux/pci-acpi.h>
-
-void __init pciehp_acpi_slot_detection_init(void);
-int pciehp_acpi_slot_detection_check(struct pci_dev *dev);
-
-static inline void pciehp_firmware_init(void)
-{
-	pciehp_acpi_slot_detection_init();
-}
-#else
-#define pciehp_firmware_init()				do {} while (0)
-static inline int pciehp_acpi_slot_detection_check(struct pci_dev *dev)
-{
-	return 0;
-}
-#endif				/* CONFIG_ACPI */
 #endif				/* _PCIEHP_H */
Index: linux-pm/drivers/pci/hotplug/Makefile
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/Makefile
+++ linux-pm/drivers/pci/hotplug/Makefile
@@ -61,9 +61,6 @@ pciehp-objs		:=	pciehp_core.o	\
 				pciehp_ctrl.o	\
 				pciehp_pci.o	\
 				pciehp_hpc.o
-ifdef CONFIG_ACPI
-pciehp-objs		+=	pciehp_acpi.o
-endif
 
 shpchp-objs		:=	shpchp_core.o	\
 				shpchp_ctrl.o	\

Re: [PATCH] PCIe / hotplug: Drop pointless ACPI-based "slot detection" check

[Rafael J. Wysocki]

On Tuesday, May 19, 2015 08:42:00 AM Jarod Wilson wrote:
> On 5/19/2015 7:43 AM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Jarod Wilson reports that the expresscard hotplug setup doesn't work
> > on HP ZBook G2.  The problem turns out to be the ACPI-based "slot
> > detection" code called from pciehp_probe() which tries to use some
> > questionable heuristics based on what ACPI objects are present for
> > the PCIe port device at hand to figure out whether or not to register
> > a hotplug slot for that port.
> >
> > That code is used if there is at least one PCIe port having an ACPI
> > device configuration object related to hotplug (such as _EJ0 or _RMV)
> > and the Thunderbolt port on the affected machine has _RMV.  Of course,
> > Thunderbolt and PCIe native hotplug need not be mutually exclusive
> > (as they aren't on the machine in question), so that rule is simply
> > incorrect.
> >
> > Moreover, the ACPI-based "slot detection" check does not add any
> > value if pciehp_probe() is called at all and the service type of the
> > device object it has been called for is PCIE_PORT_SERVICE_HP, because
> > PCIe hotplug services are only registered if the _OSC handshake in
> > acpi_pci_root_add() allows the kernel to control the PCIe native
> > hotplug feature.  No more checks need to be carried out to decide
> > whether or not to register a native PCIe hotlug slot in that case.
> >
> > For the above reasons, make pciehp_probe() check if it has been
> > called for the right service type and drop the pointless ACPI-based
> > "slot detection" check from it.  Also remove the entire code whose
> > only user is that check (the entire pciehp_acpi.c file goes away
> > as a result) and drop function headers related to it from the
> > internal PCIeHP header file.
> >
> > Link: http://marc.info/?t=143163219300002&r=1&w=2
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=98581
> > Reported-by: Jarod Wilson <jarod@redhat.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > Bjorn, that's -stable material I think.  It should be applicable at least
> > since commit 5ba113f7c4fb (PCI: acpiphp: Handle PCIe ports without native
> > hotplug capability) that was shipped in 3.10.
> >
> > Thanks!
> 
> I believe you also want to rip the pciehp_acpi.o hunk out of 
> drivers/pci/hotplug/Makefile,

Right, just sent an updated patch.

> but I made more or less the exact same 
> changes here, and am about to give them a spin on the ZBook 17.

Cool, thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [Update][PATCH] PCIe / hotplug: Drop pointless ACPI-based "slot detection" check

[Jarod Wilson]

On 5/19/2015 9:27 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Jarod Wilson reports that the expresscard hotplug setup doesn't work
> on HP ZBook G2.  The problem turns out to be the ACPI-based "slot
> detection" code called from pciehp_probe() which tries to use some
> questionable heuristics based on what ACPI objects are present for
> the PCIe port device at hand to figure out whether or not to register
> a hotplug slot for that port.
>
> That code is used if there is at least one PCIe port having an ACPI
> device configuration object related to hotplug (such as _EJ0 or _RMV)
> and the Thunderbolt port on the affected machine has _RMV.  Of course,
> Thunderbolt and PCIe native hotplug need not be mutually exclusive
> (as they aren't on the machine in question), so that rule is simply
> incorrect.
>
> Moreover, the ACPI-based "slot detection" check does not add any
> value if pciehp_probe() is called at all and the service type of the
> device object it has been called for is PCIE_PORT_SERVICE_HP, because
> PCIe hotplug services are only registered if the _OSC handshake in
> acpi_pci_root_add() allows the kernel to control the PCIe native
> hotplug feature.  No more checks need to be carried out to decide
> whether or not to register a native PCIe hotlug slot in that case.
>
> For the above reasons, make pciehp_probe() check if it has been
> called for the right service type and drop the pointless ACPI-based
> "slot detection" check from it.  Also remove the entire code whose
> only user is that check (the entire pciehp_acpi.c file goes away
> as a result) and drop function headers related to it from the
> internal PCIeHP header file.
>
> Link: http://marc.info/?t=143163219300002&r=1&w=2
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=98581
> Reported-by: Jarod Wilson <jarod@redhat.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>
> Changes in Makefile were missing from the previous version.
>
> Bjorn, that's -stable material I think.  It should be applicable at least
> since commit 5ba113f7c4fb (PCI: acpiphp: Handle PCIe ports without native
> hotplug capability) that was shipped in 3.10.
>
> Thanks!

Changes all look good to me, and they work perfectly here with the 
previously problematic ZBook 17.

Reviewed-by: Jarod Wilson <jarod@redhat.com>
Tested-by: Jarod Wilson <jarod@redhat.com>


-- 
Jarod Wilson
jarod@redhat.com

Re: [Update][PATCH] PCIe / hotplug: Drop pointless ACPI-based "slot detection" check

[Bjorn Helgaas]

On Tue, May 19, 2015 at 03:27:58PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Jarod Wilson reports that the expresscard hotplug setup doesn't work
> on HP ZBook G2.  The problem turns out to be the ACPI-based "slot
> detection" code called from pciehp_probe() which tries to use some
> questionable heuristics based on what ACPI objects are present for
> the PCIe port device at hand to figure out whether or not to register
> a hotplug slot for that port.
> 
> That code is used if there is at least one PCIe port having an ACPI
> device configuration object related to hotplug (such as _EJ0 or _RMV)
> and the Thunderbolt port on the affected machine has _RMV.  Of course,
> Thunderbolt and PCIe native hotplug need not be mutually exclusive
> (as they aren't on the machine in question), so that rule is simply
> incorrect.
> 
> Moreover, the ACPI-based "slot detection" check does not add any
> value if pciehp_probe() is called at all and the service type of the
> device object it has been called for is PCIE_PORT_SERVICE_HP, because
> PCIe hotplug services are only registered if the _OSC handshake in
> acpi_pci_root_add() allows the kernel to control the PCIe native
> hotplug feature.  No more checks need to be carried out to decide
> whether or not to register a native PCIe hotlug slot in that case.
> 
> For the above reasons, make pciehp_probe() check if it has been
> called for the right service type and drop the pointless ACPI-based
> "slot detection" check from it.  Also remove the entire code whose
> only user is that check (the entire pciehp_acpi.c file goes away
> as a result) and drop function headers related to it from the
> internal PCIeHP header file.
> 
> Link: http://marc.info/?t=143163219300002&r=1&w=2
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=98581
> Reported-by: Jarod Wilson <jarod@redhat.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

This is awesome!  Applied to pci/hotplug for v4.2, with Jarod's
reviewed/tested-by.

I suspect a lot of this stuff dates back to when acpiphp and pciehp could
be modules, and one driver really couldn't know whether the other was up
to.  In any event, I think it will be much more predictable and
maintainable now.

Bjorn

> ---
> 
> Changes in Makefile were missing from the previous version.
> 
> Bjorn, that's -stable material I think.  It should be applicable at least
> since commit 5ba113f7c4fb (PCI: acpiphp: Handle PCIe ports without native
> hotplug capability) that was shipped in 3.10.
> 
> Thanks!
> 
> ---
>  drivers/pci/hotplug/Makefile      |    3 
>  drivers/pci/hotplug/pciehp.h      |   17 ----
>  drivers/pci/hotplug/pciehp_acpi.c |  137 --------------------------------------
>  drivers/pci/hotplug/pciehp_core.c |   10 --
>  4 files changed, 3 insertions(+), 164 deletions(-)
> 
> Index: linux-pm/drivers/pci/hotplug/pciehp_core.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/hotplug/pciehp_core.c
> +++ linux-pm/drivers/pci/hotplug/pciehp_core.c
> @@ -248,12 +248,9 @@ static int pciehp_probe(struct pcie_devi
>  	struct slot *slot;
>  	u8 occupied, poweron;
>  
> -	if (pciehp_force)
> -		dev_info(&dev->device,
> -			 "Bypassing BIOS check for pciehp use on %s\n",
> -			 pci_name(dev->port));
> -	else if (pciehp_acpi_slot_detection_check(dev->port))
> -		goto err_out_none;
> +	/* If this is not a "hotplug" service, we have no business here. */
> +	if (dev->service != PCIE_PORT_SERVICE_HP)
> +		return -ENODEV;
>  
>  	if (!dev->port->subordinate) {
>  		/* Can happen if we run out of bus numbers during probe */
> @@ -366,7 +363,6 @@ static int __init pcied_init(void)
>  {
>  	int retval = 0;
>  
> -	pciehp_firmware_init();
>  	retval = pcie_port_service_register(&hpdriver_portdrv);
>  	dbg("pcie_port_service_register = %d\n", retval);
>  	info(DRIVER_DESC " version: " DRIVER_VERSION "\n");
> Index: linux-pm/drivers/pci/hotplug/pciehp_acpi.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/hotplug/pciehp_acpi.c
> +++ /dev/null
> @@ -1,137 +0,0 @@
> -/*
> - * ACPI related functions for PCI Express Hot Plug driver.
> - *
> - * Copyright (C) 2008 Kenji Kaneshige
> - * Copyright (C) 2008 Fujitsu Limited.
> - *
> - * All rights reserved.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License as published by
> - * the Free Software Foundation; either version 2 of the License, or (at
> - * your option) any later version.
> - *
> - * This program is distributed in the hope that it will be useful, but
> - * WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> - * NON INFRINGEMENT.  See the GNU General Public License for more
> - * details.
> - *
> - * You should have received a copy of the GNU General Public License
> - * along with this program; if not, write to the Free Software
> - * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> - *
> - */
> -
> -#include <linux/acpi.h>
> -#include <linux/pci.h>
> -#include <linux/pci_hotplug.h>
> -#include <linux/slab.h>
> -#include <linux/module.h>
> -#include "pciehp.h"
> -
> -#define PCIEHP_DETECT_PCIE	(0)
> -#define PCIEHP_DETECT_ACPI	(1)
> -#define PCIEHP_DETECT_AUTO	(2)
> -#define PCIEHP_DETECT_DEFAULT	PCIEHP_DETECT_AUTO
> -
> -struct dummy_slot {
> -	u32 number;
> -	struct list_head list;
> -};
> -
> -static int slot_detection_mode;
> -static char *pciehp_detect_mode;
> -module_param(pciehp_detect_mode, charp, 0444);
> -MODULE_PARM_DESC(pciehp_detect_mode,
> -	 "Slot detection mode: pcie, acpi, auto\n"
> -	 "  pcie          - Use PCIe based slot detection\n"
> -	 "  acpi          - Use ACPI for slot detection\n"
> -	 "  auto(default) - Auto select mode. Use acpi option if duplicate\n"
> -	 "                  slot ids are found. Otherwise, use pcie option\n");
> -
> -int pciehp_acpi_slot_detection_check(struct pci_dev *dev)
> -{
> -	if (slot_detection_mode != PCIEHP_DETECT_ACPI)
> -		return 0;
> -	if (acpi_pci_detect_ejectable(ACPI_HANDLE(&dev->dev)))
> -		return 0;
> -	return -ENODEV;
> -}
> -
> -static int __init parse_detect_mode(void)
> -{
> -	if (!pciehp_detect_mode)
> -		return PCIEHP_DETECT_DEFAULT;
> -	if (!strcmp(pciehp_detect_mode, "pcie"))
> -		return PCIEHP_DETECT_PCIE;
> -	if (!strcmp(pciehp_detect_mode, "acpi"))
> -		return PCIEHP_DETECT_ACPI;
> -	if (!strcmp(pciehp_detect_mode, "auto"))
> -		return PCIEHP_DETECT_AUTO;
> -	warn("bad specifier '%s' for pciehp_detect_mode. Use default\n",
> -	     pciehp_detect_mode);
> -	return PCIEHP_DETECT_DEFAULT;
> -}
> -
> -static int __initdata dup_slot_id;
> -static int __initdata acpi_slot_detected;
> -static struct list_head __initdata dummy_slots = LIST_HEAD_INIT(dummy_slots);
> -
> -/* Dummy driver for duplicate name detection */
> -static int __init dummy_probe(struct pcie_device *dev)
> -{
> -	u32 slot_cap;
> -	acpi_handle handle;
> -	struct dummy_slot *slot, *tmp;
> -	struct pci_dev *pdev = dev->port;
> -
> -	pcie_capability_read_dword(pdev, PCI_EXP_SLTCAP, &slot_cap);
> -	slot = kzalloc(sizeof(*slot), GFP_KERNEL);
> -	if (!slot)
> -		return -ENOMEM;
> -	slot->number = (slot_cap & PCI_EXP_SLTCAP_PSN) >> 19;
> -	list_for_each_entry(tmp, &dummy_slots, list) {
> -		if (tmp->number == slot->number)
> -			dup_slot_id++;
> -	}
> -	list_add_tail(&slot->list, &dummy_slots);
> -	handle = ACPI_HANDLE(&pdev->dev);
> -	if (!acpi_slot_detected && acpi_pci_detect_ejectable(handle))
> -		acpi_slot_detected = 1;
> -	return -ENODEV;         /* dummy driver always returns error */
> -}
> -
> -static struct pcie_port_service_driver __initdata dummy_driver = {
> -	.name		= "pciehp_dummy",
> -	.port_type	= PCIE_ANY_PORT,
> -	.service	= PCIE_PORT_SERVICE_HP,
> -	.probe		= dummy_probe,
> -};
> -
> -static int __init select_detection_mode(void)
> -{
> -	struct dummy_slot *slot, *tmp;
> -
> -	if (pcie_port_service_register(&dummy_driver))
> -		return PCIEHP_DETECT_ACPI;
> -	pcie_port_service_unregister(&dummy_driver);
> -	list_for_each_entry_safe(slot, tmp, &dummy_slots, list) {
> -		list_del(&slot->list);
> -		kfree(slot);
> -	}
> -	if (acpi_slot_detected && dup_slot_id)
> -		return PCIEHP_DETECT_ACPI;
> -	return PCIEHP_DETECT_PCIE;
> -}
> -
> -void __init pciehp_acpi_slot_detection_init(void)
> -{
> -	slot_detection_mode = parse_detect_mode();
> -	if (slot_detection_mode != PCIEHP_DETECT_AUTO)
> -		goto out;
> -	slot_detection_mode = select_detection_mode();
> -out:
> -	if (slot_detection_mode == PCIEHP_DETECT_ACPI)
> -		info("Using ACPI for slot detection.\n");
> -}
> Index: linux-pm/drivers/pci/hotplug/pciehp.h
> ===================================================================
> --- linux-pm.orig/drivers/pci/hotplug/pciehp.h
> +++ linux-pm/drivers/pci/hotplug/pciehp.h
> @@ -167,21 +167,4 @@ static inline const char *slot_name(stru
>  	return hotplug_slot_name(slot->hotplug_slot);
>  }
>  
> -#ifdef CONFIG_ACPI
> -#include <linux/pci-acpi.h>
> -
> -void __init pciehp_acpi_slot_detection_init(void);
> -int pciehp_acpi_slot_detection_check(struct pci_dev *dev);
> -
> -static inline void pciehp_firmware_init(void)
> -{
> -	pciehp_acpi_slot_detection_init();
> -}
> -#else
> -#define pciehp_firmware_init()				do {} while (0)
> -static inline int pciehp_acpi_slot_detection_check(struct pci_dev *dev)
> -{
> -	return 0;
> -}
> -#endif				/* CONFIG_ACPI */
>  #endif				/* _PCIEHP_H */
> Index: linux-pm/drivers/pci/hotplug/Makefile
> ===================================================================
> --- linux-pm.orig/drivers/pci/hotplug/Makefile
> +++ linux-pm/drivers/pci/hotplug/Makefile
> @@ -61,9 +61,6 @@ pciehp-objs		:=	pciehp_core.o	\
>  				pciehp_ctrl.o	\
>  				pciehp_pci.o	\
>  				pciehp_hpc.o
> -ifdef CONFIG_ACPI
> -pciehp-objs		+=	pciehp_acpi.o
> -endif
>  
>  shpchp-objs		:=	shpchp_core.o	\
>  				shpchp_ctrl.o	\
> 

Re: [Update][PATCH] PCIe / hotplug: Drop pointless ACPI-based "slot detection" check

[Rafael J. Wysocki]

On Thursday, May 21, 2015 11:11:46 AM Bjorn Helgaas wrote:
> On Tue, May 19, 2015 at 03:27:58PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Jarod Wilson reports that the expresscard hotplug setup doesn't work
> > on HP ZBook G2.  The problem turns out to be the ACPI-based "slot
> > detection" code called from pciehp_probe() which tries to use some
> > questionable heuristics based on what ACPI objects are present for
> > the PCIe port device at hand to figure out whether or not to register
> > a hotplug slot for that port.
> > 
> > That code is used if there is at least one PCIe port having an ACPI
> > device configuration object related to hotplug (such as _EJ0 or _RMV)
> > and the Thunderbolt port on the affected machine has _RMV.  Of course,
> > Thunderbolt and PCIe native hotplug need not be mutually exclusive
> > (as they aren't on the machine in question), so that rule is simply
> > incorrect.
> > 
> > Moreover, the ACPI-based "slot detection" check does not add any
> > value if pciehp_probe() is called at all and the service type of the
> > device object it has been called for is PCIE_PORT_SERVICE_HP, because
> > PCIe hotplug services are only registered if the _OSC handshake in
> > acpi_pci_root_add() allows the kernel to control the PCIe native
> > hotplug feature.  No more checks need to be carried out to decide
> > whether or not to register a native PCIe hotlug slot in that case.
> > 
> > For the above reasons, make pciehp_probe() check if it has been
> > called for the right service type and drop the pointless ACPI-based
> > "slot detection" check from it.  Also remove the entire code whose
> > only user is that check (the entire pciehp_acpi.c file goes away
> > as a result) and drop function headers related to it from the
> > internal PCIeHP header file.
> > 
> > Link: http://marc.info/?t=143163219300002&r=1&w=2
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=98581
> > Reported-by: Jarod Wilson <jarod@redhat.com>
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> This is awesome!  Applied to pci/hotplug for v4.2, with Jarod's
> reviewed/tested-by.

Thanks!

> I suspect a lot of this stuff dates back to when acpiphp and pciehp could
> be modules, and one driver really couldn't know whether the other was up
> to.  In any event, I think it will be much more predictable and
> maintainable now.

Yes, this code has been outdated since we changed ACPIPHP to look at the
host bridge _OSC bits when deciding whether or not to register a hotplug
port.

Rafael

Re: [Update][PATCH] PCIe / hotplug: Drop pointless ACPI-based "slot detection" check

[Jarod Wilson]

On 5/21/2015 9:21 PM, Rafael J. Wysocki wrote:
> On Thursday, May 21, 2015 11:11:46 AM Bjorn Helgaas wrote:
>> On Tue, May 19, 2015 at 03:27:58PM +0200, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Jarod Wilson reports that the expresscard hotplug setup doesn't work
>>> on HP ZBook G2.  The problem turns out to be the ACPI-based "slot
>>> detection" code called from pciehp_probe() which tries to use some
>>> questionable heuristics based on what ACPI objects are present for
>>> the PCIe port device at hand to figure out whether or not to register
>>> a hotplug slot for that port.
>>>
>>> That code is used if there is at least one PCIe port having an ACPI
>>> device configuration object related to hotplug (such as _EJ0 or _RMV)
>>> and the Thunderbolt port on the affected machine has _RMV.  Of course,
>>> Thunderbolt and PCIe native hotplug need not be mutually exclusive
>>> (as they aren't on the machine in question), so that rule is simply
>>> incorrect.
>>>
>>> Moreover, the ACPI-based "slot detection" check does not add any
>>> value if pciehp_probe() is called at all and the service type of the
>>> device object it has been called for is PCIE_PORT_SERVICE_HP, because
>>> PCIe hotplug services are only registered if the _OSC handshake in
>>> acpi_pci_root_add() allows the kernel to control the PCIe native
>>> hotplug feature.  No more checks need to be carried out to decide
>>> whether or not to register a native PCIe hotlug slot in that case.
>>>
>>> For the above reasons, make pciehp_probe() check if it has been
>>> called for the right service type and drop the pointless ACPI-based
>>> "slot detection" check from it.  Also remove the entire code whose
>>> only user is that check (the entire pciehp_acpi.c file goes away
>>> as a result) and drop function headers related to it from the
>>> internal PCIeHP header file.
>>>
>>> Link: http://marc.info/?t=143163219300002&r=1&w=2
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=98581
>>> Reported-by: Jarod Wilson <jarod@redhat.com>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> This is awesome!  Applied to pci/hotplug for v4.2, with Jarod's
>> reviewed/tested-by.
>
> Thanks!

Looks like I didn't test enough. I can't explain WHY, but with this 
applied, now thunderbolt hot unplug of a network adapter goes haywire, 
where prior to the patch, it worked just fine. Still looking into it...

-- 
Jarod Wilson
jarod@redhat.com

Re: [Update][PATCH] PCIe / hotplug: Drop pointless ACPI-based "slot detection" check

[Jarod Wilson]

On 6/11/2015 1:05 PM, Jarod Wilson wrote:
> On 5/21/2015 9:21 PM, Rafael J. Wysocki wrote:
>> On Thursday, May 21, 2015 11:11:46 AM Bjorn Helgaas wrote:
>>> On Tue, May 19, 2015 at 03:27:58PM +0200, Rafael J. Wysocki wrote:
>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>
>>>> Jarod Wilson reports that the expresscard hotplug setup doesn't work
>>>> on HP ZBook G2.  The problem turns out to be the ACPI-based "slot
>>>> detection" code called from pciehp_probe() which tries to use some
>>>> questionable heuristics based on what ACPI objects are present for
>>>> the PCIe port device at hand to figure out whether or not to register
>>>> a hotplug slot for that port.
>>>>
>>>> That code is used if there is at least one PCIe port having an ACPI
>>>> device configuration object related to hotplug (such as _EJ0 or _RMV)
>>>> and the Thunderbolt port on the affected machine has _RMV.  Of course,
>>>> Thunderbolt and PCIe native hotplug need not be mutually exclusive
>>>> (as they aren't on the machine in question), so that rule is simply
>>>> incorrect.
>>>>
>>>> Moreover, the ACPI-based "slot detection" check does not add any
>>>> value if pciehp_probe() is called at all and the service type of the
>>>> device object it has been called for is PCIE_PORT_SERVICE_HP, because
>>>> PCIe hotplug services are only registered if the _OSC handshake in
>>>> acpi_pci_root_add() allows the kernel to control the PCIe native
>>>> hotplug feature.  No more checks need to be carried out to decide
>>>> whether or not to register a native PCIe hotlug slot in that case.
>>>>
>>>> For the above reasons, make pciehp_probe() check if it has been
>>>> called for the right service type and drop the pointless ACPI-based
>>>> "slot detection" check from it.  Also remove the entire code whose
>>>> only user is that check (the entire pciehp_acpi.c file goes away
>>>> as a result) and drop function headers related to it from the
>>>> internal PCIeHP header file.
>>>>
>>>> Link: http://marc.info/?t=143163219300002&r=1&w=2
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=98581
>>>> Reported-by: Jarod Wilson <jarod@redhat.com>
>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> This is awesome!  Applied to pci/hotplug for v4.2, with Jarod's
>>> reviewed/tested-by.
>>
>> Thanks!
>
> Looks like I didn't test enough. I can't explain WHY, but with this
> applied, now thunderbolt hot unplug of a network adapter goes haywire,
> where prior to the patch, it worked just fine. Still looking into it...

Filed bug, dmesg spew can be found in the bug.

https://bugzilla.kernel.org/show_bug.cgi?id=99841

-- 
Jarod Wilson
jarod@redhat.com

Re: [Update][PATCH] PCIe / hotplug: Drop pointless ACPI-based "slot detection" check

[Rafael J. Wysocki]

On Thursday, June 11, 2015 04:38:12 PM Jarod Wilson wrote:
> On 6/11/2015 1:05 PM, Jarod Wilson wrote:
> > On 5/21/2015 9:21 PM, Rafael J. Wysocki wrote:
> >> On Thursday, May 21, 2015 11:11:46 AM Bjorn Helgaas wrote:
> >>> On Tue, May 19, 2015 at 03:27:58PM +0200, Rafael J. Wysocki wrote:
> >>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>>
> >>>> Jarod Wilson reports that the expresscard hotplug setup doesn't work
> >>>> on HP ZBook G2.  The problem turns out to be the ACPI-based "slot
> >>>> detection" code called from pciehp_probe() which tries to use some
> >>>> questionable heuristics based on what ACPI objects are present for
> >>>> the PCIe port device at hand to figure out whether or not to register
> >>>> a hotplug slot for that port.
> >>>>
> >>>> That code is used if there is at least one PCIe port having an ACPI
> >>>> device configuration object related to hotplug (such as _EJ0 or _RMV)
> >>>> and the Thunderbolt port on the affected machine has _RMV.  Of course,
> >>>> Thunderbolt and PCIe native hotplug need not be mutually exclusive
> >>>> (as they aren't on the machine in question), so that rule is simply
> >>>> incorrect.
> >>>>
> >>>> Moreover, the ACPI-based "slot detection" check does not add any
> >>>> value if pciehp_probe() is called at all and the service type of the
> >>>> device object it has been called for is PCIE_PORT_SERVICE_HP, because
> >>>> PCIe hotplug services are only registered if the _OSC handshake in
> >>>> acpi_pci_root_add() allows the kernel to control the PCIe native
> >>>> hotplug feature.  No more checks need to be carried out to decide
> >>>> whether or not to register a native PCIe hotlug slot in that case.
> >>>>
> >>>> For the above reasons, make pciehp_probe() check if it has been
> >>>> called for the right service type and drop the pointless ACPI-based
> >>>> "slot detection" check from it.  Also remove the entire code whose
> >>>> only user is that check (the entire pciehp_acpi.c file goes away
> >>>> as a result) and drop function headers related to it from the
> >>>> internal PCIeHP header file.
> >>>>
> >>>> Link: http://marc.info/?t=143163219300002&r=1&w=2
> >>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=98581
> >>>> Reported-by: Jarod Wilson <jarod@redhat.com>
> >>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> This is awesome!  Applied to pci/hotplug for v4.2, with Jarod's
> >>> reviewed/tested-by.
> >>
> >> Thanks!
> >
> > Looks like I didn't test enough. I can't explain WHY, but with this
> > applied, now thunderbolt hot unplug of a network adapter goes haywire,
> > where prior to the patch, it worked just fine. Still looking into it...
> 
> Filed bug, dmesg spew can be found in the bug.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=99841

If it worked for you previously, can you possibly try to re-create that
configuration and set of patches applied and retest then?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [Update][PATCH] PCIe / hotplug: Drop pointless ACPI-based "slot detection" check

[Jarod Wilson]

On 6/11/2015 5:16 PM, Rafael J. Wysocki wrote:
> On Thursday, June 11, 2015 04:38:12 PM Jarod Wilson wrote:
>> On 6/11/2015 1:05 PM, Jarod Wilson wrote:
>>> On 5/21/2015 9:21 PM, Rafael J. Wysocki wrote:
>>>> On Thursday, May 21, 2015 11:11:46 AM Bjorn Helgaas wrote:
>>>>> On Tue, May 19, 2015 at 03:27:58PM +0200, Rafael J. Wysocki wrote:
>>>>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>>
>>>>>> Jarod Wilson reports that the expresscard hotplug setup doesn't work
>>>>>> on HP ZBook G2.  The problem turns out to be the ACPI-based "slot
>>>>>> detection" code called from pciehp_probe() which tries to use some
>>>>>> questionable heuristics based on what ACPI objects are present for
>>>>>> the PCIe port device at hand to figure out whether or not to register
>>>>>> a hotplug slot for that port.
>>>>>>
>>>>>> That code is used if there is at least one PCIe port having an ACPI
>>>>>> device configuration object related to hotplug (such as _EJ0 or _RMV)
>>>>>> and the Thunderbolt port on the affected machine has _RMV.  Of course,
>>>>>> Thunderbolt and PCIe native hotplug need not be mutually exclusive
>>>>>> (as they aren't on the machine in question), so that rule is simply
>>>>>> incorrect.
>>>>>>
>>>>>> Moreover, the ACPI-based "slot detection" check does not add any
>>>>>> value if pciehp_probe() is called at all and the service type of the
>>>>>> device object it has been called for is PCIE_PORT_SERVICE_HP, because
>>>>>> PCIe hotplug services are only registered if the _OSC handshake in
>>>>>> acpi_pci_root_add() allows the kernel to control the PCIe native
>>>>>> hotplug feature.  No more checks need to be carried out to decide
>>>>>> whether or not to register a native PCIe hotlug slot in that case.
>>>>>>
>>>>>> For the above reasons, make pciehp_probe() check if it has been
>>>>>> called for the right service type and drop the pointless ACPI-based
>>>>>> "slot detection" check from it.  Also remove the entire code whose
>>>>>> only user is that check (the entire pciehp_acpi.c file goes away
>>>>>> as a result) and drop function headers related to it from the
>>>>>> internal PCIeHP header file.
>>>>>>
>>>>>> Link: http://marc.info/?t=143163219300002&r=1&w=2
>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=98581
>>>>>> Reported-by: Jarod Wilson <jarod@redhat.com>
>>>>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>>>
>>>>> This is awesome!  Applied to pci/hotplug for v4.2, with Jarod's
>>>>> reviewed/tested-by.
>>>>
>>>> Thanks!
>>>
>>> Looks like I didn't test enough. I can't explain WHY, but with this
>>> applied, now thunderbolt hot unplug of a network adapter goes haywire,
>>> where prior to the patch, it worked just fine. Still looking into it...
>>
>> Filed bug, dmesg spew can be found in the bug.
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=99841
>
> If it worked for you previously, can you possibly try to re-create that
> configuration and set of patches applied and retest then?

I tried the current Fedora 4.1-rc7 build first, everything was fine, 
then patched in JUST the one patch, and it went belly up. I'll add some 
extra debugging spew to a build both with and without the one patch, to 
see what differences there are in devices pciehp is claiming and follow 
up in the bug on your other info requests.

-- 
Jarod Wilson
jarod@redhat.com