From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support Date: Wed, 17 Dec 2014 09:02:53 +0100 Message-ID: <20141217080252.GA3303@ulmo> References: <1416597001-6097-1-git-send-email-yao.cheng@intel.com> <20141121202702.GA10379@mithrandir> <20141121203633.GB25711@phenom.ffwll.local> <20141124095544.GA25912@ulmo.nvidia.com> <20141124131448.GP25711@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1554009468==" Return-path: In-Reply-To: <20141124131448.GP25711@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: daniel.vetter@ffwll.ch, emil.l.velikov@gmail.com, dri-devel@lists.freedesktop.org, john.chehab@intel.com, fei.jiang@intel.com, intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============1554009468== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="x+6KMIRAuhnl3hBn" Content-Disposition: inline --x+6KMIRAuhnl3hBn Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Nov 24, 2014 at 02:14:48PM +0100, Daniel Vetter wrote: > On Mon, Nov 24, 2014 at 10:55:46AM +0100, Thierry Reding wrote: > > On Fri, Nov 21, 2014 at 09:36:33PM +0100, Daniel Vetter wrote: > > > On Fri, Nov 21, 2014 at 09:27:04PM +0100, Thierry Reding wrote: > > > > On Sat, Nov 22, 2014 at 03:10:01AM +0800, Yao Cheng wrote: > > > > > on vlv, if ipvr is installed, it need be manually unloaded before > > > > > i915, otherwise user might run into use-after-free issue. > > > >=20 > > > > Huh? That doesn't sound right. What exactly is it that's going wron= g? > > > > You should never have to do this. If you do you're almost certainly > > > > doing something wrong in the kernel module. > > >=20 > > > It's the hilarity called platform devices. Removing them is somewhat = racy, > > > so doing that upfront makes the entire thing a bit safer. The use aft= er > > > free is on the text, since grabbing a module refcount for the platform > > > device doesn't work (it would pin the module forever). > >=20 > > I don't understand what the issue is here. I've used platform devices > > quite extensively on ARM and I've never encountered a situation where > > they were insufficient (or racy for that matter). > >=20 > > If I understand correctly what this commit tries to achieve, then it > > unloads one module before another module that it depends on so that the > > dependency can be removed subsequently without causing a crash. That > > sounds really brittle to me. How are you going to document this for > > users so that they don't accidentally go and unload the i915 module and > > crash their system? >=20 > Module unloading taints your kernel and isn't an end-user supported > feature. That simple ;-) >=20 > Also afaik the problem is that you actually can't unload i915 until you've > unloaded the subordinate driver, since i915 registering the platform > driver prevents unload. That doesn't sound at all like use-after-free, so if that's really the only problem then the commit description should be more accurate. > Or at least that was my understanding, I didn't test this myself. I > just asked whether the unload script still works and apparently it > breaks. >=20 > I guess what's different with ARM is that DT creates all the platform > devices, and not modules themselves? No, I don't think that has anything to do with it. I'm pretty sure I've seen this work reliably with something like MFD where one module can create a number of platform devices, and remove them again, just as well. Thierry --x+6KMIRAuhnl3hBn Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUkTisAAoJEN0jrNd/PrOhy/kQALX40mrplXfr7EDO3YOLdNtw DMWlwlo5WqDkB8DmIiiqJWXUb+r2dOrE/+eR2y9SCn66lGjxUVW9o65bJqxT/Qxo 5yF4K6EfnDeupcBzgLXXvfjFFqzzqo75vm3x8rzphx4RvngGsbzLDMHYl+yQIiad hSYtg43i+UXX+//6pcKzz/3TBT0vZaUiJI6owSMP84YGfJ1R7zY1hUIjTrzo4fdb sDf8n1oooTii36d2AQy1os+6AqnhqtmGHEzyoXddJnDVJQxIxpJ778R1kJXz1Rno Y3Weg/YhKENQjl5u58f7yhTPYhwxCw8pGqVDfvLpl/FBzmflsObKfYez49PQs/3l +QvT16DDKQ0Sxr0urHWUUMBW+J2+BoODmVkKoU4RIiqE62eOLf9SodtKKFUu3n5i yGGDW8UGgJ8HnRTu1dHXvyB+nXXfEvxwUWC9gV1iRxGUsJ698giONbcySKNd5xZy Gf9gzxqKRkthqSf4Uz4RGQ1rBekU3RSKQJZfcfzd27+MngbYvBv6m8NWRHzwXxZ8 44c5v7DQaxofjnuhV6iF1q6X3m/3+hoKrcoOx02OTmteEqzUKr94E8dnrPS7VPiv bL1JDKef8z5jmjVRw+ZeEbmilYaDFh4TCtWqcDtdB1Dwg6L0HjtabNv8aUo5OBkM aInj8ybdzWkBP0Ql+6z1 =B6mg -----END PGP SIGNATURE----- --x+6KMIRAuhnl3hBn-- --===============1554009468== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============1554009468==--