Re: [PATCH] OLPC: Add XO-1 poweroff support

[Andres Salomon <dilinger@queued.net>]

On Thu,  7 Oct 2010 19:59:52 +0100 (BST)
Daniel Drake <dsd@laptop.org> wrote:

> Add a pm_power_off handler for the OLPC XO-1 laptop.
> 
> Signed-off-by: Daniel Drake <dsd@laptop.org>
> ---
>  arch/x86/Kconfig           |    6 ++
>  arch/x86/kernel/Makefile   |    1 +
>  arch/x86/kernel/olpc-xo1.c |  112
> ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 119
> insertions(+), 0 deletions(-) create mode 100644
> arch/x86/kernel/olpc-xo1.c
> 
> The new olpc-xo1.c file will also be used for further functionality
> (suspend/resume, lid switch device, etc); patches to be submitted
> shortly after the review/merge of this one.
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index cea0cd9..19e6439 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2065,6 +2065,12 @@ config OLPC
>  	  Add support for detecting the unique features of the OLPC
>  	  XO hardware.
>  
> +config OLPC_XO1
> +	bool "OLPC XO-1 support"

Any particular reason why this can't be modular?

[...]
> +
> +	pm_power_off = xo1_power_off;
> +

If this were to be modular, I'm not sure what the best option is for
pm_power_off.  If the old value was saved upon module load, and
restored upon module unload, I could imagine races like the following:

module A load:
  saved_ppo = pm_power_off; /* NULL */
  pm_power_off = frob;
module B load:
  old_ppo = pm_power_off; /* frob */
  pm_power_off = foo;
module A unload:
  pm_power_off = saved_ppo; /* NULL */


So I guess just clobbering whatever's in pm_power_off and setting back
to NULL upon unload, with the assumption that driver's only really
going to clobber it in the case of actual OLPC hardware, and the
callbacks being clobbered wouldn't really have done the correct thing
anyways?



> +	printk(KERN_INFO "OLPC XO-1 support registered\n");
> +	return 0;
> +}
> +device_initcall(olpc_xo1_init);
> +