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;
Subject: Re: [PATCH] cpufreq: intel_pstate: Fix for HWP interrupt before
 driver is ready
To:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc:     "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Len Brown <lenb@kernel.org>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20210904053703.581297-1-srinivas.pandruvada@linux.intel.com>
 <CAJZ5v0hQp8Hxf__tL22s0oOcTym5mx9tND34ijufTDE3_NSW6A@mail.gmail.com>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <926ac4b9-1bb5-e96e-ded3-6461f7a215b7@kernel.dk>
Date:   Mon, 6 Sep 2021 10:43:42 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <CAJZ5v0hQp8Hxf__tL22s0oOcTym5mx9tND34ijufTDE3_NSW6A@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 9/6/21 10:17 AM, Rafael J. Wysocki wrote:
> On Sat, Sep 4, 2021 at 7:37 AM Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
>>
>> In Lenovo X1 gen9 laptop, HWP interrupts arrive before driver is ready
>> to handle on that CPU. Basically didn't even allocated memory for per
>> cpu data structure and not even started interrupt enable process on that
>> CPU. So interrupt handler observes a NULL pointer to schedule work.
>>
>> This interrupt was probably for SMM, but since it is redirected to
>> OS by OSC call, OS receives it, but not ready to handle. That redirection
>> of interrupt to OS was also done to solve one SMM crash on Yoga 260 for
>> HWP interrupt a while back.
>>
>> To solve this the HWP interrupt handler should ignore such request if the
>> driver is not ready. This will require some flag to wait till the driver
>> setup a workqueue to handle on a CPU. We can't simply assume cpudata to
>> be NULL and avoid processing as it may not be NULL but data structure is
>> not in consistent state.
>>
>> So created a cpumask which sets the CPU on which interrupt was setup. If
>> not setup, simply clear the interrupt status and return. Since the
>> similar issue can happen during S3 resume, clear the bit during offline.
>>
>> Since interrupt timing may be before HWP is enabled, use safe MSR read
>> writes as before the change for HWP interrupt.
>>
>> Fixes: d0e936adbd22 ("cpufreq: intel_pstate: Process HWP Guaranteed change notification")
>> Reported-and-tested-by: Jens Axboe <axboe@kernel.dk>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> ---
>>  drivers/cpufreq/intel_pstate.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index b4ffe6c8a0d0..5ac86bfa1080 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -298,6 +298,8 @@ static bool hwp_boost __read_mostly;
>>
>>  static struct cpufreq_driver *intel_pstate_driver __read_mostly;
>>
>> +static cpumask_t hwp_intr_enable_mask;
>> +
>>  #ifdef CONFIG_ACPI
>>  static bool acpi_ppc;
>>  #endif
>> @@ -1067,11 +1069,15 @@ static void intel_pstate_hwp_set(unsigned int cpu)
>>         wrmsrl_on_cpu(cpu, MSR_HWP_REQUEST, value);
>>  }
>>
>> +static void intel_pstate_disable_hwp_interrupt(struct cpudata *cpudata);
>> +
>>  static void intel_pstate_hwp_offline(struct cpudata *cpu)
>>  {
>>         u64 value = READ_ONCE(cpu->hwp_req_cached);
>>         int min_perf;
>>
>> +       intel_pstate_disable_hwp_interrupt(cpu);
>> +
>>         if (boot_cpu_has(X86_FEATURE_HWP_EPP)) {
>>                 /*
>>                  * In case the EPP has been set to "performance" by the
>> @@ -1645,20 +1651,35 @@ void notify_hwp_interrupt(void)
>>         if (!hwp_active || !boot_cpu_has(X86_FEATURE_HWP_NOTIFY))
>>                 return;
>>
>> -       rdmsrl(MSR_HWP_STATUS, value);
>> +       rdmsrl_safe(MSR_HWP_STATUS, &value);
>>         if (!(value & 0x01))
>>                 return;
>>
>> +       if (!cpumask_test_cpu(this_cpu, &hwp_intr_enable_mask)) {
>> +               wrmsrl_safe(MSR_HWP_STATUS, 0);
>> +               return;
>> +       }
> 
> Without additional locking, there is a race between this and
> intel_pstate_disable_hwp_interrupt().
> 
> 1. notify_hwp_interrupt() checks hwp_intr_enable_mask() and the target
> CPU is in there, so it will go for scheduling the delayed work.
> 2. intel_pstate_disable_hwp_interrupt() runs between the check and the
> cpudata load below.
> 3. hwp_notify_work is scheduled on the CPU that isn't there in the
> mask any more.

I noticed that too, not clear to me how much of an issue that is in
practice. But there's definitely a race there.

Can we just revert d0e936adbd22 for now so this can get solved in due
time? I doubt I'm the only one that got bit by this, but I might be the
only one that bothered to actually figured out what caused my laptop to
crash with a black screen on boot.

-- 
Jens Axboe