From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [Intel-gfx] [PATCH 09/19] drm: Fix race between drm_vblank_off() and drm_queue_vblank_event() Date: Wed, 6 Aug 2014 16:33:51 +0300 Message-ID: <20140806133351.GU4193@intel.com> References: <1407325803-6944-1-git-send-email-ville.syrjala@linux.intel.com> <1407325803-6944-10-git-send-email-ville.syrjala@linux.intel.com> <20140806132301.GK8727@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Content-Disposition: inline In-Reply-To: <20140806132301.GK8727@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Wed, Aug 06, 2014 at 03:23:01PM +0200, Daniel Vetter wrote: > On Wed, Aug 06, 2014 at 02:49:52PM +0300, ville.syrjala@linux.intel.com w= rote: > > From: Ville Syrj=E4l=E4 > > = > > Currently it's possible that the following will happen: > > 1. drm_wait_vblank() calls drm_vblank_get() > > 2. drm_vblank_off() gets called > > 3. drm_wait_vblank() calls drm_queue_vblank_event() which > > adds the event to the queue event though vblank interrupts > > are currently disabled (and may not be re-enabled ever again). > > = > > To fix the problem, add another vblank->enabled check into > > drm_queue_vblank_event(). > > = > > drm_vblank_off() holds event_lock around the vblank disable, > > so no further locking needs to be added to drm_queue_vblank_event(). > > vblank disable from another source is not possible since > > drm_wait_vblank() already holds a vblank reference. > > = > > Reviewed-by: Matt Roper > > Signed-off-by: Ville Syrj=E4l=E4 > = > I guess the window is too small here to make this reproducible in a test? I must admit that I didn't even try. I supposed I could try it now. > Especially since each attempt will take a few hundred ms ... > -Daniel > = > > --- > > drivers/gpu/drm/drm_irq.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > = > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > > index 9353609..b2428cb 100644 > > --- a/drivers/gpu/drm/drm_irq.c > > +++ b/drivers/gpu/drm/drm_irq.c > > @@ -1270,6 +1270,7 @@ static int drm_queue_vblank_event(struct drm_devi= ce *dev, int pipe, > > union drm_wait_vblank *vblwait, > > struct drm_file *file_priv) > > { > > + struct drm_vblank_crtc *vblank =3D &dev->vblank[pipe]; > > struct drm_pending_vblank_event *e; > > struct timeval now; > > unsigned long flags; > > @@ -1293,6 +1294,18 @@ static int drm_queue_vblank_event(struct drm_dev= ice *dev, int pipe, > > = > > spin_lock_irqsave(&dev->event_lock, flags); > > = > > + /* > > + * drm_vblank_off() might have been called after we called > > + * drm_vblank_get(). drm_vblank_off() holds event_lock > > + * around the vblank disable, so no need for further locking. > > + * The reference from drm_vblank_get() protects against > > + * vblank disable from another source. > > + */ > > + if (!vblank->enabled) { > > + ret =3D -EINVAL; > > + goto err_unlock; > > + } > > + > > if (file_priv->event_space < sizeof e->event) { > > ret =3D -EBUSY; > > goto err_unlock; > > -- = > > 1.8.5.5 > > = > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > = > -- = > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- = Ville Syrj=E4l=E4 Intel OTC