[PATCH 0/3] cpufreq: qcom-hw: a few fixes in qcom cpufreq driver

[PATCH 0/3] cpufreq: qcom-hw: a few fixes in qcom cpufreq driver

[Vladimir Zapolskiy]

I find it essential to resend a fix from Ard and also add two more
lesser fixes to the set, review and comments are more than welcome.

Ard Biesheuvel (1):
  cpufreq: qcom-cpufreq-hw: Avoid stack buffer for IRQ name

Vladimir Zapolskiy (2):
  cpufreq: qcom-hw: Fix probable nested interrupt handling
  cpufreq: qcom-hw: Set CPU affinity of dcvsh interrupts

 drivers/cpufreq/qcom-cpufreq-hw.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

-- 
2.32.0

[PATCH 1/3][RESEND] cpufreq: qcom-cpufreq-hw: Avoid stack buffer for IRQ name

[Vladimir Zapolskiy]

From: Ard Biesheuvel <ardb@kernel.org>

Registering an IRQ requires the string buffer containing the name to
remain allocated, as the name is not copied into another buffer.

So let's add a irq_name field to the data struct instead, which is
guaranteed to have the appropriate lifetime.

Cc: Thara Gopinath <thara.gopinath@linaro.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Andy Gross <agross@kernel.org>
Cc: linux-arm-msm@vger.kernel.org
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Reviewed-by: Thara Gopinath <thara.gopinath@linaro.org>
Tested-by: Steev Klimaszewski <steev@kali.org>
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
vzapolskiy: rebased, added all collected tags and resend the change from Ard

 drivers/cpufreq/qcom-cpufreq-hw.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index a2be0df7e174..3b5835336658 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -46,6 +46,7 @@ struct qcom_cpufreq_data {
 	 */
 	struct mutex throttle_lock;
 	int throttle_irq;
+	char irq_name[15];
 	bool cancel_throttle;
 	struct delayed_work throttle_work;
 	struct cpufreq_policy *policy;
@@ -375,7 +376,6 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
 {
 	struct qcom_cpufreq_data *data = policy->driver_data;
 	struct platform_device *pdev = cpufreq_get_driver_data();
-	char irq_name[15];
 	int ret;
 
 	/*
@@ -392,11 +392,11 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
 	mutex_init(&data->throttle_lock);
 	INIT_DEFERRABLE_WORK(&data->throttle_work, qcom_lmh_dcvs_poll);
 
-	snprintf(irq_name, sizeof(irq_name), "dcvsh-irq-%u", policy->cpu);
+	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, irq_name, data);
+				   IRQF_ONESHOT, data->irq_name, data);
 	if (ret) {
-		dev_err(&pdev->dev, "Error registering %s: %d\n", irq_name, ret);
+		dev_err(&pdev->dev, "Error registering %s: %d\n", data->irq_name, ret);
 		return 0;
 	}
 
-- 
2.32.0

Re: [PATCH 1/3][RESEND] cpufreq: qcom-cpufreq-hw: Avoid stack buffer for IRQ name

[Matthias Kaehlcke]

On Thu, Nov 11, 2021 at 05:48:06PM +0200, Vladimir Zapolskiy wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Registering an IRQ requires the string buffer containing the name to
> remain allocated, as the name is not copied into another buffer.
> 
> So let's add a irq_name field to the data struct instead, which is
> guaranteed to have the appropriate lifetime.
> 
> Cc: Thara Gopinath <thara.gopinath@linaro.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Andy Gross <agross@kernel.org>
> Cc: linux-arm-msm@vger.kernel.org
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> Reviewed-by: Thara Gopinath <thara.gopinath@linaro.org>
> Tested-by: Steev Klimaszewski <steev@kali.org>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 1/3][RESEND] cpufreq: qcom-cpufreq-hw: Avoid stack buffer for IRQ name

[Dmitry Baryshkov]

On 11/11/2021 18:48, Vladimir Zapolskiy wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Registering an IRQ requires the string buffer containing the name to
> remain allocated, as the name is not copied into another buffer.
> 
> So let's add a irq_name field to the data struct instead, which is
> guaranteed to have the appropriate lifetime.
> 
> Cc: Thara Gopinath <thara.gopinath@linaro.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Andy Gross <agross@kernel.org>
> Cc: linux-arm-msm@vger.kernel.org
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> Reviewed-by: Thara Gopinath <thara.gopinath@linaro.org>
> Tested-by: Steev Klimaszewski <steev@kali.org>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
> vzapolskiy: rebased, added all collected tags and resend the change from Ard
> 
>   drivers/cpufreq/qcom-cpufreq-hw.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index a2be0df7e174..3b5835336658 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -46,6 +46,7 @@ struct qcom_cpufreq_data {
>   	 */
>   	struct mutex throttle_lock;
>   	int throttle_irq;
> +	char irq_name[15];
>   	bool cancel_throttle;
>   	struct delayed_work throttle_work;
>   	struct cpufreq_policy *policy;
> @@ -375,7 +376,6 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
>   {
>   	struct qcom_cpufreq_data *data = policy->driver_data;
>   	struct platform_device *pdev = cpufreq_get_driver_data();
> -	char irq_name[15];
>   	int ret;
>   
>   	/*
> @@ -392,11 +392,11 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
>   	mutex_init(&data->throttle_lock);
>   	INIT_DEFERRABLE_WORK(&data->throttle_work, qcom_lmh_dcvs_poll);
>   
> -	snprintf(irq_name, sizeof(irq_name), "dcvsh-irq-%u", policy->cpu);
> +	snprintf(data->irq_name, sizeof(data->irq_name), "dcvsh-irq-%u", policy->cpu);

It might be easier to use devm_kasprintf() here.

>   	ret = request_threaded_irq(data->throttle_irq, NULL, qcom_lmh_dcvs_handle_irq,
> -				   IRQF_ONESHOT, irq_name, data);
> +				   IRQF_ONESHOT, data->irq_name, data);
>   	if (ret) {
> -		dev_err(&pdev->dev, "Error registering %s: %d\n", irq_name, ret);
> +		dev_err(&pdev->dev, "Error registering %s: %d\n", data->irq_name, ret);
>   		return 0;
>   	}
>   
> 


-- 
With best wishes
Dmitry

Re: [PATCH 1/3][RESEND] cpufreq: qcom-cpufreq-hw: Avoid stack buffer for IRQ name

[Bjorn Andersson]

On Fri 12 Nov 14:16 CST 2021, Dmitry Baryshkov wrote:

> On 11/11/2021 18:48, Vladimir Zapolskiy wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> > 
> > Registering an IRQ requires the string buffer containing the name to
> > remain allocated, as the name is not copied into another buffer.
> > 
> > So let's add a irq_name field to the data struct instead, which is
> > guaranteed to have the appropriate lifetime.
> > 
> > Cc: Thara Gopinath <thara.gopinath@linaro.org>
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: Andy Gross <agross@kernel.org>
> > Cc: linux-arm-msm@vger.kernel.org
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > Reviewed-by: Thara Gopinath <thara.gopinath@linaro.org>
> > Tested-by: Steev Klimaszewski <steev@kali.org>
> > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> > ---
> > vzapolskiy: rebased, added all collected tags and resend the change from Ard
> > 
> >   drivers/cpufreq/qcom-cpufreq-hw.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > index a2be0df7e174..3b5835336658 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > @@ -46,6 +46,7 @@ struct qcom_cpufreq_data {
> >   	 */
> >   	struct mutex throttle_lock;
> >   	int throttle_irq;
> > +	char irq_name[15];
> >   	bool cancel_throttle;
> >   	struct delayed_work throttle_work;
> >   	struct cpufreq_policy *policy;
> > @@ -375,7 +376,6 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
> >   {
> >   	struct qcom_cpufreq_data *data = policy->driver_data;
> >   	struct platform_device *pdev = cpufreq_get_driver_data();
> > -	char irq_name[15];
> >   	int ret;
> >   	/*
> > @@ -392,11 +392,11 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
> >   	mutex_init(&data->throttle_lock);
> >   	INIT_DEFERRABLE_WORK(&data->throttle_work, qcom_lmh_dcvs_poll);
> > -	snprintf(irq_name, sizeof(irq_name), "dcvsh-irq-%u", policy->cpu);
> > +	snprintf(data->irq_name, sizeof(data->irq_name), "dcvsh-irq-%u", policy->cpu);
> 
> It might be easier to use devm_kasprintf() here.
> 

Yes, in itself that would be nice. But this function might be called > 1
times per device, in which case we would end up with N unused copies of
the string on the heap, until the device is removed.

I'm working on some patches where these things are allocated at probe
time, with those in place it makes more sense to just devm_kasprintf()
this and "forget" about the pointer.

Regards,
Bjorn

> >   	ret = request_threaded_irq(data->throttle_irq, NULL, qcom_lmh_dcvs_handle_irq,
> > -				   IRQF_ONESHOT, irq_name, data);
> > +				   IRQF_ONESHOT, data->irq_name, data);
> >   	if (ret) {
> > -		dev_err(&pdev->dev, "Error registering %s: %d\n", irq_name, ret);
> > +		dev_err(&pdev->dev, "Error registering %s: %d\n", data->irq_name, ret);
> >   		return 0;
> >   	}
> > 
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [PATCH 1/3][RESEND] cpufreq: qcom-cpufreq-hw: Avoid stack buffer for IRQ name

[Bjorn Andersson]

On Thu 11 Nov 09:48 CST 2021, Vladimir Zapolskiy wrote:

> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Registering an IRQ requires the string buffer containing the name to
> remain allocated, as the name is not copied into another buffer.
> 
> So let's add a irq_name field to the data struct instead, which is
> guaranteed to have the appropriate lifetime.
> 
> Cc: Thara Gopinath <thara.gopinath@linaro.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Andy Gross <agross@kernel.org>
> Cc: linux-arm-msm@vger.kernel.org
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> Reviewed-by: Thara Gopinath <thara.gopinath@linaro.org>
> Tested-by: Steev Klimaszewski <steev@kali.org>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

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

Thanks for reposting this Vladimir.

Regards,
Bjorn

> ---
> vzapolskiy: rebased, added all collected tags and resend the change from Ard
> 
>  drivers/cpufreq/qcom-cpufreq-hw.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index a2be0df7e174..3b5835336658 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -46,6 +46,7 @@ struct qcom_cpufreq_data {
>  	 */
>  	struct mutex throttle_lock;
>  	int throttle_irq;
> +	char irq_name[15];
>  	bool cancel_throttle;
>  	struct delayed_work throttle_work;
>  	struct cpufreq_policy *policy;
> @@ -375,7 +376,6 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
>  {
>  	struct qcom_cpufreq_data *data = policy->driver_data;
>  	struct platform_device *pdev = cpufreq_get_driver_data();
> -	char irq_name[15];
>  	int ret;
>  
>  	/*
> @@ -392,11 +392,11 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
>  	mutex_init(&data->throttle_lock);
>  	INIT_DEFERRABLE_WORK(&data->throttle_work, qcom_lmh_dcvs_poll);
>  
> -	snprintf(irq_name, sizeof(irq_name), "dcvsh-irq-%u", policy->cpu);
> +	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, irq_name, data);
> +				   IRQF_ONESHOT, data->irq_name, data);
>  	if (ret) {
> -		dev_err(&pdev->dev, "Error registering %s: %d\n", irq_name, ret);
> +		dev_err(&pdev->dev, "Error registering %s: %d\n", data->irq_name, ret);
>  		return 0;
>  	}
>  
> -- 
> 2.32.0
> 

[PATCH 2/3] cpufreq: qcom-hw: Fix probable nested interrupt handling

[Vladimir Zapolskiy]

Re-enabling an interrupt from its own interrupt handler may cause
an interrupt storm, if there is a pending interrupt and because its
handling is disabled due to already done entrance into the handler
above in the stack.

Also, apparently it is improper to lock a mutex in an interrupt contex.

Fixes: 275157b367f4 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 3b5835336658..5d55217caa8b 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -343,9 +343,9 @@ static irqreturn_t qcom_lmh_dcvs_handle_irq(int irq, void *data)
 
 	/* Disable interrupt and enable polling */
 	disable_irq_nosync(c_data->throttle_irq);
-	qcom_lmh_dcvs_notify(c_data);
+	schedule_delayed_work(&c_data->throttle_work, 0);
 
-	return 0;
+	return IRQ_HANDLED;
 }
 
 static const struct qcom_cpufreq_soc_data qcom_soc_data = {
-- 
2.32.0

Re: [PATCH 2/3] cpufreq: qcom-hw: Fix probable nested interrupt handling

[Matthias Kaehlcke]

On Thu, Nov 11, 2021 at 05:48:07PM +0200, Vladimir Zapolskiy wrote:
> Re-enabling an interrupt from its own interrupt handler may cause
> an interrupt storm, if there is a pending interrupt and because its
> handling is disabled due to already done entrance into the handler
> above in the stack.
> 
> Also, apparently it is improper to lock a mutex in an interrupt contex.
> 
> Fixes: 275157b367f4 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 2/3] cpufreq: qcom-hw: Fix probable nested interrupt handling

[Bjorn Andersson]

On Thu 11 Nov 09:48 CST 2021, Vladimir Zapolskiy wrote:

> Re-enabling an interrupt from its own interrupt handler may cause
> an interrupt storm, if there is a pending interrupt and because its
> handling is disabled due to already done entrance into the handler
> above in the stack.
> 
> Also, apparently it is improper to lock a mutex in an interrupt contex.

There shouldn't be, given that it's a threaded irq...

As a fix for the immediate problem, the change looks good. But I wonder
if we want to make sure the thermal pressure is updated in the irq
handler still? Perhaps it's not worth the resulting complexity of the
implementation...


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

Regards,
Bjorn

> 
> Fixes: 275157b367f4 ("cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index 3b5835336658..5d55217caa8b 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -343,9 +343,9 @@ static irqreturn_t qcom_lmh_dcvs_handle_irq(int irq, void *data)
>  
>  	/* Disable interrupt and enable polling */
>  	disable_irq_nosync(c_data->throttle_irq);
> -	qcom_lmh_dcvs_notify(c_data);
> +	schedule_delayed_work(&c_data->throttle_work, 0);
>  
> -	return 0;
> +	return IRQ_HANDLED;
>  }
>  
>  static const struct qcom_cpufreq_soc_data qcom_soc_data = {
> -- 
> 2.32.0
> 

[PATCH 3/3] cpufreq: qcom-hw: Set CPU affinity of dcvsh interrupts

[Vladimir Zapolskiy]

In runtime CPU cluster specific dcvsh interrupts may be handled on
unrelated CPU cores, it leads to an issue of too excessive number of
received and handled interrupts, but this is not observed, if CPU
affinity of the interrupt handler is set in accordance to CPU clusters.

The change reduces a number of received interrupts in about 10-100 times.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 5d55217caa8b..3967191836fb 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -400,6 +400,11 @@ static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index)
 		return 0;
 	}
 
+	ret = irq_set_affinity_hint(data->throttle_irq, policy->cpus);
+	if (ret)
+		dev_err(&pdev->dev, "Failed to set CPU affinity of %s[%d]\n",
+			data->irq_name, data->throttle_irq);
+
 	return 0;
 }
 
-- 
2.32.0

Re: [PATCH 3/3] cpufreq: qcom-hw: Set CPU affinity of dcvsh interrupts

[Matthias Kaehlcke]

On Thu, Nov 11, 2021 at 05:48:08PM +0200, Vladimir Zapolskiy wrote:
> In runtime CPU cluster specific dcvsh interrupts may be handled on
> unrelated CPU cores, it leads to an issue of too excessive number of
> received and handled interrupts, but this is not observed, if CPU
> affinity of the interrupt handler is set in accordance to CPU clusters.
> 
> The change reduces a number of received interrupts in about 10-100 times.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

Re: [PATCH 0/3] cpufreq: qcom-hw: a few fixes in qcom cpufreq driver

[Viresh Kumar]

On 11-11-21, 17:48, Vladimir Zapolskiy wrote:
> I find it essential to resend a fix from Ard and also add two more
> lesser fixes to the set, review and comments are more than welcome.
> 
> Ard Biesheuvel (1):
>   cpufreq: qcom-cpufreq-hw: Avoid stack buffer for IRQ name
> 
> Vladimir Zapolskiy (2):
>   cpufreq: qcom-hw: Fix probable nested interrupt handling
>   cpufreq: qcom-hw: Set CPU affinity of dcvsh interrupts
> 
>  drivers/cpufreq/qcom-cpufreq-hw.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)

Applied. Thanks.

-- 
viresh