[PATCH v2 1/2] cpufreq: Reintroduce ready() callback

[PATCH v2 1/2] cpufreq: Reintroduce ready() callback

[Bjorn Andersson]

This effectively revert '4bf8e582119e ("cpufreq: Remove ready()
callback")' (except the Chinese translation), in order to reintroduce
the ready callback.

This is needed in order to be able to leave the thermal pressure
interrupts in the Qualcomm CPUfreq driver disabled during
initialization, so that it doesn't fire while related_cpus are still 0.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v1:
- New patch

 Documentation/cpu-freq/cpu-drivers.rst | 3 +++
 drivers/cpufreq/cpufreq.c              | 4 ++++
 include/linux/cpufreq.h                | 3 +++
 3 files changed, 10 insertions(+)

diff --git a/Documentation/cpu-freq/cpu-drivers.rst b/Documentation/cpu-freq/cpu-drivers.rst
index 3b32336a7803..d84ededb66f9 100644
--- a/Documentation/cpu-freq/cpu-drivers.rst
+++ b/Documentation/cpu-freq/cpu-drivers.rst
@@ -75,6 +75,9 @@ And optionally
  .resume - A pointer to a per-policy resume function which is called
  with interrupts disabled and _before_ the governor is started again.
 
+ .ready - A pointer to a per-policy ready function which is called after
+ the policy is fully initialized.
+
  .attr - A pointer to a NULL-terminated list of "struct freq_attr" which
  allow to export values to sysfs.
 
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index b8d95536ee22..80f535cc8a75 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1518,6 +1518,10 @@ static int cpufreq_online(unsigned int cpu)
 
 	kobject_uevent(&policy->kobj, KOBJ_ADD);
 
+	/* Callback for handling stuff after policy is ready */
+	if (cpufreq_driver->ready)
+		cpufreq_driver->ready(policy);
+
 	if (cpufreq_thermal_control_enabled(cpufreq_driver))
 		policy->cdev = of_cpufreq_cooling_register(policy);
 
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 1ab29e61b078..3522a272b74d 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -382,6 +382,9 @@ struct cpufreq_driver {
 	int		(*suspend)(struct cpufreq_policy *policy);
 	int		(*resume)(struct cpufreq_policy *policy);
 
+	/* Will be called after the driver is fully initialized */
+	void		(*ready)(struct cpufreq_policy *policy);
+
 	struct freq_attr **attr;
 
 	/* platform specific boost support code */
-- 
2.33.1

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

[Bjorn Andersson]

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(-)

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)
-- 
2.33.1

Re: [PATCH v2 1/2] cpufreq: Reintroduce ready() callback

[Lukasz Luba]

Hi Bjorn,

On 1/28/22 3:25 AM, Bjorn Andersson wrote:
> This effectively revert '4bf8e582119e ("cpufreq: Remove ready()
> callback")' (except the Chinese translation), in order to reintroduce

Is there something wrong with the Chinese translation that it has to be
dropped? Someone has put an effort to create it, I'd assume (and also
based on online translator) that it's correct.

> the ready callback.
> 
> This is needed in order to be able to leave the thermal pressure
> interrupts in the Qualcomm CPUfreq driver disabled during
> initialization, so that it doesn't fire while related_cpus are still 0.

If you are going to push the 2nd patch into stable tree, then you would
also need this one.

Regards,
Lukasz

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

[Lukasz Luba]

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

Re: [PATCH v2 1/2] cpufreq: Reintroduce ready() callback

[Bjorn Andersson]

On Fri 28 Jan 00:52 PST 2022, Lukasz Luba wrote:

> Hi Bjorn,
> 
> On 1/28/22 3:25 AM, Bjorn Andersson wrote:
> > This effectively revert '4bf8e582119e ("cpufreq: Remove ready()
> > callback")' (except the Chinese translation), in order to reintroduce
> 
> Is there something wrong with the Chinese translation that it has to be
> dropped? Someone has put an effort to create it, I'd assume (and also
> based on online translator) that it's correct.
> 

I don't expect there to be anything wrong with the Chinese translation,
unfortunately "git revert" trips on a merge conflict and I'm
unfortunately not able to resolve that on my machine.

> > the ready callback.
> > 
> > This is needed in order to be able to leave the thermal pressure
> > interrupts in the Qualcomm CPUfreq driver disabled during
> > initialization, so that it doesn't fire while related_cpus are still 0.
> 
> If you are going to push the 2nd patch into stable tree, then you would
> also need this one.
> 

That's correct. This patch is however not a stable change in itself, so
I didn't mark it as such. I can work with the stable maintainers to let
them know that this patch is needs to go along with patch 2 - although
I've seen cases before where they automagically resolved that.

Regards,
Bjorn

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

[Bjorn Andersson]

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

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

[Lukasz Luba]

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

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

[Viresh Kumar]

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.

-- 
viresh

Re: [PATCH v2 1/2] cpufreq: Reintroduce ready() callback

[Viresh Kumar]

On 27-01-22, 19:25, Bjorn Andersson wrote:
> This effectively revert '4bf8e582119e ("cpufreq: Remove ready()
> callback")' (except the Chinese translation), in order to reintroduce
> the ready callback.

I have added the Chinese translation back and applied both patches. Thanks.

-- 
viresh