Add option to passively listen for PCIE hotplug events

Various pieces of hardware (such as the Acer Aspire One and Asus EEE) use PCIE hotplug to change the state of devices in response to events such as the removal of SD cards or disabling the wireless radio. However, they do not provide firmware support for this. As a consequence pciehp will refuse to load and various things break.

The existing workaround has been to use the pciehp_force option. This is undesirable as there is little guarantee that manipulating the power file in the slot directory will actually result in anything happening, leading to potential user confusion and hardware damage. This patch adds a new option, pciehp_passive. In this configuration pciehp will listen for events and notify the PCI core appropriately. However, it will not provide any user controllable sysfs attributes and so the risk of confusion or damage is averted. Any system slots that do have firmware support will continue to provide full functionality.

Signed-off-by Matthew Garrett <[email protected]>

---

drivers/pci/hotplug/pciehp_core.c +18 -4

index 4b23bc3..b878432 100644
@@ -41,6 +41,7 @@ int pciehp_debug;
 int pciehp_poll_mode;
 int pciehp_poll_time;
 int pciehp_force;
+int pciehp_passive;
 struct workqueue_struct *pciehp_wq;
 
 #define DRIVER_VERSION	"0.4"

@@ -55,10 +56,12 @@ module_param(pciehp_debug, bool, 0644);
 module_param(pciehp_poll_mode, bool, 0644);
 module_param(pciehp_poll_time, int, 0644);
 module_param(pciehp_force, bool, 0644);
+module_param(pciehp_passive, bool, 0644);
 MODULE_PARM_DESC(pciehp_debug, "Debugging mode enabled or not");
 MODULE_PARM_DESC(pciehp_poll_mode, "Using polling mechanism for hot-plug events or not");
 MODULE_PARM_DESC(pciehp_poll_time, "Polling mechanism frequency, in seconds");
 MODULE_PARM_DESC(pciehp_force, "Force pciehp, even if _OSC and OSHP are missing");
+MODULE_PARM_DESC(pciehp_passive, "Listen for pciehp events, even if _OSC and OSHP are missing");
 
 #define PCIE_MODULE_NAME "pciehp"
 

@@ -85,6 +88,13 @@ static struct hotplug_slot_ops pciehp_hotplug_slot_ops = {
   	.get_cur_bus_speed =	get_cur_bus_speed,
 };
 
+static struct hotplug_slot_ops pciehp_passive_hotplug_slot_ops = {
+	.owner =		THIS_MODULE,
+	.get_adapter_status =	get_adapter_status,
+  	.get_max_bus_speed =	get_max_bus_speed,
+  	.get_cur_bus_speed =	get_cur_bus_speed,
+};
+
 /*
  * Check the status of the Electro Mechanical Interlock (EMI)
  */

@@ -212,7 +222,11 @@ static int init_slots(struct controller *ctrl)
 		hotplug_slot->info = info;
 		hotplug_slot->private = slot;
 		hotplug_slot->release = &release_slot;
-		hotplug_slot->ops = &pciehp_hotplug_slot_ops;
+		if (pciehp_passive &&
+		    pciehp_get_hp_hw_control_from_firmware(ctrl->pci_dev))
+			hotplug_slot->ops = &pciehp_passive_hotplug_slot_ops;
+		else
+			hotplug_slot->ops = &pciehp_hotplug_slot_ops;
 		slot->hotplug_slot = hotplug_slot;
 		snprintf(name, SLOT_NAME_SIZE, "%u", slot->number);
 

@@ -407,7 +421,7 @@ static int pciehp_probe(struct pcie_device *dev, const struct pcie_port_service_
 	u8 value;
 	struct pci_dev *pdev = dev->port;
 
-	if (pciehp_force)
+	if (pciehp_force || pciehp_passive)
 		dev_info(&dev->device,
 			 "Bypassing BIOS check for pciehp use on %s\n",
 			 pci_name(pdev));

@@ -435,7 +449,7 @@ static int pciehp_probe(struct pcie_device *dev, const struct pcie_port_service_
 	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 && pciehp_force) {
+	if (value && (pciehp_force || pciehp_passive)) {
 		rc = pciehp_enable_slot(t_slot);
 		if (rc)	/* -ENODEV: shouldn't happen, but deal with it */
 			value = 0;

@@ -474,7 +488,7 @@ static int pciehp_suspend (struct pcie_device *dev, pm_message_t state)
 static int pciehp_resume (struct pcie_device *dev)
 {
 	dev_info(&dev->device, "%s ENTRY\n", __func__);
-	if (pciehp_force) {
+	if (pciehp_force || pciehp_passive) {
 		struct controller *ctrl = get_service_data(dev);
 		struct slot *t_slot;
 		u8 status;

Signature · 6 lines

-- 
Matthew Garrett | [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Various pieces of hardware (such as the Acer Aspire One and Asus EEE) use PCIE hotplug to change the state of devices in response to events such as the removal of SD cards or disabling the wireless radio. However, they do not provide firmware support for this. As a consequence pciehp will refuse to load and various things break.

The existing workaround has been to use the pciehp_force option. This is undesirable as there is little guarantee that manipulating the power file in the slot directory will actually result in anything happening, leading to potential user confusion and hardware damage. This patch adds a new option, pciehp_passive. In this configuration pciehp will listen for events and notify the PCI core appropriately. However, it will not provide any user controllable sysfs attributes and so the risk of confusion or damage is averted. Any system slots that do have firmware support will continue to provide full functionality.

Signed-off-by Matthew Garrett <[email protected]>

On my system (a hp laptop) I have to use pciehp_force=1 to have the service driver hpdriver loaded and an IRQ (MSI-Edge) assigned.

Does this patch mean that in the future I'd have to use the new pciehp_passive=1 switch instead?

Various pieces of hardware (such as the Acer Aspire One and Asus EEE) use PCIE hotplug to change the state of devices in response to events such as the removal of SD cards or disabling the wireless radio. However, they do not provide firmware support for this. As a consequence pciehp will refuse to load and various things break.

The existing workaround has been to use the pciehp_force option. This is undesirable as there is little guarantee that manipulating the power file in the slot directory will actually result in anything happening, leading to potential user confusion and hardware damage. This patch adds a new option, pciehp_passive. In this configuration pciehp will listen for events and notify the PCI core appropriately. However, it will not provide any user controllable sysfs attributes and so the risk of confusion or damage is averted. Any system slots that do have firmware support will continue to provide full functionality.

Signed-off-by Matthew Garrett <[email protected]>

---

Includes a minor code change suggested by Matthew Wilcox to avoid calling pciehp_get_hp_hw_control_from_firmware() twice and documentation updates as suggested Grant Grundler.

Documentation/kernel-parameters.txt +4 -0

index 1bbcaa8..2503ee0 100644
@@ -1711,6 +1711,20 @@ and is between 256 and 4096 characters. It is defined in the file
 		force	Enable ASPM even on devices that claim not to support it.
 			WARNING: Forcing ASPM on may cause system lockups.
 
+	pciehp.pciehp_force= [PCIE] Forcibly enable PCIE hotplug support on a 
+			     slot even if the firmware provides no support for
+			     it.
+

Documentation/kernel-parameters.txt +0 -0

 	pcmv=		[HW,PCMCIA] BadgePAD 4
 
 	pd.		[PARIDE]

drivers/pci/hotplug/pciehp_core.c +1 -0

index 4b23bc3..2bb2f4b 100644
@@ -41,6 +41,7 @@ int pciehp_debug;
 int pciehp_poll_mode;
 int pciehp_poll_time;
 int pciehp_force;
+int pciehp_passive;
 struct workqueue_struct *pciehp_wq;
 
 #define DRIVER_VERSION	"0.4"

@@ -55,10 +56,12 @@ module_param(pciehp_debug, bool, 0644);
 module_param(pciehp_poll_mode, bool, 0644);
 module_param(pciehp_poll_time, int, 0644);

Is it too late to comment on these parameters?
Please don't make users repeat the "pciehp.pciehp" name. This can be changed easily in the source code by keeping the variable names as is but using module_param_named() instead of module_param(). One good example is drivers/video/uvesafb.c.

Various pieces of hardware (such as the Acer Aspire One and Asus EEE) use PCIE hotplug to change the state of devices in response to events such as the removal of SD cards or disabling the wireless radio. However, they do not provide firmware support for this. As a consequence pciehp will refuse to load and various things break.

The existing workaround has been to use the pciehp_force option. This is undesirable as there is little guarantee that manipulating the power file in the slot directory will actually result in anything happening, leading to potential user confusion and hardware damage. This patch adds a new option, pciehp_passive. In this configuration pciehp will listen for events and notify the PCI core appropriately. However, it will not provide any user controllable sysfs attributes and so the risk of confusion or damage is averted. Any system slots that do have firmware support will continue to provide full functionality.

---

drivers/pci/hotplug/pciehp_core.c +1 -0

index 4b23bc3..b878432 100644
@@ -41,6 +41,7 @@ int pciehp_debug;
 int pciehp_poll_mode;
 int pciehp_poll_time;
 int pciehp_force;
+int pciehp_passive;
 struct workqueue_struct *pciehp_wq;
 
 #define DRIVER_VERSION	"0.4"

drivers/pci/hotplug/pciehp_core.c +14 -3

 #define PCIE_MODULE_NAME "pciehp"
 

@@ -85,6 +88,13 @@ static struct hotplug_slot_ops pciehp_hotplug_slot_ops = {
   	.get_cur_bus_speed =	get_cur_bus_speed,
 };
 
+static struct hotplug_slot_ops pciehp_passive_hotplug_slot_ops = {
+	.owner =		THIS_MODULE,
+	.get_adapter_status =	get_adapter_status,
+  	.get_max_bus_speed =	get_max_bus_speed,
+  	.get_cur_bus_speed =	get_cur_bus_speed,
+};
+
 /*
  * Check the status of the Electro Mechanical Interlock (EMI)
  */

@@ -212,7 +222,11 @@ static int init_slots(struct controller *ctrl)
 		hotplug_slot->info = info;
 		hotplug_slot->private = slot;
 		hotplug_slot->release = &release_slot;
-		hotplug_slot->ops = &pciehp_hotplug_slot_ops;
+		if (pciehp_passive &&
+		    pciehp_get_hp_hw_control_from_firmware(ctrl->pci_dev))
+			hotplug_slot->ops = &pciehp_passive_hotplug_slot_ops;
+		else
+			hotplug_slot->ops = &pciehp_hotplug_slot_ops;
 		slot->hotplug_slot = hotplug_slot;
 		snprintf(name, SLOT_NAME_SIZE, "%u", slot->number);
 

@@ -407,7 +421,7 @@ static int pciehp_probe(struct pcie_device *dev, const struct pcie_port_service_
 	u8 value;
 	struct pci_dev *pdev = dev->port;
 
-	if (pciehp_force)
+	if (pciehp_force || pciehp_passive)
 		dev_info(&dev->device,
 			 "Bypassing BIOS check for pciehp use on %s\n",
 			 pci_name(pdev));

@@ -435,7 +449,7 @@ static int pciehp_probe(struct pcie_device *dev, const struct pcie_port_service_
 	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 && pciehp_force) {
+	if (value && (pciehp_force || pciehp_passive)) {
 		rc = pciehp_enable_slot(t_slot);
 		if (rc)	/* -ENODEV: shouldn't happen, but deal with it */
 			value = 0;

However, what worries me a bit is how our users are to understand whether they need to use this option, how to use it, etc. Does it require too much knowledge of PCIE internals to be very useful? Is there any way in which we can make it more user-friendly?

Various pieces of hardware (such as the Acer Aspire One and Asus EEE) use PCIE hotplug to change the state of devices in response to events such as the removal of SD cards or disabling the wireless radio. However, they do not provide firmware support for this. As a consequence pciehp will refuse to load and various things break.

The existing workaround has been to use the pciehp_force option. This is undesirable as there is little guarantee that manipulating the power file in the slot directory will actually result in anything happening, leading to potential user confusion and hardware damage. This patch adds a new option, pciehp_passive. In this configuration pciehp will listen for events and notify the PCI core appropriately. However, it will not provide any user controllable sysfs attributes and so the risk of confusion or damage is averted. Any system slots that do have firmware support will continue to provide full functionality.

Signed-off-by Matthew Garrett <[email protected]>

---

drivers/pci/hotplug/pciehp_core.c +18 -4

index 4b23bc3..b878432 100644
@@ -41,6 +41,7 @@ int pciehp_debug;
 int pciehp_poll_mode;
 int pciehp_poll_time;
 int pciehp_force;
+int pciehp_passive;
 struct workqueue_struct *pciehp_wq;
 
 #define DRIVER_VERSION	"0.4"

@@ -55,10 +56,12 @@ module_param(pciehp_debug, bool, 0644);
 module_param(pciehp_poll_mode, bool, 0644);
 module_param(pciehp_poll_time, int, 0644);
 module_param(pciehp_force, bool, 0644);
+module_param(pciehp_passive, bool, 0644);
 MODULE_PARM_DESC(pciehp_debug, "Debugging mode enabled or not");
 MODULE_PARM_DESC(pciehp_poll_mode, "Using polling mechanism for hot-plug events or not");
 MODULE_PARM_DESC(pciehp_poll_time, "Polling mechanism frequency, in seconds");
 MODULE_PARM_DESC(pciehp_force, "Force pciehp, even if _OSC and OSHP are missing");
+MODULE_PARM_DESC(pciehp_passive, "Listen for pciehp events, even if _OSC and OSHP are missing");
 
 #define PCIE_MODULE_NAME "pciehp"
 

@@ -85,6 +88,13 @@ static struct hotplug_slot_ops pciehp_hotplug_slot_ops = {
   	.get_cur_bus_speed =	get_cur_bus_speed,
 };
 
+static struct hotplug_slot_ops pciehp_passive_hotplug_slot_ops = {
+	.owner =		THIS_MODULE,
+	.get_adapter_status =	get_adapter_status,
+  	.get_max_bus_speed =	get_max_bus_speed,
+  	.get_cur_bus_speed =	get_cur_bus_speed,
+};
+
 /*
  * Check the status of the Electro Mechanical Interlock (EMI)
  */

@@ -212,7 +222,11 @@ static int init_slots(struct controller *ctrl)
 		hotplug_slot->info = info;
 		hotplug_slot->private = slot;
 		hotplug_slot->release = &release_slot;
-		hotplug_slot->ops = &pciehp_hotplug_slot_ops;
+		if (pciehp_passive &&
+		    pciehp_get_hp_hw_control_from_firmware(ctrl->pci_dev))
+			hotplug_slot->ops = &pciehp_passive_hotplug_slot_ops;
+		else
+			hotplug_slot->ops = &pciehp_hotplug_slot_ops;
 		slot->hotplug_slot = hotplug_slot;
 		snprintf(name, SLOT_NAME_SIZE, "%u", slot->number);
 

@@ -407,7 +421,7 @@ static int pciehp_probe(struct pcie_device *dev, const struct pcie_port_service_
 	u8 value;
 	struct pci_dev *pdev = dev->port;
 
-	if (pciehp_force)
+	if (pciehp_force || pciehp_passive)
 		dev_info(&dev->device,
 			 "Bypassing BIOS check for pciehp use on %s\n",
 			 pci_name(pdev));

@@ -435,7 +449,7 @@ static int pciehp_probe(struct pcie_device *dev, const struct pcie_port_service_
 	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 && pciehp_force) {
+	if (value && (pciehp_force || pciehp_passive)) {
 		rc = pciehp_enable_slot(t_slot);
 		if (rc)	/* -ENODEV: shouldn't happen, but deal with it */
 			value = 0;

@@ -474,7 +488,7 @@ static int pciehp_suspend (struct pcie_device *dev, pm_message_t state)
 static int pciehp_resume (struct pcie_device *dev)
 {
 	dev_info(&dev->device, "%s ENTRY\n", __func__);
-	if (pciehp_force) {
+	if (pciehp_force || pciehp_passive) {
 		struct controller *ctrl = get_service_data(dev);
 		struct slot *t_slot;
 		u8 status;

- Even with pciehp_passive option, pciehp driver controls hotplug related registers at the initialization time, enabling software notification mechanism for hotplug events, trying to turn power on the slot and so on. Is this your intended behaviour?

- Maybe I don't understand what "pciehp will listen for events..." in your patch description means. But if you expect the pciehp's interrupts for hotplug events, it would not work properly when hotplug control is not granted through _OSC or OSHP.

Quoted 7 lines

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

drivers/pci/hotplug/pciehp_core.c +1 -1

-	if (value && pciehp_force) {
+	if (value && (pciehp_force || pciehp_passive)) {

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

Quoted 4 lines · Kenji Kaneshige

- Even with pciehp_passive option, pciehp driver controls hotplug related registers at the initialization time, enabling software notification mechanism for hotplug events, trying to turn power on the slot and so on. Is this your intended behaviour?

Yes, although the most recent version of the patch doesn't do the forcible power on if no card is detected.

Quoted 4 lines

- Maybe I don't understand what "pciehp will listen for events..." in your patch description means. But if you expect the pciehp's interrupts for hotplug events, it would not work properly when hotplug control is not granted through _OSC or OSHP.

What do you mean by "not work properly"? The hardware we've tested with fires events even without an OSHP method being present. That's the case we're trying to deal with.

Quoted 7 lines

Quoted 7 lines

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

drivers/pci/hotplug/pciehp_core.c +1 -1

-	if (value && pciehp_force) {
+	if (value && (pciehp_force || pciehp_passive)) {

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

This code no longer runs in the pciehp_passive case. However, by the looks of it it still does in the resume case - that probably wants fixing.

Quoted 4 lines · Matthew Garrett

What do you mean by "not work properly"? The hardware we've tested with fires events even without an OSHP method being present. That's the case we're trying to deal with.

Because the explanation of PCI Express Native Hot Plug control bit in _OSC return value in PCI firmware spec says

"... If firmware allows the operating system control of this feature, then in the context of the _OSC method, it must ensure that all hot plug events are routed to device interrupts as described in the PCI Express Base Specification. ..."

My understanding is that it is not ensured that all hot plug events are routed to device interrupts, if hotplug control is not granted.

Quoted 11 lines · Kenji Kaneshige

Quoted 2 lines · Matthew Garrett

What do you mean by "not work properly"? The hardware we've tested with fires events even without an OSHP method being present. That's the case we're trying to deal with.

Because the explanation of PCI Express Native Hot Plug control bit in _OSC return value in PCI firmware spec says

"... If firmware allows the operating system control of this feature, then in the context of the _OSC method, it must ensure that all hot plug events are routed to device interrupts as described in the PCI Express Base Specification. ..."

Quoted 2 lines

My understanding is that it is not ensured that all hot plug events are routed to device interrupts, if hotplug control is not granted.

My understanding is that in the worst case the hardware/firmware won't give us any events - that's basically equivalent to not having the driver loaded at all. On hardware that gives us the events anyway, we get some extra functionality.

Thanks for the feedback!

Quoted 7 lines · Kenji Kaneshige

Quoted 7 lines

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

drivers/pci/hotplug/pciehp_core.c +1 -1

-	if (value && pciehp_force) {
+	if (value && (pciehp_force || pciehp_passive)) {

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

This code no longer runs in the pciehp_passive case. However, by the looks of it it still does in the resume case - that probably wants fixing.

Quoted 9 lines · Matthew Garrett

Quoted 7 lines · Kenji Kaneshige

Quoted 7 lines

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

drivers/pci/hotplug/pciehp_core.c +1 -1

-	if (value && pciehp_force) {
+	if (value && (pciehp_force || pciehp_passive)) {

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

This code no longer runs in the pciehp_passive case. However, by the looks of it it still does in the resume case - that probably wants fixing.

Thinking about this - you said that the problem occurs because pciehp_force=1 causes it to try to enable an already enabled slot, and then tries to power down the slot as a result? It sounds like this code should actually be checking whether the return value is ENODEV or EINVAL, and in the latter case not powering the slot down. That sounds like a separate bugfix that I'll send later on.

Quoted 1 line

Quoted 1 line

Quoted 1 line

			value = 0;

I can't understand why the 'value' is set to 0 when pciehp_enable_slot() returns error. The 'value' here is representing whether the slot is occupied or not. Even if pciehp_enable_slot() returns error, it doesn't mean slot is not occupied. So I think it is clearly wrong thing that changing 'value' to 0 from 1 here.

How about just ignore the return value from pciehp_enable_slot()? The code would be as follows.