linux-kernel - Re: [PATCH 2/5] PM / Wakeirq: Add automated device wake IRQ handling

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: <3371662.LPKmgz3rb9@vostro.rjw.lan>
Date:	Tue, 19 May 2015 16:26:37 +0200
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Tony Lindgren <tony@...mide.com>
Cc:	Felipe Balbi <balbi@...com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
	Alan Stern <stern@...land.harvard.edu>,
	Andreas Fenkart <afenkart@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Huiquan Zhong <huiquan.zhong@...el.com>,
	Kevin Hilman <khilman@...nel.org>, NeilBrown <neilb@...e.de>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Nishanth Menon <nm@...com>,
	Peter Hurley <peter@...leysoftware.com>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	linux-omap@...r.kernel.org
Subject: Re: [PATCH 2/5] PM / Wakeirq: Add automated device wake IRQ handling

On Tuesday, May 19, 2015 04:04:43 PM Rafael J. Wysocki wrote:
> On Monday, May 18, 2015 04:44:01 PM Tony Lindgren wrote:
> > * Tony Lindgren <tony@...mide.com> [150518 15:06]:
> > > +/**
> > > + * dev_pm_set_wake_irq - Attach device IO interrupt as wake IRQ
> > > + * @dev: Device entry
> > > + * @irq: Device IO interrupt
> > > + *
> > > + * Attach a device IO interrupt as a wake IRQ. The wake IRQ gets
> > > + * automatically configured for wake-up from suspend  based
> > > +void dev_pm_clear_wake_irq(struct device *dev)
> > > +{
> > > +	struct wake_irq *wirq = dev->power.wakeirq;
> > > +	unsigned long flags;
> > > +
> > > +	if (!wirq)
> > > +		return;
> > > +
> > > +	device_wakeup_detach_irq(dev);
> > > +
> > > +	spin_lock_irqsave(&dev->power.lock, flags);
> > > +	if (wirq->manage_irq) {
> > > +		free_irq(wirq->irq, wirq);
> > > +		wirq->manage_irq = false;
> > > +	}
> > > +	dev->power.wakeirq = NULL;
> > > +	spin_unlock_irqrestore(&dev->power.lock, flags);
> > > +
> > > +	wirq->irq = -EINVAL;
> > > +	kfree(wirq);
> > > +}
> > 
> > I just noticed most of the dev_pm_clear_wake_irq is no longer needed.
> > We're now freeing it anyways. so it can be just:
> > 
> > void dev_pm_clear_wake_irq(struct device *dev)
> > {
> > 	struct wake_irq *wirq = dev->power.wakeirq;
> > 	unsigned long flags;
> > 
> > 	if (!wirq)
> > 		return;
> > 
> > 	spin_lock_irqsave(&dev->power.lock, flags);
> > 	dev->power.wakeirq = NULL;
> > 	spin_unlock_irqrestore(&dev->power.lock, flags);
> > 
> > 	device_wakeup_detach_irq(dev);
> > 	if (wirq->manage_irq)
> > 		free_irq(wirq->irq, wirq);
> > 	kfree(wirq);
> > }
> > 
> > 
> > Regards,
> > 
> > Tony
> > 
> > 8< ---------------------
> > From: Tony Lindgren <tony@...mide.com>
> > Date: Mon, 18 May 2015 15:40:29 -0700
> > Subject: [PATCH] PM / Wakeirq: Add automated device wake IRQ handling
> > 
> > Turns out we can automate the handling for the device_may_wakeup()
> > quite a bit by using the kernel wakeup source list.
> > 
> > And as some hardware has separate dedicated wake-up interrupt
> > in addition to the IO interrupt, we can automate the handling by
> > adding a generic threaded interrupt handler that just calls the
> > device PM runtime to wake up the device.
> > 
> > This allows dropping code from device drivers as we currently
> > are doing it in multiple ways, and often wrong.
> > 
> > For most drivers, we should be able to drop the following
> > boilerplate code from runtime_suspend and runtime_resume
> > functions:
> > 
> > 	...
> > 	device_init_wakeup(dev, true);
> > 	...
> > 	if (device_may_wakeup(dev)
> > 		enable_irq_wake(irq);
> > 	...
> > 	if (device_may_wakeup(dev)
> > 		enable_irq_wake(irq);
> 
> Closing parens are missin in the above two if () statements.
> 
> Also, should the second one be disable_irq_wake(irq)?
> 
> > 	...
> > 	device_init_wakeup(dev, false);
> > 	...
> > 
> > We can replace it with just the following init and exit
> > time code:
> > 
> > 	...
> > 	device_init_wakeup(dev, true);
> > 	dev_pm_set_wake_irq(dev, irq);
> > 	...
> > 	dev_pm_clear_wake_irq(dev);
> > 	device_init_wakeup(dev, false);
> > 	...
> > 
> > And for hardware with dedicated wake-up interrupts:
> > 
> > 	...
> > 	device_init_wakeup(dev, true);
> > 	dev_pm_set_dedicated_wake_irq(dev, irq);
> > 	...
> > 	dev_pm_clear_wake_irq(dev);
> > 	device_init_wakeup(dev, false);
> > 	...
> > 
> > For now, let's only enable it for select PM_WAKEIRQ.
> 
> Why?  What would be wrong with doing that unconditionally?

I mean, what about making it depend on CONFIG_PM directly?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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/