From: Michael Ellerman <michael@ellerman•id.au>
To: Jesse Larrew <jlarrew@linux•vnet.ibm.com>
Cc: markn@au1•ibm.com, pmac@au1•ibm.com, tbreeds@au1•ibm.com,
lkessler@us•ibm.com, mjwolf@us•ibm.com,
linuxppc-dev@lists•ozlabs.org
Subject: Re: [PATCH 3/3] powerpc: Disable VPHN polling during a suspend operation
Date: Wed, 03 Nov 2010 23:12:42 +1100 [thread overview]
Message-ID: <1288786362.989.66.camel@concordia> (raw)
In-Reply-To: <20101029002921.17835.88560.sendpatchset@manic8ball.ltc.austin.ibm.com>
[-- 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 --]
next prev parent reply other threads:[~2010-11-03 12:12 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-10-29 0:27 [PATCH 0/3][RFC][REPOST] Add Support for Virtual Processor Home Node (VPHN) Jesse Larrew
2010-10-29 0:28 ` [PATCH 1/3] powerpc: Add VPHN firmware feature Jesse Larrew
2010-10-29 0:30 ` [PATCH 3/3] powerpc: Disable VPHN polling during a suspend operation Jesse Larrew
2010-11-03 12:12 ` Michael Ellerman [this message]
2010-11-05 20:33 ` Jesse Larrew
2010-10-29 0:33 ` [PATCH 2/3] powerpc: Poll VPA for topology changes and update NUMA maps Jesse Larrew
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1288786362.989.66.camel@concordia \
--to=michael@ellerman$(echo .)id.au \
--cc=jlarrew@linux$(echo .)vnet.ibm.com \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=lkessler@us$(echo .)ibm.com \
--cc=markn@au1$(echo .)ibm.com \
--cc=mjwolf@us$(echo .)ibm.com \
--cc=pmac@au1$(echo .)ibm.com \
--cc=tbreeds@au1$(echo .)ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox