[PATCH v2 1/2] drm/panfrost: Add missing OPP table refcnt decremental

[PATCH v2 1/2] drm/panfrost: Add missing OPP table refcnt decremental

[Adrián Larumbe]

Commit f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
retrieves the OPP for the maximum device clock frequency, but forgets to
keep the reference count balanced by putting the returned OPP object. This
eventually leads to an OPP core warning when removing the device.

Fix it by putting OPP objects as many times as they're retrieved. That
includes putting the OPP object in case setting it fails.

Also remove an unnecessary whitespace.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Fixes: f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 2d30da38c2c3..4b8840a41779 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -38,7 +38,7 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
 		return PTR_ERR(opp);
 	dev_pm_opp_put(opp);
 
-	err =  dev_pm_opp_set_rate(dev, *freq);
+	err = dev_pm_opp_set_rate(dev, *freq);
 	if (!err)
 		ptdev->pfdevfreq.current_frequency = *freq;
 
@@ -184,9 +184,12 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 	ret = dev_pm_opp_set_opp(dev, opp);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
+		dev_pm_opp_put(opp);
 		return ret;
 	}
 
+	dev_pm_opp_put(opp);
+
 	/* Find the fastest defined rate  */
 	opp = dev_pm_opp_find_freq_floor(dev, &freq);
 	if (IS_ERR(opp))
-- 
2.46.2

[PATCH v2 2/2] drm/panthor: Fix OPP refcnt leaks in devfreq initialisation

[Adrián Larumbe]

Make sure in case of errors between the first fetch of an OPP in
panthor_devfreq_init and its successive put, the error path decrements its
reference count to avoid OPP object leaks when removing the device.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Fixes: fac9b22df4b1 ("drm/panthor: Add the devfreq logical block")
---
 drivers/gpu/drm/panthor/panthor_devfreq.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
index 9d0f891b9b53..ce0ac4563f65 100644
--- a/drivers/gpu/drm/panthor/panthor_devfreq.c
+++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
@@ -197,7 +197,7 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
 	if (ret && ret != -ENODEV) {
 		if (ret != -EPROBE_DEFER)
 			DRM_DEV_ERROR(dev, "Couldn't retrieve/enable sram supply\n");
-		return ret;
+		goto opp_err;
 	}
 
 	/*
@@ -207,7 +207,7 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
 	ret = dev_pm_opp_set_opp(dev, opp);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
-		return ret;
+		goto opp_err;
 	}
 
 	dev_pm_opp_put(opp);
@@ -242,6 +242,10 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
 		DRM_DEV_INFO(dev, "Failed to register cooling device\n");
 
 	return 0;
+
+opp_err:
+	dev_pm_opp_put(opp);
+	return ret;
 }
 
 int panthor_devfreq_resume(struct panthor_device *ptdev)
-- 
2.46.2

Re: [PATCH v2 2/2] drm/panthor: Fix OPP refcnt leaks in devfreq initialisation

[Steven Price]

On 03/10/2024 14:30, Adrián Larumbe wrote:
> Make sure in case of errors between the first fetch of an OPP in
> panthor_devfreq_init and its successive put, the error path decrements its
> reference count to avoid OPP object leaks when removing the device.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Fixes: fac9b22df4b1 ("drm/panthor: Add the devfreq logical block")

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_devfreq.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
> index 9d0f891b9b53..ce0ac4563f65 100644
> --- a/drivers/gpu/drm/panthor/panthor_devfreq.c
> +++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
> @@ -197,7 +197,7 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
>  	if (ret && ret != -ENODEV) {
>  		if (ret != -EPROBE_DEFER)
>  			DRM_DEV_ERROR(dev, "Couldn't retrieve/enable sram supply\n");
> -		return ret;
> +		goto opp_err;
>  	}
>  
>  	/*
> @@ -207,7 +207,7 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
>  	ret = dev_pm_opp_set_opp(dev, opp);
>  	if (ret) {
>  		DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> -		return ret;
> +		goto opp_err;
>  	}
>  
>  	dev_pm_opp_put(opp);
> @@ -242,6 +242,10 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
>  		DRM_DEV_INFO(dev, "Failed to register cooling device\n");
>  
>  	return 0;
> +
> +opp_err:
> +	dev_pm_opp_put(opp);
> +	return ret;
>  }
>  
>  int panthor_devfreq_resume(struct panthor_device *ptdev)

Re: [PATCH v2 2/2] drm/panthor: Fix OPP refcnt leaks in devfreq initialisation

[Boris Brezillon]

On Thu,  3 Oct 2024 14:30:29 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Make sure in case of errors between the first fetch of an OPP in
> panthor_devfreq_init and its successive put, the error path decrements its
> reference count to avoid OPP object leaks when removing the device.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Fixes: fac9b22df4b1 ("drm/panthor: Add the devfreq logical block")
> ---
>  drivers/gpu/drm/panthor/panthor_devfreq.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
> index 9d0f891b9b53..ce0ac4563f65 100644
> --- a/drivers/gpu/drm/panthor/panthor_devfreq.c
> +++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
> @@ -197,7 +197,7 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
>  	if (ret && ret != -ENODEV) {
>  		if (ret != -EPROBE_DEFER)
>  			DRM_DEV_ERROR(dev, "Couldn't retrieve/enable sram supply\n");
> -		return ret;
> +		goto opp_err;
>  	}
>  
>  	/*
> @@ -207,7 +207,7 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
>  	ret = dev_pm_opp_set_opp(dev, opp);
>  	if (ret) {
>  		DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> -		return ret;
> +		goto opp_err;
>  	}
>  
>  	dev_pm_opp_put(opp);
> @@ -242,6 +242,10 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
>  		DRM_DEV_INFO(dev, "Failed to register cooling device\n");
>  
>  	return 0;
> +
> +opp_err:
> +	dev_pm_opp_put(opp);
> +	return ret;

If you re-order things (see the following diff), you shouldn't need
this error path.

--->8---

diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
index 9d0f891b9b53..4f1a30f29c06 100644
--- a/drivers/gpu/drm/panthor/panthor_devfreq.c
+++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
@@ -163,13 +163,6 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
 
        cur_freq = clk_get_rate(ptdev->clks.core);
 
-       opp = devfreq_recommended_opp(dev, &cur_freq, 0);
-       if (IS_ERR(opp))
-               return PTR_ERR(opp);
-
-       panthor_devfreq_profile.initial_freq = cur_freq;
-       ptdev->current_frequency = cur_freq;
-
        /* Regulator coupling only takes care of synchronizing/balancing voltage
         * updates, but the coupled regulator needs to be enabled manually.
         *
@@ -200,17 +193,24 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
                return ret;
        }
 
+       opp = devfreq_recommended_opp(dev, &cur_freq, 0);
+       if (IS_ERR(opp))
+               return PTR_ERR(opp);
+
+       panthor_devfreq_profile.initial_freq = cur_freq;
+       ptdev->current_frequency = cur_freq;
+
        /*
         * Set the recommend OPP this will enable and configure the regulator
         * if any and will avoid a switch off by regulator_late_cleanup()
         */
        ret = dev_pm_opp_set_opp(dev, opp);
+       dev_pm_opp_put(opp);
        if (ret) {
                DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
                return ret;
        }
 
-       dev_pm_opp_put(opp);
 
        /* Find the fastest defined rate  */
        opp = dev_pm_opp_find_freq_floor(dev, &freq);

Re: [PATCH v2 2/2] drm/panthor: Fix OPP refcnt leaks in devfreq initialisation

[Boris Brezillon]

On Thu,  3 Oct 2024 14:30:29 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Make sure in case of errors between the first fetch of an OPP in
> panthor_devfreq_init and its successive put, the error path decrements its
> reference count to avoid OPP object leaks when removing the device.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Fixes: fac9b22df4b1 ("drm/panthor: Add the devfreq logical block")

It doesn't apply on top of drm-misc-fixes. Could you send a v3 based
on drm-misc-fixes please?

> ---
>  drivers/gpu/drm/panthor/panthor_devfreq.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
> index 9d0f891b9b53..ce0ac4563f65 100644
> --- a/drivers/gpu/drm/panthor/panthor_devfreq.c
> +++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
> @@ -197,7 +197,7 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
>  	if (ret && ret != -ENODEV) {
>  		if (ret != -EPROBE_DEFER)
>  			DRM_DEV_ERROR(dev, "Couldn't retrieve/enable sram supply\n");
> -		return ret;
> +		goto opp_err;
>  	}
>  
>  	/*
> @@ -207,7 +207,7 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
>  	ret = dev_pm_opp_set_opp(dev, opp);
>  	if (ret) {
>  		DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> -		return ret;
> +		goto opp_err;
>  	}
>  
>  	dev_pm_opp_put(opp);
> @@ -242,6 +242,10 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
>  		DRM_DEV_INFO(dev, "Failed to register cooling device\n");
>  
>  	return 0;
> +
> +opp_err:
> +	dev_pm_opp_put(opp);
> +	return ret;
>  }
>  
>  int panthor_devfreq_resume(struct panthor_device *ptdev)

Re: [PATCH v2 2/2] drm/panthor: Fix OPP refcnt leaks in devfreq initialisation

[Boris Brezillon]

On Wed, 30 Oct 2024 16:13:58 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Thu,  3 Oct 2024 14:30:29 +0100
> Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> 
> > Make sure in case of errors between the first fetch of an OPP in
> > panthor_devfreq_init and its successive put, the error path decrements its
> > reference count to avoid OPP object leaks when removing the device.
> > 
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > Fixes: fac9b22df4b1 ("drm/panthor: Add the devfreq logical block")  
> 
> It doesn't apply on top of drm-misc-fixes. Could you send a v3 based
> on drm-misc-fixes please?

Sorry, I meant v4. v3 exists, but doesn't apply correctly on
drm-misc-fixes.

> 
> > ---
> >  drivers/gpu/drm/panthor/panthor_devfreq.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_devfreq.c b/drivers/gpu/drm/panthor/panthor_devfreq.c
> > index 9d0f891b9b53..ce0ac4563f65 100644
> > --- a/drivers/gpu/drm/panthor/panthor_devfreq.c
> > +++ b/drivers/gpu/drm/panthor/panthor_devfreq.c
> > @@ -197,7 +197,7 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
> >  	if (ret && ret != -ENODEV) {
> >  		if (ret != -EPROBE_DEFER)
> >  			DRM_DEV_ERROR(dev, "Couldn't retrieve/enable sram supply\n");
> > -		return ret;
> > +		goto opp_err;
> >  	}
> >  
> >  	/*
> > @@ -207,7 +207,7 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
> >  	ret = dev_pm_opp_set_opp(dev, opp);
> >  	if (ret) {
> >  		DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> > -		return ret;
> > +		goto opp_err;
> >  	}
> >  
> >  	dev_pm_opp_put(opp);
> > @@ -242,6 +242,10 @@ int panthor_devfreq_init(struct panthor_device *ptdev)
> >  		DRM_DEV_INFO(dev, "Failed to register cooling device\n");
> >  
> >  	return 0;
> > +
> > +opp_err:
> > +	dev_pm_opp_put(opp);
> > +	return ret;
> >  }
> >  
> >  int panthor_devfreq_resume(struct panthor_device *ptdev)  
> 

Re: [PATCH v2 1/2] drm/panfrost: Add missing OPP table refcnt decremental

[Steven Price]

On 03/10/2024 14:30, Adrián Larumbe wrote:
> Commit f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
> retrieves the OPP for the maximum device clock frequency, but forgets to
> keep the reference count balanced by putting the returned OPP object. This
> eventually leads to an OPP core warning when removing the device.
> 
> Fix it by putting OPP objects as many times as they're retrieved. That
> includes putting the OPP object in case setting it fails.
> 
> Also remove an unnecessary whitespace.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Fixes: f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 2d30da38c2c3..4b8840a41779 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -38,7 +38,7 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
>  		return PTR_ERR(opp);
>  	dev_pm_opp_put(opp);
>  
> -	err =  dev_pm_opp_set_rate(dev, *freq);
> +	err = dev_pm_opp_set_rate(dev, *freq);
>  	if (!err)
>  		ptdev->pfdevfreq.current_frequency = *freq;
>  
> @@ -184,9 +184,12 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>  	ret = dev_pm_opp_set_opp(dev, opp);
>  	if (ret) {
>  		DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> +		dev_pm_opp_put(opp);
>  		return ret;
>  	}
>  
> +	dev_pm_opp_put(opp);
> +
>  	/* Find the fastest defined rate  */
>  	opp = dev_pm_opp_find_freq_floor(dev, &freq);
>  	if (IS_ERR(opp))

Re: [PATCH v2 1/2] drm/panfrost: Add missing OPP table refcnt decremental

[Boris Brezillon]

On Thu,  3 Oct 2024 14:30:28 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Commit f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
> retrieves the OPP for the maximum device clock frequency, but forgets to
> keep the reference count balanced by putting the returned OPP object. This
> eventually leads to an OPP core warning when removing the device.
> 
> Fix it by putting OPP objects as many times as they're retrieved. That
> includes putting the OPP object in case setting it fails.
> 
> Also remove an unnecessary whitespace.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Fixes: f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
> ---
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 2d30da38c2c3..4b8840a41779 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -38,7 +38,7 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
>  		return PTR_ERR(opp);
>  	dev_pm_opp_put(opp);
>  
> -	err =  dev_pm_opp_set_rate(dev, *freq);
> +	err = dev_pm_opp_set_rate(dev, *freq);
>  	if (!err)
>  		ptdev->pfdevfreq.current_frequency = *freq;
>  
> @@ -184,9 +184,12 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>  	ret = dev_pm_opp_set_opp(dev, opp);

Put the dev_pm_opp_put(opp) here, and you can drop the one in the error
path.

>  	if (ret) {
>  		DRM_DEV_ERROR(dev, "Couldn't set recommended OPP\n");
> +		dev_pm_opp_put(opp);
>  		return ret;
>  	}
>  
> +	dev_pm_opp_put(opp);
> +
>  	/* Find the fastest defined rate  */
>  	opp = dev_pm_opp_find_freq_floor(dev, &freq);
>  	if (IS_ERR(opp))