[PATCH] unload input modules when they are no longer used

[Peter Hutterer]

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