Re: [PATCH 6/6] drm/rockchip: fix race with kms hotplug and fbdev

[Mark yao]

On 2017å07æ31æ 20:28, Daniel Vetter wrote:
> On Mon, Jul 31, 2017 at 1:57 PM, Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote:
> > On 31 July 2017 at 10:50, Mark Yao <mark.yao@xxxxxxxxxxxxxx> wrote:
> > > Since fb_helper is not a pointer on rockchip_drm_private, it's no
> > > need to check pointer.
> > >
> > > Kms hotplug event may race into fbdev helper initial, and fb_helper->dev
> > > may be NULL pointer, that would cause the bug:
> > >
> > > [    0.735411] [00000200] *pgd=00000000f6ffe003, *pud=00000000f6ffe003, *pmd=0000000000000000
> > > [    0.736156] Internal error: Oops: 96000005 [#1] PREEMPT SMP
> > > [    0.736648] Modules linked in:
> > > [    0.736930] CPU: 2 PID: 20 Comm: kworker/2:0 Not tainted 4.4.41 #20
> > > [    0.737480] Hardware name: Rockchip RK3399 Board rev2 (BOX) (DT)
> > > [    0.738020] Workqueue: events cdn_dp_pd_event_work
> > > [    0.738447] task: ffffffc0f21f3100 ti: ffffffc0f2218000 task.ti: ffffffc0f2218000
> > > [    0.739109] PC is at mutex_lock+0x14/0x44
> > > [    0.739469] LR is at drm_fb_helper_hotplug_event+0x30/0x114
> > > [    0.756253] [<ffffff8008a344f4>] mutex_lock+0x14/0x44
> > > [    0.756260] [<ffffff8008445708>] drm_fb_helper_hotplug_event+0x30/0x114
> > > [    0.756271] [<ffffff8008473c84>] rockchip_drm_output_poll_changed+0x18/0x20
> > > [    0.756280] [<ffffff8008439fcc>] drm_kms_helper_hotplug_event+0x28/0x34
> > > [    0.756286] [<ffffff800846c444>] cdn_dp_pd_event_work+0x394/0x3c4
> > > [    0.756295] [<ffffff80080b2b38>] process_one_work+0x218/0x3e0
> > > [    0.756302] [<ffffff80080b3538>] worker_thread+0x2e8/0x404
> > > [    0.756308] [<ffffff80080b7e70>] kthread+0xe8/0xf0
> > > [    0.756316] [<ffffff8008082690>] ret_from_fork+0x10/0x40
> > >
> > > Signed-off-by: Mark Yao <mark.yao@xxxxxxxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > index 81f9548..e6bd0f4 100644
> > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > > @@ -170,7 +170,7 @@ static void rockchip_drm_output_poll_changed(struct drm_device *dev)
> > >          struct rockchip_drm_private *private = dev->dev_private;
> > >          struct drm_fb_helper *fb_helper = &private->fbdev_helper;
> > >
> > > -       if (fb_helper)
> > > +       if (fb_helper->dev)
> > >                  drm_fb_helper_hotplug_event(fb_helper);
> > Food for thought:
> >
> > Quick grep shows that no other drivers have such a ->dev check. Does
> > this mean that either the issue is rockchip specific?
> > If not, one could look into resolving the problem directly in drm core.
> >
> > Or at least update the other users, so they don't stumble upon the problem?
> The fbdev helpers support already handling hotplug events before you
> have finalized the fbdev setup. Please read the kerneldoc for the
> various fbdev functions, they explain what you should be doing. This
> hack here should indeed not be needed.
> -Daniel

Hi Daniel

Right, the doc[0] already detail this:
    It is possible, though perhaps somewhat tricky, to implement race-free hotplug detection using
    the fbdev helpers. The drm_fb_helper_prepare() helper must be called first to initialize the
    minimum required to make hotplug detection work.Drivers also need to make sure to properly
    set up the drm_mode_config.funcs member. After calling drm_kms_helper_poll_init() it is safe to
    enable interrupts and start processing hotplug events.

The problem is drm/rockchip do the wrong initial, call drm_kms_helper_poll_init before fbdev setup.

will fix it at next version.

[0]: https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms-helpers.html#fbdev-helper-functions-reference

Best regards.

--
ïark Yao