[PATCH] pciback: restore PCI BARs on D3->D0 transition

[PATCH] pciback: restore PCI BARs on D3->D0 transition

[Alex Williamson]

   Ever since xen-unstable cset 14308 ("pci back: Fix registration of of
filters on subsections of config space") I've been getting an MCA on the
*2nd* boot of a driver domain using an e1000 NIC.  Cset 14308 allowed
the proper setup of the PM control registers, so the NIC is put in the
D3 power state when the driver domain shuts down.  Unfortunately,
pci_set_power_state() won't restore the BARs on a D3hot/cold to D0
transition.  There are some comments in this upstream cset about why
that is:

http://www.kernel.org/hg/linux-2.6/rev/8f33e29c89a7

However, we obviously can't expect PCI devices to work without their
BARs properly programmed.  The patch below adds a call to
pci_restore_bars() as a workaround until we can figure out why the PCI
code doesn't do this for us.  Thanks,

	Alex

Signed-off-by: Alex Williamson <alex.williamson@hp.com>
---

diff -r 56caf0e37e6a linux-2.6-xen-sparse/drivers/xen/pciback/conf_space_capability_pm.c
--- a/linux-2.6-xen-sparse/drivers/xen/pciback/conf_space_capability_pm.c	Mon Mar 26 10:10:31 2007 -0600
+++ b/linux-2.6-xen-sparse/drivers/xen/pciback/conf_space_capability_pm.c	Mon Mar 26 13:13:23 2007 -0600
@@ -33,7 +33,7 @@ static int pm_ctrl_write(struct pci_dev 
 {
 	int err;
 	u16 cur_value;
-	pci_power_t new_state;
+	pci_power_t new_state, old_state;
 
 	/* Handle setting power state separately */
 	new_state = (pci_power_t)(new_value & PCI_PM_CTRL_STATE_MASK);
@@ -52,9 +52,20 @@ static int pm_ctrl_write(struct pci_dev 
 
 	/* Let pci core handle the power management change */
 	dev_dbg(&dev->dev, "set power state to %x\n", new_state);
+	old_state = dev->current_state;
 	err = pci_set_power_state(dev, new_state);
 	if (err)
 		err = PCIBIOS_SET_FAILED;
+
+	/*
+	 * Linux does not currently restore the BARs on a transition from
+	 * D3hot/cold to D0.  Fixup here.
+	 */
+	if (new_state == PCI_D0 &&
+	    (old_state == PCI_D3hot || old_state == PCI_D3cold)) {
+		if (!(cur_value & PCI_PM_CTRL_NO_SOFT_RESET))
+			pci_restore_bars(dev);
+	}
 
       out:
 	return err;

Re: [PATCH] pciback: restore PCI BARs on D3->D0 transition

[Alex Williamson]

On Mon, 2007-03-26 at 13:29 -0600, Alex Williamson wrote:
> However, we obviously can't expect PCI devices to work without their
> BARs properly programmed.  The patch below adds a call to
> pci_restore_bars() as a workaround until we can figure out why the PCI
> code doesn't do this for us.  Thanks,

   From what I can gather, it seems when a driver induces a transition
to D3, its expected to use pci_save_state() and pci_restore_state().  I
don't think we necessarily need to go to that extent for pciback, but it
does appear to be pciback's responsibility to restore some state on the
card when transitioning out of D3, since it is effectively the driver
for the device.  So, I think the pci_restore_bars() call and the patch I
sent previously are correct.  Thanks,

	Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: [PATCH] pciback: restore PCI BARs on D3->D0 transition

[Keir Fraser]

On 26/3/07 21:23, "Alex Williamson" <alex.williamson@hp.com> wrote:

> don't think we necessarily need to go to that extent for pciback, but it
> does appear to be pciback's responsibility to restore some state on the
> card when transitioning out of D3, since it is effectively the driver
> for the device.

Well, really it's supposed to be a filter (and perhaps translator) for PCI
config space accesses. It's not really a driver. But obviously something
like this is needed. The driver maintainer (I think!) is Chris Bookholt
(cc'ed).

 -- Keir

Re: [PATCH] pciback: restore PCI BARs on D3->D0 transition

[Chris]

Keir Fraser wrote:
> On 26/3/07 21:23, "Alex Williamson" <alex.williamson@hp.com> wrote:
> 
>> don't think we necessarily need to go to that extent for pciback, but it
>> does appear to be pciback's responsibility to restore some state on the
>> card when transitioning out of D3, since it is effectively the driver
>> for the device.
> 
> Well, really it's supposed to be a filter (and perhaps translator) for PCI
> config space accesses. It's not really a driver. But obviously something
> like this is needed.

Sorry for the delay, I've been out of the office for a while.

Indeed pciback was intended to be more of a filter than a driver.  For
the time being it seems reasonable to implement BAR-related PM functions
in pciback as you've done (many thanks for that).  As time permits I'll
double-check that your solution makes the fix in the most appropriate
location.

Cheers,
Chris