On Thu, 2005-05-12 at 14:21 +0800, Adam Belay wrote:
> On Thu, May 12, 2005 at 09:24:47AM +0800, Shaohua Li wrote:
> > Hi,
> > There are still many PCI drivers don't implement
> their .suspend/.resume
> > methods correctly. I felt it would be quite helpful there is a
> reference
> > implementation. Here is my thought:
> >
> > .suspend()
> > {
> > driver specific operations;
> > pci_save_state();
> > pci_enable_wake();
> > /* as a result, PIC/IOAPIC pin is disabled */
> > free_irq();
>
> We can do this earlier, under driver specific operations, right?
Right, I just list some operations. Some operations are optional and
some order can be slightly adjusted.
>
> > /* as a result, bus master/irq router are disabled */
> > pci_disable_device();
> > pci_set_power_state();
> > }
> >
> > .resume()
> > {
> > pci_set_power_state(PCI_D0);
> > pci_restore_state();
> > /* device's irq possibly is changed, driver should take care
> */
>
> So what do you have in mind here? Could/should the device's resource
> configuration also possibly change.
Resources balance need it. For suspend/resume, it's unnecessary
(pci_restore_state will restore the resource configuration). But we must
clean irq to avoid unexpected interrupt issue. Currently, I'd like we
temporarily ignore resources balance.
> > pci_enable_device();
> > pci_set_master();
> > request_irq();
> > driver specific operations;
> > }
>
> MSI support may require something here too, I need to look into it.
>
> Also, we should disable the irq bit in the PCI configuration header,
> I'm not
> sure if pci_disable_device() does this now.
Disabling the INTX for MSI? pci_enable_msi isn't called at
pci_enable_device, so pci_disable_msi should be the same. Yes,
pci_enable_msi/pci_disable_msi should be added in the suspend/resume
methods.
>
> Finally, I'm not sure if pci_save_state() and pci_restore_state()
> should be
> part of the default procedure. I'd rather see drivers restoring the
> specific
> configuration areas they need.
I think pci_save_state and pci_store_state are mandatory and drivers can
do extra config save/restore.
> >
> > Currently many drivers don't call
> > free_irq/request_irq/pci_disable_device, which makes unexpected
> > interrupt occur and even break suspend/resume in some systems. The
> > reason is we disable PIC/IOAPIC/irq router very later (they are
> treated
> > as a sysdev), so there is a window between when a device is suspend
> and
> > when its PIC/IOAPIC/irq router pin are disabled. The ideal way is
> > PIC/IOAPIC/irq router's pins are disabled soon after no device is
> > referencing them. I'm now working on disabling irq router if no
> > reference on it, but first I'd like to know your opinions about the
> > proposal (the reference .suspend/.resume implementation).
> So if we find that over 99% of PCI device drivers need to do exactly
> this
> sequence, then why not just have the ->suspend and ->resume wrappers
> do it
> automatically?
>
> And let's say there's a good argument or it's better coding style to
> allow
> drivers do it on thier own. We still have two problems:
> a.) many drivers will get it wrong
> b.) if we want to change this sequence, every driver must be changed
>
> But say we added the following functions:
> pci_suspend_power(PCI-[D1-D3]);
> pci_restore_power();
>
> These could do exactly what we have above, and the majority of PCI
> drivers
> could utilize them. I think this would simplify things.
It's ok. The question is how many drivers can use it.
Thanks,
Shaohua
