linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Maulik Shah <mkshah@codeaurora.org>,
	Stephen Boyd <swboyd@chromium.org>,
	bjorn.andersson@linaro.org, evgreen@chromium.org,
	linus.walleij@linaro.org, maz@kernel.org, mka@chromium.org
Cc: linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-gpio@vger.kernel.org, agross@kernel.org,
	jason@lakedaemon.net, dianders@chromium.org,
	rnayak@codeaurora.org, ilina@codeaurora.org,
	lsrao@codeaurora.org
Subject: Re: [PATCH v5 3/6] genirq/PM: Introduce IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND flag
Date: Wed, 26 Aug 2020 12:15:37 +0200	[thread overview]
Message-ID: <87y2m1vhkm.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <8763521f-b121-877a-1d59-5f969dd75e51@codeaurora.org>

On Wed, Aug 26 2020 at 15:22, Maulik Shah wrote:
> On 8/26/2020 3:08 AM, Thomas Gleixner wrote:
>>> Where is the corresponding change to resume_irq()? Don't we need to
>>> disable an irq if it was disabled on suspend and forcibly enabled here?
>>>
> I should have added comment explaining why i did not added.
> I thought of having corresponding change to resume_irq() but i did not 
> kept intentionally since i didn't
> observe any issue in my testing.

That makes it correct in which way? Did not explode in my face is hardly
proof of anything.

> Actually the drivers which called (disable_irq() + enable_irq_wake()), 
> are invoking enable_irq()
> in the resume path everytime. With the driver's call to enable_irq() 
> things are restoring back already.

No, that's just wrong because you again create inconsistent state.

> If above is not true in some corner case, then the IRQ handler of
> driver won't get invoked, in such case, why even to wake up with such
> IRQs in the first place, right?

I don't care about the corner case. If the driver misses to do it is
buggy in the first place. Silently papering over it is just mindless
hackery.

There are two reasonable choices here:

1) Do the symmetric thing

2) Let the drivers call a new function disable_wakeup_irq_for_suspend()
   which marks the interrupt to be enabled from the core on suspend and
   remove the enable call on the resume callback of the driver.

   Then you don't need the resume part in the core and state still is
   consistent.

I'm leaning towards #2 because that makes a lot of sense.

Thanks,

        tglx

  reply	other threads:[~2020-08-26 10:15 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-22 16:16 [PATCH v5 0/6] irqchip: qcom: pdc: Introduce irq_set_wake call Maulik Shah
2020-08-22 16:16 ` [PATCH v5 1/6] pinctrl: qcom: Set IRQCHIP_SET_TYPE_MASKED and IRQCHIP_MASK_ON_SUSPEND flags Maulik Shah
2020-08-31 18:33   ` Bjorn Andersson
2020-08-22 16:16 ` [PATCH v5 2/6] pinctrl: qcom: Use return value from irq_set_wake() call Maulik Shah
2020-08-31 18:19   ` Bjorn Andersson
2020-08-22 16:16 ` [PATCH v5 3/6] genirq/PM: Introduce IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND flag Maulik Shah
2020-08-25 10:12   ` Stephen Boyd
2020-08-25 21:38     ` Thomas Gleixner
2020-08-26  9:52       ` Maulik Shah
2020-08-26 10:15         ` Thomas Gleixner [this message]
2020-08-31 15:12           ` Doug Anderson
2020-09-01  9:51             ` Thomas Gleixner
2020-09-02 20:26               ` Doug Anderson
2020-09-03 12:57                 ` Thomas Gleixner
2020-09-03 23:19                   ` Doug Anderson
2020-09-04  9:54                     ` Thomas Gleixner
2020-09-08 19:05                       ` Doug Anderson
2020-09-10  8:51                         ` Thomas Gleixner
2020-08-22 16:16 ` [PATCH v5 4/6] pinctrl: qcom: Set " Maulik Shah
2020-08-25 10:14   ` Stephen Boyd
2020-08-25 19:58   ` Doug Anderson
2020-08-22 16:17 ` [PATCH v5 5/6] irqchip: qcom-pdc: " Maulik Shah
2020-08-25 10:14   ` Stephen Boyd
2020-08-25 19:59   ` Doug Anderson
2020-08-22 16:17 ` [PATCH v5 6/6] irqchip: qcom-pdc: Reset PDC interrupts during init Maulik Shah
2020-08-25 10:14   ` Stephen Boyd
2020-08-25 20:00   ` Doug Anderson
2020-08-27 22:48 ` [PATCH v5 0/6] irqchip: qcom: pdc: Introduce irq_set_wake call Linus Walleij

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87y2m1vhkm.fsf@nanos.tec.linutronix.de \
    --to=tglx@linutronix.de \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=dianders@chromium.org \
    --cc=evgreen@chromium.org \
    --cc=ilina@codeaurora.org \
    --cc=jason@lakedaemon.net \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=lsrao@codeaurora.org \
    --cc=maz@kernel.org \
    --cc=mka@chromium.org \
    --cc=mkshah@codeaurora.org \
    --cc=rnayak@codeaurora.org \
    --cc=swboyd@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).