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

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Tuesday 03 February 2009, Linus Torvalds wrote:
> 
> 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.

So, perhaps we can just loop over the interrupt links and disable them all
at this point?

> > > 	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.

Ah, OK.

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

Sounds good.

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

Good idea anyway.

>  	return 0;
>  }

Or perhaps put

current_state = PCI_UNKNOWN;

under that 'if (machine_is(powermac))' ?

Thanks,
Rafael