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]

Date:	Thu, 14 May 2015 11:09:02 -0500
From:	Felipe Balbi <balbi@...com>
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

Hi,

On Thu, May 14, 2015 at 08:59:46AM -0700, Tony Lindgren wrote:
> > > +int dev_pm_request_wake_irq(struct device *dev,
> > > +			    int irq,
> > > +			    irq_handler_t handler,
> > > +			    unsigned long irqflags,
> > > +			    void *data)
> > > +{
> > > +	struct wake_irq *wirq;
> > > +	int err;
> > > +
> > > +	wirq = devm_kzalloc(dev, sizeof(*wirq), GFP_KERNEL);
> > > +	if (!wirq)
> > > +		return -ENOMEM;
> > > +
> > > +	wirq->dev = dev;
> > > +	wirq->irq = irq;
> > > +	wirq->handler = handler;
> > 
> > you need to make sure that IRQF_ONESHOT is set in cases where handler is
> > NULL. Either set it by default:
> > 
> > 	if (!handler)
> > 		irqflags |= IRQF_ONESHOT;
> > 
> > or WARN() about it:
> > 
> > 	WARN_ON((!handler && !(irqflags & IRQF_ONESHOT));
> > 
> > Actually, after reading more of the code, you have some weird circular
> > call chain going on here. If handler is a valid function pointer, you
> > use as primary handler, so IRQ core will call it from hardirq context.
> > But you also save that pointer as wirq->handler, and call that from
> > within handle_threaded_wakeirq(). Essentially, handler() is called
> > twice, once with IRQs disabled, one with IRQs (potentially) enabled.
> > 
> > What did you have in mind for handler() anyway, it seems completely
> > unnecessary.
> 
> Yeah.. Let's just leave it out. You already mentioned it earlier and
> there's no use for it.
> 
> The device driver can deal with the situation anyways in runtime resume.
> 
> And like you said, it must be IRQF_ONESHOT, now there's a chance of
> consumer drivers passing other flags too.
> 
> The the IO wake-up interrupt vs dedicated wake-up interrupt functions
> can be just something like:
> 
> int dev_pm_request_wake_irq(struct device *dev, int irq);

right, then it's always IRQF_ONESHOT and always threaded.

> int dev_pm_request_wake_irq_managed(struct device *dev, int irq);

I don't get this. Would this request with devm_ while the former
wouldn't use devm_ ?

> > > +void dev_pm_free_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);
> > > +	wirq->manage_irq = false;
> > > +	spin_unlock_irqrestore(&dev->power.lock, flags);
> > > +	devm_free_irq(wirq->dev, wirq->irq, wirq);
> > 
> > this seems unnecessary, the IRQ will be freed anyway when the device
> > kobj is destroyed, dev_pm_clear_wake_irq() seems important, however.
> > 
> > > +	dev_pm_clear_wake_irq(dev);
> > > +}
> 
> The life cycle of the request and free of the wake irq is not the
> same as the life cycle of the device driver. For example, serial
> drivers can request interrupts on startup and free them on shutdown.

fair enough, but then we start to consider the benefits of using
devm_ IRQ :-)

> > > +void dev_pm_disable_wake_irq(struct device *dev)
> > > +{
> > > +	struct wake_irq *wirq = dev->power.wakeirq;
> > > +
> > > +	if (wirq && wirq->manage_irq)
> > > +		disable_irq_nosync(wirq->irq);
> > > +}
> > > +EXPORT_SYMBOL_GPL(dev_pm_disable_wake_irq);
> > 
> > likewise, call this automatically from rpm_resume().
> 
> Right.
>  
> > This brings up a question, actually. What to do with devices which were
> > already runtime suspended when user initiated suspend-to-ram ? Do we
> > leave wakeups enabled, or do we revisit device_may_wakeup() and
> > conditionally runtime_resume the device, disable wakeup, and let its
> > ->suspend() callback be called ?
> 
> I believe that's already handled properly, but implemented in each
> consumer driver with the if device_may_wakeup enabled_irq_wake.

I see, but perhaps even that can be partially automated at some point
:-)

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)