[RFC PATCH] set current_state to D0 in register_slot

[RFC PATCH] set current_state to D0 in register_slot

[Stefano Stabellini]

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 <stefano.stabellini@eu.citrix.com>

---

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

Re: [RFC PATCH] set current_state to D0 in register_slot

[Jesse Barnes]

On Mon, 28 Feb 2011 16:20:11 +0000
Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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 <stefano.stabellini@eu.citrix.com>

Yeah, looks fine.  ACPIPHP is happy for the attention. :)

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [RFC PATCH] set current_state to D0 in register_slot

[Stefano Stabellini]

On Fri, 4 Mar 2011, Jesse Barnes wrote:
> On Mon, 28 Feb 2011 16:20:11 +0000
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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 <stefano.stabellini@eu.citrix.com>
> 
> 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?

Re: [Xen-devel] Re: [RFC PATCH] set current_state to D0 in register_slot

[Ian Campbell]

On Fri, 2011-03-04 at 18:42 +0000, Jesse Barnes wrote:
> On Mon, 28 Feb 2011 16:20:11 +0000
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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 <stefano.stabellini@eu.citrix.com>
> 
> 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 <ian.campbell@citrix.com>

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

Re: [Xen-devel] Re: [RFC PATCH] set current_state to D0 in register_slot

[Ian Campbell]

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 <stefano.stabellini@eu.citrix.com> 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 <stefano.stabellini@eu.citrix.com>
> > 
> > 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 <ian.campbell@citrix.com>

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 <ian.campbell@citrix.com>
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 <ian.campbell@citrix.com>
---
 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

Re: [Xen-devel] Re: [RFC PATCH] set current_state to D0 in register_slot

[Stefano Stabellini]

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 <ian.campbell@citrix.com>
> 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 <ian.campbell@citrix.com>
> ---
>  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
> 
> 

Re: [Xen-devel] Re: [RFC PATCH] set current_state to D0 in register_slot

[Jesse Barnes]

On Wed, 16 Mar 2011 12:34:10 +0000
Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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

Re: [Xen-devel] Re: [RFC PATCH] set current_state to D0 in register_slot

[Stefano Stabellini]

On Wed, 16 Mar 2011, Jesse Barnes wrote:
> On Wed, 16 Mar 2011 12:34:10 +0000
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> 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

Re: Re: [RFC PATCH] set current_state to D0 in register_slot

[Ian Campbell]

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 <stefano.stabellini@eu.citrix.com> 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.

Re: [Xen-devel] Re: [RFC PATCH] set current_state to D0 in register_slot

[Jesse Barnes]

On Wed, 11 May 2011 14:33:38 +0100
Ian Campbell <ijc@hellion.org.uk> 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 <stefano.stabellini@eu.citrix.com> 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

[PATCH] acpiphp: assume device is in state D0 after powering on a slot.

[Ian Campbell]

Devices which do not support PCI configuration space based power
management may not otherwise be enabled.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 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

Re: [PATCH] acpiphp: assume device is in state D0 after powering on a slot.

[Jesse Barnes]

On Wed, 11 May 2011 17:00:32 +0100
Ian Campbell <ian.campbell@citrix.com> wrote:

> Devices which do not support PCI configuration space based power
> management may not otherwise be enabled.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  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

Re: [PATCH] acpiphp: assume device is in state D0 after powering on a slot.

[Ian Campbell]

On Wed, 2011-05-11 at 09:14 -0700, Jesse Barnes wrote:
> On Wed, 11 May 2011 17:00:32 +0100
> Ian Campbell <ian.campbell@citrix.com> 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.