Re: [PATCH 07/22] drm/tegra: Remove module ownership from the tegra_fb_ops

[Dmitry Osipenko]

On 13.06.2017 18:07, Thierry Reding wrote:
> On Tue, Jun 13, 2017 at 05:00:28PM +0300, Dmitry Osipenko wrote:
>> On 13.06.2017 16:43, Thierry Reding wrote:
>>> On Tue, May 23, 2017 at 03:14:22AM +0300, Dmitry Osipenko wrote:
 The framebuffers console fbcon_startup() increments the tegra_drm module
 'use' refcount via try_module_get(), causing an interlock of the DRM subsys
 and the tegra_drm modules. In result, the tegra_drm module can't be 
 unloaded
 using rmmod.

 Signed-off-by: Dmitry Osipenko 
 ---
  drivers/gpu/drm/tegra/fb.c | 1 -
  1 file changed, 1 deletion(-)
>>>
>>> That's done on purpose because otherwise you could just rip out the
>>> driver from under the framebuffer emulation and things would crash.
>>>
>>> My understanding is that the right way to unload a module is to unbind
>>> the driver first (which will cause the framebuffer to be removed and
>>> hence the reference to be dropped) before the rmmod.
>>>
>>> Thierry
>>>
>>
>> Aha, interesting. I'll try the unbinding and will drop this patch from the
>> series, thank you for the clarification. I haven't observed any crashes on a
>> module reloading (framebuffer detached/attached just fine), maybe I was 
>> lucky then.
> 
> It's possible that it works by accident. The driver removes the
> framebuffer as part of the driver removal process, so technically
> nothing should crash. However, if, for any reason, anyone was holding
> on to a reference to the framebuffer (not sure if that is even possible)
> the module needs to stay around long enough as well, otherwise the
> function pointers would become dangling. The module reference makes sure
> that this doesn't happen (as long as a framebuffer exists, the ops will
> stick around).
> 
> So even if there isn't a way to make it crash today, the code is still
> correct in taking the reference.

Thank you very much for a more detailed explanation. I've successfully unloaded
the tegra_drm module after unbinding the VT's, the patch is dropped from the 
series.

echo 0 | tee /sys/devices/virtual/vtconsole/vtcon*/bind

-- 
Dmitry
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/22] drm/tegra: Remove module ownership from the tegra_fb_ops

[Dmitry Osipenko]

On 13.06.2017 16:43, Thierry Reding wrote:
> On Tue, May 23, 2017 at 03:14:22AM +0300, Dmitry Osipenko wrote:
>> The framebuffers console fbcon_startup() increments the tegra_drm module
>> 'use' refcount via try_module_get(), causing an interlock of the DRM subsys
>> and the tegra_drm modules. In result, the tegra_drm module can't be unloaded
>> using rmmod.
>>
>> Signed-off-by: Dmitry Osipenko 
>> ---
>>  drivers/gpu/drm/tegra/fb.c | 1 -
>>  1 file changed, 1 deletion(-)
> 
> That's done on purpose because otherwise you could just rip out the
> driver from under the framebuffer emulation and things would crash.
> 
> My understanding is that the right way to unload a module is to unbind
> the driver first (which will cause the framebuffer to be removed and
> hence the reference to be dropped) before the rmmod.
> 
> Thierry
> 

Aha, interesting. I'll try the unbinding and will drop this patch from the
series, thank you for the clarification. I haven't observed any crashes on a
module reloading (framebuffer detached/attached just fine), maybe I was lucky 
then.

-- 
Dmitry
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/22] drm/tegra: Remove module ownership from the tegra_fb_ops

[Thierry Reding]

On Tue, Jun 13, 2017 at 05:00:28PM +0300, Dmitry Osipenko wrote:
> On 13.06.2017 16:43, Thierry Reding wrote:
> > On Tue, May 23, 2017 at 03:14:22AM +0300, Dmitry Osipenko wrote:
> >> The framebuffers console fbcon_startup() increments the tegra_drm module
> >> 'use' refcount via try_module_get(), causing an interlock of the DRM subsys
> >> and the tegra_drm modules. In result, the tegra_drm module can't be 
> >> unloaded
> >> using rmmod.
> >>
> >> Signed-off-by: Dmitry Osipenko 
> >> ---
> >>  drivers/gpu/drm/tegra/fb.c | 1 -
> >>  1 file changed, 1 deletion(-)
> > 
> > That's done on purpose because otherwise you could just rip out the
> > driver from under the framebuffer emulation and things would crash.
> > 
> > My understanding is that the right way to unload a module is to unbind
> > the driver first (which will cause the framebuffer to be removed and
> > hence the reference to be dropped) before the rmmod.
> > 
> > Thierry
> > 
> 
> Aha, interesting. I'll try the unbinding and will drop this patch from the
> series, thank you for the clarification. I haven't observed any crashes on a
> module reloading (framebuffer detached/attached just fine), maybe I was lucky 
> then.

It's possible that it works by accident. The driver removes the
framebuffer as part of the driver removal process, so technically
nothing should crash. However, if, for any reason, anyone was holding
on to a reference to the framebuffer (not sure if that is even possible)
the module needs to stay around long enough as well, otherwise the
function pointers would become dangling. The module reference makes sure
that this doesn't happen (as long as a framebuffer exists, the ops will
stick around).

So even if there isn't a way to make it crash today, the code is still
correct in taking the reference.

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/22] drm/tegra: Remove module ownership from the tegra_fb_ops

[Thierry Reding]

On Tue, May 23, 2017 at 03:14:22AM +0300, Dmitry Osipenko wrote:
> The framebuffers console fbcon_startup() increments the tegra_drm module
> 'use' refcount via try_module_get(), causing an interlock of the DRM subsys
> and the tegra_drm modules. In result, the tegra_drm module can't be unloaded
> using rmmod.
> 
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/gpu/drm/tegra/fb.c | 1 -
>  1 file changed, 1 deletion(-)

That's done on purpose because otherwise you could just rip out the
driver from under the framebuffer emulation and things would crash.

My understanding is that the right way to unload a module is to unbind
the driver first (which will cause the framebuffer to be removed and
hence the reference to be dropped) before the rmmod.

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/22] drm/tegra: Remove module ownership from the tegra_fb_ops

[Dmitry Osipenko]

The framebuffers console fbcon_startup() increments the tegra_drm module
'use' refcount via try_module_get(), causing an interlock of the DRM subsys
and the tegra_drm modules. In result, the tegra_drm module can't be unloaded
using rmmod.

Signed-off-by: Dmitry Osipenko 
---
 drivers/gpu/drm/tegra/fb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
index 25acb73ee728..75e102ffdc86 100644
--- a/drivers/gpu/drm/tegra/fb.c
+++ b/drivers/gpu/drm/tegra/fb.c
@@ -202,7 +202,6 @@ struct drm_framebuffer *tegra_fb_create(struct drm_device 
*drm,
 
 #ifdef CONFIG_DRM_FBDEV_EMULATION
 static struct fb_ops tegra_fb_ops = {
-   .owner = THIS_MODULE,
DRM_FB_HELPER_DEFAULT_OPS,
.fb_fillrect = drm_fb_helper_sys_fillrect,
.fb_copyarea = drm_fb_helper_sys_copyarea,
-- 
2.13.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel