2018-07-03 22:07:52

by Lyude Paul

[permalink] [raw]
Subject: [PATCH 0/2] drm/nouveau: Fix runtime PM leaks

One very easy to trigger runtime PM leak, along with a rare never before
seen runtime PM leak!

Lyude Paul (2):
drm/nouveau: Fix runtime PM leak in drm_open()
drm/nouveau: Fix runtime PM leak in nv50_disp_atomic_commit()

drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_drm.c | 6 ++++--
2 files changed, 5 insertions(+), 3 deletions(-)

--
2.17.1



2018-07-03 22:08:27

by Lyude Paul

[permalink] [raw]
Subject: [PATCH 1/2] drm/nouveau: Fix runtime PM leak in drm_open()

Noticed this as I was skimming through, if we fail to allocate memory
for cli we'll end up returning without dropping the runtime PM ref we
got. Additionally, we'll even return the wrong return code! (ret most
likely will == 0 here, we want -ENOMEM).

Signed-off-by: Lyude Paul <[email protected]>
Cc: [email protected]
---
drivers/gpu/drm/nouveau/nouveau_drm.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 0452b18d36b9..0f668e275ee1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -919,8 +919,10 @@ nouveau_drm_open(struct drm_device *dev, struct drm_file *fpriv)
get_task_comm(tmpname, current);
snprintf(name, sizeof(name), "%s[%d]", tmpname, pid_nr(fpriv->pid));

- if (!(cli = kzalloc(sizeof(*cli), GFP_KERNEL)))
- return ret;
+ if (!(cli = kzalloc(sizeof(*cli), GFP_KERNEL))) {
+ ret = -ENOMEM;
+ goto done;
+ }

ret = nouveau_cli_init(drm, name, cli);
if (ret)
--
2.17.1


2018-07-03 22:09:22

by Lyude Paul

[permalink] [raw]
Subject: [PATCH 2/2] drm/nouveau: Fix runtime PM leak in nv50_disp_atomic_commit()

A CRTC being enabled doesn't mean it's on! It doesn't even necessarily
mean it's being used. This fixes runtime PM leaks on the P50 I've got
next to me.

Signed-off-by: Lyude Paul <[email protected]>
Cc: [email protected]
---
drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index d9da69c83ae7..9bae4db84cfb 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1878,7 +1878,7 @@ nv50_disp_atomic_commit(struct drm_device *dev,
nv50_disp_atomic_commit_tail(state);

drm_for_each_crtc(crtc, dev) {
- if (crtc->state->enable) {
+ if (crtc->state->active) {
if (!drm->have_disp_power_ref) {
drm->have_disp_power_ref = true;
return 0;
--
2.17.1


2018-07-04 02:41:59

by Lukas Wunner

[permalink] [raw]
Subject: Re: [PATCH 1/2] drm/nouveau: Fix runtime PM leak in drm_open()

[cc -= stable]

On Tue, Jul 03, 2018 at 06:05:59PM -0400, Lyude Paul wrote:
> Noticed this as I was skimming through, if we fail to allocate memory
> for cli we'll end up returning without dropping the runtime PM ref we
> got. Additionally, we'll even return the wrong return code! (ret most
> likely will == 0 here, we want -ENOMEM).
>
> Signed-off-by: Lyude Paul <[email protected]>

Reviewed-by: Lukas Wunner <[email protected]>