Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org BFE8AC433C6
Subject: Re: [PATCH v5 3/6] genirq/PM: Introduce
 IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND flag
To:     Thomas Gleixner <tglx@linutronix.de>,
        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
References: <1598113021-4149-1-git-send-email-mkshah@codeaurora.org>
 <1598113021-4149-4-git-send-email-mkshah@codeaurora.org>
 <159835036999.334488.14725849347753031927@swboyd.mtv.corp.google.com>
 <874koqxv6t.fsf@nanos.tec.linutronix.de>
From:   Maulik Shah <mkshah@codeaurora.org>
Message-ID: <8763521f-b121-877a-1d59-5f969dd75e51@codeaurora.org>
Date:   Wed, 26 Aug 2020 15:22:51 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <874koqxv6t.fsf@nanos.tec.linutronix.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-GB
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On 8/26/2020 3:08 AM, Thomas Gleixner wrote:
> On Tue, Aug 25 2020 at 03:12, Stephen Boyd wrote:
>> Quoting Maulik Shah (2020-08-22 09:16:58)
>>> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
>>> index c6c7e18..2cc800b 100644
>>> --- a/kernel/irq/pm.c
>>> +++ b/kernel/irq/pm.c
>>> @@ -69,12 +69,17 @@ void irq_pm_remove_action(struct irq_desc *desc, struct irqaction *action)
>>>   
>>>   static bool suspend_device_irq(struct irq_desc *desc)
>>>   {
>>> +       unsigned long chipflags = irq_desc_get_chip(desc)->flags;
>>> +
>>>          if (!desc->action || irq_desc_is_chained(desc) ||
>>>              desc->no_suspend_depth)
>>>                  return false;
>>>   
>>>          if (irqd_is_wakeup_set(&desc->irq_data)) {
>>>                  irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED);
>>> +
>>> +               if (chipflags & IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND)
>>> +                       irq_enable(desc);
>> 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.

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.

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?

However If we don't want to rely on the drivers doing things correctly, 
state can be restored in resume_irq()
I explored this, During suspend,

1. Some IRQs are unmasked/enabled + marked for wakeup
2. Some IRQs are masked/disabled + marked for wakeup

So have to track and restore only IRQs in category (2).
With current patch we don't have way to track IRQ is in (1) or (2).
It may be done with the new IRQD flag saying like 
IRQD_IRQ_ENABLED_ON_SUSPEND

During suspend,
First check if the IRQ was in disabled/masked state to invoke 
irq_enable() only for category (2) and set the new flag.

     if (irqd_irq_disabled(&desc->irq_data) && (chipflags & 
IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND)) {
         irq_enable(desc);
         irq_state_set_enabled_on_suspend(desc); => this will set new 
IRQD_IRQ_ENABLED_ON_SUSPEND
     }

During resume,
Simply calling irq_disable(desc); don't work in resume_irq(), since by 
default this API tries to lazily disable at HW, which won't quite 
restore the state,
So instead of adding below

     if (irqd_irq_disabled(&desc->irq_data) && (chipflags & 
IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND)
     && (irqd_is_enabled_on_suspend(desc)))
     {
         irq_disable(desc);
         irq_state_clear_enabled_on_suspend(desc); => clear flag
     }

we can replicate the irq_disable() with removal of lazy part, something 
like,

     if (irqd_irq_disabled(&desc->irq_data) && (chipflags & 
IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND) &&
     (irqd_is_enabled_on_suspend(desc))) { ==> The new flag used to 
determine if IRQ was enabled during suspend path, then only restore.
         irq_state_set_disabled(desc);
         if (desc->irq_data.chip->irq_disable) {
desc->irq_data.chip->irq_disable(&desc->irq_data);
             irq_state_set_masked(desc);
         } else {
         mask_irq(desc);
     }
     irq_state_clear_enabled_on_suspend(desc);
}

which is matching exactly reverse of what is done in suspend entry.
Let me know if above is good i can include this in v6.

Thanks,
Maulik

> That part was below the POC code I provided in the fine print:
>
>   "plus the counterpart in the resume path. This also ensures that state is
>    consistent."
>
> Who reads the fine print? :)

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation