diff mbox series

[v2,2/2] cpufreq: qcom-hw: Delay enabling throttle_irq

Message ID 20220128032554.155132-2-bjorn.andersson@linaro.org
State Accepted
Commit ef8ee1cb8fc8976a68f5e89cd5f7b6f7de80c66f
Headers show
Series [v2,1/2] cpufreq: Reintroduce ready() callback | expand

Commit Message

Bjorn Andersson Jan. 28, 2022, 3:25 a.m. UTC
In the event that the SoC is under thermal pressure while booting it's
possible for the dcvs notification to happen inbetween the cpufreq
framework calling init and it actually updating the policy's
related_cpus cpumask.

Prior to the introduction of the thermal pressure update helper an empty
cpumask would simply result in the thermal pressure of no cpus being
updated, but the new code will attempt to dereference an invalid per_cpu
variable.

Avoid this problem by using the newly reintroduced "ready" callback, to
postpone enabling the IRQ until the related_cpus cpumask is filled in.

Fixes: 0258cb19c77d ("cpufreq: qcom-cpufreq-hw: Use new thermal pressure update function")
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v2:
- Switched back to applying thermal pressure on "related_cpus", as
  "policy->cpus" is adjusted based on CPU hotplug.
- Reintroduced "ready" callback (in patch 1), as the maintainers of
  topology_update_thermal_pressure() where not interested in allowing a cpumask
  of 0.

 drivers/cpufreq/qcom-cpufreq-hw.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Lukasz Luba Jan. 28, 2022, 10:39 a.m. UTC | #1
On 1/28/22 3:25 AM, Bjorn Andersson wrote:
> In the event that the SoC is under thermal pressure while booting it's
> possible for the dcvs notification to happen inbetween the cpufreq
> framework calling init and it actually updating the policy's
> related_cpus cpumask.
> 
> Prior to the introduction of the thermal pressure update helper an empty
> cpumask would simply result in the thermal pressure of no cpus being
> updated, but the new code will attempt to dereference an invalid per_cpu
> variable.

Just to confirm, is that per-cpu var the 'policy->related_cpus' in this
driver?

> 
> Avoid this problem by using the newly reintroduced "ready" callback, to
> postpone enabling the IRQ until the related_cpus cpumask is filled in.
> 
> Fixes: 0258cb19c77d ("cpufreq: qcom-cpufreq-hw: Use new thermal pressure update function")

You have 'Fixes' tagging here, which might be picked by the stable tree.
The code uses the reverted callback .ready(), which might be missing
there (since patch 1/2 doesn't have tagging). This patch looks like a
proper fix for the root cause.

Anyway, I'm going to send a patch, which adds a check for null cpumask
in the topology_update_thermal_pressure()
It was removed after the review comments:
https://lore.kernel.org/linux-pm/20211028054459.dve6s2my2tq7odem@vireshk-i7/

I'll also push that change for the stable tree.

Regards,
Lukasz
Bjorn Andersson Jan. 28, 2022, 6:30 p.m. UTC | #2
On Fri 28 Jan 02:39 PST 2022, Lukasz Luba wrote:

> 
> 
> On 1/28/22 3:25 AM, Bjorn Andersson wrote:
> > In the event that the SoC is under thermal pressure while booting it's
> > possible for the dcvs notification to happen inbetween the cpufreq
> > framework calling init and it actually updating the policy's
> > related_cpus cpumask.
> > 
> > Prior to the introduction of the thermal pressure update helper an empty
> > cpumask would simply result in the thermal pressure of no cpus being
> > updated, but the new code will attempt to dereference an invalid per_cpu
> > variable.
> 
> Just to confirm, is that per-cpu var the 'policy->related_cpus' in this
> driver?
> 

Correct, we boot under thermal pressure, so the interrupt fires before
we return from "init", which means that related_cpus is still 0.

> > 
> > Avoid this problem by using the newly reintroduced "ready" callback, to
> > postpone enabling the IRQ until the related_cpus cpumask is filled in.
> > 
> > Fixes: 0258cb19c77d ("cpufreq: qcom-cpufreq-hw: Use new thermal pressure update function")
> 
> You have 'Fixes' tagging here, which might be picked by the stable tree.
> The code uses the reverted callback .ready(), which might be missing
> there (since patch 1/2 doesn't have tagging). This patch looks like a
> proper fix for the root cause.
> 

Yes, the pair would need to be picked up.

> Anyway, I'm going to send a patch, which adds a check for null cpumask
> in the topology_update_thermal_pressure()
> It was removed after the review comments:
> https://lore.kernel.org/linux-pm/20211028054459.dve6s2my2tq7odem@vireshk-i7/
> 

I attempted that in v1:
https://lore.kernel.org/all/20220118185612.2067031-2-bjorn.andersson@linaro.org/

And while patch 1 is broken, I think Greg and Sudeep made it clear that
they didn't want a condition to guard against the caller passing cpus of
0.

That's why I in v2 reverted to postpone the thermal pressure IRQ until
cpufreq is "ready".

Regards,
Bjorn

> I'll also push that change for the stable tree.
> 
> Regards,
> Lukasz
Lukasz Luba Jan. 31, 2022, 8:59 a.m. UTC | #3
On 1/28/22 6:30 PM, Bjorn Andersson wrote:
> On Fri 28 Jan 02:39 PST 2022, Lukasz Luba wrote:
> 
>>
>>
>> On 1/28/22 3:25 AM, Bjorn Andersson wrote:
>>> In the event that the SoC is under thermal pressure while booting it's
>>> possible for the dcvs notification to happen inbetween the cpufreq
>>> framework calling init and it actually updating the policy's
>>> related_cpus cpumask.
>>>
>>> Prior to the introduction of the thermal pressure update helper an empty
>>> cpumask would simply result in the thermal pressure of no cpus being
>>> updated, but the new code will attempt to dereference an invalid per_cpu
>>> variable.
>>
>> Just to confirm, is that per-cpu var the 'policy->related_cpus' in this
>> driver?
>>
> 
> Correct, we boot under thermal pressure, so the interrupt fires before
> we return from "init", which means that related_cpus is still 0.
> 
>>>
>>> Avoid this problem by using the newly reintroduced "ready" callback, to
>>> postpone enabling the IRQ until the related_cpus cpumask is filled in.
>>>
>>> Fixes: 0258cb19c77d ("cpufreq: qcom-cpufreq-hw: Use new thermal pressure update function")
>>
>> You have 'Fixes' tagging here, which might be picked by the stable tree.
>> The code uses the reverted callback .ready(), which might be missing
>> there (since patch 1/2 doesn't have tagging). This patch looks like a
>> proper fix for the root cause.
>>
> 
> Yes, the pair would need to be picked up.
> 
>> Anyway, I'm going to send a patch, which adds a check for null cpumask
>> in the topology_update_thermal_pressure()
>> It was removed after the review comments:
>> https://lore.kernel.org/linux-pm/20211028054459.dve6s2my2tq7odem@vireshk-i7/
>>
> 
> I attempted that in v1:
> https://lore.kernel.org/all/20220118185612.2067031-2-bjorn.andersson@linaro.org/
> 
> And while patch 1 is broken, I think Greg and Sudeep made it clear that
> they didn't want a condition to guard against the caller passing cpus of
> 0.

Thanks for the link, I missed that conversation.

> 
> That's why I in v2 reverted to postpone the thermal pressure IRQ until
> cpufreq is "ready".

Which is fixing the root cause, but involves this backporting
of the new API callback into stable.

Sorry to hear that you had to fight with this tricky mem fault.
There is a 'good' outcome from this: we know that the platform
instantly has thermal issues during boot.

Regards,
Lukasz
Viresh Kumar Feb. 9, 2022, 7:38 a.m. UTC | #4
On 28-01-22, 10:30, Bjorn Andersson wrote:
> On Fri 28 Jan 02:39 PST 2022, Lukasz Luba wrote:
> > On 1/28/22 3:25 AM, Bjorn Andersson wrote:
> > > In the event that the SoC is under thermal pressure while booting it's
> > > possible for the dcvs notification to happen inbetween the cpufreq
> > > framework calling init and it actually updating the policy's
> > > related_cpus cpumask.
> > > 
> > > Prior to the introduction of the thermal pressure update helper an empty
> > > cpumask would simply result in the thermal pressure of no cpus being
> > > updated, but the new code will attempt to dereference an invalid per_cpu
> > > variable.
> > 
> > Just to confirm, is that per-cpu var the 'policy->related_cpus' in this
> > driver?
> > 
> 
> Correct, we boot under thermal pressure, so the interrupt fires before
> we return from "init", which means that related_cpus is still 0.

Just to clarify here a bit, policy->related_cpus is already allocated at this
point of time. AFAICT, the dereferencing of the invalid per-cpu variable refers
to the per-cpu freq_factor in arch_topology.c, which happens because the cpumask
isn't initialized yet.
diff mbox series

Patch

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 05f3d7876e44..effbb680b453 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -388,7 +388,7 @@  static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
 
 	snprintf(data->irq_name, sizeof(data->irq_name), "dcvsh-irq-%u", policy->cpu);
 	ret = request_threaded_irq(data->throttle_irq, NULL, qcom_lmh_dcvs_handle_irq,
-				   IRQF_ONESHOT, data->irq_name, data);
+				   IRQF_ONESHOT | IRQF_NO_AUTOEN, data->irq_name, data);
 	if (ret) {
 		dev_err(&pdev->dev, "Error registering %s: %d\n", data->irq_name, ret);
 		return 0;
@@ -542,6 +542,14 @@  static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
 	return 0;
 }
 
+static void qcom_cpufreq_ready(struct cpufreq_policy *policy)
+{
+	struct qcom_cpufreq_data *data = policy->driver_data;
+
+	if (data->throttle_irq >= 0)
+		enable_irq(data->throttle_irq);
+}
+
 static struct freq_attr *qcom_cpufreq_hw_attr[] = {
 	&cpufreq_freq_attr_scaling_available_freqs,
 	&cpufreq_freq_attr_scaling_boost_freqs,
@@ -561,6 +569,7 @@  static struct cpufreq_driver cpufreq_qcom_hw_driver = {
 	.fast_switch    = qcom_cpufreq_hw_fast_switch,
 	.name		= "qcom-cpufreq-hw",
 	.attr		= qcom_cpufreq_hw_attr,
+	.ready		= qcom_cpufreq_ready,
 };
 
 static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)