Re: PCI PM: Restore standard config registers of all devices early

[Linus Torvalds]

On Mon, 2 Feb 2009, Rafael J. Wysocki wrote:
> > 
> > 	b) disable_device_irq()'s: things are live, but device interrupts 
> > 	   are turned off by essentially looping over the irq_desc_ptr[] 
> > 	   table. 
> 
> Well, do we actually need to turn off all device interrupts?
> 
> Shared interrupts are the source of the problem, so perhaps we can
> only disable interrupts of devices that use interrupt pins at this point
> (MSI/MSI-X need not be disabled, for example, and the timer interrupts most
> probably too)?

We could try that, yes.

> > 	d) disable CPU interrupts.
> 
> At what point do we disable the other CPUs?

I left it out, because I don't much care or think it matters. So take your 
pick. I'd suggest keeping the current setup, and literally just insert the 
new point between "device_power_off()" and "sysdev_suspend()", with _zero_ 
other changes.

> Well, it means reworking the entire suspend sequence (again) or we will
> break assumptions made by some existing drivers (interrupts off during
> suspend_late and resume_early).  And that affects all drivers, not only PCI.

No it doesn't.

No changes AT ALL to the suspend sequence. We do everything in the same 
order: look at my patch. The only difference is that instead of doing that 
"cli" we do the "for_each_irq(disable_irq)" instead (and do the 'cli' a 
bit later).

ZERO effect on drivers. The calling convention is 100% the same as far as 
the driver is concerned: ->suspend() is called with interrupts on and a 
fully working machine, and ->suspend_late() called with interrupts off.

The only difference is the _mechanism_ of turning interrupts off. NOTHING 
else.

> I first would like to understand what _exactly_ breaks on the iBook reported to
> have problems.

I bet it's code like the USB one:

	int usb_hcd_pci_resume(struct pci_dev *dev)
	{

	#ifdef CONFIG_PPC_PMAC
	        /* Reenable ASIC clocks for USB */
	        if (machine_is(powermac)) {
	                struct device_node *of_node;

	                of_node = pci_device_to_OF_node(dev);
	                if (of_node)
	                        pmac_call_feature(PMAC_FTR_USB_ENABLE,
	                                                of_node, 0, 1);
	        }
	#endif
	..
	        retval = pci_enable_device(dev);  

and now pci_enable_device() calls pci_raw_set_power_state(), which does:

	        if (dev->current_state == state) {
	                /* we're already there */
	                return 0;
		} else ..

which means that it doesn't actually _do_ anything, because it thinks that 
'current_state' was already PCI_D0. But if the device was totally turned 
off, that's wrong. 

(background: pci_restore_standard_config() will have done 
pci_raw_set_power_state(PCI_D0) with the device clocks off, which wouldn't 
actualyl have _done_ anythign to the device, but then it does

	dev->current_state = PCI_D0;

Maybe the simplest thing to do migth be to replace that with a

	pci_update_current_state(dev, PCI_D0);

instead, to try to read back the state explicitly in 
pci_restore_standard_config()).

Best test:

Ben, does this trivial patch make any difference for those powermacs?

		Linus
---
 drivers/pci/pci.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 17bd932..97e1c38 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1419,7 +1419,7 @@ int pci_restore_standard_config(struct pci_dev *dev)
 		}
 	}
 
-	dev->current_state = PCI_D0;
+	pci_update_current_state(dev, PCI_D0);
 
 	return 0;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/