Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: SoftFail (protection.outlook.com: domain of transitioning
 lenovo.com discourages use of 104.232.225.7 as permitted sender)
Message-ID: <df1c3d64-7255-0b7d-51a4-01a615a8c07b@lenovo.com>
Date:   Thu, 13 Oct 2022 09:53:23 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.0
Subject: Re: Fw: [External] Re: [PATCH 1/1] pinctrl: amd: Add dynamic
 debugging for active GPIOs
Content-Language: en-US
To:     Mario Limonciello <mario.limonciello@amd.com>
References: <20221012221028.4817-1-mario.limonciello@amd.com>
 <20221012221028.4817-2-mario.limonciello@amd.com>
 <CAAd53p4sKdFL-DcPet2srtisiX_Qr9qpiV159Xj9YSUM9yiLPw@mail.gmail.com>
 <TYZPR03MB59942B6A03B43132DADF1C5CBD259@TYZPR03MB5994.apcprd03.prod.outlook.com>
CC:     Kai-Heng Feng <kai.heng.feng@canonical.com>,
        <linus.walleij@linaro.org>, <Basavaraj.Natikar@amd.com>,
        "S-k, Shyam-sundar" <Shyam-sundar.S-k@amd.com>,
        Mark Pearson <mpearson@lenovo.com>,
        <renjith.pananchikkal@amd.com>, <linux-gpio@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
From:   Mark Pearson <markpearson@lenovo.com>
In-Reply-To: <TYZPR03MB59942B6A03B43132DADF1C5CBD259@TYZPR03MB5994.apcprd03.prod.outlook.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Oct 2022 13:53:26.7514
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 457d89d8-3223-4b68-6d51-08daad224dd5
X-MS-Exchange-CrossTenant-Id: 5c7d0b28-bdf8-410c-aa93-4df372b16203
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=5c7d0b28-bdf8-410c-aa93-4df372b16203;Ip=[104.232.225.7];Helo=[mail.lenovo.com]
X-MS-Exchange-CrossTenant-AuthSource: SG2APC01FT0057.eop-APC01.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SG2PR03MB6658
X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


>>
>> Some laptops have been reported to wake up from s2idle when plugging
>> in the AC adapter or by closing the lid.  This is a surprising
>> behavior that is further clarified by commit cb3e7d624c3ff ("PM:
>> wakeup: Add extra debugging statement for multiple active IRQs").
>>
>> With that commit in place the following interaction can be seen
>> when the lid is closed:
>>
>> [   28.946038] PM: suspend-to-idle
>> [   28.946083] ACPI: EC: ACPI EC GPE status set
>> [   28.946101] ACPI: PM: Rearming ACPI SCI for wakeup
>> [   28.950152] Timekeeping suspended for 3.320 seconds
>> [   28.950152] PM: Triggering wakeup from IRQ 9
>> [   28.950152] ACPI: EC: ACPI EC GPE status set
>> [   28.950152] ACPI: EC: ACPI EC GPE dispatched
>> [   28.995057] ACPI: EC: ACPI EC work flushed
>> [   28.995075] ACPI: PM: Rearming ACPI SCI for wakeup
>> [   28.995131] PM: Triggering wakeup from IRQ 9
>> [   28.995271] ACPI: EC: ACPI EC GPE status set
>> [   28.995291] ACPI: EC: ACPI EC GPE dispatched
>> [   29.098556] ACPI: EC: ACPI EC work flushed
>> [   29.207020] ACPI: EC: ACPI EC work flushed
>> [   29.207037] ACPI: PM: Rearming ACPI SCI for wakeup
>> [   29.211095] Timekeeping suspended for 0.739 seconds
>> [   29.211095] PM: Triggering wakeup from IRQ 9
>> [   29.211079] PM: Triggering wakeup from IRQ 7
>> [   29.211095] ACPI: PM: ACPI non-EC GPE wakeup
>> [   29.211095] PM: resume from suspend-to-idle
>>
>> * IRQ9 on this laptop is used for the ACPI SCI.
>> * IRQ7 on this laptop is used for the GPIO controller.
>>
>> What has occurred is when the lid was closed the EC woke up the
>> SoC from it's deepest sleep state and the kernel's s2idle loop
>> processed all EC events.  When it was finished processing EC events,
>> it checked for any other reasons to wake (break the s2idle loop).
>>
>> The IRQ for the GPIO controller was active so the loop broke, and
>> then this IRQ was processed.  This is not a kernel bug but it is
>> certainly a surprising behavior, and to better debug it we should
>> have a dynamic debugging message that we can enact to catch it.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> 
> This is very useful, thanks for the patch!
> 
> Acked-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
Seconded! Helped tracking down some issues on our platforms. Thanks Mario
and AMD team.

Acked-by: markpearson@lenovo.com

>> ---
>>  drivers/pinctrl/pinctrl-amd.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
>> index 4691a33bc374f..8378b4115ec0d 100644
>> --- a/drivers/pinctrl/pinctrl-amd.c
>> +++ b/drivers/pinctrl/pinctrl-amd.c
>> @@ -628,13 +628,16 @@ static bool do_amd_gpio_irq_handler(int irq, void *dev_id)
>>                 /* Each status bit covers four pins */
>>                 for (i = 0; i < 4; i++) {
>>                         regval = readl(regs + i);
>> -                       /* caused wake on resume context for shared IRQ */
>> -                       if (irq < 0 && (regval & BIT(WAKE_STS_OFF))) {
>> +
>> +                       if (regval & BIT(WAKE_STS_OFF) ||
>> +                           regval & BIT(INTERRUPT_STS_OFF))
>>                                 dev_dbg(&gpio_dev->pdev->dev,
>> -                                       "Waking due to GPIO %d: 0x%x",
>> +                                       "GPIO %d is active: 0x%x",
>>                                         irqnr + i, regval);
>> +
>> +                       /* caused wake on resume context for shared IRQ */
>> +                       if (irq < 0 && (regval & BIT(WAKE_STS_OFF)))
>>                                 return true;
>> -                       }
>>
>>                         if (!(regval & PIN_IRQ_PENDING) ||
>>                             !(regval & BIT(INTERRUPT_MASK_OFF)))
>> --
>> 2.34.1
>>