Fix two PEIe hotplug issues

Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks (and others?) in conjunction with the modparam of pciehp_force=1.

The PCIe Hotplug driver has two issues when used on Dell notebooks which lack ACPI BIOS support for PCIe hotplug:

1. The driver does not recognise cards that were inserted prior to the driver being modprobe'd.

2. The driver stops functioning after a suspend/resume (RAM) cycle, and needs to be rmmod'd and modprobe'd to get it working again.

This patch series addresses those issues, resulting in a completely functional PCIe Hotplug driver for Dell notebooks, and possibly others as well which may lack ACPI BIOS support for this.
Note that pciehp_force=1 is (still) required.

There are three patches in this series:

01_pciehp_handle_preinserted_card.patch
-- fixes problem number 1 (above).

02_pciehp_split_pcie_init.patch
-- preparation for the resume patch.

03_pciehp_resume_reinit_hardware.patch
-- fixes problem number 2 (above).

diffstat summary for the three patches combined:

drivers/pci/hotplug/pciehp.h      |    3
drivers/pci/hotplug/pciehp_core.c |   23 +++
drivers/pci/hotplug/pciehp_ctrl.c |    2
drivers/pci/hotplug/pciehp_hpc.c  |  185 +++++++++++++++-------------
4 files changed, 130 insertions(+), 83 deletions(-)

Cheers
--
Mark Lord <[email protected]>

01_pciehp_handle_preinserted_card.patch:

Fix pciehp_probe() to deal with ExpressCard cards that were inserted prior to the driver being loaded.

---

@@ -37,7 +37,6 @@

#include "pciehp.h"

static void interrupt_event_handler(struct work_struct *work); -static int pciehp_enable_slot(struct slot *p_slot); static int pciehp_disable_slot(struct slot *p_slot);

static int queue_interrupt_event(struct slot *p_slot, u32 event_type)

@@ -161,6 +161,7 @@

extern int pciehp_unconfigure_device(struct slot *p_slot); extern void pciehp_queue_pushbutton_work(struct work_struct *work); int pcie_init(struct controller *ctrl, struct pcie_device *dev); +int pciehp_enable_slot(struct slot *p_slot);

static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device) {

@@ -471,6 +471,11 @@

	t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);

	t_slot->hpc_ops->get_adapter_status(t_slot, &value); /* Check if slot is occupied */

+ if (value) {
+ rc = pciehp_enable_slot(t_slot);
+ if (rc) /* -ENODEV: shouldn't happen, but deal with it */
+ value = 0;
+ }

	if ((POWER_CTRL(ctrl->ctrlcap)) && !value) {
		rc = t_slot->hpc_ops->power_off_slot(t_slot); /* Power off slot if not occupied*/
		if (rc)

02_pciehp_split_pcie_init.patch:

Split out the hotplug hardware initialization code from pcie_init() into pcie_init_enable_events(), without changing any functionality.

---

@@ -1067,99 +1067,22 @@

}
#endif

-int pcie_init(struct controller * ctrl, struct pcie_device *dev) +int pcie_init_hardware(struct controller *ctrl, struct pcie_device *dev)

{
	int rc;
	u16 temp_word;
-	u16 cap_reg;
	u16 intr_enable = 0;
	u32 slot_cap;
-	int cap_base;
-	u16 slot_status, slot_ctrl;
+	u16 slot_status;
	struct pci_dev *pdev;

	pdev = dev->port;

- ctrl->pci_dev = pdev; /* save pci_dev in context */
-
- dbg("%s: hotplug controller vendor id 0x%x device id 0x%x\n",
- __FUNCTION__, pdev->vendor, pdev->device);
-
- if ((cap_base = pci_find_capability(pdev, PCI_CAP_ID_EXP)) == 0) {
- dbg("%s: Can't find PCI_CAP_ID_EXP (0x10)\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
-
- ctrl->cap_base = cap_base;
-
- dbg("%s: pcie_cap_base %x\n", __FUNCTION__, cap_base);
-
- rc = pciehp_readw(ctrl, CAPREG, &cap_reg);
- if (rc) {
- err("%s: Cannot read CAPREG register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: CAPREG offset %x cap_reg %x\n",
- __FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
-
- if (((cap_reg & SLOT_IMPL) == 0) ||
- (((cap_reg & DEV_PORT_TYPE) != 0x0040)
- && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
- dbg("%s : This is not a root port or the port is not "
- "connected to a slot\n", __FUNCTION__);
- goto abort_free_ctlr;
- }

	rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
	if (rc) {
		err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
		goto abort_free_ctlr;
	}

- dbg("%s: SLOTCAP offset %x slot_cap %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTCAP, slot_cap);
-
- if (!(slot_cap & HP_CAP)) {
- dbg("%s : This slot is not hot-plug capable\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- /* For debugging purpose */
- rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
- if (rc) {
- err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: SLOTSTATUS offset %x slot_status %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTSTATUS, slot_status);
-
- rc = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
- if (rc) {
- err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: SLOTCTRL offset %x slot_ctrl %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTCTRL, slot_ctrl);
-
- for (rc = 0; rc < DEVICE_COUNT_RESOURCE; rc++)
- if (pci_resource_len(pdev, rc) > 0)
- dbg("pci resource[%d] start=0x%llx(len=0x%llx)\n", rc,
- (unsigned long long)pci_resource_start(pdev, rc),
- (unsigned long long)pci_resource_len(pdev, rc));
-
- info("HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n",
- pdev->vendor, pdev->device,
- pdev->subsystem_vendor, pdev->subsystem_device);
-
- mutex_init(&ctrl->crit_sect);
- mutex_init(&ctrl->ctrl_lock);
- spin_lock_init(&ctrl->lock);
-
- /* setup wait queue */
- init_waitqueue_head(&ctrl->queue);
-
- /* return PCI Controller Info */
- ctrl->slot_device_offset = 0;
- ctrl->num_slots = 1;
- ctrl->first_slot = slot_cap >> 19;
- ctrl->ctrlcap = slot_cap & 0x0000007f;

	/* Mask Hot-plug Interrupt Enable */
	rc = pciehp_readw(ctrl, SLOTCTRL, &temp_word);
@@ -1280,8 +1203,6 @@
			goto abort_disable_intr;
	}

- ctrl->hpc_ops = &pciehp_hpc_ops;
-

	return 0;

	/* We end up here for the many possible ways to fail this API. */
@@ -1303,3 +1224,105 @@
abort_free_ctlr:
	return -1;
}

+
+int pcie_init(struct controller *ctrl, struct pcie_device *dev) +{
+ int rc;
+ u16 cap_reg;
+ u32 slot_cap;
+ int cap_base;
+ u16 slot_status, slot_ctrl;
+ struct pci_dev *pdev;
+
+ pdev = dev->port;
+ ctrl->pci_dev = pdev; /* save pci_dev in context */
+
+ dbg("%s: hotplug controller vendor id 0x%x device id 0x%x\n",
+ __FUNCTION__, pdev->vendor, pdev->device);
+
+ cap_base = pci_find_capability(pdev, PCI_CAP_ID_EXP);
+ if (cap_base == 0) {
+ dbg("%s: Can't find PCI_CAP_ID_EXP (0x10)\n", __FUNCTION__);
+ goto abort;
+ }
+
+ ctrl->cap_base = cap_base;
+
+ dbg("%s: pcie_cap_base %x\n", __FUNCTION__, cap_base);
+
+ rc = pciehp_readw(ctrl, CAPREG, &cap_reg);
+ if (rc) {
+ err("%s: Cannot read CAPREG register\n", __FUNCTION__);
+ goto abort;
+ }
+ dbg("%s: CAPREG offset %x cap_reg %x\n",
+ __FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
+
+ if (((cap_reg & SLOT_IMPL) == 0) ||
+ (((cap_reg & DEV_PORT_TYPE) != 0x0040)
+ && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
+ dbg("%s : This is not a root port or the port is not "
+ "connected to a slot\n", __FUNCTION__);
+ goto abort;
+ }
+
+ rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
+ if (rc) {
+ err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
+ goto abort;
+ }
+ dbg("%s: SLOTCAP offset %x slot_cap %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTCAP, slot_cap);
+
+ if (!(slot_cap & HP_CAP)) {
+ dbg("%s : This slot is not hot-plug capable\n", __FUNCTION__);
+ goto abort;
+ }
+ /* For debugging purpose */
+ rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
+ if (rc) {
+ err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
+ goto abort;
+ }
+ dbg("%s: SLOTSTATUS offset %x slot_status %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTSTATUS, slot_status);
+
+ rc = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
+ if (rc) {
+ err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
+ goto abort;
+ }
+ dbg("%s: SLOTCTRL offset %x slot_ctrl %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTCTRL, slot_ctrl);
+
+ for (rc = 0; rc < DEVICE_COUNT_RESOURCE; rc++)
+ if (pci_resource_len(pdev, rc) > 0)
+ dbg("pci resource[%d] start=0x%llx(len=0x%llx)\n", rc,
+ (unsigned long long)pci_resource_start(pdev, rc),
+ (unsigned long long)pci_resource_len(pdev, rc));
+
+ info("HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n",
+ pdev->vendor, pdev->device,
+ pdev->subsystem_vendor, pdev->subsystem_device);
+
+ mutex_init(&ctrl->crit_sect);
+ mutex_init(&ctrl->ctrl_lock);
+ spin_lock_init(&ctrl->lock);
+
+ /* setup wait queue */
+ init_waitqueue_head(&ctrl->queue);
+
+ /* return PCI Controller Info */
+ ctrl->slot_device_offset = 0;
+ ctrl->num_slots = 1;
+ ctrl->first_slot = slot_cap >> 19;
+ ctrl->ctrlcap = slot_cap & 0x0000007f;
+
+ rc = pcie_init_hardware(ctrl, dev);
+ if (rc == 0) {
+ ctrl->hpc_ops = &pciehp_hpc_ops;
+ return 0;
+ }
+abort:
+ return -1;
+}

03_pciehp_resume_reinit_hardware.patch:

Make use of the previously split out pcie_init_enable_events() function to reinitialize the hotplug hardware on resume from suspend, but only when pciehp_force==1. Otherwise behaviour is unmodified.

---

@@ -37,7 +37,6 @@

#include "pciehp.h"

static void interrupt_event_handler(struct work_struct *work); -static int pciehp_disable_slot(struct slot *p_slot);

static int queue_interrupt_event(struct slot *p_slot, u32 event_type) {

@@ -162,6 +162,8 @@

extern void pciehp_queue_pushbutton_work(struct work_struct *work); int pcie_init(struct controller *ctrl, struct pcie_device *dev); int pciehp_enable_slot(struct slot *p_slot);
+int pciehp_disable_slot(struct slot *p_slot);
+int pcie_init_hardware(struct controller *ctrl, struct pcie_device *dev);

static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device) {

@@ -510,6 +510,24 @@

static int pciehp_resume (struct pcie_device *dev)

{
	printk("%s ENTRY\n", __FUNCTION__);

+ if (pciehp_force) {
+ struct pci_dev *pdev = dev->port;
+ struct controller *ctrl = pci_get_drvdata(pdev);
+ struct slot *t_slot;
+ u8 status;
+
+ /* reinitialize the chipset's event detection logic */
+ pcie_init_hardware(ctrl, dev);
+
+ t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
+
+ /* Check if slot is occupied */
+ t_slot->hpc_ops->get_adapter_status(t_slot, &status);
+ if (status)
+ pciehp_enable_slot(t_slot);
+ else
+ pciehp_disable_slot(t_slot);
+ }

	return 0;
}

#endif

01_pciehp_handle_preinserted_card.patch:

Fix pciehp_probe() to deal with ExpressCard cards that were inserted prior to the driver being loaded.

---

@@ -37,7 +37,6 @@

#include "pciehp.h"

static void interrupt_event_handler(struct work_struct *work); -static int pciehp_enable_slot(struct slot *p_slot); static int pciehp_disable_slot(struct slot *p_slot);

static int queue_interrupt_event(struct slot *p_slot, u32 event_type)

@@ -161,6 +161,7 @@

extern int pciehp_unconfigure_device(struct slot *p_slot); extern void pciehp_queue_pushbutton_work(struct work_struct *work); int pcie_init(struct controller *ctrl, struct pcie_device *dev); +int pciehp_enable_slot(struct slot *p_slot);

static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device) {

@@ -471,6 +471,11 @@

	t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);

	t_slot->hpc_ops->get_adapter_status(t_slot, &value); /* Check if slot is occupied */

+ if (value) {
+ rc = pciehp_enable_slot(t_slot);
+ if (rc) /* -ENODEV: shouldn't happen, but deal with it */
+ value = 0;
+ }

	if ((POWER_CTRL(ctrl->ctrlcap)) && !value) {
		rc = t_slot->hpc_ops->power_off_slot(t_slot); /* Power off slot if not occupied*/
		if (rc)

02_pciehp_split_pcie_init.patch:

Split out the hotplug hardware initialization code from pcie_init() into pcie_init_enable_events(), without changing any functionality.

---

@@ -1067,99 +1067,22 @@

}
#endif

-int pcie_init(struct controller * ctrl, struct pcie_device *dev) +int pcie_init_hardware(struct controller *ctrl, struct pcie_device *dev)

{
	int rc;
	u16 temp_word;
-	u16 cap_reg;
	u16 intr_enable = 0;
	u32 slot_cap;
-	int cap_base;
-	u16 slot_status, slot_ctrl;
+	u16 slot_status;
	struct pci_dev *pdev;

	pdev = dev->port;

- ctrl->pci_dev = pdev; /* save pci_dev in context */
-
- dbg("%s: hotplug controller vendor id 0x%x device id 0x%x\n",
- __FUNCTION__, pdev->vendor, pdev->device);
-
- if ((cap_base = pci_find_capability(pdev, PCI_CAP_ID_EXP)) == 0) {
- dbg("%s: Can't find PCI_CAP_ID_EXP (0x10)\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
-
- ctrl->cap_base = cap_base;
-
- dbg("%s: pcie_cap_base %x\n", __FUNCTION__, cap_base);
-
- rc = pciehp_readw(ctrl, CAPREG, &cap_reg);
- if (rc) {
- err("%s: Cannot read CAPREG register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: CAPREG offset %x cap_reg %x\n",
- __FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
-
- if (((cap_reg & SLOT_IMPL) == 0) ||
- (((cap_reg & DEV_PORT_TYPE) != 0x0040)
- && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
- dbg("%s : This is not a root port or the port is not "
- "connected to a slot\n", __FUNCTION__);
- goto abort_free_ctlr;
- }

	rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
	if (rc) {
		err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
		goto abort_free_ctlr;
	}

- dbg("%s: SLOTCAP offset %x slot_cap %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTCAP, slot_cap);
-
- if (!(slot_cap & HP_CAP)) {
- dbg("%s : This slot is not hot-plug capable\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- /* For debugging purpose */
- rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
- if (rc) {
- err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: SLOTSTATUS offset %x slot_status %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTSTATUS, slot_status);
-
- rc = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
- if (rc) {
- err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: SLOTCTRL offset %x slot_ctrl %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTCTRL, slot_ctrl);
-
- for (rc = 0; rc < DEVICE_COUNT_RESOURCE; rc++)
- if (pci_resource_len(pdev, rc) > 0)
- dbg("pci resource[%d] start=0x%llx(len=0x%llx)\n", rc,
- (unsigned long long)pci_resource_start(pdev, rc),
- (unsigned long long)pci_resource_len(pdev, rc));
-
- info("HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n",
- pdev->vendor, pdev->device,
- pdev->subsystem_vendor, pdev->subsystem_device);
-
- mutex_init(&ctrl->crit_sect);
- mutex_init(&ctrl->ctrl_lock);
- spin_lock_init(&ctrl->lock);
-
- /* setup wait queue */
- init_waitqueue_head(&ctrl->queue);
-
- /* return PCI Controller Info */
- ctrl->slot_device_offset = 0;
- ctrl->num_slots = 1;
- ctrl->first_slot = slot_cap >> 19;
- ctrl->ctrlcap = slot_cap & 0x0000007f;

	/* Mask Hot-plug Interrupt Enable */
	rc = pciehp_readw(ctrl, SLOTCTRL, &temp_word);
@@ -1280,8 +1203,6 @@
			goto abort_disable_intr;
	}

- ctrl->hpc_ops = &pciehp_hpc_ops;
-

	return 0;

	/* We end up here for the many possible ways to fail this API. */
@@ -1303,3 +1224,105 @@
abort_free_ctlr:
	return -1;
}

+
+int pcie_init(struct controller *ctrl, struct pcie_device *dev) +{
+ int rc;
+ u16 cap_reg;
+ u32 slot_cap;
+ int cap_base;
+ u16 slot_status, slot_ctrl;
+ struct pci_dev *pdev;
+
+ pdev = dev->port;
+ ctrl->pci_dev = pdev; /* save pci_dev in context */
+
+ dbg("%s: hotplug controller vendor id 0x%x device id 0x%x\n",
+ __FUNCTION__, pdev->vendor, pdev->device);
+
+ cap_base = pci_find_capability(pdev, PCI_CAP_ID_EXP);
+ if (cap_base == 0) {
+ dbg("%s: Can't find PCI_CAP_ID_EXP (0x10)\n", __FUNCTION__);
+ goto abort;
+ }
+
+ ctrl->cap_base = cap_base;
+
+ dbg("%s: pcie_cap_base %x\n", __FUNCTION__, cap_base);
+
+ rc = pciehp_readw(ctrl, CAPREG, &cap_reg);
+ if (rc) {
+ err("%s: Cannot read CAPREG register\n", __FUNCTION__);
+ goto abort;
+ }
+ dbg("%s: CAPREG offset %x cap_reg %x\n",
+ __FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
+
+ if (((cap_reg & SLOT_IMPL) == 0) ||
+ (((cap_reg & DEV_PORT_TYPE) != 0x0040)
+ && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
+ dbg("%s : This is not a root port or the port is not "
+ "connected to a slot\n", __FUNCTION__);
+ goto abort;
+ }
+
+ rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
+ if (rc) {
+ err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
+ goto abort;
+ }
+ dbg("%s: SLOTCAP offset %x slot_cap %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTCAP, slot_cap);
+
+ if (!(slot_cap & HP_CAP)) {
+ dbg("%s : This slot is not hot-plug capable\n", __FUNCTION__);
+ goto abort;
+ }
+ /* For debugging purpose */
+ rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
+ if (rc) {
+ err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
+ goto abort;
+ }
+ dbg("%s: SLOTSTATUS offset %x slot_status %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTSTATUS, slot_status);
+
+ rc = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
+ if (rc) {
+ err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
+ goto abort;
+ }
+ dbg("%s: SLOTCTRL offset %x slot_ctrl %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTCTRL, slot_ctrl);
+
+ for (rc = 0; rc < DEVICE_COUNT_RESOURCE; rc++)
+ if (pci_resource_len(pdev, rc) > 0)
+ dbg("pci resource[%d] start=0x%llx(len=0x%llx)\n", rc,
+ (unsigned long long)pci_resource_start(pdev, rc),
+ (unsigned long long)pci_resource_len(pdev, rc));
+
+ info("HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n",
+ pdev->vendor, pdev->device,
+ pdev->subsystem_vendor, pdev->subsystem_device);
+
+ mutex_init(&ctrl->crit_sect);
+ mutex_init(&ctrl->ctrl_lock);
+ spin_lock_init(&ctrl->lock);
+
+ /* setup wait queue */
+ init_waitqueue_head(&ctrl->queue);
+
+ /* return PCI Controller Info */
+ ctrl->slot_device_offset = 0;
+ ctrl->num_slots = 1;
+ ctrl->first_slot = slot_cap >> 19;
+ ctrl->ctrlcap = slot_cap & 0x0000007f;
+
+ rc = pcie_init_hardware(ctrl, dev);
+ if (rc == 0) {
+ ctrl->hpc_ops = &pciehp_hpc_ops;
+ return 0;
+ }
+abort:
+ return -1;
+}

Kristen,

If you're happy with this set, please add your Acked-by (or whatever).

Thanks,

Mark

To make things simpler for distro people, I'm contemplating another patch in this series, to allow something like: pciehp_force=2

This would then *try* the ACPI BIOS calls, and only fall back to pcie_force=1 if the BIOS support fails. This would be an ideal default for most desktop/notebook distros to consider using.

Without this, they don't have any good choice: use the defaults, and things fail on the most popular brand of machines in the marketplace. Use pciehp_force=1, and they may break (?) on other brands.

So a hybrid of the two seems best. Pity it couldn't be the default behaviour, though. Or could it? We're early enough in the 2.6.24 cycle for it..

Opinions?

(repost to conform with akpm's subject line conventions)

One of three patches to fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks (and others?) in conjunction with modparam of pciehp_force=1.

Fix pciehp_probe() to deal with ExpressCard cards that were inserted prior to the driver being loaded.

---

@@ -37,7 +37,6 @@

#include "pciehp.h"

static void interrupt_event_handler(struct work_struct *work); -static int pciehp_enable_slot(struct slot *p_slot); static int pciehp_disable_slot(struct slot *p_slot);

static int queue_interrupt_event(struct slot *p_slot, u32 event_type)

@@ -161,6 +161,7 @@

extern int pciehp_unconfigure_device(struct slot *p_slot); extern void pciehp_queue_pushbutton_work(struct work_struct *work); int pcie_init(struct controller *ctrl, struct pcie_device *dev); +int pciehp_enable_slot(struct slot *p_slot);

static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device) {

@@ -471,6 +471,11 @@

	t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);

	t_slot->hpc_ops->get_adapter_status(t_slot, &value); /* Check if slot is occupied */

+ if (value) {
+ rc = pciehp_enable_slot(t_slot);
+ if (rc) /* -ENODEV: shouldn't happen, but deal with it */
+ value = 0;
+ }

	if ((POWER_CTRL(ctrl->ctrlcap)) && !value) {
		rc = t_slot->hpc_ops->power_off_slot(t_slot); /* Power off slot if not occupied*/
		if (rc)

(repost to conform with akpm's subject line conventions)

One of three patches to fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks (and others?) in conjunction with modparam of pciehp_force=1

Split out the hotplug hardware initialization code from pcie_init() into pcie_init_enable_events(), without changing any functionality.

---

@@ -1067,99 +1067,22 @@

}
#endif

-int pcie_init(struct controller * ctrl, struct pcie_device *dev) +int pcie_init_hardware(struct controller *ctrl, struct pcie_device *dev)

{
	int rc;
	u16 temp_word;
-	u16 cap_reg;
	u16 intr_enable = 0;
	u32 slot_cap;
-	int cap_base;
-	u16 slot_status, slot_ctrl;
+	u16 slot_status;
	struct pci_dev *pdev;

	pdev = dev->port;

- ctrl->pci_dev = pdev; /* save pci_dev in context */
-
- dbg("%s: hotplug controller vendor id 0x%x device id 0x%x\n",
- __FUNCTION__, pdev->vendor, pdev->device);
-
- if ((cap_base = pci_find_capability(pdev, PCI_CAP_ID_EXP)) == 0) {
- dbg("%s: Can't find PCI_CAP_ID_EXP (0x10)\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
-
- ctrl->cap_base = cap_base;
-
- dbg("%s: pcie_cap_base %x\n", __FUNCTION__, cap_base);
-
- rc = pciehp_readw(ctrl, CAPREG, &cap_reg);
- if (rc) {
- err("%s: Cannot read CAPREG register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: CAPREG offset %x cap_reg %x\n",
- __FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
-
- if (((cap_reg & SLOT_IMPL) == 0) ||
- (((cap_reg & DEV_PORT_TYPE) != 0x0040)
- && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
- dbg("%s : This is not a root port or the port is not "
- "connected to a slot\n", __FUNCTION__);
- goto abort_free_ctlr;
- }

	rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
	if (rc) {
		err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
		goto abort_free_ctlr;
	}

- dbg("%s: SLOTCAP offset %x slot_cap %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTCAP, slot_cap);
-
- if (!(slot_cap & HP_CAP)) {
- dbg("%s : This slot is not hot-plug capable\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- /* For debugging purpose */
- rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
- if (rc) {
- err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: SLOTSTATUS offset %x slot_status %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTSTATUS, slot_status);
-
- rc = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
- if (rc) {
- err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
- goto abort_free_ctlr;
- }
- dbg("%s: SLOTCTRL offset %x slot_ctrl %x\n",
- __FUNCTION__, ctrl->cap_base + SLOTCTRL, slot_ctrl);
-
- for (rc = 0; rc < DEVICE_COUNT_RESOURCE; rc++)
- if (pci_resource_len(pdev, rc) > 0)
- dbg("pci resource[%d] start=0x%llx(len=0x%llx)\n", rc,
- (unsigned long long)pci_resource_start(pdev, rc),
- (unsigned long long)pci_resource_len(pdev, rc));
-
- info("HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n",
- pdev->vendor, pdev->device,
- pdev->subsystem_vendor, pdev->subsystem_device);
-
- mutex_init(&ctrl->crit_sect);
- mutex_init(&ctrl->ctrl_lock);
- spin_lock_init(&ctrl->lock);
-
- /* setup wait queue */
- init_waitqueue_head(&ctrl->queue);
-
- /* return PCI Controller Info */
- ctrl->slot_device_offset = 0;
- ctrl->num_slots = 1;
- ctrl->first_slot = slot_cap >> 19;
- ctrl->ctrlcap = slot_cap & 0x0000007f;

	/* Mask Hot-plug Interrupt Enable */
	rc = pciehp_readw(ctrl, SLOTCTRL, &temp_word);
@@ -1280,8 +1203,6 @@
			goto abort_disable_intr;
	}

- ctrl->hpc_ops = &pciehp_hpc_ops;
-

	return 0;

	/* We end up here for the many possible ways to fail this API. */
@@ -1303,3 +1224,105 @@
abort_free_ctlr:
	return -1;
}

+
+int pcie_init(struct controller *ctrl, struct pcie_device *dev) +{
+ int rc;
+ u16 cap_reg;
+ u32 slot_cap;
+ int cap_base;
+ u16 slot_status, slot_ctrl;
+ struct pci_dev *pdev;
+
+ pdev = dev->port;
+ ctrl->pci_dev = pdev; /* save pci_dev in context */
+
+ dbg("%s: hotplug controller vendor id 0x%x device id 0x%x\n",
+ __FUNCTION__, pdev->vendor, pdev->device);
+
+ cap_base = pci_find_capability(pdev, PCI_CAP_ID_EXP);
+ if (cap_base == 0) {
+ dbg("%s: Can't find PCI_CAP_ID_EXP (0x10)\n", __FUNCTION__);
+ goto abort;
+ }
+
+ ctrl->cap_base = cap_base;
+
+ dbg("%s: pcie_cap_base %x\n", __FUNCTION__, cap_base);
+
+ rc = pciehp_readw(ctrl, CAPREG, &cap_reg);
+ if (rc) {
+ err("%s: Cannot read CAPREG register\n", __FUNCTION__);
+ goto abort;
+ }
+ dbg("%s: CAPREG offset %x cap_reg %x\n",
+ __FUNCTION__, ctrl->cap_base + CAPREG, cap_reg);
+
+ if (((cap_reg & SLOT_IMPL) == 0) ||
+ (((cap_reg & DEV_PORT_TYPE) != 0x0040)
+ && ((cap_reg & DEV_PORT_TYPE) != 0x0060))) {
+ dbg("%s : This is not a root port or the port is not "
+ "connected to a slot\n", __FUNCTION__);
+ goto abort;
+ }
+
+ rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
+ if (rc) {
+ err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
+ goto abort;
+ }
+ dbg("%s: SLOTCAP offset %x slot_cap %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTCAP, slot_cap);
+
+ if (!(slot_cap & HP_CAP)) {
+ dbg("%s : This slot is not hot-plug capable\n", __FUNCTION__);
+ goto abort;
+ }
+ /* For debugging purpose */
+ rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
+ if (rc) {
+ err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
+ goto abort;
+ }
+ dbg("%s: SLOTSTATUS offset %x slot_status %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTSTATUS, slot_status);
+
+ rc = pciehp_readw(ctrl, SLOTCTRL, &slot_ctrl);
+ if (rc) {
+ err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
+ goto abort;
+ }
+ dbg("%s: SLOTCTRL offset %x slot_ctrl %x\n",
+ __FUNCTION__, ctrl->cap_base + SLOTCTRL, slot_ctrl);
+
+ for (rc = 0; rc < DEVICE_COUNT_RESOURCE; rc++)
+ if (pci_resource_len(pdev, rc) > 0)
+ dbg("pci resource[%d] start=0x%llx(len=0x%llx)\n", rc,
+ (unsigned long long)pci_resource_start(pdev, rc),
+ (unsigned long long)pci_resource_len(pdev, rc));
+
+ info("HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n",
+ pdev->vendor, pdev->device,
+ pdev->subsystem_vendor, pdev->subsystem_device);
+
+ mutex_init(&ctrl->crit_sect);
+ mutex_init(&ctrl->ctrl_lock);
+ spin_lock_init(&ctrl->lock);
+
+ /* setup wait queue */
+ init_waitqueue_head(&ctrl->queue);
+
+ /* return PCI Controller Info */
+ ctrl->slot_device_offset = 0;
+ ctrl->num_slots = 1;
+ ctrl->first_slot = slot_cap >> 19;
+ ctrl->ctrlcap = slot_cap & 0x0000007f;
+
+ rc = pcie_init_hardware(ctrl, dev);
+ if (rc == 0) {
+ ctrl->hpc_ops = &pciehp_hpc_ops;
+ return 0;
+ }
+abort:
+ return -1;
+}

(repost to conform with akpm's subject line conventions)

Make use of the previously split out pcie_init_enable_events() function to reinitialize the hotplug hardware on resume from suspend, but only when pciehp_force==1. Otherwise behaviour is unmodified.

---

@@ -37,7 +37,6 @@

#include "pciehp.h"

static void interrupt_event_handler(struct work_struct *work); -static int pciehp_disable_slot(struct slot *p_slot);

static int queue_interrupt_event(struct slot *p_slot, u32 event_type) {

@@ -162,6 +162,8 @@

extern void pciehp_queue_pushbutton_work(struct work_struct *work); int pcie_init(struct controller *ctrl, struct pcie_device *dev); int pciehp_enable_slot(struct slot *p_slot);
+int pciehp_disable_slot(struct slot *p_slot);
+int pcie_init_hardware(struct controller *ctrl, struct pcie_device *dev);

static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device) {

@@ -510,6 +510,24 @@

static int pciehp_resume (struct pcie_device *dev)

{
	printk("%s ENTRY\n", __FUNCTION__);

+ if (pciehp_force) {
+ struct pci_dev *pdev = dev->port;
+ struct controller *ctrl = pci_get_drvdata(pdev);
+ struct slot *t_slot;
+ u8 status;
+
+ /* reinitialize the chipset's event detection logic */
+ pcie_init_hardware(ctrl, dev);
+
+ t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
+
+ /* Check if slot is occupied */
+ t_slot->hpc_ops->get_adapter_status(t_slot, &status);
+ if (status)
+ pciehp_enable_slot(t_slot);
+ else
+ pciehp_disable_slot(t_slot);
+ }

	return 0;
}

#endif

Hi Mark,

I still don't understand what the problems is very much. Could you give me answers against the following questions?

(1) Did you try "echo 1 > /sys/bus/pci/slots/XXX/power"? (XXX is the slot number to which your card had been inserted)
(2) If the answer against (1) is yes, did "echo 1 > ..." work?
(3) If the answer against (1) is no, could you try that?
(4) If the "echo 1 > ..." works, does it solve your problem?
(5) If the "echo 1 > ..." doesn't work, could you give me the output of "cat /sys/bus/pci/slots/XXX/*"?
(6) I think your slot is surprise removable. Is it correct?

Thanks,
Kenji Kaneshige

If a PCIe ExpressCard34 is inserted into the slot *before* I modprobe pciehp (with pciehp_force=1), then the card is not detected nor enabled. The patch provided here fixes that.

After 1. inserting the card, and then 2. modprobing pciehp, the value of /sys/bus/pci/slots/XXX/power is already "1".

But, yes, doing "echo 1" into it does trigger card detection and things work afterwards. This doesn't really help, because it doesn't happen automatically, unless the kernel patch is applied.

Yes, it's an externally accessible ExpressCard slot on a notebook, which is intended to be fully hotpluggable by surprise or otherwise.

Cheers

NAK! Absolutely no way will I take a patch that does this. I've been actually having philosophical issues with
even having pciehp_force as a module parameter at all. As I said before, using pciehp_force violates the PCI spec. Vendors often deliberately do not support OSC due to hardware errata. Users who use this option risk breaking their laptop. I was thinking actually that we should make it more difficult to use this option, like make it a compile time option, or perhaps rename it to this_could_seriously_break_your_hardware_dont_use.

Probably we shouldn't have the functionality in the driver at all, but I don't really want to take it out all together, because it's handy for those of us who work on early hardware, where we can ask the BIOS people why they didn't implement OSC, and then consciously force it knowing exactly what we are doing and the potential consequences. But, maybe we should consider the possibility that we should remove the code so as to really really prevent distros from using this option.

No big deal. I personally don't have a distro to maintain, so no pain for me here.

No, it just provides a way to use the hardware when the vendor didn't include BIOS functionality for it.

These notebooks fully support hotplug in the external ExpressCard slot (which is the *whole point* of an external slot), and according to Dell they work just fine with that other OS. I don't use the "other OS" here, but the hardware also works just fine with Linux now.

I'm guessing they just left out the BIOS functionality because it was one of the very first machines to market with such slots,
and the BIOS wasn't mature enough. So they rely upon more easily
updated software drivers instead. The only gotcha is they do specify that booting from the slot is not possible (because no BIOS support).

Cheers

No, it actually does violate the spec. Feel free to read it yourself. We are not supposed to do Native PCIe without first successfully executing OSC. Period. If you do, you are violating the spec.

that's because the other OS probably uses ACPI based hotplug. As anyone will tell you the arguement "well, it works fine on X" is bogus, because often vendors will put workarounds in the drivers or in the case of ACPI the firmware.

the point is that we don't know why they left it out. I know specifically of hardware where they left it out on PURPOSE because they don't want people to use it. Maybe your hardware is behaving fine, maybe it will break on you eventually after you use it, maybe not, but I've already got specific examples of broken hardware, so we will not do this.

To be fair, we have violated the spec before in order to get crappy hardware or to work around crappy ACPI implementations. acpi_sleep=s3_bios for example violates the spec, but yet it is the only way to get certain laptops to suspend/resume correctly.

The question to ask is whether violating the spec will lead to (a) a potential system hang/crash, (b) data corruption, (c) physical harm to the device/laptop.

Regards,

- Ted

And the answer is most definitely yes to at least a) and c) on specific hardware I know about.

I'm hearing a lot of FUD here, with no details.
Please, tell us more..

Thanks.

If we religiously relied upon BIOS support for everything in Linux, then SATA drives wouldn't be doing anything more than emulating PATA, and we'd still be doing PIO transfers for most PATA drives.

And ditto for many other areas in the kernel,
but that's the main one that I know a bit about.

So far, after a couple of days of intensive (disk!) I/O over the slot, the answers are NO, NO, and NO.

Cheers

According to pciehp_debug output from you, your slot doesn't have software programmable power controller. So your hardware automatically power on the slot. I think this is why the value of "power" file is already "1". (But I'm not sure if it is correct, especially because your firmware doesn't have _OSC.)

On the other hand, my slot has software programmable power controller. So the value of "power" file is "0" in this case.

Ok. I think your patch is trying to solve the following two problems. Right?

- When the card is inserted *after* modprobing pciehp, the card is automatically enabled (pciehp tries to automatically enable slot when the card is inserted if the slot is surprise remove capable). But if the card is inserted *before* modprobing pciehp, the card is not automatically enabled even after modprobing pciehp.

- When the card is inserted *before* modprobing pciehp, the card is automatically powered on by hardware, but it is not detected by OS. That is, "power" file tells the card is working, but it is not working actually. It is strange.

But those are not problems on the slot which is not surprise remove capable and has software programmable power controller because:

- When the card is inserted *after* modprobing pciehp, the card is *not* automatically powered on/detected. So it is very natural that the card, which had been inserted before modprobing pciehp, is not automatically enabled at the pciehp modprobe time.

- When the card is inserted *before* modprobing pciehp, the card is not automatically powered on by hardware if the slot has sortware programmable power controller. So there is no contradiction.

I think your patch has a bad effect to this kind of slots. So I think your patch needs additional checks to see if the slot should be enabled or not.

And you need to test your patch on PCIe Native hotplug capable hardware:)

Thanks,
Kenji Kaneshige

Not true. Once pciehp is loaded, it automatically detects and handles inserted and removed cards just fine. Until after the next suspend/resume.

Cheers

Mark Lord さんは書きました:

This is because your slot is surprise remove capable. On my slot, which is not surprise remove capable, the card is not enabled even after pciehp is loaded.

Thanks,
Kenji Kaneshige

pciehp_fix_double_init_bug.patch:

Earlier patches to split out the hardware init for PCIe hotplug resulted in some one-time initializations being redone on every resume cycle. Eg. irq/polling initialization.

This patch splits the hardware init into two parts, and separates the one-time initializations from those so that they only ever get done once, as intended.

---

This patch is for -mm and for Kristen's queue. Not for 2.6.24.

drivers/pci/hotplug/pciehp.h      |    2
drivers/pci/hotplug/pciehp_core.c |    2
drivers/pci/hotplug/pciehp_hpc.c  |  119 +++++++++++++++-------------
3 files changed, 69 insertions(+), 54 deletions(-)

@@ -163,7 +163,7 @@

int pcie_init(struct controller *ctrl, struct pcie_device *dev); int pciehp_enable_slot(struct slot *p_slot);
int pciehp_disable_slot(struct slot *p_slot);
-int pcie_init_hardware(struct controller *ctrl, struct pcie_device *dev); +int pcie_init_hardware_part2(struct controller *ctrl, struct pcie_device *dev);

static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device) {

@@ -521,7 +521,7 @@

		u8 status;

		/* reinitialize the chipset's event detection logic */

- pcie_init_hardware(ctrl, dev);
+ pcie_init_hardware_part2(ctrl, dev);

		t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);

@@ -1067,28 +1067,25 @@

}
#endif

-int pcie_init_hardware(struct controller *ctrl, struct pcie_device *dev) +static int pcie_init_hardware_part1(struct controller *ctrl,
+ struct pcie_device *dev)

{
	int rc;
	u16 temp_word;
-	u16 intr_enable = 0;
	u32 slot_cap;
	u16 slot_status;

- struct pci_dev *pdev;
-
- pdev = dev->port;

	rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
	if (rc) {
		err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
-		goto abort_free_ctlr;
+		return -1;
	}

	/* Mask Hot-plug Interrupt Enable */
	rc = pciehp_readw(ctrl, SLOTCTRL, &temp_word);
	if (rc) {
		err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
-		goto abort_free_ctlr;
+		return -1;
	}

	dbg("%s: SLOTCTRL %x value read %x\n",
@@ -1099,62 +1096,46 @@
	rc = pciehp_writew(ctrl, SLOTCTRL, temp_word);
	if (rc) {
		err("%s: Cannot write to SLOTCTRL register\n", __FUNCTION__);
-		goto abort_free_ctlr;
+		return -1;
	}

	rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
	if (rc) {
		err("%s: Cannot read SLOTSTATUS register\n", __FUNCTION__);
-		goto abort_free_ctlr;
+		return -1;
	}

	temp_word = 0x1F; /* Clear all events */
	rc = pciehp_writew(ctrl, SLOTSTATUS, temp_word);
	if (rc) {
		err("%s: Cannot write to SLOTSTATUS register\n", __FUNCTION__);
-		goto abort_free_ctlr;
+		return -1;
	}

+ return 0;
+}

- if (pciehp_poll_mode) {
- /* Install interrupt polling timer. Start with 10 sec delay */
- init_timer(&ctrl->poll_timer);
- start_int_poll_timer(ctrl, 10);
- } else {
- /* Installs the interrupt handler */
- rc = request_irq(ctrl->pci_dev->irq, pcie_isr, IRQF_SHARED,
- MY_NAME, (void *)ctrl);
- dbg("%s: request_irq %d for hpc%d (returns %d)\n",
- __FUNCTION__, ctrl->pci_dev->irq,
- atomic_read(&pciehp_num_controllers), rc);
- if (rc) {
- err("Can't get irq %d for the hotplug controller\n",
- ctrl->pci_dev->irq);
- goto abort_free_ctlr;
- }
- }
- dbg("pciehp ctrl b:d:f:irq=0x%x:%x:%x:%x\n", pdev->bus->number,
- PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), dev->irq);
-
- /*
- * If this is the first controller to be initialized,
- * initialize the pciehp work queue
- */
- if (atomic_add_return(1, &pciehp_num_controllers) == 1) {
- pciehp_wq = create_singlethread_workqueue("pciehpd");
- if (!pciehp_wq) {
- rc = -ENOMEM;
- goto abort_free_irq;
- }
- }
+int pcie_init_hardware_part2(struct controller *ctrl, struct pcie_device *dev) +{
+ int rc;
+ u16 temp_word;
+ u16 intr_enable = 0;
+ u32 slot_cap;
+ u16 slot_status;

	rc = pciehp_readw(ctrl, SLOTCTRL, &temp_word);
	if (rc) {
		err("%s: Cannot read SLOTCTRL register\n", __FUNCTION__);
-		goto abort_free_irq;
+		goto abort;
	}

	intr_enable = intr_enable | PRSN_DETECT_ENABLE;

+ rc = pciehp_readl(ctrl, SLOTCAP, &slot_cap);
+ if (rc) {
+ err("%s: Cannot read SLOTCAP register\n", __FUNCTION__);
+ goto abort;
+ }
+

	if (ATTN_BUTTN(slot_cap))
		intr_enable = intr_enable | ATTN_BUTTN_ENABLE;

@@ -1179,7 +1160,7 @@

	rc = pciehp_writew(ctrl, SLOTCTRL, temp_word);
	if (rc) {
		err("%s: Cannot write to SLOTCTRL register\n", __FUNCTION__);
-		goto abort_free_irq;
+		goto abort;
	}
	rc = pciehp_readw(ctrl, SLOTSTATUS, &slot_status);
	if (rc) {
@@ -1214,14 +1195,7 @@
	}
	if (rc)
		err("%s : disabling interrupts failed\n", __FUNCTION__);

-
-abort_free_irq:
- if (pciehp_poll_mode)
- del_timer_sync(&ctrl->poll_timer);
- else
- free_irq(ctrl->pci_dev->irq, ctrl);
-
-abort_free_ctlr:
+abort:

	return -1;
}

@@ -1318,11 +1292,52 @@

	ctrl->first_slot = slot_cap >> 19;
	ctrl->ctrlcap = slot_cap & 0x0000007f;

- rc = pcie_init_hardware(ctrl, dev);
+ rc = pcie_init_hardware_part1(ctrl, dev);
+ if (rc)
+ goto abort;
+
+ if (pciehp_poll_mode) {
+ /* Install interrupt polling timer. Start with 10 sec delay */
+ init_timer(&ctrl->poll_timer);
+ start_int_poll_timer(ctrl, 10);
+ } else {
+ /* Installs the interrupt handler */
+ rc = request_irq(ctrl->pci_dev->irq, pcie_isr, IRQF_SHARED,
+ MY_NAME, (void *)ctrl);
+ dbg("%s: request_irq %d for hpc%d (returns %d)\n",
+ __FUNCTION__, ctrl->pci_dev->irq,
+ atomic_read(&pciehp_num_controllers), rc);
+ if (rc) {
+ err("Can't get irq %d for the hotplug controller\n",
+ ctrl->pci_dev->irq);
+ goto abort;
+ }
+ }
+ dbg("pciehp ctrl b:d:f:irq=0x%x:%x:%x:%x\n", pdev->bus->number,
+ PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn), dev->irq);
+
+ /*
+ * If this is the first controller to be initialized,
+ * initialize the pciehp work queue
+ */
+ if (atomic_add_return(1, &pciehp_num_controllers) == 1) {
+ pciehp_wq = create_singlethread_workqueue("pciehpd");
+ if (!pciehp_wq) {
+ rc = -ENOMEM;
+ goto abort_free_irq;
+ }
+ }
+

+	rc = pcie_init_hardware_part2(ctrl, dev);
	if (rc == 0) {
		ctrl->hpc_ops = &pciehp_hpc_ops;
		return 0;
	}

+abort_free_irq:
+ if (pciehp_poll_mode)
+ del_timer_sync(&ctrl->poll_timer);
+ else
+ free_irq(ctrl->pci_dev->irq, ctrl);
abort:

	return -1;
}

Which kernel is it against?

"Premature optimization is the root of all evil." - Donald Knuth

Rafael J. Wysocki wrote:
..

..

..

Patch was generated against 2.6.24-rc2-git4.

..

That is, against 2.6.24-rc2-git4 with the earlier PCIe hotplug patches also already applied, as they are in -mm and in Kristen's tree.