xf86UnloadSubModule is still buggy

[Peter Hutterer]

cc-ing Michal

On Sun, Jun 03, 2012 at 10:39:37PM +0200, Matthieu Herrb wrote:
> Hi
> 
> > commit 0d4bb5442ceb8e8e4a8de6cfc4203cae469eee72
> > Author: Michal Suchanek <hramrach at centrum.cz>
> > Date:   Sat Oct 8 14:13:33 2011 +0200
> >
> >    Unload submodules.
> >    
> >    Signed-off-by: Michal Suchanek <hramrach at centrum.cz>
> >    Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>
> >
> > diff --git a/hw/xfree86/common/xf86Helper.c
> > b/hw/xfree86/common/xf86Helper.c
> > index a8aa316..4e9bcad 100644
> > --- a/hw/xfree86/common/xf86Helper.c
> > +++ b/hw/xfree86/common/xf86Helper.c
> > @@ -1643,13 +1643,7 @@ xf86LoadOneModule(char *name, pointer opt)
> > void
> > xf86UnloadSubModule(pointer mod)
> > {
> > -    /*
> > -     * This is disabled for now.  The loader isn't smart enough yet
> > to undo
> > -     * relocations.
> > -     */
> > -#if 0
> >     UnloadSubModule(mod);
> > -#endif
> > }
> > 
> > Bool
> 
> This commit is like dropping a live hand grenade in the X server.
> 
> UnloadSubModule does not behave like you think it does.
> 
> If you look at the code, you'll see that it will unload a submodule
> and then recursively unload all siblings (and childrens) of that
> submodule. 
> 
> The problem is with siblings... generally it doesn't make sense to
> unload siblings of the submodule you're unloading.
> 
> There is at least one practical case where this is not the expected
> behaviour. It's a case dealing with an older driver, but I'm sure
> other cases can be found. 
> 
> The issue is with the intel driver in non-kms mode that we use on
> OpenBSD. 
> 
> The driver loads several sub-modules, the first one is vgahw, then it
> tries several output driver submodules: "sil164", "ch7xxx", "ivch",
> "tfp410", "ch7017", unloading each of them if it's not useful. 
> 
> Since "vgahw" is the first loaded submoudle, it became a sibling of
> sil164, and gets un-loaded when the code unloads the latter. But since
> no-one intened to remove it, there are still tons of references to
> that module.
> 
> This issue goes unoticed for 2 reasons: most of the submodules were
> turned into built-ins recently, and Linux still doesn't overwrite
> memory in free() so the X server can keep on using the unloaded
> modules for a while.
> 
> I suggest to revert that commit untill a proper solution for unloading
> only a sub-module is found.

there are three patches that would need a revert here to undo the
af3f64fb77c13180e513ee99d1fd9a1b624fd8ea merge (the background -none
documentation can stay). Michal, opinons?
 
Cheers,
  Peter