linux-kernel - Re: [PATCH] PCI / PM: Block races between runtime PM and system sleep

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]

Date:	Mon, 20 Jun 2011 21:42:45 +0200
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Alan Stern <stern@...land.harvard.edu>
Cc:	Linux PM mailing list <linux-pm@...ts.linux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: Re: [PATCH] PCI / PM: Block races between runtime PM and system sleep

On Monday, June 20, 2011, Alan Stern wrote:
> On Sun, 19 Jun 2011, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@...k.pl>
> > 
> > After commit e8665002477f0278f84f898145b1f141ba26ee26
> > (PM: Allow pm_runtime_suspend() to succeed during system suspend) it
> > is possible that a device resumed by the pm_runtime_resume(dev) in
> > pci_pm_prepare() will be suspended immediately from a work item,
> > timer function or otherwise, defeating the very purpose of calling
> > pm_runtime_resume(dev) from there.  To prevent that from happening
> > it is necessary to increment the runtime PM usage counter of the
> > device by replacing pm_runtime_resume() with pm_runtime_get_sync().
> > Moreover, the incremented runtime PM usage counter has to be
> > decremented by the corresponding pci_pm_complete(), via
> > pm_runtime_put_noidle().
> 
> In both this and the previous patch, the final decrement should be done 
> by pm_runtime_put_sync() instead of pm_runtime_put_idle().  Otherwise 
> you face the possibility that the usage_count may go to 0 but the 
> device will be left active.

OK, that's how the old code worked, BTW, I overlooked that.

> Furthermore, since we're going to disable runtime PM as soon as the
> suspend callback returns anyway, why not increment usage_count before
> invoking the callback?  This will prevent runtime suspends from 
> occurring while the callback runs, so no changes will be needed in the 
> PCI or USB subsystems.

The PCI case is different from the USB one.  PCI needs to resume devices
before calling their drivers' .suspend() callbacks, so it does that in
.prepare().  If the core acquired a reference to every device  before executing
the subsystem .suspend(), then pm_runtime_resume() could be moved from
pci_pm_prepare() to pci_prepare_suspend(), but then additionally it would
have to be called from pci_pm_freeze() and pci_pm_poweroff().  It simply is
more efficient to call it once from pci_pm_prepare(), but then PCI needs to
take the reference by itself.

Also the core doesn't call the subsystem-level .runtime_idle() after the
subsystem-level .complete() has run, which is useful as you pointed out above. :-)

> It also will prevent Kevin from calling pm_runtime_suspend from within
> his suspend callbacks, but you have already determined that subsystems
> and drivers should never do that in any case.

Then reverting commit e8665002477f0278f84f898145b1f141ba26ee26 would be
even better. :-)

Thanks,
Rafael
--
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/