[PATCH] unload input modules when they are no longer used
Peter Hutterer
peter.hutterer at who-t.net
Thu Jun 21 18:55:34 PDT 2012
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 <hramrach at gmail.com>
> 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 <hramrach at gmail.com>
> ---
> 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 <hramrach at gmail.com>
> 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 <hramrach at gmail.com>
> ---
> 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 <hramrach at gmail.com>
> Date: Mon, 4 Jun 2012 17:29:37 +0200
> Subject: [PATCH 3/3] xfree86: unload unused input drivers.
>
> Signed-off-by: Michal Suchanek <hramrach at gmail.com>
> ---
> 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
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
More information about the xorg-devel
mailing list