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

[Ian Campbell <ijc@hellion.org.uk>]

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