diff mbox

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

Message ID 1573807.HQlNd2BFYP@vostro.rjw.lan
State Accepted
Headers show

Commit Message

Rafael J. Wysocki May 19, 2015, 1:27 p.m. UTC 
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(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jarod Wilson May 19, 2015, 2:40 p.m. UTC | #1 
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>
Bjorn Helgaas May 21, 2015, 4:11 p.m. UTC | #2 
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	\
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" 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 May 22, 2015, 1:21 a.m. UTC | #3 
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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarod Wilson June 11, 2015, 5:05 p.m. UTC | #4 
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 June 11, 2015, 8:38 p.m. UTC | #5 
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
Rafael J. Wysocki June 11, 2015, 9:16 p.m. UTC | #6 
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?
Jarod Wilson June 11, 2015, 9:49 p.m. UTC | #7 
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.
diff mbox

Patch

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	\