Hi all,
if a device doesn't support power management (pm_cap == 0) but it is
acpi_pci_power_manageable() because there is a _PS0 method declared for
it and _EJ0 is also declared for the slot then nobody is going to set
current_state = PCI_D0 for this device. This is what I think it is
happening:
pci_enable_device
|
__pci_enable_device_flags
/* here we do not set current_state because !pm_cap */
|
do_pci_enable_device
|
pci_set_power_state
|
__pci_start_power_transition
|
pci_platform_power_transition
/* platform_pci_power_manageable() calls acpi_pci_power_manageable that
* returns true */
|
platform_pci_set_power_state
/* acpi_pci_set_power_state gets called and does nothing because the
* acpi device has _EJ0, see the comment "If the ACPI device has _EJ0,
* ignore the device" */
at this point if we refer to the commit message that introduced the
comment above (10b3dcae0f275e2546e55303d64ddbb58cec7599), it is up to
the hotplug driver to set the state to D0.
However AFAICT the pci hotplug driver never does, in fact
drivers/pci/hotplug/acpiphp_glue.c:register_slot sets the slot flags to
(SLOT_ENABLED | SLOT_POWEREDON) but it does not set the pci device
current state to PCI_D0.
So my proposed fix is also to set current_state = PCI_D0 in
register_slot.
Comments are very welcome.
Signed-off-by: Stefano Stabellini <[email protected]>
---
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index cb23aa2..e610cfe 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -212,6 +212,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
pdev = pci_get_slot(pbus, PCI_DEVFN(device, function));
if (pdev) {
+ pdev->current_state = PCI_D0;
slot->flags |= (SLOT_ENABLED | SLOT_POWEREDON);
pci_dev_put(pdev);
}
On Mon, 28 Feb 2011 16:20:11 +0000
Stefano Stabellini <[email protected]> wrote:
> Hi all,
> if a device doesn't support power management (pm_cap == 0) but it is
> acpi_pci_power_manageable() because there is a _PS0 method declared for
> it and _EJ0 is also declared for the slot then nobody is going to set
> current_state = PCI_D0 for this device. This is what I think it is
> happening:
>
>
> pci_enable_device
> |
> __pci_enable_device_flags
> /* here we do not set current_state because !pm_cap */
> |
> do_pci_enable_device
> |
> pci_set_power_state
> |
> __pci_start_power_transition
> |
> pci_platform_power_transition
> /* platform_pci_power_manageable() calls acpi_pci_power_manageable that
> * returns true */
> |
> platform_pci_set_power_state
> /* acpi_pci_set_power_state gets called and does nothing because the
> * acpi device has _EJ0, see the comment "If the ACPI device has _EJ0,
> * ignore the device" */
>
>
> at this point if we refer to the commit message that introduced the
> comment above (10b3dcae0f275e2546e55303d64ddbb58cec7599), it is up to
> the hotplug driver to set the state to D0.
> However AFAICT the pci hotplug driver never does, in fact
> drivers/pci/hotplug/acpiphp_glue.c:register_slot sets the slot flags to
> (SLOT_ENABLED | SLOT_POWEREDON) but it does not set the pci device
> current state to PCI_D0.
>
> So my proposed fix is also to set current_state = PCI_D0 in
> register_slot.
> Comments are very welcome.
>
>
> Signed-off-by: Stefano Stabellini <[email protected]>
Yeah, looks fine. ACPIPHP is happy for the attention. :)
--
Jesse Barnes, Intel Open Source Technology Center
On Fri, 4 Mar 2011, Jesse Barnes wrote:
> On Mon, 28 Feb 2011 16:20:11 +0000
> Stefano Stabellini <[email protected]> wrote:
>
> > Hi all,
> > if a device doesn't support power management (pm_cap == 0) but it is
> > acpi_pci_power_manageable() because there is a _PS0 method declared for
> > it and _EJ0 is also declared for the slot then nobody is going to set
> > current_state = PCI_D0 for this device. This is what I think it is
> > happening:
> >
> >
> > pci_enable_device
> > |
> > __pci_enable_device_flags
> > /* here we do not set current_state because !pm_cap */
> > |
> > do_pci_enable_device
> > |
> > pci_set_power_state
> > |
> > __pci_start_power_transition
> > |
> > pci_platform_power_transition
> > /* platform_pci_power_manageable() calls acpi_pci_power_manageable that
> > * returns true */
> > |
> > platform_pci_set_power_state
> > /* acpi_pci_set_power_state gets called and does nothing because the
> > * acpi device has _EJ0, see the comment "If the ACPI device has _EJ0,
> > * ignore the device" */
> >
> >
> > at this point if we refer to the commit message that introduced the
> > comment above (10b3dcae0f275e2546e55303d64ddbb58cec7599), it is up to
> > the hotplug driver to set the state to D0.
> > However AFAICT the pci hotplug driver never does, in fact
> > drivers/pci/hotplug/acpiphp_glue.c:register_slot sets the slot flags to
> > (SLOT_ENABLED | SLOT_POWEREDON) but it does not set the pci device
> > current state to PCI_D0.
> >
> > So my proposed fix is also to set current_state = PCI_D0 in
> > register_slot.
> > Comments are very welcome.
> >
> >
> > Signed-off-by: Stefano Stabellini <[email protected]>
>
> Yeah, looks fine. ACPIPHP is happy for the attention. :)
Great, thanks!
Would you be OK with taking this patch in your tree?
Or do you prefer me to send to it Linus myself?
On Fri, 2011-03-04 at 18:42 +0000, Jesse Barnes wrote:
> On Mon, 28 Feb 2011 16:20:11 +0000
> Stefano Stabellini <[email protected]> wrote:
>
> > Hi all,
> > if a device doesn't support power management (pm_cap == 0) but it is
> > acpi_pci_power_manageable() because there is a _PS0 method declared for
> > it and _EJ0 is also declared for the slot then nobody is going to set
> > current_state = PCI_D0 for this device. This is what I think it is
> > happening:
> >
> >
> > pci_enable_device
> > |
> > __pci_enable_device_flags
> > /* here we do not set current_state because !pm_cap */
> > |
> > do_pci_enable_device
> > |
> > pci_set_power_state
> > |
> > __pci_start_power_transition
> > |
> > pci_platform_power_transition
> > /* platform_pci_power_manageable() calls acpi_pci_power_manageable that
> > * returns true */
> > |
> > platform_pci_set_power_state
> > /* acpi_pci_set_power_state gets called and does nothing because the
> > * acpi device has _EJ0, see the comment "If the ACPI device has _EJ0,
> > * ignore the device" */
> >
> >
> > at this point if we refer to the commit message that introduced the
> > comment above (10b3dcae0f275e2546e55303d64ddbb58cec7599), it is up to
> > the hotplug driver to set the state to D0.
> > However AFAICT the pci hotplug driver never does, in fact
> > drivers/pci/hotplug/acpiphp_glue.c:register_slot sets the slot flags to
> > (SLOT_ENABLED | SLOT_POWEREDON) but it does not set the pci device
> > current state to PCI_D0.
> >
> > So my proposed fix is also to set current_state = PCI_D0 in
> > register_slot.
> > Comments are very welcome.
> >
> >
> > Signed-off-by: Stefano Stabellini <[email protected]>
>
> Yeah, looks fine. ACPIPHP is happy for the attention. :)
This helps for coldplug of devices which lack the PCI CFG space power
management capability but doesn't help with hotplugging such devices.
Is it fair to assume that any PCI device in a slot which has just been
powered on will be in D0?
e.g. Stefano's patch plus the following seem to fix the crashes I've
been having with 82599 VFs for both hot and cold plug.
Ian.
acpiphp: set each new function to state D0 after powering on a slot.
Signed-off-by: Ian Campbell <[email protected]>
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index e610cfe..b4befbd 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -819,9 +819,17 @@ static int __ref enable_device(struct acpiphp_slot *slot)
}
}
- list_for_each_entry(func, &slot->funcs, sibling)
+ list_for_each_entry(func, &slot->funcs, sibling) {
acpiphp_bus_add(func);
+ dev = pci_get_slot(bus, PCI_DEVFN(slot->device,
+ func->function));
+ if (!dev)
+ continue;
+ dev->current_state = PCI_D0;
+ pci_dev_put(dev);
+ }
+
pci_bus_assign_resources(bus);
acpiphp_sanitize_bus(bus);
acpiphp_set_hpp_values(bus);
On Tue, 2011-03-08 at 14:50 +0000, Ian Campbell wrote:
> On Fri, 2011-03-04 at 18:42 +0000, Jesse Barnes wrote:
> > On Mon, 28 Feb 2011 16:20:11 +0000
> > Stefano Stabellini <[email protected]> wrote:
> >
> > > Hi all,
> > > if a device doesn't support power management (pm_cap == 0) but it is
> > > acpi_pci_power_manageable() because there is a _PS0 method declared for
> > > it and _EJ0 is also declared for the slot then nobody is going to set
> > > current_state = PCI_D0 for this device. This is what I think it is
> > > happening:
> > >
> > >
> > > pci_enable_device
> > > |
> > > __pci_enable_device_flags
> > > /* here we do not set current_state because !pm_cap */
> > > |
> > > do_pci_enable_device
> > > |
> > > pci_set_power_state
> > > |
> > > __pci_start_power_transition
> > > |
> > > pci_platform_power_transition
> > > /* platform_pci_power_manageable() calls acpi_pci_power_manageable that
> > > * returns true */
> > > |
> > > platform_pci_set_power_state
> > > /* acpi_pci_set_power_state gets called and does nothing because the
> > > * acpi device has _EJ0, see the comment "If the ACPI device has _EJ0,
> > > * ignore the device" */
> > >
> > >
> > > at this point if we refer to the commit message that introduced the
> > > comment above (10b3dcae0f275e2546e55303d64ddbb58cec7599), it is up to
> > > the hotplug driver to set the state to D0.
> > > However AFAICT the pci hotplug driver never does, in fact
> > > drivers/pci/hotplug/acpiphp_glue.c:register_slot sets the slot flags to
> > > (SLOT_ENABLED | SLOT_POWEREDON) but it does not set the pci device
> > > current state to PCI_D0.
> > >
> > > So my proposed fix is also to set current_state = PCI_D0 in
> > > register_slot.
> > > Comments are very welcome.
> > >
> > >
> > > Signed-off-by: Stefano Stabellini <[email protected]>
> >
> > Yeah, looks fine. ACPIPHP is happy for the attention. :)
>
> This helps for coldplug of devices which lack the PCI CFG space power
> management capability but doesn't help with hotplugging such devices.
>
> Is it fair to assume that any PCI device in a slot which has just been
> powered on will be in D0?
>
> e.g. Stefano's patch plus the following seem to fix the crashes I've
> been having with 82599 VFs for both hot and cold plug.
>
> Ian.
>
> acpiphp: set each new function to state D0 after powering on a slot.
>
> Signed-off-by: Ian Campbell <[email protected]>
That version did it too soon and/or to devices which were already known.
I think this one is more like it:
8<------
>From 5cb5d576e7177762abe29cfba3f616c48e619145 Mon Sep 17 00:00:00 2001
From: Ian Campbell <[email protected]>
Date: Tue, 8 Mar 2011 16:16:34 +0000
Subject: [PATCH] acpiphp: assume device is in state D0 after powering on a slot.
Devices which do not support PCI configuration space based power
management may not otherwise be enabled.
Signed-off-by: Ian Campbell <[email protected]>
---
drivers/pci/hotplug/acpiphp_glue.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index e610cfe..a502fef 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -827,6 +827,13 @@ static int __ref enable_device(struct acpiphp_slot *slot)
acpiphp_set_hpp_values(bus);
acpiphp_set_acpi_region(slot);
pci_enable_bridges(bus);
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ /* Assume that newly added devices are powered on already. */
+ if (!dev->is_added)
+ dev->current_state = PCI_D0;
+ }
+
pci_bus_add_devices(bus);
list_for_each_entry(func, &slot->funcs, sibling) {
--
1.5.6.5
--
Ian Campbell
Current Noise: Behemoth - Daimonos
Unix is a Registered Bell of AT&T Trademark Laboratories.
-- Donn Seeley
Jesse,
do you have any comments on the following patch?
Also, are you OK with taking "set current_state to D0 in register_slot"
in your tree? Otherwise should I send a pull request to Linus with the
patch and your ack?
Cheers,
Stefano
On Tue, 8 Mar 2011, Ian Campbell wrote:
> From 5cb5d576e7177762abe29cfba3f616c48e619145 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <[email protected]>
> Date: Tue, 8 Mar 2011 16:16:34 +0000
> Subject: [PATCH] acpiphp: assume device is in state D0 after powering on a slot.
>
> Devices which do not support PCI configuration space based power
> management may not otherwise be enabled.
>
> Signed-off-by: Ian Campbell <[email protected]>
> ---
> drivers/pci/hotplug/acpiphp_glue.c | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index e610cfe..a502fef 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -827,6 +827,13 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> acpiphp_set_hpp_values(bus);
> acpiphp_set_acpi_region(slot);
> pci_enable_bridges(bus);
> +
> + list_for_each_entry(dev, &bus->devices, bus_list) {
> + /* Assume that newly added devices are powered on already. */
> + if (!dev->is_added)
> + dev->current_state = PCI_D0;
> + }
> +
> pci_bus_add_devices(bus);
>
> list_for_each_entry(func, &slot->funcs, sibling) {
> --
> 1.5.6.5
>
>
>
> --
> Ian Campbell
> Current Noise: Behemoth - Daimonos
>
> Unix is a Registered Bell of AT&T Trademark Laboratories.
> -- Donn Seeley
>
>
On Wed, 16 Mar 2011 12:34:10 +0000
Stefano Stabellini <[email protected]> wrote:
> Jesse,
> do you have any comments on the following patch?
> Also, are you OK with taking "set current_state to D0 in register_slot"
> in your tree? Otherwise should I send a pull request to Linus with the
> patch and your ack?
Yeah, I think the patch is ok, I can send it to Linus either as part of
my 2.6.39 pull req (though it's probably too late for that) or in my
next -fixes pull.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
On Wed, 16 Mar 2011, Jesse Barnes wrote:
> On Wed, 16 Mar 2011 12:34:10 +0000
> Stefano Stabellini <[email protected]> wrote:
>
> > Jesse,
> > do you have any comments on the following patch?
> > Also, are you OK with taking "set current_state to D0 in register_slot"
> > in your tree? Otherwise should I send a pull request to Linus with the
> > patch and your ack?
>
> Yeah, I think the patch is ok, I can send it to Linus either as part of
> my 2.6.39 pull req (though it's probably too late for that) or in my
> next -fixes pull.
Great, thanks!
Just to be clear there are two different patches, the first one is "set
current_state to D0 in register_slot":
https://lkml.org/lkml/2011/2/28/296
the second one by Ian Campbell is "acpiphp: assume device is in state D0
after powering on a slot.":
https://lkml.org/lkml/2011/3/8/212
Hi Jesse,
On Wed, 2011-03-16 at 16:13 +0000, Stefano Stabellini wrote:
> On Wed, 16 Mar 2011, Jesse Barnes wrote:
> > On Wed, 16 Mar 2011 12:34:10 +0000
> > Stefano Stabellini <[email protected]> wrote:
> >
> > > Jesse,
> > > do you have any comments on the following patch?
> > > Also, are you OK with taking "set current_state to D0 in register_slot"
> > > in your tree? Otherwise should I send a pull request to Linus with the
> > > patch and your ack?
> >
> > Yeah, I think the patch is ok, I can send it to Linus either as part of
> > my 2.6.39 pull req (though it's probably too late for that) or in my
> > next -fixes pull.
>
> Great, thanks!
>
> Just to be clear there are two different patches, the first one is "set
> current_state to D0 in register_slot":
>
> https://lkml.org/lkml/2011/2/28/296
>
> the second one by Ian Campbell is "acpiphp: assume device is in state D0
> after powering on a slot.":
>
> https://lkml.org/lkml/2011/3/8/212
Looks like the first of these is in your tree but not the second. Do you
want me to resend and/or rework it?
Ian.
--
Ian Campbell
Current Noise: Anathema - Dreaming Light
I never met a woman I couldn't drink pretty.
On Wed, 11 May 2011 14:33:38 +0100
Ian Campbell <[email protected]> wrote:
> Hi Jesse,
>
> On Wed, 2011-03-16 at 16:13 +0000, Stefano Stabellini wrote:
> > On Wed, 16 Mar 2011, Jesse Barnes wrote:
> > > On Wed, 16 Mar 2011 12:34:10 +0000
> > > Stefano Stabellini <[email protected]> wrote:
> > >
> > > > Jesse,
> > > > do you have any comments on the following patch?
> > > > Also, are you OK with taking "set current_state to D0 in register_slot"
> > > > in your tree? Otherwise should I send a pull request to Linus with the
> > > > patch and your ack?
> > >
> > > Yeah, I think the patch is ok, I can send it to Linus either as part of
> > > my 2.6.39 pull req (though it's probably too late for that) or in my
> > > next -fixes pull.
> >
> > Great, thanks!
> >
> > Just to be clear there are two different patches, the first one is "set
> > current_state to D0 in register_slot":
> >
> > https://lkml.org/lkml/2011/2/28/296
> >
> > the second one by Ian Campbell is "acpiphp: assume device is in state D0
> > after powering on a slot.":
> >
> > https://lkml.org/lkml/2011/3/8/212
>
> Looks like the first of these is in your tree but not the second. Do you
> want me to resend and/or rework it?
Yeah, please re-send it, I confused the two and thought Stefano's was
sufficient.
--
Jesse Barnes, Intel Open Source Technology Center
Devices which do not support PCI configuration space based power
management may not otherwise be enabled.
Signed-off-by: Ian Campbell <[email protected]>
---
drivers/pci/hotplug/acpiphp_glue.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 2f67e9b..a70fa89 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -827,6 +827,13 @@ static int __ref enable_device(struct acpiphp_slot *slot)
acpiphp_set_hpp_values(bus);
acpiphp_set_acpi_region(slot);
pci_enable_bridges(bus);
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ /* Assume that newly added devices are powered on already. */
+ if (!dev->is_added)
+ dev->current_state = PCI_D0;
+ }
+
pci_bus_add_devices(bus);
list_for_each_entry(func, &slot->funcs, sibling) {
--
1.7.2.5
On Wed, 11 May 2011 17:00:32 +0100
Ian Campbell <[email protected]> wrote:
> Devices which do not support PCI configuration space based power
> management may not otherwise be enabled.
>
> Signed-off-by: Ian Campbell <[email protected]>
> ---
> drivers/pci/hotplug/acpiphp_glue.c | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 2f67e9b..a70fa89 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -827,6 +827,13 @@ static int __ref enable_device(struct acpiphp_slot *slot)
> acpiphp_set_hpp_values(bus);
> acpiphp_set_acpi_region(slot);
> pci_enable_bridges(bus);
> +
> + list_for_each_entry(dev, &bus->devices, bus_list) {
> + /* Assume that newly added devices are powered on already. */
> + if (!dev->is_added)
> + dev->current_state = PCI_D0;
> + }
> +
> pci_bus_add_devices(bus);
>
> list_for_each_entry(func, &slot->funcs, sibling) {
applied to linux-next, thanks.
--
Jesse Barnes, Intel Open Source Technology Center
On Wed, 2011-05-11 at 09:14 -0700, Jesse Barnes wrote:
> On Wed, 11 May 2011 17:00:32 +0100
> Ian Campbell <[email protected]> wrote:
>
> > Devices which do not support PCI configuration space based power
> > management may not otherwise be enabled.
[...]
> applied to linux-next, thanks.
Thanks Jesse.
Ian.
--
Ian Campbell
Current Noise: Centurions Ghost - Hyena Circle
I've been there.