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:13:05 +0100 Message-ID: <20141217081304.GB3303@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> <8FF7D634BEE4C2428EFFAB6B7E919E4B01835053@shsmsx102.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0510720306==" Return-path: In-Reply-To: <8FF7D634BEE4C2428EFFAB6B7E919E4B01835053@shsmsx102.ccr.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: "Cheng, Yao" Cc: "daniel.vetter@ffwll.ch" , "intel-gfx@lists.freedesktop.org" , "emil.l.velikov@gmail.com" , "dri-devel@lists.freedesktop.org" , "Chehab, John" , "Jiang, Fei" List-Id: dri-devel@lists.freedesktop.org --===============0510720306== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="neYutvxvOLaeuPCA" Content-Disposition: inline --neYutvxvOLaeuPCA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Dec 01, 2014 at 03:06:08AM +0000, Cheng, Yao wrote: > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > > Vetter > > Sent: Monday, November 24, 2014 21:15 > > To: Thierry Reding > > Cc: Daniel Vetter; Cheng, Yao; intel-gfx@lists.freedesktop.org; dri- > > devel@lists.freedesktop.org; daniel.vetter@ffwll.ch; Kelley, Sean V; Ch= ehab, > > John; emil.l.velikov@gmail.com; Jiang, Fei > > Subject: Re: [RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr suppo= rt > >=20 > > 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. > > > > > > > > > > Huh? That doesn't sound right. What exactly is it that's going wr= ong? > > > > > You should never have to do this. If you do you're almost > > > > > certainly doing something wrong in the kernel module. > > > > > > > > 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 after free is on the text, since grabbing a module refcount for > > > > the platform device doesn't work (it would pin the module forever). > > > > > > 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). > > > > > > 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 fea= ture. > > 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 dr= iver > > prevents unload. Or at least that was my understanding, I didn't test t= his > > myself. I just asked whether the unload script still works and apparent= ly it > > breaks. > >=20 > > I guess what's different with ARM is that DT creates all the platform d= evices, > > and not modules themselves? > > -Daniel >=20 > Thierry/Daniel, the actual symptom is, after "rmmod i915", though > drm_drv_release() is also called on the child device "ipvr", I still > see the module exist in the system (check it by "lsmod"). Which module? ipvr or i915? > This causes issue when I modprobe i915 and ipvr again later. What issue are you seeing? If your driver can't deal with a situation where it's probed again after being removed then you have a bug. > I don't understand why this happens but I believe what Daniel said: > "grabbing a module refcount for the platform device doesn't work (it > would pin the module forever)" What I'd expect to happen is this: # modprobe i915 i915 registers a platform devices # modprobe ipvr driver core probes ipvr device # modprobe -r i915 i915 removes the platform device (ipvr's ->remove() is called) I guess if you don't do anything else, then indeed the ipvr module will stay around, but the above should work idempotently, that is you should be able to repeat it an unlimited number of times and nothing should break. In fact you should be able to run the following in any permutation without causing a crash: # modprobe i915 # modprobe ipvr # modprobe -r ipvr # modprobe -r i915 If any permutation results in a crash you have a bug. Thierry --neYutvxvOLaeuPCA Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUkTsQAAoJEN0jrNd/PrOhZT0QAKCYLdwF6FD0/cb3STkE61VU ZRZfkBKWv5y5YCXxdO5UKC+rfOQNNCZZzM6I0qGzYvHiwegvLosGj1spzsWdtDyB JRG7bRbLEQ3Sw2M7CQZDxaTZ+njOscRaHO6u6PGXlY+bdNEi0JRX/lhi4mHgipPj ugoILHU4NQODxZWBvYSw6mqBOdD0OoGsaV9EGATNYHBvU+Elhcq2njM7UM6eDutl 2Nyxlb1s4cTONtOnq4iujr0H/17ME/7UF2/KP1oPgLiHEp5mR98jL4iEI9gcg23g AdfUMGRAxL5FLqd46bg+lXtMOLXidznStgPlWpcQC+8L9hJcemSVkqy1r/ubhGGQ DDuz1NbK7WPxIhSgfzAIO6VUAXYsfCuJroTmvdOJoT/5bjiw9O4Tkn7lveW9Jl0x cEssuKyfJiX28MVB2FWpFuqYjQ9q5y0XjQkhE9mAMAo4pN5NMzJy1E4s4CxU2XQ+ EN7U8KqndfQkoxTppM/HPqAVz0tHqIePNIoM5eWzmM9kjQhZSGzEBM5PUhP4S1pu zVbAwYO0RtZfAVQ7LteLKsJknwsPJOs0vvx4OsAx7tyECcdoPe9E/EjJhSqBpBoS 8cPpFPGQZLZVUUUhSHa3Z7kjCsyg7r3yIZYy+VNjBgt9bs06lXhk3BK/OM3HnA6F 2h9pEyirCTcxQ0DVcrxI =q6ES -----END PGP SIGNATURE----- --neYutvxvOLaeuPCA-- --===============0510720306== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============0510720306==--