* [PATCH 1/2] drm: factor out drm_close_helper() function
@ 2019-01-14 8:44 Emil Velikov
2019-01-14 8:44 ` [PATCH 2/2] drm: plug memory leak on drm_setup() failure Emil Velikov
0 siblings, 1 reply; 5+ messages in thread
From: Emil Velikov @ 2019-01-14 8:44 UTC (permalink / raw)
To: dri-devel; +Cc: emil.l.velikov
From: Emil Velikov <emil.velikov@collabora.com>
Will be used to plug an existing memory leak.
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
drivers/gpu/drm/drm_file.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index ffa8dc35515f..149506a20bdc 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -262,6 +262,18 @@ void drm_file_free(struct drm_file *file)
kfree(file);
}
+static void drm_close_helper(struct file *filp)
+{
+ struct drm_file *file_priv = filp->private_data;
+ struct drm_device *dev = file_priv->minor->dev;
+
+ mutex_lock(&dev->filelist_mutex);
+ list_del(&file_priv->lhead);
+ mutex_unlock(&dev->filelist_mutex);
+
+ drm_file_free(file_priv);
+}
+
static int drm_setup(struct drm_device * dev)
{
int ret;
@@ -473,11 +485,7 @@ int drm_release(struct inode *inode, struct file *filp)
DRM_DEBUG("open_count = %d\n", dev->open_count);
- mutex_lock(&dev->filelist_mutex);
- list_del(&file_priv->lhead);
- mutex_unlock(&dev->filelist_mutex);
-
- drm_file_free(file_priv);
+ drm_close_helper(filp);
if (!--dev->open_count) {
drm_lastclose(dev);
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] drm: plug memory leak on drm_setup() failure
2019-01-14 8:44 [PATCH 1/2] drm: factor out drm_close_helper() function Emil Velikov
@ 2019-01-14 8:44 ` Emil Velikov
2019-01-14 10:12 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Emil Velikov @ 2019-01-14 8:44 UTC (permalink / raw)
To: dri-devel; +Cc: emil.l.velikov
From: Emil Velikov <emil.velikov@collabora.com>
Currently we fail to free and detach the drm_file when drm_setup() fails.
Use the drm_close_helper to do address that.
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
drivers/gpu/drm/drm_file.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 149506a20bdc..871dcd8c7545 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -330,8 +330,10 @@ int drm_open(struct inode *inode, struct file *filp)
goto err_undo;
if (need_setup) {
retcode = drm_setup(dev);
- if (retcode)
+ if (retcode) {
+ drm_close_helper(filp);
goto err_undo;
+ }
}
return 0;
--
2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] drm: plug memory leak on drm_setup() failure
2019-01-14 8:44 ` [PATCH 2/2] drm: plug memory leak on drm_setup() failure Emil Velikov
@ 2019-01-14 10:12 ` Daniel Vetter
2019-01-16 11:40 ` Emil Velikov
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2019-01-14 10:12 UTC (permalink / raw)
To: Emil Velikov; +Cc: dri-devel
On Mon, Jan 14, 2019 at 08:44:10AM +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
>
> Currently we fail to free and detach the drm_file when drm_setup() fails.
> Use the drm_close_helper to do address that.
>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
> drivers/gpu/drm/drm_file.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 149506a20bdc..871dcd8c7545 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -330,8 +330,10 @@ int drm_open(struct inode *inode, struct file *filp)
> goto err_undo;
> if (need_setup) {
> retcode = drm_setup(dev);
> - if (retcode)
> + if (retcode) {
> + drm_close_helper(filp);
I freaked out mildly because drm_open_helper already adds the drm_file to
the filelist (hence publishes it), and publishing objects before they're
fully set up is a Bad Idea :-)
But drm_setup only does legacy setup, so who cares. On both patches:
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
And if you feel like doing an s/drm_setup()/drm_legacy_setup()/, with or
w/o changing the condition to if (need_setup &&
drm_core_check_feature(dev, DRIVER_LEGACY)), then that patch would also
have my
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> goto err_undo;
> + }
> }
> return 0;
>
> --
> 2.20.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] drm: plug memory leak on drm_setup() failure
2019-01-14 10:12 ` Daniel Vetter
@ 2019-01-16 11:40 ` Emil Velikov
2019-01-16 11:55 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Emil Velikov @ 2019-01-16 11:40 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel
On 2019/01/14, Daniel Vetter wrote:
> On Mon, Jan 14, 2019 at 08:44:10AM +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> >
> > Currently we fail to free and detach the drm_file when drm_setup() fails.
> > Use the drm_close_helper to do address that.
> >
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > ---
> > drivers/gpu/drm/drm_file.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index 149506a20bdc..871dcd8c7545 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -330,8 +330,10 @@ int drm_open(struct inode *inode, struct file *filp)
> > goto err_undo;
> > if (need_setup) {
> > retcode = drm_setup(dev);
> > - if (retcode)
> > + if (retcode) {
> > + drm_close_helper(filp);
>
> I freaked out mildly because drm_open_helper already adds the drm_file to
> the filelist (hence publishes it), and publishing objects before they're
> fully set up is a Bad Idea :-)
>
> But drm_setup only does legacy setup, so who cares. On both patches:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
Thanks.
> And if you feel like doing an s/drm_setup()/drm_legacy_setup()/, with or
> w/o changing the condition to if (need_setup &&
> drm_core_check_feature(dev, DRIVER_LEGACY)), then that patch would also
> have my
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
I'll leave that for another day, since the inverse vfunc (firstopen <>
lastclose) is not a legacy only one. Which kind of irks me a bit.
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] drm: plug memory leak on drm_setup() failure
2019-01-16 11:40 ` Emil Velikov
@ 2019-01-16 11:55 ` Daniel Vetter
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2019-01-16 11:55 UTC (permalink / raw)
To: Emil Velikov; +Cc: dri-devel
On Wed, Jan 16, 2019 at 11:40:41AM +0000, Emil Velikov wrote:
> On 2019/01/14, Daniel Vetter wrote:
> > On Mon, Jan 14, 2019 at 08:44:10AM +0000, Emil Velikov wrote:
> > > From: Emil Velikov <emil.velikov@collabora.com>
> > >
> > > Currently we fail to free and detach the drm_file when drm_setup() fails.
> > > Use the drm_close_helper to do address that.
> > >
> > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > > ---
> > > drivers/gpu/drm/drm_file.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > > index 149506a20bdc..871dcd8c7545 100644
> > > --- a/drivers/gpu/drm/drm_file.c
> > > +++ b/drivers/gpu/drm/drm_file.c
> > > @@ -330,8 +330,10 @@ int drm_open(struct inode *inode, struct file *filp)
> > > goto err_undo;
> > > if (need_setup) {
> > > retcode = drm_setup(dev);
> > > - if (retcode)
> > > + if (retcode) {
> > > + drm_close_helper(filp);
> >
> > I freaked out mildly because drm_open_helper already adds the drm_file to
> > the filelist (hence publishes it), and publishing objects before they're
> > fully set up is a Bad Idea :-)
> >
> > But drm_setup only does legacy setup, so who cares. On both patches:
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> Thanks.
>
> > And if you feel like doing an s/drm_setup()/drm_legacy_setup()/, with or
> > w/o changing the condition to if (need_setup &&
> > drm_core_check_feature(dev, DRIVER_LEGACY)), then that patch would also
> > have my
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> I'll leave that for another day, since the inverse vfunc (firstopen <>
> lastclose) is not a legacy only one. Which kind of irks me a bit.
We need lastclose for the fbdev emulation. Which is already fixed through
the new drm_client stuff and the new generic fbdev. It's a matter of
rolling that out. The other use is for delayed vgaswitcheroo changes,
which I think is a bit a hack ... Since I don't have a solution for that I
guess lasclose will stay with us for a while longer.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-01-16 11:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-14 8:44 [PATCH 1/2] drm: factor out drm_close_helper() function Emil Velikov
2019-01-14 8:44 ` [PATCH 2/2] drm: plug memory leak on drm_setup() failure Emil Velikov
2019-01-14 10:12 ` Daniel Vetter
2019-01-16 11:40 ` Emil Velikov
2019-01-16 11:55 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.