[PATCH] Add option to passively listen for PCIE hotplug events

[PATCH] Add option to passively listen for PCIE hotplug events

[Matthew Garrett]

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 <mjg@redhat.com>

---

diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 4b23bc3..b878432 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -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;

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Add option to passively listen for PCIE hotplug events

[Grant Grundler]

On Wed, Oct 29, 2008 at 08:09:03PM +0000, Matthew Garrett wrote:
> 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 <mjg@redhat.com>

Can you add something to Documentation/ for this new option?
Either kernel-parameters.txt or something in PCI/ seems appropriate.

Just cutting/pasting the above would be good enough for me.

thanks,
grant

> 
> ---
> 
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 4b23bc3..b878432 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -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;
> 
> -- 
> Matthew Garrett | mjg59@srcf.ucam.org
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] Add option to passively listen for PCIE hotplug events

[Matthew Garrett]

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 <mjg@redhat.com>

---

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.

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 1bbcaa8..2503ee0 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -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.
+
+	pciehp.pciehp_passive= [PCIE] Forcibly enable listening for PCIE 
+			       hotplug events on a slot even if the firmware
+			       provides no support for it. Unlike pciehp.force,
+			       does not provide user interface for triggering
+			       hotplug events.
+
+	pciehp.pciehp_poll_mode= [PCIE] Poll for PCIE hotplug events
+
+	pciehp.pciehp_poll_time= [PCIE] Seconds to wait between polls
+
 	pcmv=		[HW,PCMCIA] BadgePAD 4
 
 	pd.		[PARIDE]
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index b2801a7..c9f18f9 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -224,6 +224,10 @@ static inline int pciehp_get_hp_hw_control_from_firmware(struct pci_dev *dev)
 {
 	u32 flags = (OSC_PCI_EXPRESS_NATIVE_HP_CONTROL |
 		     OSC_PCI_EXPRESS_CAP_STRUCTURE_CONTROL);
+	if (pciehp_force) {
+		dev_info(&dev->dev, "Bypassing BIOS check for pciehp\n");
+		return 0;
+	}
 	return acpi_get_hp_hw_control_from_firmware(dev, flags);
 }
 
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 4b23bc3..2bb2f4b 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -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,11 +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)
-		dev_info(&dev->device,
-			 "Bypassing BIOS check for pciehp use on %s\n",
-			 pci_name(pdev));
-	else if (pciehp_get_hp_hw_control_from_firmware(pdev))
+	if (!pciehp_passive && pciehp_get_hp_hw_control_from_firmware(pdev))
 		goto err_out_none;
 
 	ctrl = pcie_init(dev);
@@ -474,7 +484,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;

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v2] Add option to passively listen for PCIE hotplug events

[Fabio Comolli]

Hi.

On Mon, Nov 3, 2008 at 2:26 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> 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 <mjg@redhat.com>
>

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?

Sorry if I wasn't clear, I'm not an English native speaker.

Regards,
Fabio

Re: [PATCH v2] Add option to passively listen for PCIE hotplug events

[Matthew Garrett]

On Mon, Nov 03, 2008 at 02:43:14PM +0100, Fabio Comolli wrote:

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

You wouldn't /have/ to - force should still work. However, the passive 
option may be a better plan.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v2] Add option to passively listen for PCIE hotplug events

[Randy Dunlap]

On Mon, 3 Nov 2008 13:26:06 +0000 Matthew Garrett wrote:

> 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 <mjg@redhat.com>
> 
> ---
> 
> 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.
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 1bbcaa8..2503ee0 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -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.
> +
> +	pciehp.pciehp_passive= [PCIE] Forcibly enable listening for PCIE 
> +			       hotplug events on a slot even if the firmware
> +			       provides no support for it. Unlike pciehp.force,
> +			       does not provide user interface for triggering
> +			       hotplug events.
> +
> +	pciehp.pciehp_poll_mode= [PCIE] Poll for PCIE hotplug events
> +
> +	pciehp.pciehp_poll_time= [PCIE] Seconds to wait between polls
> +

Hi,

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.



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

> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 4b23bc3..2bb2f4b 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -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"
>  

---
~Randy

Re: [PATCH v2] Add option to passively listen for PCIE hotplug events

[Matthew Garrett]

On Fri, Nov 14, 2008 at 08:56:27AM -0800, Randy Dunlap wrote:

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

It maintains consistency with the existing parameter names, but after 
looking at the problem more closely I'm no longer planning to push this 
anyway.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Add option to passively listen for PCIE hotplug events

[Andrew Morton]

On Wed, 29 Oct 2008 20:09:03 +0000
Matthew Garrett <mjg59@srcf.ucam.org> wrote:

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

Gad.  I don't think I understood any of that.  But that's OK - it's par for
the course.

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?

> 
> ---
> 
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 4b23bc3..b878432 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -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");

I don't think that helped much ;)

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

Was this change so obvious that no code comments were needed? 
Perhaps...

Re: [PATCH] Add option to passively listen for PCIE hotplug events

[Matthew Garrett]

On Mon, Nov 03, 2008 at 02:23:46PM -0800, Andrew Morton wrote:

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

I'd lean towards this being the default behaviour in the long run, but 
right now there's not a great deal of understanding of what the failure 
cases may be. We're planning to give it a go in Fedora and see what the 
feedback from that is.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Add option to passively listen for PCIE hotplug events

[Kenji Kaneshige]

Matthew Garrett wrote:
> 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.
> 

I have several comments/questions below.

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

- Using pciehp_passive or pciehp_force option causes system reset
  on my system because of Master-Abort. I think it is an already
  existing problem with pciehp_force option. The cause of this
  problem is the code below.

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

  The pciehp_enable_slot() here returns error on my system because
  it detects the slot is already powered on state when it tries to
  turn power on the slot. As a result 'value' is set with 0. After
  that, pciehp turn power off the slot by the following code even
  though the adapter card on the slot is currently working, and it
  caused Master-Abort.

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

  I guess the reason why you didn't encounter this problem is that
  your laptop doesn't have power controller on hotplug controller.
  I'll try to make a patch to fix this problem soon.

Thanks,
Kenji Kaneshige




> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 
> ---
> 
> diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
> index 4b23bc3..b878432 100644
> --- a/drivers/pci/hotplug/pciehp_core.c
> +++ b/drivers/pci/hotplug/pciehp_core.c
> @@ -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;
> 

Re: [PATCH] Add option to passively listen for PCIE hotplug events

[Matthew Garrett]

On Tue, Nov 04, 2008 at 10:58:11AM +0900, Kenji Kaneshige wrote:

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

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

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

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.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Add option to passively listen for PCIE hotplug events

[Kenji Kaneshige]

Matthew Garrett wrote:
> On Tue, Nov 04, 2008 at 10:58:11AM +0900, Kenji Kaneshige wrote:
> 
>> - 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.
>

Oh, sorry.
I didn't noticed it was already changed in your v2 patch.
 
>> - 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.
>

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.

Thanks,
Kenji Kaneshige


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

Re: [PATCH] Add option to passively listen for PCIE hotplug events

[Matthew Garrett]

On Tue, Nov 04, 2008 at 11:29:43AM +0900, Kenji Kaneshige wrote:
> Matthew Garrett wrote:
> >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.

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!
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Add option to passively listen for PCIE hotplug events

[Kenji Kaneshige]

Matthew Garrett wrote:
> On Tue, Nov 04, 2008 at 11:29:43AM +0900, Kenji Kaneshige wrote:
>> Matthew Garrett wrote:
>>> 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.
> 
> 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!

Ok, I understood that my worries are already within the scope of the
assumption.

Thanks,
Kenji Kaneshige

Re: [PATCH] Add option to passively listen for PCIE hotplug events

[Matthew Garrett]

On Tue, Nov 04, 2008 at 02:07:00AM +0000, Matthew Garrett wrote:
> On Tue, Nov 04, 2008 at 10:58:11AM +0900, Kenji Kaneshige wrote:
> > > 	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;
> 
> 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.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Add option to passively listen for PCIE hotplug events

[Kenji Kaneshige]

Matthew Garrett wrote:
> On Tue, Nov 04, 2008 at 02:07:00AM +0000, Matthew Garrett wrote:
>> On Tue, Nov 04, 2008 at 10:58:11AM +0900, Kenji Kaneshige wrote:
>>>> 	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;
>> 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.
> 

I think the root cause of this problem is the following 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.

	t_slot = pciehp_find_slot(ctrl, ctrl->slot_device_offset);
	t_slot->hpc_ops->get_adapter_status(t_slot, &value);
	if (value) {
		if (pciehp_force)
			pciehp_enable_slot(t_slot);
	} else {
		/* Power off the slot if not occupied, just in case */
		if (POWER_CTRL(ctrl))
			if (t_slot->hpc_ops->power_off_slot(t_slot))
				goto err_out_free_ctrl_slots;
	}

Thanks,
Kenji Kaneshige

Re: [PATCH] Add option to passively listen for PCIE hotplug events

[Matthew Garrett]

On Tue, Nov 04, 2008 at 02:46:20PM +0900, Kenji Kaneshige wrote:

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

Yeah, I agree with that.

-- 
Matthew Garrett | mjg59@srcf.ucam.org