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]>
---
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 | [email protected]
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 <[email protected]>
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 | [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]>
---
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 | [email protected]
Hi.
On Mon, Nov 3, 2008 at 2:26 PM, Matthew Garrett <[email protected]> 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 <[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?
Sorry if I wasn't clear, I'm not an English native speaker.
Regards,
Fabio
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 | [email protected]
On Wed, 29 Oct 2008 20:09:03 +0000
Matthew Garrett <[email protected]> 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...
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 | [email protected]
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 <[email protected]>
>
> ---
>
> 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;
>
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 | [email protected]
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.
>
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 | [email protected]
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
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 | [email protected]
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
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 | [email protected]
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 <[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.
>
> 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
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 | [email protected]