[PATCH] drm/nouveau: Fix memleak in nv50_wndw_new_
When nv50_lut_init() fails, *pwndw should be freed
just like when drm_universal_plane_init() fails.
It's the same for the subsequent error paths.
Signed-off-by: Dinghao Liu
---
drivers/gpu/drm/nouveau/dispnv50/wndw.c | 22 --
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
index 0356474ad6f6..47ce1df2ae5f 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
@@ -732,18 +732,15 @@ nv50_wndw_new_(const struct nv50_wndw_func *func, struct
drm_device *dev,
format, nformat,
nouveau_display(dev)->format_modifiers,
type, "%s-%d", name, index);
- if (ret) {
- kfree(*pwndw);
- *pwndw = NULL;
- return ret;
- }
+ if (ret)
+ goto err_free;
drm_plane_helper_add(&wndw->plane, &nv50_wndw_helper);
if (wndw->func->ilut) {
ret = nv50_lut_init(disp, mmu, &wndw->ilut);
if (ret)
- return ret;
+ goto err_free;
}
wndw->notify.func = nv50_wndw_notify;
@@ -752,26 +749,31 @@ nv50_wndw_new_(const struct nv50_wndw_func *func, struct
drm_device *dev,
ret = drm_plane_create_zpos_property(&wndw->plane,
nv50_wndw_zpos_default(&wndw->plane), 0, 254);
if (ret)
- return ret;
+ goto err_free;
ret = drm_plane_create_alpha_property(&wndw->plane);
if (ret)
- return ret;
+ goto err_free;
ret = drm_plane_create_blend_mode_property(&wndw->plane,
BIT(DRM_MODE_BLEND_PIXEL_NONE) |
BIT(DRM_MODE_BLEND_PREMULTI) |
BIT(DRM_MODE_BLEND_COVERAGE));
if (ret)
- return ret;
+ goto err_free;
} else {
ret = drm_plane_create_zpos_immutable_property(&wndw->plane,
nv50_wndw_zpos_default(&wndw->plane));
if (ret)
- return ret;
+ goto err_free;
}
return 0;
+
+err_free:
+ kfree(*pwndw);
+ *pwndw = NULL;
+ return ret;
}
int
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] [v2] drm/omap: Fix runtime PM imbalance on error
pm_runtime_get_sync() increments the runtime PM usage counter
even when it returns an error code. However, users of its
direct wrappers in omapdrm assume that PM usage counter will
not change on error. Thus a pairing decrement is needed on
the error handling path for these wrappers to keep the counter
balanced.
Signed-off-by: Dinghao Liu
---
Changelog:
v2: - Fix 5 additional similar cases in omapdrm.
---
drivers/gpu/drm/omapdrm/dss/dispc.c | 7 +--
drivers/gpu/drm/omapdrm/dss/dsi.c | 7 +--
drivers/gpu/drm/omapdrm/dss/dss.c | 7 +--
drivers/gpu/drm/omapdrm/dss/hdmi4.c | 6 +++---
drivers/gpu/drm/omapdrm/dss/hdmi5.c | 6 +++---
drivers/gpu/drm/omapdrm/dss/venc.c | 7 +--
6 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c
b/drivers/gpu/drm/omapdrm/dss/dispc.c
index 48593932bddf..599183879caf 100644
--- a/drivers/gpu/drm/omapdrm/dss/dispc.c
+++ b/drivers/gpu/drm/omapdrm/dss/dispc.c
@@ -653,8 +653,11 @@ int dispc_runtime_get(struct dispc_device *dispc)
DSSDBG("dispc_runtime_get\n");
r = pm_runtime_get_sync(&dispc->pdev->dev);
- WARN_ON(r < 0);
- return r < 0 ? r : 0;
+ if (WARN_ON(r < 0)) {
+ pm_runtime_put_noidle(&dispc->pdev->dev);
+ return r;
+ }
+ return 0;
}
void dispc_runtime_put(struct dispc_device *dispc)
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c
b/drivers/gpu/drm/omapdrm/dss/dsi.c
index eeccf40bae41..973bfa14a104 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -1112,8 +1112,11 @@ static int dsi_runtime_get(struct dsi_data *dsi)
DSSDBG("dsi_runtime_get\n");
r = pm_runtime_get_sync(dsi->dev);
- WARN_ON(r < 0);
- return r < 0 ? r : 0;
+ if (WARN_ON(r < 0)) {
+ pm_runtime_put_noidle(dsi->dev);
+ return r;
+ }
+ return 0;
}
static void dsi_runtime_put(struct dsi_data *dsi)
diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c
b/drivers/gpu/drm/omapdrm/dss/dss.c
index 6ccbc29c4ce4..d7b2f5bcac16 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss.c
+++ b/drivers/gpu/drm/omapdrm/dss/dss.c
@@ -858,8 +858,11 @@ int dss_runtime_get(struct dss_device *dss)
DSSDBG("dss_runtime_get\n");
r = pm_runtime_get_sync(&dss->pdev->dev);
- WARN_ON(r < 0);
- return r < 0 ? r : 0;
+ if (WARN_ON(r < 0)) {
+ pm_runtime_put_noidle(&dss->pdev->dev);
+ return r;
+ }
+ return 0;
}
void dss_runtime_put(struct dss_device *dss)
diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
index 2578c95570f6..2cc804bdc2e4 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
@@ -44,10 +44,10 @@ static int hdmi_runtime_get(struct omap_hdmi *hdmi)
DSSDBG("hdmi_runtime_get\n");
r = pm_runtime_get_sync(&hdmi->pdev->dev);
- WARN_ON(r < 0);
- if (r < 0)
+ if (WARN_ON(r < 0)) {
+ pm_runtime_put_noidle(&hdmi->pdev->dev);
return r;
-
+ }
return 0;
}
diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
index 4d4c1fabd0a1..92fe05d3da5a 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
@@ -45,10 +45,10 @@ static int hdmi_runtime_get(struct omap_hdmi *hdmi)
DSSDBG("hdmi_runtime_get\n");
r = pm_runtime_get_sync(&hdmi->pdev->dev);
- WARN_ON(r < 0);
- if (r < 0)
+ if (WARN_ON(r < 0)) {
+ pm_runtime_put_noidle(&hdmi->pdev->dev);
return r;
-
+ }
return 0;
}
diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c
b/drivers/gpu/drm/omapdrm/dss/venc.c
index e0817934ee16..4844743fd507 100644
--- a/drivers/gpu/drm/omapdrm/dss/venc.c
+++ b/drivers/gpu/drm/omapdrm/dss/venc.c
@@ -361,8 +361,11 @@ static int venc_runtime_get(struct venc_device *venc)
DSSDBG("venc_runtime_get\n");
r = pm_runtime_get_sync(&venc->pdev->dev);
- WARN_ON(r < 0);
- return r < 0 ? r : 0;
+ if (WARN_ON(r < 0)) {
+ pm_runtime_put_noidle(&venc->pdev->dev);
+ return r;
+ }
+ return 0;
}
static void venc_runtime_put(struct venc_device *venc)
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Re: [PATCH] drm/omap: Fix runtime PM imbalance in dsi_runtime_get
> Hi,
>
> On 21/08/2020 10:45, Dinghao Liu wrote:
> > pm_runtime_get_sync() increments the runtime PM usage counter
> > even when it returns an error code. However, users of
> > dsi_runtime_get(), a direct wrapper of pm_runtime_get_sync(),
> > assume that PM usage counter will not change on error. Thus a
> > pairing decrement is needed on the error handling path to keep
> > the counter balanced.
> >
> > Fixes: 4fbafaf371be7 ("OMAP: DSS2: Use PM runtime & HWMOD support")
> > Signed-off-by: Dinghao Liu
> > ---
> > drivers/gpu/drm/omapdrm/dss/dsi.c | 7 +--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c
> > b/drivers/gpu/drm/omapdrm/dss/dsi.c
> > index eeccf40bae41..973bfa14a104 100644
> > --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> > +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> > @@ -1112,8 +1112,11 @@ static int dsi_runtime_get(struct dsi_data *dsi)
> > DSSDBG("dsi_runtime_get\n");
> >
> > r = pm_runtime_get_sync(dsi->dev);
> > - WARN_ON(r < 0);
> > - return r < 0 ? r : 0;
> > + if (WARN_ON(r < 0)) {
> > + pm_runtime_put_noidle(dsi->dev);
> > + return r;
> > + }
> > + return 0;
> > }
>
> Thanks! Good catch. I think this is broken in all the other modules in
> omapdrm too (e.g. dispc.c,
> venc.c, etc).
>
> Would you like to update the patch to cover the whole omapdrm?
>
Sure. I will fix this soon.
Regards,
Dinghao
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/omap: Fix runtime PM imbalance in dsi_runtime_get
pm_runtime_get_sync() increments the runtime PM usage counter
even when it returns an error code. However, users of
dsi_runtime_get(), a direct wrapper of pm_runtime_get_sync(),
assume that PM usage counter will not change on error. Thus a
pairing decrement is needed on the error handling path to keep
the counter balanced.
Fixes: 4fbafaf371be7 ("OMAP: DSS2: Use PM runtime & HWMOD support")
Signed-off-by: Dinghao Liu
---
drivers/gpu/drm/omapdrm/dss/dsi.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c
b/drivers/gpu/drm/omapdrm/dss/dsi.c
index eeccf40bae41..973bfa14a104 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -1112,8 +1112,11 @@ static int dsi_runtime_get(struct dsi_data *dsi)
DSSDBG("dsi_runtime_get\n");
r = pm_runtime_get_sync(dsi->dev);
- WARN_ON(r < 0);
- return r < 0 ? r : 0;
+ if (WARN_ON(r < 0)) {
+ pm_runtime_put_noidle(dsi->dev);
+ return r;
+ }
+ return 0;
}
static void dsi_runtime_put(struct dsi_data *dsi)
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] video: fbdev: radeon: Fix memleak in radeonfb_pci_register
When radeon_kick_out_firmware_fb() fails, info should be
freed just like the subsequent error paths.
Fixes: 069ee21a82344 ("fbdev: Fix loading of module radeonfb on PowerMac")
Signed-off-by: Dinghao Liu
---
drivers/video/fbdev/aty/radeon_base.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/aty/radeon_base.c
b/drivers/video/fbdev/aty/radeon_base.c
index 3fe509cb9b87..13bd2bd5c043 100644
--- a/drivers/video/fbdev/aty/radeon_base.c
+++ b/drivers/video/fbdev/aty/radeon_base.c
@@ -2307,7 +2307,7 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
ret = radeon_kick_out_firmware_fb(pdev);
if (ret)
- return ret;
+ goto err_release_fb;
/* request the mem regions */
ret = pci_request_region(pdev, 0, "radeonfb framebuffer");
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/amd/display: Fix memleak in amdgpu_dm_mode_config_init
When amdgpu_display_modeset_create_props() fails, state and
state->context should be freed to prevent memleak. It's the
same when amdgpu_dm_audio_init() fails.
Signed-off-by: Dinghao Liu
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index df9338257ae0..2476e40c67ef 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2834,12 +2834,18 @@ static int amdgpu_dm_mode_config_init(struct
amdgpu_device *adev)
&dm_atomic_state_funcs);
r = amdgpu_display_modeset_create_props(adev);
- if (r)
+ if (r) {
+ dc_release_state(state->context);
+ kfree(state);
return r;
+ }
r = amdgpu_dm_audio_init(adev);
- if (r)
+ if (r) {
+ dc_release_state(state->context);
+ kfree(state);
return r;
+ }
return 0;
}
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] video: backlight: sky81452-backlight: Fix refcount imbalance on error
When of_property_read_u32_array() returns an error code, a
pairing refcount decrement is needed to keep np's refcount
balanced.
Fixes: f705806c9f355 ("backlight: Add support Skyworks SKY81452 backlight
driver")
Signed-off-by: Dinghao Liu
---
drivers/video/backlight/sky81452-backlight.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/backlight/sky81452-backlight.c
b/drivers/video/backlight/sky81452-backlight.c
index 0ce181585008..8268ac43d54f 100644
--- a/drivers/video/backlight/sky81452-backlight.c
+++ b/drivers/video/backlight/sky81452-backlight.c
@@ -217,6 +217,7 @@ static struct sky81452_bl_platform_data
*sky81452_bl_parse_dt(
num_entry);
if (ret < 0) {
dev_err(dev, "led-sources node is invalid.\n");
+ of_node_put(np);
return ERR_PTR(-EINVAL);
}
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/crc-debugfs: Fix memleak in crc_control_write
When verify_crc_source() fails, source needs to be freed.
However, current code is returning directly and ends up
leaking memory.
Fixes: c0811a7d5befe ("drm/crc: Cleanup crtc_crc_open function")
Signed-off-by: Dinghao Liu
---
drivers/gpu/drm/drm_debugfs_crc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_debugfs_crc.c
b/drivers/gpu/drm/drm_debugfs_crc.c
index 5d67a41f7c3a..3dd70d813f69 100644
--- a/drivers/gpu/drm/drm_debugfs_crc.c
+++ b/drivers/gpu/drm/drm_debugfs_crc.c
@@ -144,8 +144,10 @@ static ssize_t crc_control_write(struct file *file, const
char __user *ubuf,
source[len - 1] = '\0';
ret = crtc->funcs->verify_crc_source(crtc, source, &values_cnt);
- if (ret)
+ if (ret) {
+ kfree(source);
return ret;
+ }
spin_lock_irq(&crc->lock);
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Re: [PATCH] video: backlight: sky81452-backlight: Fix reference count imbalance on error
> On Wed, 19 Aug 2020, Markus Elfring wrote:
>
> > > When of_property_read_u32_array() returns an error code,
> > > a pairing refcount decrement is needed to keep np's refcount balanced.
> >
> > Can another imperative wording be helpful for the change description?
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=18445bf405cb331117bc98427b1ba6f12418ad17#n151
> >
> > Would an other commit message be a bit nicer?
> >
> >
> > …
> > > +++ b/drivers/video/backlight/sky81452-backlight.c
> > > @@ -217,6 +217,7 @@ static struct sky81452_bl_platform_data
> > > *sky81452_bl_parse_dt(
> > > num_entry);
> > > if (ret < 0) {
> > > dev_err(dev, "led-sources node is invalid.\n");
> > > + of_node_put(np);
> > > return ERR_PTR(-EINVAL);
> > > }
> >
> > I propose to add the jump target “put_node” so that a bit of common
> > exception
> > handling code can be better reused at the end of this function
> > implementation.
> >
> > Regards,
> > Markus
>
> You can safely ignore any review comments from Markus!
>
> However, this patch doesn't appear to be in my inbox.
>
> Any ideas as to why?
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
Thank you for your advice. My outbox shows that this patch
has reached your email server successfully. Maybe this
ended up in your junk mail file?
Regards,
Dinghao
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Re: Re: [PATCH] video: backlight: sky81452-backlight: Fix reference count imbalance on error
> On Thu, 20 Aug 2020, dinghao@zju.edu.cn wrote:
>
> > > On Wed, 19 Aug 2020, Markus Elfring wrote:
> > >
> > > > > When of_property_read_u32_array() returns an error code,
> > > > > a pairing refcount decrement is needed to keep np's refcount balanced.
> > > >
> > > > Can another imperative wording be helpful for the change description?
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=18445bf405cb331117bc98427b1ba6f12418ad17#n151
> > > >
> > > > Would an other commit message be a bit nicer?
> > > >
> > > >
> > > > …
> > > > > +++ b/drivers/video/backlight/sky81452-backlight.c
> > > > > @@ -217,6 +217,7 @@ static struct sky81452_bl_platform_data
> > > > > *sky81452_bl_parse_dt(
> > > > > num_entry);
> > > > > if (ret < 0) {
> > > > > dev_err(dev, "led-sources node is invalid.\n");
> > > > > + of_node_put(np);
> > > > > return ERR_PTR(-EINVAL);
> > > > > }
> > > >
> > > > I propose to add the jump target “put_node” so that a bit of common
> > > > exception
> > > > handling code can be better reused at the end of this function
> > > > implementation.
> > > >
> > > > Regards,
> > > > Markus
> > >
> > > You can safely ignore any review comments from Markus!
> > >
> > > However, this patch doesn't appear to be in my inbox.
> > >
> > > Any ideas as to why?
> > >
> >
> > Thank you for your advice. My outbox shows that this patch
> > has reached your email server successfully. Maybe this
> > ended up in your junk mail file?
>
> This has happened recently, so I was sure to check.
>
> Not there either unfortunately.
>
> Would you be kind enough to bounce/resend please?
>
Sure.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/etnaviv: fix runtime pm imbalance on error
pm_runtime_get_sync() increments the runtime PM usage counter even the call returns an error code. Thus a pairing decrement is needed on the error handling path to keep the counter balanced. Signed-off-by: Dinghao Liu --- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index a31eeff2b297..da3f6ca5849f 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1691,6 +1691,9 @@ static int etnaviv_gpu_bind(struct device *dev, struct device *master, return 0; out_sched: +#ifdef CONFIG_PM + pm_runtime_put_autosuspend(gpu->dev); +#endif etnaviv_sched_fini(gpu); out_workqueue: -- 2.17.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Re: [PATCH] drm/panfrost: fix runtime pm imbalance on error
Hi Steve,
There are two bailing out points in panfrost_job_hw_submit(): one is
the error path beginning from pm_runtime_get_sync(), the other one is
the error path beginning from WARN_ON() in the if statement. The pm
imbalance fixed in this patch is between these two paths. I think the
caller of panfrost_job_hw_submit() cannot distinguish this imbalance
outside this function.
panfrost_job_timedout() calls pm_runtime_put_noidle() for every job it
finds, but all jobs are added to the pfdev->jobs just before calling
panfrost_job_hw_submit(). Therefore I think the imbalance still exists.
But I'm not very sure if we should add pm_runtime_put on the error path
after pm_runtime_get_sync(), or remove pm_runtime_put one the error path
after WARN_ON().
As for the problem about panfrost_devfreq_record_busy(), this may be a
new bug and requires independent patch to fix it.
Regards,
Dinghao
> On 20/05/2020 12:05, Dinghao Liu wrote:
> > pm_runtime_get_sync() increments the runtime PM usage counter even
> > the call returns an error code. Thus a pairing decrement is needed
> > on the error handling path to keep the counter balanced.
> >
> > Signed-off-by: Dinghao Liu
>
> Actually I think we have the opposite problem. To be honest we don't
> handle this situation very well. By the time panfrost_job_hw_submit() is
> called the job has already been added to the pfdev->jobs array, so it's
> considered submitted even if it never actually lands on the hardware. So
> in the case of this function bailing out early we will then (eventually)
> hit a timeout and trigger a GPU reset.
>
> panfrost_job_timedout() iterates through the pfdev->jobs array and calls
> pm_runtime_put_noidle() for each job it finds. So there's no inbalance
> here that I can see.
>
> Have you actually observed the situation where pm_runtime_get_sync()
> returns a failure?
>
> HOWEVER, it appears that by bailing out early the call to
> panfrost_devfreq_record_busy() is never made, which as far as I can see
> means that there may be an extra call to panfrost_devfreq_record_idle()
> when the jobs have timed out. Which could underflow the counter.
>
> But equally looking at panfrost_job_timedout(), we only call
> panfrost_devfreq_record_idle() *once* even though multiple jobs might be
> processed.
>
> There's a completely untested patch below which in theory should fix that...
>
> Steve
>
> 8<---
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c
> b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 7914b1570841..f9519afca29d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -145,6 +145,8 @@ static void panfrost_job_hw_submit(struct
> panfrost_job *job, int js)
> u64 jc_head = job->jc;
> int ret;
>
> + panfrost_devfreq_record_busy(pfdev);
> +
> ret = pm_runtime_get_sync(pfdev->dev);
> if (ret < 0)
> return;
> @@ -155,7 +157,6 @@ static void panfrost_job_hw_submit(struct
> panfrost_job *job, int js)
> }
>
> cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);
> - panfrost_devfreq_record_busy(pfdev);
>
> job_write(pfdev, JS_HEAD_NEXT_LO(js), jc_head & 0x);
> job_write(pfdev, JS_HEAD_NEXT_HI(js), jc_head >> 32);
> @@ -410,12 +411,12 @@ static void panfrost_job_timedout(struct
> drm_sched_job *sched_job)
> for (i = 0; i < NUM_JOB_SLOTS; i++) {
> if (pfdev->jobs[i]) {
> pm_runtime_put_noidle(pfdev->dev);
> + panfrost_devfreq_record_idle(pfdev);
> pfdev->jobs[i] = NULL;
> }
> }
> spin_unlock_irqrestore(&pfdev->js->job_lock, flags);
>
> - panfrost_devfreq_record_idle(pfdev);
> panfrost_device_reset(pfdev);
>
> for (i = 0; i < NUM_JOB_SLOTS; i++)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/nouveau/dispnv50: fix runtime pm imbalance on error
pm_runtime_get_sync() increments the runtime PM usage counter even
the call returns an error code. Thus a pairing decrement is needed
on the error handling path to keep the counter balanced.
Signed-off-by: Dinghao Liu
---
drivers/gpu/drm/nouveau/dispnv50/disp.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 6be9df1820c5..e670756664ff 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1123,8 +1123,10 @@ nv50_mstc_detect(struct drm_connector *connector,
return connector_status_disconnected;
ret = pm_runtime_get_sync(connector->dev->dev);
- if (ret < 0 && ret != -EACCES)
+ if (ret < 0 && ret != -EACCES) {
+ pm_runtime_put_autosuspend(connector->dev->dev);
return connector_status_disconnected;
+ }
ret = drm_dp_mst_detect_port(connector, ctx, mstc->port->mgr,
mstc->port);
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/nouveau: fix runtime pm imbalance on error
pm_runtime_get_sync() increments the runtime PM usage counter even
the call returns an error code. Thus a pairing decrement is needed
on the error handling path to keep the counter balanced.
Signed-off-by: Dinghao Liu
---
drivers/gpu/drm/nouveau/nouveau_gem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
b/drivers/gpu/drm/nouveau/nouveau_gem.c
index f5ece1f94973..125cefbb6210 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -157,8 +157,8 @@ nouveau_gem_object_close(struct drm_gem_object *gem, struct
drm_file *file_priv)
if (!WARN_ON(ret < 0 && ret != -EACCES)) {
nouveau_gem_object_unmap(nvbo, vma);
pm_runtime_mark_last_busy(dev);
- pm_runtime_put_autosuspend(dev);
}
+ pm_runtime_put_autosuspend(dev);
}
}
ttm_bo_unreserve(&nvbo->bo);
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/v3d: fix runtime pm imbalance on error
pm_runtime_get_sync() increments the runtime PM usage counter even
the call returns an error code. Thus a pairing decrement is needed
on the error handling path to keep the counter balanced.
Signed-off-by: Dinghao Liu
---
drivers/gpu/drm/v3d/v3d_gem.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 549dde83408b..d53c683ed74c 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -440,8 +440,10 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file
*file_priv,
job->free = free;
ret = pm_runtime_get_sync(v3d->dev);
- if (ret < 0)
+ if (ret < 0) {
+ pm_runtime_put_autosuspend(v3d->dev);
return ret;
+ }
xa_init_flags(&job->deps, XA_FLAGS_ALLOC);
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/nouveau/debugfs: fix runtime pm imbalance on error
pm_runtime_get_sync() increments the runtime PM usage counter even
the call returns an error code. Thus a pairing decrement is needed
on the error handling path to keep the counter balanced.
Signed-off-by: Dinghao Liu
---
drivers/gpu/drm/nouveau/nouveau_debugfs.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_debugfs.c
b/drivers/gpu/drm/nouveau/nouveau_debugfs.c
index 15a3d40edf02..db3711436577 100644
--- a/drivers/gpu/drm/nouveau/nouveau_debugfs.c
+++ b/drivers/gpu/drm/nouveau/nouveau_debugfs.c
@@ -181,8 +181,11 @@ nouveau_debugfs_pstate_set(struct file *file, const char
__user *ubuf,
}
ret = pm_runtime_get_sync(drm->dev);
- if (ret < 0 && ret != -EACCES)
+ if (ret < 0 && ret != -EACCES) {
+ pm_runtime_put_autosuspend(drm->dev);
return ret;
+ }
+
ret = nvif_mthd(ctrl, NVIF_CONTROL_PSTATE_USER, &args, sizeof(args));
pm_runtime_put_autosuspend(drm->dev);
if (ret < 0)
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/nouveau: fix runtime pm imbalance on error
pm_runtime_get_sync() increments the runtime PM usage counter even
the call returns an error code. Thus a pairing decrement is needed
on the error handling path to keep the counter balanced.
Signed-off-by: Dinghao Liu
---
drivers/gpu/drm/nouveau/nouveau_gem.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
b/drivers/gpu/drm/nouveau/nouveau_gem.c
index f5ece1f94973..6697f960dd89 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -76,8 +76,10 @@ nouveau_gem_object_open(struct drm_gem_object *gem, struct
drm_file *file_priv)
return ret;
ret = pm_runtime_get_sync(dev);
- if (ret < 0 && ret != -EACCES)
+ if (ret < 0 && ret != -EACCES) {
+ pm_runtime_put_autosuspend(dev);
goto out;
+ }
ret = nouveau_vma_new(nvbo, vmm, &vma);
pm_runtime_mark_last_busy(dev);
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/panfrost: fix runtime pm imbalance on error
pm_runtime_get_sync() increments the runtime PM usage counter even
the call returns an error code. Thus a pairing decrement is needed
on the error handling path to keep the counter balanced.
Signed-off-by: Dinghao Liu
---
drivers/gpu/drm/panfrost/panfrost_job.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c
b/drivers/gpu/drm/panfrost/panfrost_job.c
index 7914b1570841..5719e356c969 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -146,8 +146,10 @@ static void panfrost_job_hw_submit(struct panfrost_job
*job, int js)
int ret;
ret = pm_runtime_get_sync(pfdev->dev);
- if (ret < 0)
+ if (ret < 0) {
+ pm_runtime_put_sync_autosuspend(pfdev->dev);
return;
+ }
if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js {
pm_runtime_put_sync_autosuspend(pfdev->dev);
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] [v2] drm/panfrost: Fix runtime PM imbalance on error
The caller expects panfrost_job_hw_submit() to increase
runtime PM usage counter. The refcount decrement on the
error branch of WARN_ON() will break the counter balance
and needs to be removed.
Signed-off-by: Dinghao Liu
---
Changelog:
v2: - Remove refcount decrement on the error path of
WARN_ON() rather than add refcount decrement
on the error path of pm_runtime_get_sync().
---
drivers/gpu/drm/panfrost/panfrost_job.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c
b/drivers/gpu/drm/panfrost/panfrost_job.c
index 7914b1570841..1092d9754f0f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -150,7 +150,6 @@ static void panfrost_job_hw_submit(struct panfrost_job
*job, int js)
return;
if (WARN_ON(job_read(pfdev, JS_COMMAND_NEXT(js {
- pm_runtime_put_sync_autosuspend(pfdev->dev);
return;
}
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Re: [PATCH] drm/panfrost: fix runtime pm imbalance on error
Thank you for your further explanation! It's all clear for me and I will write a new patch to fix this imbalance. Regards, Dinghao > On 21/05/2020 08:00, dinghao@zju.edu.cn wrote: > > Hi Steve, > > > > There are two bailing out points in panfrost_job_hw_submit(): one is > > the error path beginning from pm_runtime_get_sync(), the other one is > > the error path beginning from WARN_ON() in the if statement. The pm > > imbalance fixed in this patch is between these two paths. I think the > > caller of panfrost_job_hw_submit() cannot distinguish this imbalance > > outside this function. > > My point is the caller expects panfrost_job_hw_submit() to increase the > PM reference count. Since panfrost_job_hw_submit() cannot return an > error (it's void return) we cannot signal to the caller that the > reference hasn't been taken. > > > panfrost_job_timedout() calls pm_runtime_put_noidle() for every job it > > finds, but all jobs are added to the pfdev->jobs just before calling > > panfrost_job_hw_submit(). Therefore I think the imbalance still exists. > > My point's exactly that - the "jobs are added to pfdev->jobs just before > calling panfrost_job_hw_submit()". Since we don't have a way for > panfrost_job_hw_submit() to fail it must unconditionally take any > references that will then be freed later on. > > > But I'm not very sure if we should add pm_runtime_put on the error path > > after pm_runtime_get_sync(), or remove pm_runtime_put one the error path > > after WARN_ON(). > > The pm_runtime_put after the WARN_ON() is a bug. Sorry this is probably > what confused you - clearly the WARN_ON() situation is never meant to > happen in the first place, so hopefully this isn't actually possible. > > Feel free to send a patch removing it! ;) > > > As for the problem about panfrost_devfreq_record_busy(), this may be a > > new bug and requires independent patch to fix it. > > Indeed, I'll post a proper patch for that later - I just spotted it > while looking at the code. > > Thanks, > > Steve > > > Regards, > > Dinghao > > > > > >> On 20/05/2020 12:05, Dinghao Liu wrote: > >>> pm_runtime_get_sync() increments the runtime PM usage counter even > >>> the call returns an error code. Thus a pairing decrement is needed > >>> on the error handling path to keep the counter balanced. > >>> > >>> Signed-off-by: Dinghao Liu > >> > >> Actually I think we have the opposite problem. To be honest we don't > >> handle this situation very well. By the time panfrost_job_hw_submit() is > >> called the job has already been added to the pfdev->jobs array, so it's > >> considered submitted even if it never actually lands on the hardware. So > >> in the case of this function bailing out early we will then (eventually) > >> hit a timeout and trigger a GPU reset. > >> > >> panfrost_job_timedout() iterates through the pfdev->jobs array and calls > >> pm_runtime_put_noidle() for each job it finds. So there's no inbalance > >> here that I can see. > >> > >> Have you actually observed the situation where pm_runtime_get_sync() > >> returns a failure? > >> > >> HOWEVER, it appears that by bailing out early the call to > >> panfrost_devfreq_record_busy() is never made, which as far as I can see > >> means that there may be an extra call to panfrost_devfreq_record_idle() > >> when the jobs have timed out. Which could underflow the counter. > >> > >> But equally looking at panfrost_job_timedout(), we only call > >> panfrost_devfreq_record_idle() *once* even though multiple jobs might be > >> processed. > >> > >> There's a completely untested patch below which in theory should fix > >> that... > >> > >> Steve > >> > >> 8<--- > >> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c > >> b/drivers/gpu/drm/panfrost/panfrost_job.c > >> index 7914b1570841..f9519afca29d 100644 > >> --- a/drivers/gpu/drm/panfrost/panfrost_job.c > >> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > >> @@ -145,6 +145,8 @@ static void panfrost_job_hw_submit(struct > >> panfrost_job *job, int js) > >>u64 jc_head = job->jc; > >>int ret; > >> > >> + panfrost_devfreq_record_busy(pfdev); > >> + > >>ret = pm_runtime_get_sync(pfdev->dev); > >>
[PATCH] drm/i915/selftests: Fix runtime PM imbalance on error
When drm_dev_init() returns an error code, a pairing runtime PM usage counter decrement is needed to keep the counter balanced. For error paths after this call, things are the same. Signed-off-by: Dinghao Liu --- drivers/gpu/drm/i915/selftests/mock_gem_device.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c index 754d0eb6beaa..fdf01461acc6 100644 --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c @@ -200,6 +200,7 @@ struct drm_i915_private *mock_gem_device(void) drm_mode_config_cleanup(&i915->drm); drm_dev_fini(&i915->drm); put_device: + pm_runtime_put_noidle(&pdev->dev); put_device(&pdev->dev); err: return NULL; -- 2.17.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/panfrost: Fix runtime PM imbalance in panfrost_perfcnt_enable_locked
pm_runtime_get_sync() increments the runtime PM usage counter even
when it returns an error code. Thus a pairing decrement is needed on
the error handling path to keep the counter balanced. For all error
paths after pm_runtime_get_sync(), things are the same.
Signed-off-by: Dinghao Liu
---
drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
index 6913578d5aa7..d99bd2f0503a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
+++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
@@ -83,11 +83,13 @@ static int panfrost_perfcnt_enable_locked(struct
panfrost_device *pfdev,
ret = pm_runtime_get_sync(pfdev->dev);
if (ret < 0)
- return ret;
+ goto err_pm_get;
bo = drm_gem_shmem_create(pfdev->ddev, perfcnt->bosize);
- if (IS_ERR(bo))
- return PTR_ERR(bo);
+ if (IS_ERR(bo)) {
+ ret = PTR_ERR(bo);
+ goto err_pm_get;
+ }
/* Map the perfcnt buf in the address space attached to file_priv. */
ret = panfrost_gem_open(&bo->base, file_priv);
@@ -168,6 +170,8 @@ static int panfrost_perfcnt_enable_locked(struct
panfrost_device *pfdev,
panfrost_gem_close(&bo->base, file_priv);
err_put_bo:
drm_gem_object_put_unlocked(&bo->base);
+err_pm_get:
+ pm_runtime_put_noidle(pfdev->dev);
return ret;
}
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
When gk20a_clk_ctor() returns an error code, pointer "clk" should be released. It's the same when gm20b_clk_new() returns from elsewhere following this call. Signed-off-by: Dinghao Liu --- drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c index b284e949f732..a5aeba74d3b7 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gm20b.c @@ -1039,7 +1039,7 @@ gm20b_clk_new(struct nvkm_device *device, int index, struct nvkm_clk **pclk) ret = gk20a_clk_ctor(device, index, &gm20b_clk, clk_params, &clk->base); if (ret) - return ret; + goto out_free; /* * NAPLL can only work with max_u, clamp the m range so @@ -1067,8 +1067,8 @@ gm20b_clk_new(struct nvkm_device *device, int index, struct nvkm_clk **pclk) nvkm_warn(subdev, "no fused calibration parameters\n"); ret = gm20b_clk_init_safe_fmax(clk); - if (ret) - return ret; - return 0; +out_free: + kfree(clk); + return ret; } -- 2.17.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
Hi Ben, > > When gk20a_clk_ctor() returns an error code, pointer "clk" > > should be released. It's the same when gm20b_clk_new() > > returns from elsewhere following this call. > This shouldn't be necessary. If a subdev constructor fails, and > returns a pointer, the core will call the destructor to clean things > up. > I'm not familiar with the behavior of the caller of gm20b_clk_new(). If the subdev constructor fails, the core will check the pointer (here is "pclk"), then it's ok and there is no bug (Do you mean this?). If the core executes error handling code only according to the error code, there may be a memory leak bug (the caller cannot know if -ENOMEM comes from the failure of kzalloc or gk20a_clk_ctor). If the core always calls the destructor as long as the constructor fails (even if the kzalloc fails), we may have a double free bug. Would you like to give a more detailed explanation about the behavior of the core? Regards, Dinghao ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
> > > For security, I will release this pointer only on error paths in this > > function. > > Do you tend to release objects (which are referenced by pointers)? > I just found that clk is referenced by pclk in this function. When clk is freed, pclk will be allocated in gm20b_clk_new_speedo0(). Thus we should not release clk in this function and there is no bug here. Thank you for reminding me! Regards, Dinghao ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
> > > It's the same when gm20b_clk_new() returns from elsewhere following this > > call. > > I suggest to reconsider the interpretation of the software situation once > more. > Can it be that the allocated clock object should be kept usable even after > a successful return from this function? > It's possible that we expect an usable clk pointer, though I could not find the exact usage yet. For security, I will release this pointer only on error paths in this function. > > Would you like to add the tag “Fixes” to the commit message? > Thank you for your advice! I will add this tag in the next version of patch. Regards, Dinghao ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
> > If gk20a_clk_ctor() never returns such an error code, > > we may need not to release this clock object. > > Would you like to achieve complete exception handling > also for this function implementation? > It seems that it's possible to get -ENOMEM from gk20a_clk_ctor(). The call chain is as follows: gk20a_clk_ctor() <- nvkm_clk_ctor() <- nvkm_notify_init() When nvkm_notify_init() returns -ENOMEM, all of its callers (and callers of callers) will be influenced if there is a failed kzalloc inside which. In this case, maybe we should check the return value of gk20a_clk_ctor() and release clk if it returns -ENOMEM. And many other functions also have the same issue (e.g., gm20b_clk_new_speedo0). Do you have any idea about this problem? Regards, Dinghao ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new
> > If there's *any* error, it'll check the pointer, if it's non-NULL, > > it'll call the destructor. If kzalloc() fails, the pointer will be > > NULL, there's no double-free bug. *every* subdev is written this way > > to avoid duplicating cleanup logic. > Actually, gm20b_clk_new_speedo0() may have a bug here if kzalloc() > fails as it doesn't overwrite the previous pointer from > gm20b_clk_new(). That whole ctor() sequence is written a little > strangely. > It's clear to me, thank your for your explanation! As for gm20b_clk_new_speedo0(), I think its bug pattern is not very clear. Maybe we should keep it until we find an use chain that could lead to a bug. Regards, Dinghao ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Re: drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
> > I just found that clk is referenced by pclk in this function. When clk is > > freed, > > pclk will be allocated in gm20b_clk_new_speedo0(). Thus we should not > > release clk > > in this function and there is no bug here. > > Can there be a need to release a clock object after a failed gk20a_clk_ctor() > call? > I think this mainly depends on pclk pointer. It seems that the caller of gm20b_clk_new() always expects pclk to be allocated unless it returns -ENOMEM, which means kzalloc() failed. If gk20a_clk_ctor() never returns such an error code, we may need not to release this clock object. Regards, Dinghao ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Re: [PATCH] drm/nouveau/clk/gm20b: Fix memory leak in gm20b_clk_new()
> On Tue, Jun 02, 2020 at 01:10:34PM +0200, Markus Elfring wrote: > > > The original patch was basically fine. > > > > I propose to reconsider the interpretation of the software situation once > > more. > > > > * Should the allocated clock object be kept usable even after > > a successful return from this function? > > Heh. You're right. The patch is freeing "clk" on the success path so > that doesn't work. > Ben has explained this problem: https://lore.kernel.org/patchwork/patch/1249592/ Since the caller will check "pclk" on failure, we don't need to free "clk" in gm20b_clk_new() and I think this patch is no longer needed. Regards, Dinghao ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/amd/pm: fix a memleak in aldebaran_tables_init
When kzalloc() for smu_table->ecc_table fails, we should free
the previously allocated resources to prevent memleak.
Fixes: edd794208555 ("drm/amd/pm: add message smu to get ecc_table v2")
Signed-off-by: Dinghao Liu
---
drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
index 1a6675d70a4b..f1440869d1ce 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/aldebaran_ppt.c
@@ -257,8 +257,11 @@ static int aldebaran_tables_init(struct smu_context *smu)
}
smu_table->ecc_table = kzalloc(tables[SMU_TABLE_ECCINFO].size,
GFP_KERNEL);
- if (!smu_table->ecc_table)
+ if (!smu_table->ecc_table) {
+ kfree(smu_table->metrics_table);
+ kfree(smu_table->gpu_metrics_table);
return -ENOMEM;
+ }
return 0;
}
--
2.17.1
[PATCH] drm/plane: fix error handling in __drm_universal_plane_init
__drm_universal_plane_init() frees plane->format_types and
plane->modifiers on failure. However, sometimes its callers
will free these two pointers again, which may lead to a
double-free. One possible call chain is:
mdp5_plane_init
|-> drm_universal_plane_init
| |-> __drm_universal_plane_init (first free)
|
|-> mdp5_plane_destroy
|-> drm_plane_cleanup (second free)
Fix this by setting the two pointers to NULL after kfree.
Signed-off-by: Dinghao Liu
---
drivers/gpu/drm/drm_plane.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 24e7998d1731..1331b8224920 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -304,6 +304,7 @@ static int __drm_universal_plane_init(struct drm_device
*dev,
if (format_modifier_count && !plane->modifiers) {
DRM_DEBUG_KMS("out of memory when allocating plane\n");
kfree(plane->format_types);
+ plane->format_types = NULL;
drm_mode_object_unregister(dev, &plane->base);
return -ENOMEM;
}
@@ -317,6 +318,8 @@ static int __drm_universal_plane_init(struct drm_device
*dev,
if (!plane->name) {
kfree(plane->format_types);
kfree(plane->modifiers);
+ plane->format_types = NULL;
+ plane->modifiers = NULL;
drm_mode_object_unregister(dev, &plane->base);
return -ENOMEM;
}
--
2.17.1
