Re: [PATCH 3/3] powerpc: Disable VPHN polling during a suspend operation

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3361 bytes --]

On Thu, 2010-10-28 at 20:30 -0400, Jesse Larrew wrote: 
> From: Jesse Larrew <jlarrew@linux.vnet.ibm.com>

Hi Jesse, a few comments ...

> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
> index afe4aaa..1747d27 100644
> --- a/arch/powerpc/include/asm/topology.h
> +++ b/arch/powerpc/include/asm/topology.h
> @@ -49,7 +49,7 @@ static inline int pcibus_to_node(struct pci_bus *bus)
>  {
>  	return -1;
>  }
> -#endif
> +#endif /* CONFIG_PCI */

Random change, though not a biggy I suppose.

> #define cpumask_of_pcibus(bus)	(pcibus_to_node(bus) == -1 ?		\
>  				 cpu_all_mask :				\
> @@ -93,6 +93,8 @@ extern void __init dump_numa_cpu_topology(void);
>  extern int sysfs_add_device_to_node(struct sys_device *dev, int nid);
>  extern void sysfs_remove_device_from_node(struct sys_device *dev, int nid);
>  
> +extern int __init init_topology_update(void);
> +extern int stop_topology_update(void);

init_topology_update() is called repeatedly from post_suspend_work() so
it seems like it should be called start_topology_update(). And it can't
be __init because the suspend code is called after boot. You should get
a section mismatch warning if they are enabled.

> #else
>  
>  static inline void dump_numa_cpu_topology(void) {}
> @@ -107,6 +109,8 @@ static inline void sysfs_remove_device_from_node(struct sys_device *dev,
>  {
>  }
>  
> +static int __init init_topology_update(void) {}
> +static int stop_topology_update(void) {}

That doesn't look like it compiles to me, you want static inline, and
they both return int.

> #endif /* CONFIG_NUMA */
>  
>  #include <asm-generic/topology.h>
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 8fe8bc6..317ff2f 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -41,6 +41,7 @@
>  #include <asm/atomic.h>
>  #include <asm/time.h>
>  #include <asm/mmu.h>
> +#include <asm/topology.h>
>  
>  struct rtas_t rtas = {
>  	.lock = __ARCH_SPIN_LOCK_UNLOCKED
> @@ -706,6 +707,18 @@ void rtas_os_term(char *str)
>  
>  static int ibm_suspend_me_token = RTAS_UNKNOWN_SERVICE;
>  #ifdef CONFIG_PPC_PSERIES
> +static void pre_suspend_work(void)
> +{
> +	stop_topology_update();
> +	return;
> +}
> +
> +static void post_suspend_work(void)
> +{
> +	init_topology_update();
> +	return;
> +}

I'm not sure if it's worth splitting these out into "generic"
callbacks ..

> static int __rtas_suspend_last_cpu(struct rtas_suspend_me_data *data, int wake_when_done)
>  {
>  	u16 slb_size = mmu_slb_size;
> @@ -713,6 +726,7 @@ static int __rtas_suspend_last_cpu(struct rtas_suspend_me_data *data, int wake_w
>  	int cpu;
>  
>  	slb_set_size(SLB_MIN_SIZE);
> +	pre_suspend_work();
>  	printk(KERN_DEBUG "calling ibm,suspend-me on cpu %i\n", smp_processor_id());
>  
>  	while (rc == H_MULTI_THREADS_ACTIVE && !atomic_read(&data->done) &&

And isn't there an error case here where you're not re-enabling the
polling? See eg. the slb_set_size() call.

> @@ -728,6 +742,7 @@ static int __rtas_suspend_last_cpu(struct rtas_suspend_me_data *data, int wake_w
>  		rc = atomic_read(&data->error);
>  
>  	atomic_set(&data->error, rc);
> +	post_suspend_work();
>  
>  	if (wake_when_done) {
>  		atomic_set(&data->done, 1);

cheers



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]