[PATCH 0/3] Fix two PEIe hotplug issues

[PATCH 0/3] Fix two PEIe hotplug issues

[Mark Lord]

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 <mlord@pobox.com>

Re: [PATCH 1/3] pciehp_handle_preinserted_card

[Mark Lord]

01_pciehp_handle_preinserted_card.patch:

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

Signed-off-by: Mark Lord <mlord@pobox.com>
---
--- git12/drivers/pci/hotplug/pciehp_ctrl.c	2007-10-17 22:30:19.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_ctrl.c	2007-10-17 22:29:59.000000000 -0400
@@ -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)
--- git12/drivers/pci/hotplug/pciehp.h	2007-10-17 22:30:19.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp.h	2007-10-17 22:29:59.000000000 -0400
@@ -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)
 {
--- git12/drivers/pci/hotplug/pciehp_core.c	2007-10-17 22:30:19.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c	2007-10-17 22:36:06.000000000 -0400
@@ -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)

Re: [PATCH 2/3] pciehp_split_pcie_init

[Mark Lord]

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.

Signed-off-by: Mark Lord <mlord@pobox.com>
---
--- git12/drivers/pci/hotplug/pciehp_hpc.c	2007-10-17 19:06:38.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_hpc.c	2007-10-17 19:29:42.000000000 -0400
@@ -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;
+}

[PATCH 3/3] pciehp_resume_reinit_hardware

[Mark Lord]

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.

Signed-off-by: Mark Lord <mlord@pobox.com>
---
--- git12/drivers/pci/hotplug/pciehp_ctrl.c	2007-10-17 19:28:23.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_ctrl.c	2007-10-17 19:31:22.000000000 -0400
@@ -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)
 {
--- git12/drivers/pci/hotplug/pciehp.h	2007-10-17 19:28:23.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp.h	2007-10-17 19:32:03.000000000 -0400
@@ -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)
 {
--- git12/drivers/pci/hotplug/pciehp_core.c	2007-10-17 19:28:23.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c	2007-10-17 19:33:05.000000000 -0400
@@ -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

[PATCH 1/3] pciehp_handle_preinserted_card

[Mark Lord]

01_pciehp_handle_preinserted_card.patch:

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

Signed-off-by: Mark Lord <mlord@pobox.com>
---
--- git12/drivers/pci/hotplug/pciehp_ctrl.c	2007-10-17 22:30:19.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_ctrl.c	2007-10-17 22:29:59.000000000 -0400
@@ -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)
--- git12/drivers/pci/hotplug/pciehp.h	2007-10-17 22:30:19.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp.h	2007-10-17 22:29:59.000000000 -0400
@@ -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)
 {
--- git12/drivers/pci/hotplug/pciehp_core.c	2007-10-17 22:30:19.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c	2007-10-17 22:36:06.000000000 -0400
@@ -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)

[PATCH 2/3] pciehp_split_pcie_init

[Mark Lord]

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.

Signed-off-by: Mark Lord <mlord@pobox.com>
---
--- git12/drivers/pci/hotplug/pciehp_hpc.c	2007-10-17 19:06:38.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_hpc.c	2007-10-17 19:29:42.000000000 -0400
@@ -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;
+}

Re: [PATCH 0/3] Fix two PEIe hotplug issues

[Mark Lord]

Kristen,

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

Thanks,

Mark

Re: [PATCH 0/3] Fix two PEIe hotplug issues

[Mark Lord]

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


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?

Re: [PATCH 0/3] Fix two PEIe hotplug issues

[Kristen Carlson Accardi]

On Wed, 17 Oct 2007 23:09:45 -0400
Mark Lord <lkml@rtr.ca> wrote:

> Mark Lord wrote:
> > Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks
> > (and others?) in conjunction with the modparam of pciehp_force=1.
> 
> 
> 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?
> 

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.

Re: [PATCH 0/3] Fix two PEIe hotplug issues

[Mark Lord]

Kristen Carlson Accardi wrote:
> On Wed, 17 Oct 2007 23:09:45 -0400
> Mark Lord <lkml@rtr.ca> wrote:
> 
>> Mark Lord wrote:
>>> Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks
>>> (and others?) in conjunction with the modparam of pciehp_force=1.
>>
>> 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?
>>
> 
> NAK!  Absolutely no way will I take a patch that does this.  


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

> 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.

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

Re: [PATCH 0/3] Fix two PEIe hotplug issues

[Kristen Carlson Accardi]

On Thu, 18 Oct 2007 13:06:21 -0400
Mark Lord <lkml@rtr.ca> wrote:

> Kristen Carlson Accardi wrote:
> > On Wed, 17 Oct 2007 23:09:45 -0400
> > Mark Lord <lkml@rtr.ca> wrote:
> > 
> >> Mark Lord wrote:
> >>> Fix PCIe Hotplug so that it works with ExpressCard slots on Dell notebooks
> >>> (and others?) in conjunction with the modparam of pciehp_force=1.
> >>
> >> 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?
> >>
> > 
> > NAK!  Absolutely no way will I take a patch that does this.  
> 
> 
> No big deal.  I personally don't have a distro to maintain,
> so no pain for me here.
> 
> > 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.
> 
> No, it just provides a way to use the hardware when the vendor
> didn't include BIOS functionality for it.

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.

> 
> 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.

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.


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

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.

Re: [PATCH 0/3] Fix two PEIe hotplug issues

[Theodore Tso]

On Thu, Oct 18, 2007 at 10:06:14AM -0700, Kristen Carlson Accardi wrote:
> 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.

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

Re: [PATCH 0/3] Fix two PEIe hotplug issues

[Kristen Carlson Accardi]

On Thu, 18 Oct 2007 13:49:25 -0400
Theodore Tso <tytso@mit.edu> wrote:

> On Thu, Oct 18, 2007 at 10:06:14AM -0700, Kristen Carlson Accardi wrote:
> > 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.
> 
> 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.

Re: [PATCH 0/3] Fix two PEIe hotplug issues

[Mark Lord]

Kristen Carlson Accardi wrote:
> On Thu, 18 Oct 2007 13:49:25 -0400
> Theodore Tso <tytso@mit.edu> wrote:
> 
>> On Thu, Oct 18, 2007 at 10:06:14AM -0700, Kristen Carlson Accardi wrote:
>>> 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.
>> 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.
>
> 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.

Re: [PATCH 0/3] Fix two PEIe hotplug issues

[Mark Lord]

Theodore Tso wrote:
> On Thu, Oct 18, 2007 at 10:06:14AM -0700, Kristen Carlson Accardi wrote:
>> 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.
> 
> 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.  

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. 

> 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.

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

Cheers

[PATCH 1/3] pciehp: hotplug: deal with pre-inserted ExpressCards

[Mark Lord]

(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.

Signed-off-by: Mark Lord <mlord@pobox.com>
---
--- a/drivers/pci/hotplug/pciehp_ctrl.c	2007-10-17 22:30:19.000000000 -0400
+++ b/drivers/pci/hotplug/pciehp_ctrl.c	2007-10-17 22:29:59.000000000 -0400
@@ -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)
--- git12/drivers/pci/hotplug/pciehp.h	2007-10-17 22:30:19.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp.h	2007-10-17 22:29:59.000000000 -0400
@@ -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)
 {
--- git12/drivers/pci/hotplug/pciehp_core.c	2007-10-17 22:30:19.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c	2007-10-17 22:36:06.000000000 -0400
@@ -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)

[PATCH 2/3] pciehp: hotplug: split out hardware init from pcie_init()

[Mark Lord]

(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.

Signed-off-by: Mark Lord <mlord@pobox.com>
---
--- a/drivers/pci/hotplug/pciehp_hpc.c	2007-10-17 19:06:38.000000000 -0400
+++ b/drivers/pci/hotplug/pciehp_hpc.c	2007-10-17 19:29:42.000000000 -0400
@@ -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;
+}

[PATCH 3/3] pciehp: hotplug: reinit hotplug h/w on resume from suspend

[Mark Lord]

(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.

Signed-off-by: Mark Lord <mlord@pobox.com>
---
--- git12/drivers/pci/hotplug/pciehp_ctrl.c	2007-10-17 19:28:23.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_ctrl.c	2007-10-17 19:31:22.000000000 -0400
@@ -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)
 {
--- git12/drivers/pci/hotplug/pciehp.h	2007-10-17 19:28:23.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp.h	2007-10-17 19:32:03.000000000 -0400
@@ -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)
 {
--- git12/drivers/pci/hotplug/pciehp_core.c	2007-10-17 19:28:23.000000000 -0400
+++ linux/drivers/pci/hotplug/pciehp_core.c	2007-10-17 19:33:05.000000000 -0400
@@ -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

Re: [Pcihpd-discuss] [PATCH 1/3] pciehp: hotplug: deal with pre-inserted ExpressCards

[Kenji Kaneshige]

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


> (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.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> ---
> --- a/drivers/pci/hotplug/pciehp_ctrl.c	2007-10-17 22:30:19.000000000 -0400
> +++ b/drivers/pci/hotplug/pciehp_ctrl.c	2007-10-17 22:29:59.000000000 -0400
> @@ -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)
> --- git12/drivers/pci/hotplug/pciehp.h	2007-10-17 22:30:19.000000000 -0400
> +++ linux/drivers/pci/hotplug/pciehp.h	2007-10-17 22:29:59.000000000 -0400
> @@ -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)
>  {
> --- git12/drivers/pci/hotplug/pciehp_core.c	2007-10-17 22:30:19.000000000 -0400
> +++ linux/drivers/pci/hotplug/pciehp_core.c	2007-10-17 22:36:06.000000000 -0400
> @@ -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)
> 
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >> http://get.splunk.com/
> _______________________________________________
> Pcihpd-discuss mailing list
> Pcihpd-discuss@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/pcihpd-discuss
> 
> 

Re: [Pcihpd-discuss] [PATCH 1/3] pciehp: hotplug: deal with pre-inserted ExpressCards

[Mark Lord]

Kenji Kaneshige wrote:
> Hi Mark,
> 
> I still don't understand what the problems is very much.

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.

> 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?


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.

>  (6) I think your slot is surprise removable. Is it correct?

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

Cheers

Re: [Pcihpd-discuss] [PATCH 1/3] pciehp: hotplug: deal with pre-inserted ExpressCards

[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.
> 
>> 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?
> 
> 
> After 1. inserting the card, and then 2. modprobing pciehp,
> the value of /sys/bus/pci/slots/XXX/power is already "1".

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. 

> 
> 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.
> 
>>  (6) I think your slot is surprise removable. Is it correct?
> 
> Yes, it's an externally accessible ExpressCard slot on a notebook,
> which is intended to be fully hotpluggable by surprise or otherwise.

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

Re: [Pcihpd-discuss] [PATCH 1/3] pciehp: hotplug: deal with pre-inserted ExpressCards

[Mark Lord]

Kenji Kaneshige wrote:
>
>  - 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.


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

Cheers

Re: [Pcihpd-discuss] [PATCH 1/3] pciehp: hotplug: deal with pre-inserted ExpressCards

[Kenji Kaneshige]

Mark Lord さんは書きました:
> Kenji Kaneshige wrote:
>>
>>  - 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.
> 
> 
> Not true.  Once pciehp is loaded, it automatically detects and handles
> inserted and removed cards just fine.  Until after the next suspend/resume.
> 

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

[PATCH] Fix PCIe double initialization bug

[Mark Lord]

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.

Signed-off-by: Mark Lord <mlord@pobox.com>
---

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(-)

--- linux/drivers/pci/hotplug/pciehp.h.orig	2007-11-13 23:57:09.000000000 -0500
+++ linux/drivers/pci/hotplug/pciehp.h	2007-11-17 19:10:01.000000000 -0500
@@ -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)
 {
--- linux/drivers/pci/hotplug/pciehp_core.c.orig	2007-11-13 23:57:09.000000000 -0500
+++ linux/drivers/pci/hotplug/pciehp_core.c	2007-11-17 19:09:43.000000000 -0500
@@ -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);
 
--- linux/drivers/pci/hotplug/pciehp_hpc.c.orig	2007-11-13 23:57:09.000000000 -0500
+++ linux/drivers/pci/hotplug/pciehp_hpc.c	2007-11-17 19:13:49.000000000 -0500
@@ -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;
 }

Re: [PATCH] Fix PCIe double initialization bug

[Rafael J. Wysocki]

On Sunday, 18 of November 2007, Mark Lord wrote:
> 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.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>

Which kernel is it against?

> ---
> 
> 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(-)
> 
> --- linux/drivers/pci/hotplug/pciehp.h.orig	2007-11-13 23:57:09.000000000 -0500
> +++ linux/drivers/pci/hotplug/pciehp.h	2007-11-17 19:10:01.000000000 -0500
> @@ -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)
>  {
> --- linux/drivers/pci/hotplug/pciehp_core.c.orig	2007-11-13 23:57:09.000000000 -0500
> +++ linux/drivers/pci/hotplug/pciehp_core.c	2007-11-17 19:09:43.000000000 -0500
> @@ -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);
>  
> --- linux/drivers/pci/hotplug/pciehp_hpc.c.orig	2007-11-13 23:57:09.000000000 -0500
> +++ linux/drivers/pci/hotplug/pciehp_hpc.c	2007-11-17 19:13:49.000000000 -0500
> @@ -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;
>  }
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 



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

Re: [PATCH] Fix PCIe double initialization bug

[Mark Lord]

Rafael J. Wysocki wrote:
..
> Which kernel is it against?
..
>> This patch is for -mm and for Kristen's queue.  Not for 2.6.24.
..

Patch was generated against 2.6.24-rc2-git4.

Re: [PATCH] Fix PCIe double initialization bug

[Mark Lord]

Mark Lord wrote:
> Rafael J. Wysocki wrote:
> ..
>> Which kernel is it against?
> ..
>>> This patch is for -mm and for Kristen's queue.  Not for 2.6.24.
> ..
> 
> 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.