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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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