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

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 3873 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]