Re: [RFC] PCI: Runtime power management

[Matthew Garrett <mjg59@srcf.ucam.org>]

On Fri, Aug 14, 2009 at 11:22:27PM +0200, Rafael J. Wysocki wrote:

> Do you have any prototypes for that?  I started working on it some time ago,
> but then I focused on the core runtime PM framework.

The native PCIe PME code? There's some in the final patchset at 
http://bugzilla.kernel.org/show_bug.cgi?id=6892 but I haven't had time 
to look into merging that into the current kernel. I also don't have 
anything to test against, which makes life more awkward.

> > +static int acpi_pci_runtime_wake(struct pci_dev *dev, bool enable)
> > +{
> > +	acpi_status status;
> > +	acpi_handle handle = DEVICE_ACPI_HANDLE(&dev->dev);
> > +	struct acpi_device *acpi_dev;
> > +
> 
> Hm, I'd move that into ACPI as
> 
> int acp_runtime_wake_enable(acpi_handle handle, bool enable)
> 
> in which form it could also be useful to non-PCI devices.

Hm. Yeah, that's not too bad an idea.

> > +		acpi_disable_gpe(acpi_dev->wakeup.gpe_device,
> > +				 acpi_dev->wakeup.gpe_number);
> > +	}
> > +	return 0;
> > +}
> 
> Ah, that's the part I've always been missing!
> 
> How exactly do we figure out which GPE is a wake-up one for given device?
> IOW, how are the wakeup.gpe_device and wakeup.gpe_number fields populated?

There's a field in the ACPI device definition in the DSDT that defines 
the needed GPE and which runlevels it can resume from.

> > +	error = pci_pm_suspend(dev);
> 
> This has a chance to be confusing IMO.  pci_pm_suspend() calls the driver's
> ->suspend() routine, which is specific to suspend to RAM.  So, this means
> that drivers are supposed to implement ->runtime_suspend() only if they
> want to do something _in_ _addition_ to the things done by
> ->suspend() and ->suspend_noirq().

Yes, that was how I'd planned it. An alternative would be for 
runtime_suspend to return a negative value if there's an error, 0 if the 
bus code should continue or a positive value if the runtime_suspend() 
call handles all of it and the bus code should just return immediately?

> > +	disable_irq(pci_dev->irq);
> 
> I don't really think it's necessary to disable the interrupt here.  We prevent
> drivers from receiving interrupts while pci_pm_suspend_noirq() is being run
> during system-wide power transitions to protect them from receiving "alien"
> interrupts they might be unable to handle, but in the runtime case I think the
> driver should take care of protecting itself from that.

That sounds fine. I didn't want to take a risk in that respect, but if 
we should be safe here I can just drop that.

> > +	if (!enable || pci_pme_capable(dev, PCI_D3hot)) {
> > +		pci_pme_active(dev, enable);
> > +		pme_done = true;
> > +	}
> 
> I don't really follow your intention here.  The condition means that PME is
> going to be enabled unless 'enable' is set and the device is not capable
> of generating PMEs.  However, if 'enable' is unset, we're still going to try
> to enable the PME, even if the device can't generate it.  Shouldn't that
> be

Hmm. That was copied from pci_enable_wake() just above, but it does seem 
a little bit odd. I suspect that that needs some clarification as well.

> Also, that assumes the device is going to be put into D3_hot, but do we know
> that for sure?

I'd be surprised if there's any hardware that supports wakeups from D2 
but not D3hot, so I just kept the code simple for now.

-- 
Matthew Garrett | mjg59@srcf.ucam.org