Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <4d6307d0-cd67-dfb2-6d6d-2f37bf4a271b@gmail.com>
Date:   Tue, 5 Apr 2022 03:25:37 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH] Revert "ACPI: processor: idle: Only flush cache on
 entering C3"
Content-Language: en-US
To:     "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
        Andi Kleen <ak@linux.intel.com>,
        Borislav Petkov <bp@alien8.de>,
        Dan Williams <dan.j.williams@intel.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Kuppuswamy Sathyanarayanan <knsathya@kernel.org>,
        ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Tony Luck <tony.luck@intel.com>,
        the arch/x86 maintainers <x86@kernel.org>
References: <20220403062322.3168-1-akihiko.odaki@gmail.com>
 <CAJZ5v0gaRr-r8VFCEGSP1nTX0CwrOi35DXZB5Z8A9tiLufNxPg@mail.gmail.com>
From:   Akihiko Odaki <akihiko.odaki@gmail.com>
In-Reply-To: <CAJZ5v0gaRr-r8VFCEGSP1nTX0CwrOi35DXZB5Z8A9tiLufNxPg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022/04/05 3:13, Rafael J. Wysocki wrote:
> On Sun, Apr 3, 2022 at 8:25 AM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>
>> This reverts commit 87ebbb8c612b1214f227ebb8f25442c6d163e802.
>>
>> ACPI processor power states can be transitioned in two distinct
>> situations: 1. when CPU goes idle and 2. before CPU goes offline
>> ("playing dead") to suspend or hibernate. Case 1 is handled by
>> acpi_idle_enter or acpi_idle_enter_s2idle. Case 2 is handled by
>> acpi_idle_play_dead.
>>
>> It is necessary to flush CPU caches in case 2 even if it is not
>> required to transit ACPI processor power states as CPU will go
>> offline soon. However, the reverted commit incorrectly removed CPU
>> cache flushing in such a condition.
> 
> I think what you mean is that the CPU cache must always be flushed in
> acpi_idle_play_dead(), regardless of the target C-state that is going
> to be requested, because this is likely to be part of a CPU offline
> procedure or preparation for entering a system-wide sleep state and
> the stale cache contents may lead to problems going forward, for
> example when the CPU is taken back online.
> 
> If so, I will put the above information into the patch changelog.

I guess it is causing problems because the dirty caches will not get 
written back and the RAM becomes stale if they are not flushed. From my 
understanding, the CPU should have an empty cache and read back contents 
from RAM when it is taken back online.

> 
>> In fact, it made resuming from
>> suspend-to-RAM occasionally fail on Lenovo ThinkPad C13 Yoga.
> 
> So this probably means that resume from suspend-to-RAM occasionally
> fails on Lenovo ThinkPad C13 Yoga and reverting the commit in question
> fixes this problem.  Is that correct?

Yes, that is what I meant.

Regards,
Akihiko Odaki

> 
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
>> ---
>>   drivers/acpi/processor_idle.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index f8e9fa82cb9b..05b3985a1984 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -570,8 +570,7 @@ static int acpi_idle_play_dead(struct cpuidle_device *dev, int index)
>>   {
>>          struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu);
>>
>> -       if (cx->type == ACPI_STATE_C3)
>> -               ACPI_FLUSH_CPU_CACHE();
>> +       ACPI_FLUSH_CPU_CACHE();
>>
>>          while (1) {
>>
>> --
>> 2.35.1
>>