On Fri, Jun 08, 2012 at 04:19:17PM +0200, Michal Suchanek wrote: > Hello, > > sending the patch witch should fix issue with unloading sibling > modules along with a couple of patches that allow actually unloading > modules.
in the future, please send patches out separately. it's a bit of a pain to review and comment otherwise, especially as I have to edit the mimetype for from application/octet-stream to text/plain all of them. > I can unload a wacom module when wacom tablet is unplugged. > > Tested on X 1.12 as master requires some libraries I don't have to build. > > The part with moving around the input global array is not really > tested because the unloaded driver was the last anyway. > > There are changes which might break ABI but perhaps they would not. > > The ModuleDesc size changes but that is allocated by loader anyway. > > The DeleteInputModule interface changes but there are no users. AFAIK > this patch introduces the first. > > Thanks > > Michal > From 9fb5c9a05fabd6f6d1c46c09c1d3c9671aa41a7c Mon Sep 17 00:00:00 2001 > From: Michal Suchanek <[email protected]> > Date: Mon, 4 Jun 2012 14:22:14 +0200 > Subject: [PATCH 1/3] xfree86/loader: Do not unload sibling modules. generally, for all but the most straightforward of changes we need some explanatory commit message. you may know why you did this change now, but in a years time someone looking at it doesn't have this knowledge anymore. so please explain why this change is needed. http://who-t.blogspot.com.au/2009/12/on-commit-messages.html > Signed-off-by: Michal Suchanek <[email protected]> > --- > hw/xfree86/loader/loadmod.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c > index 706b9b3..bd5e5fa 100644 > --- a/hw/xfree86/loader/loadmod.c > +++ b/hw/xfree86/loader/loadmod.c > @@ -1103,10 +1103,21 @@ UnloadModuleOrDriver(ModuleDescPtr mod) > LoaderUnload(mod->name, mod->handle); > } > > - if (mod->child) > + while (mod->child) > UnloadModuleOrDriver(mod->child); > - if (mod->sib) > - UnloadModuleOrDriver(mod->sib); > + if (mod->parent) { > + ModuleDescPtr sib_list = mod->parent->child; > + if (sib_list == mod) > + mod->parent->child = mod->sib; > + else braces for this block please > + while (sib_list) { > + if (sib_list->sib == mod) { > + sib_list->sib = mod->sib; > + break; > + } > + sib_list = sib_list -> sib; no spaces around -> > + } > + } > free(mod->path); > free(mod->name); > free(mod); > -- > 1.7.10 > > From 21bc14873e7131a51358c94c98510d6f5c14124e Mon Sep 17 00:00:00 2001 > From: Michal Suchanek <[email protected]> > Date: Mon, 4 Jun 2012 16:46:03 +0200 > Subject: [PATCH 2/3] xfree86/loader: add back silly referece counting. [ABI typo "referece". also, I had to look up the history to get the "silly reference counting", erm, reference. again, we need more explanatory commit messages. Is this really an ABI change? I don't see this used outside the loard. > change] > > Signed-off-by: Michal Suchanek <[email protected]> > --- > hw/xfree86/loader/loaderProcs.h | 2 ++ > hw/xfree86/loader/loadmod.c | 21 +++++++++++++-------- > 2 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/hw/xfree86/loader/loaderProcs.h b/hw/xfree86/loader/loaderProcs.h > index a7b752b..9349f76 100644 > --- a/hw/xfree86/loader/loaderProcs.h > +++ b/hw/xfree86/loader/loaderProcs.h > @@ -66,6 +66,8 @@ typedef struct module_desc { > ModuleTearDownProc TearDownProc; > void *TearDownData; /* returned from SetupProc */ > const XF86ModuleVersionInfo *VersionInfo; > + struct module_desc *mold; what does "mold" stand for? why not use "mod_desc", or "original" or something more obvious? > + int count; "refcount", please > } ModuleDesc, *ModuleDescPtr; > > /* External API for the loader */ > diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c > index bd5e5fa..65ec9d9 100644 > --- a/hw/xfree86/loader/loadmod.c > +++ b/hw/xfree86/loader/loadmod.c > @@ -93,8 +93,6 @@ const ModuleVersions LoaderVersionInfo = { > ABI_FONT_VERSION > }; > > -static int ModuleDuplicated[] = { }; > - > static void > FreeStringList(char **paths) > { > @@ -816,10 +814,11 @@ DuplicateModule(ModuleDescPtr mod, ModuleDescPtr parent) > return NULL; > > ret->handle = mod->handle; > - > + /* some code may check the procs are the same */ > ret->SetupProc = mod->SetupProc; > ret->TearDownProc = mod->TearDownProc; > - ret->TearDownData = ModuleDuplicated; > + ret->mold = (mod->mold) ? (mod->mold) : mod; > + ret->mold->count++; this isn't safe. having ret->mold->count and ret->count is just asking for trouble. this calls for getter/setter functions that do the right thing instead of offloading this to the caller. > ret->child = DuplicateModule(mod->child, ret); > ret->sib = DuplicateModule(mod->sib, parent); > ret->parent = parent; > @@ -1097,10 +1096,16 @@ UnloadModuleOrDriver(ModuleDescPtr mod) > else > xf86MsgVerb(X_INFO, 3, "UnloadModule: \"%s\"\n", mod->name); > > - if (mod->TearDownData != ModuleDuplicated) { > - if ((mod->TearDownProc) && (mod->TearDownData)) > - mod->TearDownProc(mod->TearDownData); > - LoaderUnload(mod->name, mod->handle); > + if (mod->mold) { > + mod->mold->count--; > + } else { > + if (mod->count) > + xf86MsgVerb(X_WARNING, 3, "Cannot unload duplicated module: > \"%s\"\n", mod->name); > + else { > + if ((mod->TearDownProc) && (mod->TearDownData)) > + mod->TearDownProc(mod->TearDownData); > + LoaderUnload(mod->name, mod->handle); > + } > } > > while (mod->child) > -- > 1.7.10 > > From 438aa2a00441d3be460f95dbbd917d0b8bd1a874 Mon Sep 17 00:00:00 2001 > From: Michal Suchanek <[email protected]> > Date: Mon, 4 Jun 2012 17:29:37 +0200 > Subject: [PATCH 3/3] xfree86: unload unused input drivers. > > Signed-off-by: Michal Suchanek <[email protected]> > --- > hw/xfree86/common/xf86Helper.c | 16 ++++++++++++++-- > hw/xfree86/common/xf86Xinput.c | 4 ++++ > hw/xfree86/common/xf86Xinput.h | 2 +- > 3 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/hw/xfree86/common/xf86Helper.c b/hw/xfree86/common/xf86Helper.c > index fb56a0b..10074a9 100644 > --- a/hw/xfree86/common/xf86Helper.c > +++ b/hw/xfree86/common/xf86Helper.c > @@ -127,12 +127,24 @@ xf86AddInputDriver(InputDriverPtr driver, pointer > module, int flags) > } for this hunk: please use empty lines to make this code more readable. as it is, it's just a mass of text. > void > -xf86DeleteInputDriver(int drvIndex) > -{ > +xf86DeleteInputDriver(InputDriverPtr drv) > +{ > + int drvIndex; > + for (drvIndex = 0; drvIndex < xf86NumInputDrivers; drvIndex ++) > + if (xf86InputDriverList[drvIndex] == drv) break; > + if ( !(drvIndex < xf86NumInputDrivers)) return; /* not found */ no space before !(drvIndex..., newline before return. aside, !(a < b) should be expressed as (a >= b) or (a == b) > + if (xf86InputDriverList[drvIndex] && > xf86InputDriverList[drvIndex]->module && > + ((ModuleDescPtr)xf86InputDriverList[drvIndex]->module)->count) > + return; /* cannot unload yet */ urgh, no. why does the caller need to care about the module count at all? the input code should just call unload and let the loader sort it out. > if (xf86InputDriverList[drvIndex] && > xf86InputDriverList[drvIndex]->module) > UnloadModule(xf86InputDriverList[drvIndex]->module); > free(xf86InputDriverList[drvIndex]); > xf86InputDriverList[drvIndex] = NULL; > + if (drvIndex + 1 < xf86NumInputDrivers) > + memmove(xf86InputDriverList[drvIndex], > xf86InputDriverList[drvIndex+1], > + sizeof(xf86InputDriverList[drvIndex]) * (xf86NumInputDrivers > - drvIndex - 1)); > + xf86NumInputDrivers--; > + xf86InputDriverList[xf86NumInputDrivers] = NULL; this sounds like a prime target for a xorg_list switch to avoid this code. Cheers, Peter > } > InputDriverPtr > diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c > index bee407b..ff3b5e9 100644 > --- a/hw/xfree86/common/xf86Xinput.c > +++ b/hw/xfree86/common/xf86Xinput.c > @@ -753,6 +753,10 @@ xf86DeleteInput(InputInfoPtr pInp, int flags) > if (pInp->module) > UnloadModule(pInp->module); > > + /* Attempt to unload the driver */ > + if (pInp->drv) > + xf86DeleteInputDriver(pInp->drv); > + > /* This should *really* be handled in drv->UnInit(dev) call instead, but > * if the driver forgets about it make sure we free it or at least crash > * with flying colors */ > diff --git a/hw/xfree86/common/xf86Xinput.h b/hw/xfree86/common/xf86Xinput.h > index 1d4363a..12ce0be 100644 > --- a/hw/xfree86/common/xf86Xinput.h > +++ b/hw/xfree86/common/xf86Xinput.h > @@ -180,7 +180,7 @@ InputInfoPtr xf86AllocateInput(void); > /* xf86Helper.c */ > extern _X_EXPORT void xf86AddInputDriver(InputDriverPtr driver, pointer > module, > int flags); > -extern _X_EXPORT void xf86DeleteInputDriver(int drvIndex); > +extern _X_EXPORT void xf86DeleteInputDriver(InputDriverPtr drv); > extern _X_EXPORT InputDriverPtr xf86LookupInputDriver(const char *name); > extern _X_EXPORT InputInfoPtr xf86LookupInput(const char *name); > extern _X_EXPORT void xf86DeleteInput(InputInfoPtr pInp, int flags); > -- > 1.7.10 > > _______________________________________________ > [email protected]: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
