From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mario Kleiner Subject: Re: [PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup() Date: Thu, 07 Aug 2014 02:50:31 +0200 Message-ID: <53E2CD57.2090307@gmail.com> References: <1407288166-19881-1-git-send-email-mario.kleiner.de@gmail.com> <1407288166-19881-4-git-send-email-mario.kleiner.de@gmail.com> <20140806105141.GQ4193@intel.com> <20140806135719.GM8727@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-wi0-f172.google.com (mail-wi0-f172.google.com [209.85.212.172]) by gabe.freedesktop.org (Postfix) with ESMTP id 531D36E704 for ; Wed, 6 Aug 2014 17:50:35 -0700 (PDT) Received: by mail-wi0-f172.google.com with SMTP id n3so9729619wiv.11 for ; Wed, 06 Aug 2014 17:50:34 -0700 (PDT) In-Reply-To: <20140806135719.GM8727@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 , =?windows-1252?Q?Ville_Syrj=E4l=E4?= Cc: airlied@redhat.com, dri-devel@lists.freedesktop.org, stable@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org On 08/06/2014 03:57 PM, Daniel Vetter wrote: > On Wed, Aug 06, 2014 at 01:51:41PM +0300, Ville Syrj=E4l=E4 wrote: >> On Wed, Aug 06, 2014 at 03:22:46AM +0200, Mario Kleiner wrote: >>> Calling vblank_disable_fn() will cause that function to no-op >>> if !dev->vblank_disable_allowed for some kms drivers, e.g., >>> on nouveau-kms. This can cause the gpu vblank irq's to not get >>> disabled before freeing the dev->vblank array, so if a >>> vblank irq fires and calls into drm_handle_vblank() after >>> drm_vblank_cleanup() completes, it will cause use-after-free >>> access to dev->vblank array. >>> >>> Call vblank_disable_and_save unconditionally, so vblank irqs >>> are guaranteed to be off, before we delete the data structures >>> on which they operate. >>> >>> Signed-off-by: Mario Kleiner >>> Cc: stable@vger.kernel.org > Imo cc: stable isn't justified for these patches which fix stuff that > normal users don't really see (driver load failure and module reload for > kms drivers never tends to happen for normal users). > > So I've dropped that and pulled the 2 patches Ville reviewd into my > topic/vblank-rework branch for 3.18. > > Thanks, Daniel Ok, good with me, thanks. Ville, thanks for the review. I'll review and = test your vblank series next week when i have access to suitable = machines and enough time. I need to go through this in single-step mode, = vblank on/off changes always make me nervous, given how dependent my = main application is on this for its timing, so i want to move through it = in slow motion. Btw. wrt to nouveau "No idea what games nouveau is playign with that = flag, but this patch should be fine at least for drivers that don't do = such things." (Villes comment). Nouveau currently doesn't support hw vblank counter queries at all. The = dev->driver->get_vblank_count() is just hooked up to drm_vblank_count(), = so it's a no-op. Therefore nouveau can't allow disabling of vblank irq = during "normal" operation as it would lose all vblank counts during the = off period. That's why it leaves dev->vblank_disable_allowed =3D false; Pre NV-50 apparently doesn't have any hw vblank counter register, but = NV50+ seems to have one. I'll probably give implementing this a try for = 3.18 if nobody else does. I'm not sure about all the new embedded drivers, if they have hw vblank = counters? thanks, -mario > >> No idea what games nouveau is playign with that flag, but this patch >> should be fine at least for drivers that don't do such things. >> >> Reviewed-by: Ville Syrj=E4l=E4 >> >>> --- >>> drivers/gpu/drm/drm_irq.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c >>> index 89e91e3..22e2bba9 100644 >>> --- a/drivers/gpu/drm/drm_irq.c >>> +++ b/drivers/gpu/drm/drm_irq.c >>> @@ -164,6 +164,7 @@ static void vblank_disable_fn(unsigned long arg) >>> void drm_vblank_cleanup(struct drm_device *dev) >>> { >>> int crtc; >>> + unsigned long irqflags; >>> = >>> /* Bail if the driver didn't call drm_vblank_init() */ >>> if (dev->num_crtcs =3D=3D 0) >>> @@ -171,7 +172,9 @@ void drm_vblank_cleanup(struct drm_device *dev) >>> = >>> for (crtc =3D 0; crtc < dev->num_crtcs; crtc++) { >>> del_timer_sync(&dev->vblank[crtc].disable_timer); >>> - vblank_disable_fn((unsigned long)&dev->vblank[crtc]); >>> + spin_lock_irqsave(&dev->vbl_lock, irqflags); >>> + vblank_disable_and_save(dev, crtc); >>> + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); >>> } >>> = >>> kfree(dev->vblank); >>> -- = >>> 1.9.1 >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >> -- = >> Ville Syrj=E4l=E4 >> Intel OTC >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel