From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH] drm: vc4: Fix race during binding Date: Sun, 08 Oct 2017 10:09:29 -0700 Message-ID: <87zi91bm7q.fsf@anholt.net> References: <1507315383-8107-1-git-send-email-stefan.wahren@i2se.com> <87y3oo2hbx.fsf@anholt.net> <1623255519.108982.1507363997162@email.1und1.de> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1845191415==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id C22316E256 for ; Sun, 8 Oct 2017 17:09:36 +0000 (UTC) In-Reply-To: <1623255519.108982.1507363997162@email.1und1.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Stefan Wahren Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1845191415== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Stefan Wahren writes: > Hi Eric, > >> Eric Anholt hat am 6. Oktober 2017 um 21:42 geschriebe= n: >>=20 >>=20 >> Stefan Wahren writes: >>=20 >> > This fixes the race between vc4_overflow_mem_work and the init of the >> > job lock. Otherwise we could trigger a NULL pointer dereference >> > during VC4 binding. >> > >> > Link: https://github.com/anholt/linux/issues/114 >> > Signed-off-by: Stefan Wahren >> > Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.") >> > --- >> > drivers/gpu/drm/vc4/vc4_gem.c | 1 - >> > drivers/gpu/drm/vc4/vc4_irq.c | 1 + >> > 2 files changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_g= em.c >> > index d0c6bfb..47d964f 100644 >> > --- a/drivers/gpu/drm/vc4/vc4_gem.c >> > +++ b/drivers/gpu/drm/vc4/vc4_gem.c >> > @@ -1088,7 +1088,6 @@ vc4_gem_init(struct drm_device *dev) >> > INIT_LIST_HEAD(&vc4->render_job_list); >> > INIT_LIST_HEAD(&vc4->job_done_list); >> > INIT_LIST_HEAD(&vc4->seqno_cb_list); >> > - spin_lock_init(&vc4->job_lock); >> >=20=20 >> > INIT_WORK(&vc4->hangcheck.reset_work, vc4_reset_work); >> > setup_timer(&vc4->hangcheck.timer, >> > diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_i= rq.c >> > index 7d7af3a..d508d13 100644 >> > --- a/drivers/gpu/drm/vc4/vc4_irq.c >> > +++ b/drivers/gpu/drm/vc4/vc4_irq.c >> > @@ -195,6 +195,7 @@ vc4_irq_preinstall(struct drm_device *dev) >> > struct vc4_dev *vc4 =3D to_vc4_dev(dev); >> >=20=20 >> > init_waitqueue_head(&vc4->job_wait_queue); >> > + spin_lock_init(&vc4->job_lock); >> > INIT_WORK(&vc4->overflow_mem_work, vc4_overflow_mem_work); >> >=20=20 >> > /* Clear any pending interrupts someone might have left around >>=20 >> Are you sure this is a fix? We don't attach the IRQ handler until V3D >> bind, and vc4_gem_init happens before component_bind_all(), right? > > i agree i should have mark it as a RFC. But is it really impossible > that vc4_overflow_mem_work is triggered before vc4_gem_init? As far as I can see, it gets queued from the IRQ handler, the IRQ handler is added during V3D bind, and binding happens after GEM init. Hmm. If we fail out of component binding and try again, we'll end up doing the job_wait_queue and overflow_mem_work initialization on the same pointer twice. Will that cause any trouble? We cancel any pending work during uninstall (V3D unbind path), but does drm_irq_uninstall() make sure that the irq handler has finished? --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlnaW8kACgkQtdYpNtH8 nuhKWg/+Jhr9LNF+yfGKlayPZZJ7J3pwvJ69+GnWccNiWfDyq+Xrk4buGDvIENuo z0NLcwpGUoyin+Z4SJk7fjUl6Df6kWK42Jk/o+exawzWu16oguhDt7B0wKz5WuRT 6DwAodu8rhuNnugKu6MsywslTpjxa7xwEJAL1lulicVfd/AxGArBU9Kt7013WLj0 1kAO7mEzJyogB521L0cphw6f2xSi+PCIKw6vLCrE86hkOqSRp7jMzNI3KTcbU3Qt dLtQ3B8MzCul6Y0hVPQN5PxZhm3bCsVA0Pvxb5ADLB1bR1qCn+peE5uK1opd3BCI pqL3t2zVEMPZ8SjKeLjpe8vAF4zNLMXXDWPE0OiqLfQ2T+vNYRsNoV4ue3plBMKe qT3SVP/87VxiWBWOLT46VLJWcozi3QnMFNGKLLrStNDjdFjCglBFc2HaiezATOPX Ut4LC/OFnU4bGOxVBhab1iqIEdaIQpWbwMJNTiUCyBbreQmiLUE7nhY++Ogth4U1 GjpjmeQMTlYQYwETX/kk1qtWwtXmBVV8hjL4zZVQsEGBprXUYDUXCxUEPUIVjGm/ geAJ+Hfdgi8n0C2ZpylUEhLMz58vBGTjWyUFPlUr/THcW7KmfckQcVbcN4adhluB ak/t5TJqLavhLnb03AMJ2P1qd7c2w8VfWWsSkm5nRaD+u5DCrsw= =LgeC -----END PGP SIGNATURE----- --=-=-=-- --===============1845191415== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1845191415==--