linux-kernel - Re: [RFC] PCI: Runtime power management

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090813214701.GC14532@srcf.ucam.org>
Date:	Thu, 13 Aug 2009 22:47:01 +0100
From:	Matthew Garrett <mjg@...hat.com>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	"Rafael J. Wysocki" <rjw@...k.pl>, Greg KH <gregkh@...e.de>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux-pm mailing list <linux-pm@...ts.linux-foundation.org>,
	linux-pci@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [RFC] PCI: Runtime power management

On Thu, Aug 13, 2009 at 11:17:05AM -0400, Alan Stern wrote:
> On Thu, 13 Aug 2009, Matthew Garrett wrote:
> 
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> 
> > +#ifdef CONFIG_PM_RUNTIME
> > +
> > +static int pci_pm_runtime_suspend(struct device *dev)
> > +{
> > +	struct pci_dev *pci_dev = to_pci_dev(dev);
> > +	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > +	int error;
> > +
> > +	device_set_wakeup_enable(dev, 1);
> 
> This is a userspace policy parameter.  Kernel code should not alter it.
> Instead you should test device_may_wakeup.

Ugh. I'd really prefer us to assume that drivers are able to cope unless 
proven otherwise. Userspace policy makes sense where we don't have any 
idea whether something will work or not, but I'd really expect that most 
PCI drivers will either cope (in which case they'll have enabling code) 
or won't (in which case they won't). Why would we want userspace to 
influence this?

> > +	enable_irq(pci_dev->irq);
> > +resume:
> > +	pci_pm_resume(dev);
> > +out:
> > +	pci_enable_runtime_wake(pci_dev, false);
> > +	return error;
> > +}
> 
> The goto statements and unwinding code don't match up.

I'll look at that.

> > +	if (error)
> > +		return error;
> > +
> > +	return 0;
> > +}
> 
> Log an error message when something goes wrong?

Seems fair.

> > +static void pci_pm_runtime_idle(struct device *dev)
> > +{
> > +	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > +
> > +	if (pm && pm->runtime_idle)
> > +		pm->runtime_idle(dev);
> > +
> > +	pm_schedule_suspend(dev, 0);
> > +}
> 
> This misses the point.  The whole idea of runtime_idle is to tell you 
> that the device is idle and might be ready to be suspended.  If you're 
> going to call pm_schedule_suspend anyway, there's no reason to invoke 
> pm->runtime_idle.

My understanding of the API was that pm_device_put() invokes 
runtime_idle if the refcount hits 0. The bus layer has no idea of the 
refcount, and calling suspend directly from the driver would defeat the 
point of the system-wide recounting.

-- 
Matthew Garrett | mjg59@...f.ucam.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/