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

This is a discussion on [PATCH] Add option to passively listen for PCIE hotplug events - Kernel ; 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 ...

+ Reply to Thread
Results 1 to 15 of 15

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

  1. [PATCH] 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

    ---

    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-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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


    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. [PATCH v2] 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

    ---

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    Hi.

    On Mon, Nov 3, 2008 at 2:26 PM, 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
    >


    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Wed, 29 Oct 2008 20:09:03 +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.
    >


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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

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



    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

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







    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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





    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread