Re: [RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support

From: "Cheng, Yao" <yao.cheng@intel.com>
To: Daniel Vetter <daniel@ffwll.ch>,
	Thierry Reding <thierry.reding@gmail.com>
Cc: "daniel.vetter@ffwll.ch" <daniel.vetter@ffwll.ch>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"emil.l.velikov@gmail.com" <emil.l.velikov@gmail.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"Chehab, John" <john.chehab@intel.com>,
	"Jiang, Fei" <fei.jiang@intel.com>
Subject: Re: [RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support
Date: Mon, 1 Dec 2014 03:06:08 +0000	[thread overview]
Message-ID: <8FF7D634BEE4C2428EFFAB6B7E919E4B01835053@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <20141124131448.GP25711@phenom.ffwll.local>

> -----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; Chehab,
> John; emil.l.velikov@gmail.com; Jiang, Fei
> Subject: Re: [RFC PATCH v3 4/4] tests/drv_module_reload: add ipvr support
> 
> 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 wrong?
> > > > 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?
> 
> Module unloading taints your kernel and isn't an end-user supported feature.
> That simple ;-)
> 
> 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. 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.
> 
> I guess what's different with ARM is that DT creates all the platform devices,
> and not modules themselves?
> -Daniel

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"). This causes issue when I modprobe i915 and ipvr again later. 
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)".

> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx