linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Viresh Kumar <viresh.kumar@linaro.org>
To: Thara Gopinath <thara.gopinath@linaro.org>
Cc: agross@kernel.org, bjorn.andersson@linaro.org,
	rui.zhang@intel.com, daniel.lezcano@linaro.org,
	rjw@rjwysocki.net, robh+dt@kernel.org, tdas@codeaurora.org,
	mka@chromium.org, linux-arm-msm@vger.kernel.org,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support
Date: Mon, 12 Jul 2021 10:11:12 +0530	[thread overview]
Message-ID: <20210712044112.svhlagrktcfvyj35@vireshk-i7> (raw)
In-Reply-To: <5a98ef2a-d170-f52d-cc48-b838cddaa5c2@linaro.org>

On 09-07-21, 11:37, Thara Gopinath wrote:
> On 7/9/21 2:46 AM, Viresh Kumar wrote:
> > > @@ -389,6 +503,10 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
> > >   	dev_pm_opp_remove_all_dynamic(cpu_dev);
> > >   	dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
> > > +	if (data->lmh_dcvs_irq > 0) {
> > > +		devm_free_irq(cpu_dev, data->lmh_dcvs_irq, data);
> > 
> > Why using devm variants here and while requesting the irq ?

Missed this one ?

> > 
> > > +		cancel_delayed_work_sync(&data->lmh_dcvs_poll_work);
> > > +	}
> > 
> > Please move this to qcom_cpufreq_hw_lmh_exit() or something.
> 
> Ok.
> 
> > 
> > Now with sequence of disabling interrupt, etc, I see a potential
> > problem.
> > 
> > CPU0                                    CPU1
> > 
> > qcom_cpufreq_hw_cpu_exit()
> > -> devm_free_irq();
> >                                          qcom_lmh_dcvs_poll()
> >                                          -> qcom_lmh_dcvs_notify()
> >                                            -> enable_irq()
> > 
> > -> cancel_delayed_work_sync();
> > 
> > 
> > What will happen if enable_irq() gets called after freeing the irq ?
> > Not sure, but it looks like you will hit this then from manage.c:
> > 
> > WARN(!desc->irq_data.chip, KERN_ERR "enable_irq before
> >                                       setup/request_irq: irq %u\n", irq))
> > 
> > ?
> > 
> > You got a chicken n egg problem :)
> 
> Yes indeed! But also it is a very rare chicken and egg problem.
> The scenario here is that the cpus are busy and running load causing a
> thermal overrun and lmh is engaged. At the same time for this issue to be
> hit the cpu is trying to exit/disable cpufreq.

Yes, it is a very specific case but it needs to be resolved anyway. You don't
want to get this ever :)

> Calling
> cancel_delayed_work_sync first could solve this issue, right ?
> cancel_delayed_work_sync guarantees the work not to be pending even if
> it requeues itself on return. So once the delayed work is cancelled, the
> interrupts can be safely disabled. Thoughts ?

I don't think even that would provide such guarantees to you here, as there is
a chance the work gets queued again because of an interrupt that triggers right
after you cancel the work.

The basic way of solving such issues is that once you cancel something, you need
to guarantee that it doesn't get triggered again, no matter what.

The problem here I see is with your design itself, both delayed work and irq can
enable each other, so no matter which one you disable first, won't be
sufficient. You need to fix that design somehow.

-- 
viresh

  parent reply	other threads:[~2021-07-12  4:41 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-08 12:06 [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Thara Gopinath
2021-07-08 12:06 ` [Patch v3 1/6] firmware: qcom_scm: Introduce SCM calls to access LMh Thara Gopinath
2021-07-10  4:02   ` Bjorn Andersson
2021-07-08 12:06 ` [Patch v3 2/6] thermal: qcom: Add support for LMh driver Thara Gopinath
2021-07-10  4:15   ` Bjorn Andersson
2021-07-13  0:49     ` Thara Gopinath
2021-07-08 12:06 ` [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support Thara Gopinath
2021-07-08 20:00   ` kernel test robot
2021-07-09  6:46   ` Viresh Kumar
2021-07-09 15:37     ` Thara Gopinath
2021-07-12  4:35       ` Viresh Kumar
2021-07-12  4:41       ` Viresh Kumar [this message]
2021-07-13  1:18         ` Thara Gopinath
2021-07-13  3:18           ` Viresh Kumar
2021-07-14 12:37             ` Thara Gopinath
2021-07-10  4:57   ` Bjorn Andersson
2021-07-13  1:09     ` Thara Gopinath
2021-07-08 12:06 ` [Patch v3 4/6] arm64: boot: dts: qcom: sdm45: Add support for LMh node Thara Gopinath
2021-07-10  4:17   ` Bjorn Andersson
2021-07-19 16:33   ` Bjorn Andersson
2021-07-19 22:44     ` Thara Gopinath
2021-07-08 12:06 ` [Patch v3 5/6] arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices for CPU thermal zones Thara Gopinath
2021-07-10  4:17   ` Bjorn Andersson
2021-07-08 12:06 ` [Patch v3 6/6] dt-bindings: thermal: Add dt binding for QCOM LMh Thara Gopinath
2021-07-10  4:21   ` Bjorn Andersson
2021-07-13  0:54     ` Thara Gopinath
2021-07-12 17:32   ` Rob Herring
2021-07-22  3:14 ` [Patch v3 0/6] Introduce LMh driver for Qualcomm SoCs Steev Klimaszewski
2021-07-27 15:29   ` Thara Gopinath
2021-07-27 17:43     ` Steev Klimaszewski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210712044112.svhlagrktcfvyj35@vireshk-i7 \
    --to=viresh.kumar@linaro.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=daniel.lezcano@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mka@chromium.org \
    --cc=rjw@rjwysocki.net \
    --cc=robh+dt@kernel.org \
    --cc=rui.zhang@intel.com \
    --cc=tdas@codeaurora.org \
    --cc=thara.gopinath@linaro.org \
    --subject='Re: [Patch v3 3/6] cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).