Re: [PATCH v2 2/4] cpufreq: qcom-hw: fix the race between LMH worker and cpuhp

Dmitry Baryshkov
2022-03-17T23:21:45Z

On 18/03/2022 02:10, Vladimir Zapolskiy wrote:
> Hi Dmitry,
>
> On 3/10/22 12:39 AM, Dmitry Baryshkov wrote:
>> qcom_lmh_dcvs_poll() can be running when the cpu is being put offline.
>> This results in the following warnings and an oops. The driver would
>> disable the worker, but it happens closer to the end of
>> cpufreq_offline(). Change the locking in the qcom_lmh_dcvs_poll(), so
>> that the worker can not run in parallel with cpufreq_offline() call.
>>
>> [   55.650435] (NULL device *): dev_pm_opp_find_freq_floor: Invalid
>> argument freq=00000000709ccbf9
>> [   55.659420] (NULL device *): Can't find the OPP for throttling:
>> -EINVAL!
>> [   55.666329] Unable to handle kernel paging request at virtual
>> address ffffadfba4bb6d81
>> [   55.674491] Mem abort info:
>> [   55.677363]   ESR = 0x96000004
>> [   55.680527]   EC = 0x25: DABT (current EL), IL = 32 bits
>> [   55.686001]   SET = 0, FnV = 0
>> [   55.689164]   EA = 0, S1PTW = 0
>> [   55.692418]   FSC = 0x04: level 0 translation fault
>> [   55.697449] Data abort info:
>> [   55.700426]   ISV = 0, ISS = 0x00000004
>> [   55.704383]   CM = 0, WnR = 0
>> [   55.707455] swapper pgtable: 4k pages, 48-bit VAs,
>> pgdp=00000000a98e9000
>> [   55.714354] [ffffadfba4bb6d81] pgd=0000000000000000,
>> p4d=0000000000000000
>> [   55.721388] Internal error: Oops: 96000004 [#1] SMP
>> [   55.726397] Modules linked in:
>> [   55.729542] CPU: 7 PID: 162 Comm: kworker/7:1H Tainted: G S
>> W         5.17.0-rc6-00100-g04890a1d9672 #724
>> [   55.746066] Workqueue: events_highpri qcom_lmh_dcvs_poll
>> [   55.751527] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS
>> BTYPE=--)
>> [   55.758669] pc : cpufreq_cpu_get_raw+0x20/0x44
>> [   55.763233] lr : qcom_cpufreq_hw_get+0x10/0x64
>> [   55.767799] sp : ffff800009983d10
>> [   55.771207] x29: ffff800009983d10 x28: ffffaa13a4f2b000 x27:
>> ffff7b31329f9305
>> [   55.778530] x26: ffffaa13a4f30af8 x25: ffffaa13a4f4e4c8 x24:
>> ffff7b2ec2eda000
>> [   55.785851] x23: ffffffffffffffea x22: ffff7b2ec2e9fc18 x21:
>> ffff7b2ec2e9fc00
>> [   55.793170] x20: 0000000000000100 x19: ffff7b2ec2e9fcc0 x18:
>> ffffffffffffffff
>> [   55.800490] x17: 726620746e656d75 x16: 6772612064696c61 x15:
>> ffff8000899839c7
>> [   55.807812] x14: 0000000000000000 x13: 0000000000000000 x12:
>> 0000000000000000
>> [   55.815140] x11: ffff7b2ec2e9fc80 x10: ffffaa13a59a1a70 x9 :
>> 0000000000000000
>> [   55.822468] x8 : ffff7b2eca6917c0 x7 : ffffaa13a528b000 x6 :
>> 0000000000000001
>> [   55.829788] x5 : 0000000000040000 x4 : 000000000000024f x3 :
>> 0000000000000000
>> [   55.837116] x2 : 0000000000000100 x1 : ffffaa13a4bb6d80 x0 :
>> 000003e800000001
>> [   55.844439] Call trace:
>> [   55.846951]  cpufreq_cpu_get_raw+0x20/0x44
>> [   55.851155]  qcom_lmh_dcvs_poll+0x104/0x160
>> [   55.855452]  process_one_work+0x288/0x69c
>> [   55.859574]  worker_thread+0x74/0x470
>> [   55.863336]  kthread+0xfc/0x100
>> [   55.866568]  ret_from_fork+0x10/0x20
>> [   55.870246] Code: b00065c1 91360021 d503233f f8625800 (f8616800)
>> [   55.876501] ---[ end trace 0000000000000000 ]---
>>
>> Fixes: 275157b367f4 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt
>> support")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/cpufreq/qcom-cpufreq-hw.c | 24 +++++++++++++++++++++---
>>   1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c
>> b/drivers/cpufreq/qcom-cpufreq-hw.c
>> index 44d46e52baea..7c1bb002e1c3 100644
>> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
>> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
>> @@ -296,6 +296,23 @@ static void qcom_lmh_dcvs_notify(struct
>> qcom_cpufreq_data *data)
>>       struct dev_pm_opp *opp;
>>       unsigned int freq;
>> +    /*
>> +     * Synchronize against CPU going offline.
>> +     * cpufreq_offline() will get the write lock on policy->rwsem.
>> +     */
>> +retry:
>> +    if (unlikely(!down_read_trylock(&policy->rwsem))) {
>> +        mutex_lock(&data->throttle_lock);
>> +        if (data->cancel_throttle) {
>> +            mutex_unlock(&data->throttle_lock);
>> +            return;
>> +        }
>> +
>> +        mutex_unlock(&data->throttle_lock);
>> +
>> +        schedule();
>> +        goto retry;
>> +    }
>
> do you see an option here to base the logic on policy->cpus mask value
> instead of
> the semaphore lock status? I believe it should be possible, and, if so,
> likely
> it would be preferable.

I used semaphore to remove a race with cpufreq_offline() code.
Using policy->cpus seems like a hack.

>
>>       /*
>>        * Get the h/w throttled frequency, normalize it using the
>>        * registered opp table and use it to calculate thermal pressure.
>> @@ -314,9 +331,10 @@ static void qcom_lmh_dcvs_notify(struct
>> qcom_cpufreq_data *data)
>>       /*
>>        * In the unlikely case policy is unregistered do not enable
>> -     * polling or h/w interrupt
>> +     * polling or h/w interrupt.
>> +     * If we are here, we have the policy->rwsem read lock,
>> +     * cancel_throttle can be toggled only with the write lock.
>>        */
>> -    mutex_lock(&data->throttle_lock);
>>       if (data->cancel_throttle)
>>           goto out;
>> @@ -331,7 +349,7 @@ static void qcom_lmh_dcvs_notify(struct
>> qcom_cpufreq_data *data)
>>                    msecs_to_jiffies(10));
>>   out:
>> -    mutex_unlock(&data->throttle_lock);
>> +    up_read(&policy->rwsem);
>>   }
>>   static void qcom_lmh_dcvs_poll(struct work_struct *work)
>>
>
> FWIW I can add my:
>
> Tested-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>
> --
> Best wishes,
> Vladimir

--
With best wishes
Dmitry