Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup

[Thomas Gleixner]

On Thu, 18 Sep 2014, Nishanth Menon wrote:
> On 17:57-20140918, Thomas Gleixner wrote:
> 
> I suppose I can improve the commit message to elaborate this better?
> Will that help?

You also want to improve the comment in the empty handler.

> > 
> > > +	 */
> > > +	return IRQ_NONE;

And it still does not explain WHY you think that returning IRQ_NONE is
the right thing to do here. You actually handle the interrupt, right?
Just because the handler is an NOP does not mean you did not handle
it.

> > > +static int palmas_i2c_suspend(struct i2c_client *i2c,  pm_message_t mesg)
> > > +{
> > > +	struct palmas *palmas = i2c_get_clientdata(i2c);
> > > +	struct device *dev = &i2c->dev;
> > > +
> > > +	if (!palmas->wakeirq)
> > > +		return 0;
> > > +
> > > +	if (device_may_wakeup(dev))
> > > +		enable_irq(palmas->wakeirq);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int palmas_i2c_resume(struct i2c_client *i2c)
> > > +{
> > > +	struct palmas *palmas = i2c_get_clientdata(i2c);
> > > +	struct device *dev = &i2c->dev;
> > > +
> > > +	if (!palmas->wakeirq)
> > > +		return 0;
> > > +
> > > +	if (device_may_wakeup(dev))
> > > +		disable_irq_nosync(palmas->wakeirq);
> > 
> > Again, why nosync?
> true - nosync is not necessary at here. disable_irq is however necessary
> as we are not interested in wakeup events for level changes.
> 
> We just use the enable/disable to control when we'd want to arm the pin
> for waking up from suspend state.

And what is issuing the call to enable/disable_irq_wake()? 

So if that interrupt is not marked proper then you can bring your
device into a wont resume state easily

       start suspend
       enable wakeirq
       disable_device_irqs()
	   if (!iswakeup_irq())
	      disable_irq() // does not mask due to lazy masking

       ....
       wakeirq fires
          if (irq_is_disabled())
	     mask_irq();

       transition into suspend

Now your pinctrl irq is masked at the HW level and wont wake the
machine up ever again.

So now looking at that pinctrl irq chip thing, which seems to be
designed to handle these kind of wakeups. That thing looks massivly
wrong as well, simply because it enforces to use
enable_irq/disable_irq().

So because the sole purpose of this chip is to handle the separate
wakeup style interrupt, it should actually NOT enable the interrupt in
the irq_unmask callback.

Simply because during normal operation nothing is interested in the
interrupt and any operation which might enable it (including request
irq) is just making the system handle completely pointless interrupts
and hoops and loops juggling with enable/disable irq.

So the right thing here is to have an empty unmask function and do the
actual unmask only in the irq_set_wake() callback. mask of course
needs to do what it says. The point is, that the following sequence of
code will just work w/o generating an interrupt on the wakeirq line
outside of the wake enabled context.

dev_init()
	request_wakeirq();

suspend()
	if (may_wake())
	   enable_irq_wake();

resume()
	if (may_wake())
	   disable_irq_wake();

The other omap drivers using this have the same issue ... And of
course they are subtly different.

The uart one handles the actual device interrupt, which is violating
the general rule of possible interrupt reentrancy in the pm-runtime
case if the two interrupts are affine to two different cores. Yes,
it's protected by a lock and works by chance ....

The mmc one issues a disable_irq_nosync() in the wakeup irq handler
itself.

WHY does one driver need that and the other does not? You are not even
able to come up with a common scheme for OMAP. I don't want to see the
mess others are going to create when this stuff becomes more used.

Thanks,

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