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

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>
>
> --
> Best wishes,
> Vladimir


--
With best wishes
Dmitry