Re: [PATCH] drm/probe-helper: don't lose hotplug event

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Jan 25, 2015 at 12:37:31PM -0500, Rob Clark wrote:
> On Wed, Jan 21, 2015 at 4:40 PM, Rob Clark <robdclark@xxxxxxxxx> wrote:
> > On Wed, Jan 21, 2015 at 9:41 AM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
> >> There's a race window (small for hpd, 10s large for polled outputs)
> >> where userspace could sneak in with an unrelated connnector probe
> >> ioctl call and eat the hotplug event (since neither the hpd nor the
> >> poll code see a state change).
> >>
> >> To avoid this, check whether the connector state changes in all other
> >> ->detect calls (in the current helper code that's only probe_single)
> >> and if that's the case, fire off a hotplug event. Note that we can't
> >> directly call the hotplug event handler, since that expects that no
> >> locks are held (due to reentrancy with the fb code to update the kms
> >> console).
> >>
> >> Also, this requires that drivers using the probe_single helper
> >> function set up the poll work. All current drivers do that already,
> >> and with the reworked hpd handling there'll be no downside to
> >> unconditionally setting up the poll work any more.
> >>
> >> v2: Review from Rob Clark
> >> - Don't bail out of the output poll work immediately if it's disabled
> >>   to make sure we deliver the delayed hoptplug events. Instead just
> >>   jump to the tail.
> >> - Don't scheduel the work when it's not set up. Would be a driver bug
> >>   since using probe helpers for anything dynamic without them
> >>   initialized makes them all noops.
> >>
> >> Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> (v1)
> >> Cc: Rob Clark <robdclark@xxxxxxxxx>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >
> > Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>
> >
> > and with the monitor that started all this, plus the addition of:
> > http://lists.freedesktop.org/archives/dri-devel/2015-January/075901.html
> >
> > Tested-by: Rob Clark <robdclark@xxxxxxxxx>
> 
> bleh, but on latest upstream (plus drm-next, etc).. possibly just not
> caught earlier due to insufficient debug stuff enabled..
> 
> I could probably solve this by moving drm_kms_helper_poll_init()
> earlier.. otoh that might cause other surprises and other drivers also
> seem to do this late in their load..

Well I'm confused - I check for poll_enabled and that is only set _after_
INIT_DELAYED_WORK. Which should prevent schedule_delayed_work on an
uninitialized work and was the entire point of v2. Nothing else writes to
poll_enabled. What do I miss?

Totally confused ...
-Daniel

> 
> [    4.053197] WARNING: CPU: 3 PID: 52 at ../kernel/workqueue.c:1427
> __queue_delayed_work+0x14c/0x15c()
> [    4.058040] Modules linked in:
> [    4.070017] CPU: 3 PID: 52 Comm: kworker/u8:1 Tainted: G     U
>    3.19.0-rc5-00821-g415f9e0-dirty #1113
> [    4.070107] Hardware name: Qualcomm (Flattened Device Tree)
> [    4.080016] Workqueue: deferwq deferred_probe_work_func
> [    4.090549] [<c021654c>] (unwind_backtrace) from [<c0211f68>]
> (show_stack+0x10/0x14)
> [    4.090714] [<c0211f68>] (show_stack) from [<c091d3bc>]
> (dump_stack+0x8c/0x9c)
> [    4.098519] [<c091d3bc>] (dump_stack) from [<c0246a2c>]
> (warn_slowpath_common+0x84/0xb4)
> [    4.105547] [<c0246a2c>] (warn_slowpath_common) from [<c0246af8>]
> (warn_slowpath_null+0x1c/0x24)
> [    4.113790] [<c0246af8>] (warn_slowpath_null) from [<c0259b5c>]
> (__queue_delayed_work+0x14c/0x15c)
> [    4.122556] [<c0259b5c>] (__queue_delayed_work) from [<c0259bbc>]
> (queue_delayed_work_on+0x50/0x5c)
> [    4.131337] [<c0259bbc>] (queue_delayed_work_on) from [<c0561114>]
> (drm_helper_probe_single_connector_modes_merge_bits+0x2fc/0x49c)
> [    4.140290] [<c0561114>]
> (drm_helper_probe_single_connector_modes_merge_bits) from [<c0569824>]
> (drm_fb_helper_probe_connector_modes.isra.5+0x4c/0x6c)
> [    4.152089] [<c0569824>]
> (drm_fb_helper_probe_connector_modes.isra.5) from [<c056a8a8>]
> (drm_fb_helper_initial_config+0x34/0x3d8)
> [    4.165624] [<c056a8a8>] (drm_fb_helper_initial_config) from
> [<c05a62dc>] (msm_fbdev_init+0x70/0xa0)
> [    4.177342] [<c05a62dc>] (msm_fbdev_init) from [<c05a1614>]
> (msm_load+0x268/0x36c)
> [    4.186537] [<c05a1614>] (msm_load) from [<c0573af4>]
> (drm_dev_register+0xa8/0x104)
> [    4.193912] [<c0573af4>] (drm_dev_register) from [<c05759dc>]
> (drm_platform_init+0x44/0xd8)
> [    4.201475] [<c05759dc>] (drm_platform_init) from [<c05c1654>]
> (try_to_bring_up_master.part.3+0xc8/0x108)
> [    4.209804] [<c05c1654>] (try_to_bring_up_master.part.3) from
> [<c05c1740>] (component_master_add_with_match+0xac/0x124)
> [    4.219525] [<c05c1740>] (component_master_add_with_match) from
> [<c05a116c>] (msm_pdev_probe+0x64/0x70)
> [    4.230116] [<c05a116c>] (msm_pdev_probe) from [<c05c73c8>]
> (platform_drv_probe+0x44/0xa4)
> [    4.239485] [<c05c73c8>] (platform_drv_probe) from [<c05c5c48>]
> (driver_probe_device+0x108/0x23c)
> [    4.247813] [<c05c5c48>] (driver_probe_device) from [<c05c42ac>]
> (bus_for_each_drv+0x64/0x98)
> [    4.256752] [<c05c42ac>] (bus_for_each_drv) from [<c05c5b10>]
> (device_attach+0x74/0x88)
> [    4.265259] [<c05c5b10>] (device_attach) from [<c05c51fc>]
> (bus_probe_device+0x84/0xa8)
> [    4.273071] [<c05c51fc>] (bus_probe_device) from [<c05c561c>]
> (deferred_probe_work_func+0x64/0x94)
> [    4.281064] [<c05c561c>] (deferred_probe_work_func) from
> [<c025a354>] (process_one_work+0x134/0x334)
> [    4.290091] [<c025a354>] (process_one_work) from [<c025ac30>]
> (worker_thread+0x4c/0x4b0)
> [    4.299383] [<c025ac30>] (worker_thread) from [<c025ecfc>]
> (kthread+0xdc/0xf4)
> [    4.307459] [<c025ecfc>] (kthread) from [<c020e978>]
> (ret_from_fork+0x14/0x3c)
> 
> BR,
> -R
> 
> >
> >> ---
> >>  drivers/gpu/drm/drm_probe_helper.c | 36 ++++++++++++++++++++++++++++++++++--
> >>  include/drm/drm_crtc.h             |  1 +
> >>  2 files changed, 35 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> >> index 2fbdcca7ca9a..33bf550a1d3f 100644
> >> --- a/drivers/gpu/drm/drm_probe_helper.c
> >> +++ b/drivers/gpu/drm/drm_probe_helper.c
> >> @@ -103,6 +103,7 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
> >>         int count = 0;
> >>         int mode_flags = 0;
> >>         bool verbose_prune = true;
> >> +       enum drm_connector_status old_status;
> >>
> >>         WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> >>
> >> @@ -121,7 +122,33 @@ static int drm_helper_probe_single_connector_modes_merge_bits(struct drm_connect
> >>                 if (connector->funcs->force)
> >>                         connector->funcs->force(connector);
> >>         } else {
> >> +               old_status = connector->status;
> >> +
> >>                 connector->status = connector->funcs->detect(connector, true);
> >> +
> >> +               /*
> >> +                * Normally either the driver's hpd code or the poll loop should
> >> +                * pick up any changes and fire the hotplug event. But if
> >> +                * userspace sneaks in a probe, we might miss a change. Hence
> >> +                * check here, and if anything changed start the hotplug code.
> >> +                */
> >> +               if (old_status != connector->status) {
> >> +                       DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
> >> +                                     connector->base.id,
> >> +                                     connector->name,
> >> +                                     old_status, connector->status);
> >> +
> >> +                       /*
> >> +                        * The hotplug event code might call into the fb
> >> +                        * helpers, and so expects that we do not hold any
> >> +                        * locks. Fire up the poll struct instead, it will
> >> +                        * disable itself again.
> >> +                        */
> >> +                       dev->mode_config.delayed_event = true;
> >> +                       if (dev->mode_config.poll_enabled)
> >> +                               schedule_delayed_work(&dev->mode_config.output_poll_work,
> >> +                                                     0);
> >> +               }
> >>         }
> >>
> >>         /* Re-enable polling in case the global poll config changed. */
> >> @@ -274,10 +301,14 @@ static void output_poll_execute(struct work_struct *work)
> >>         struct drm_device *dev = container_of(delayed_work, struct drm_device, mode_config.output_poll_work);
> >>         struct drm_connector *connector;
> >>         enum drm_connector_status old_status;
> >> -       bool repoll = false, changed = false;
> >> +       bool repoll = false, changed;
> >> +
> >> +       /* Pick up any changes detected by the probe functions. */
> >> +       changed = dev->mode_config.delayed_event;
> >> +       dev->mode_config.delayed_event = false;
> >>
> >>         if (!drm_kms_helper_poll)
> >> -               return;
> >> +               goto out;
> >>
> >>         mutex_lock(&dev->mode_config.mutex);
> >>         list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> >> @@ -319,6 +350,7 @@ static void output_poll_execute(struct work_struct *work)
> >>
> >>         mutex_unlock(&dev->mode_config.mutex);
> >>
> >> +out:
> >>         if (changed)
> >>                 drm_kms_helper_hotplug_event(dev);
> >>
> >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >> index f444263055c5..65da9fb939a7 100644
> >> --- a/include/drm/drm_crtc.h
> >> +++ b/include/drm/drm_crtc.h
> >> @@ -1089,6 +1089,7 @@ struct drm_mode_config {
> >>         /* output poll support */
> >>         bool poll_enabled;
> >>         bool poll_running;
> >> +       bool delayed_event;
> >>         struct delayed_work output_poll_work;
> >>
> >>         /* pointers to standard properties */
> >> --
> >> 2.1.4
> >>

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux