| Message ID | 20190114084410.15266-2-emil.l.velikov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm: factor out drm_close_helper() function | expand |
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
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
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
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;