On 08:37-20140919, Thomas Gleixner wrote:
> 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.
OK. will do the same. Thanks for suggesting.
>
> > >
> > > > + */
> > > > + 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.
Hmm.. My motivation for IRQ_NONE was because this specific handler does
not handle the interrupt. Now, from this discussion, I understand that I
should rather use IRQ_HANDLED since the event is indeed handled (just
not here).
Thank you for correcting my understanding. Will update in my next rev
(once we solve the following discussion)..
>
> > > > +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.
True.
>
> 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
I think I understand your concern - I request Tony to comment about
this. I mean, I can try and hook things like uart in other drivers
(like https://patchwork.kernel.org/patch/4759171/ ), but w.r.t overall
generic usage guideline wise, I would prefer Tony to comment.
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
