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

[ message continues ]

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

[ message continues ]

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 : ...
[ message continues ]

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 ...
[ message continues ]

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

[ message continues ]

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 : ...
[ message continues ]

Kristen,

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

Thanks,

Mark
-

[ message continues ]

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

[ message continues ]

On Wed, 17 Oct 2007 23:09:45 -0400

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.

-

[ message continues ]

No big deal.  I personally don't have a distro to maintain,

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
-

[ message continues ]

On Thu, 18 Oct 2007 13:06:21 -0400

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

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

[ message continues ]

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
-

[ message continues ]

On Thu, 18 Oct 2007 13:49:25 -0400

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

[ message continues ]

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

Thanks.
-

[ message continues ]

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,

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

Cheers
-

[ message continues ]

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

[ message continues ]

(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__);
 ...
[ message continues ]

(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 ...
[ message continues ]

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,


-

[ message continues ]

If a PCIe ExpressCard34 is inserted into the slot *before*
I modprobe pciehp (with pciehp_force=1), then the card is not


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

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

Cheers


-

[ message continues ]

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

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





-

[ message continues ]

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

Cheers
-

[ message continues ]

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


-

[ message continues ]

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 ...
[ message continues ]

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

[ message continues ]

..

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

[ message continues ]