[PATCH] Make cpu hotplug driver lock part of ppc_md

[PATCH] Make cpu hotplug driver lock part of ppc_md

[Nathan Fontenot]

The recently introduced cpu_hotplug_driver_lock used to serialize
cpu hotplug operations, namely for the pseries platform, causes a build
issue for other platforms.  The base cpu hotplug code attempts
to take this lock, but it may not be needed for all platforms.  This patch
moves the lock/unlock routines to be part of the ppc_md structure
so that platforms needing the lock can take it.  This also makes the
previous cpu_hotplug_driver_lock, defined in pseries code, pseries specific.

The past failure without this patch was seen when building pmac and may
be present in other platform builds.  The error is included below for reference.

drivers/built-in.o: In function `.store_online':
cpu.c:(.ref.text+0xf5c): undefined reference to `.cpu_hotplug_driver_lock'
cpu.c:(.ref.text+0xfc8): undefined reference to `.cpu_hotplug_driver_unlock'
make: *** [.tmp_vmlinux1] Error 1

Signed-of-by: Nathan Fontenot <nfont@austin.ibm.com>
---
 arch/powerpc/include/asm/machdep.h     |    2 ++
 arch/powerpc/kernel/smp.c              |   14 ++++++++++++++
 arch/powerpc/platforms/pseries/dlpar.c |    6 ++++--
 3 files changed, 20 insertions(+), 2 deletions(-)

Index: powerpc/arch/powerpc/include/asm/machdep.h
===================================================================
--- powerpc.orig/arch/powerpc/include/asm/machdep.h	2009-12-21 20:51:49.000000000 -0600
+++ powerpc/arch/powerpc/include/asm/machdep.h	2009-12-21 21:07:40.000000000 -0600
@@ -270,6 +270,8 @@
 #ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
 	ssize_t (*cpu_probe)(const char *, size_t);
 	ssize_t (*cpu_release)(const char *, size_t);
+	void (*cpu_hotplug_driver_lock)(void);
+	void (*cpu_hotplug_driver_unlock)(void);
 #endif
 };
 
Index: powerpc/arch/powerpc/platforms/pseries/dlpar.c
===================================================================
--- powerpc.orig/arch/powerpc/platforms/pseries/dlpar.c	2009-12-21 20:51:49.000000000 -0600
+++ powerpc/arch/powerpc/platforms/pseries/dlpar.c	2009-12-21 21:26:23.000000000 -0600
@@ -346,12 +346,12 @@
 
 static DEFINE_MUTEX(pseries_cpu_hotplug_mutex);
 
-void cpu_hotplug_driver_lock()
+static void pseries_cpu_hotplug_driver_lock(void)
 {
 	mutex_lock(&pseries_cpu_hotplug_mutex);
 }
 
-void cpu_hotplug_driver_unlock()
+static void pseries_cpu_hotplug_driver_unlock(void)
 {
 	mutex_unlock(&pseries_cpu_hotplug_mutex);
 }
@@ -550,6 +550,8 @@
 {
 	ppc_md.cpu_probe = dlpar_cpu_probe;
 	ppc_md.cpu_release = dlpar_cpu_release;
+	ppc_md.cpu_hotplug_driver_lock = pseries_cpu_hotplug_driver_lock;
+	ppc_md.cpu_hotplug_driver_unlock = pseries_cpu_hotplug_driver_unlock;
 
 	return 0;
 }
Index: powerpc/arch/powerpc/kernel/smp.c
===================================================================
--- powerpc.orig/arch/powerpc/kernel/smp.c	2009-12-21 20:51:49.000000000 -0600
+++ powerpc/arch/powerpc/kernel/smp.c	2009-12-21 21:24:23.000000000 -0600
@@ -619,4 +619,18 @@
 	if (smp_ops->cpu_die)
 		smp_ops->cpu_die(cpu);
 }
+
+#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
+void cpu_hotplug_driver_lock(void)
+{
+	if (ppc_md.cpu_hotplug_driver_lock)
+		ppc_md.cpu_hotplug_driver_lock();
+}
+
+void cpu_hotplug_driver_unlock(void)
+{
+	if (ppc_md.cpu_hotplug_driver_unlock)
+		ppc_md.cpu_hotplug_driver_unlock();
+}
 #endif
+#endif /* CONFIG_HOTPLUG_CPU */

Re: [PATCH] Make cpu hotplug driver lock part of ppc_md

[Michael Ellerman]

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

On Tue, 2009-12-22 at 08:45 -0600, Nathan Fontenot wrote:
> The recently introduced cpu_hotplug_driver_lock used to serialize
> cpu hotplug operations, namely for the pseries platform, causes a build
> issue for other platforms.  The base cpu hotplug code attempts
> to take this lock, but it may not be needed for all platforms.  This patch
> moves the lock/unlock routines to be part of the ppc_md structure
> so that platforms needing the lock can take it.  This also makes the
> previous cpu_hotplug_driver_lock, defined in pseries code, pseries specific.
> 
> The past failure without this patch was seen when building pmac and may
> be present in other platform builds.  The error is included below for reference.
> 
> drivers/built-in.o: In function `.store_online':
> cpu.c:(.ref.text+0xf5c): undefined reference to `.cpu_hotplug_driver_lock'
> cpu.c:(.ref.text+0xfc8): undefined reference to `.cpu_hotplug_driver_unlock'
> make: *** [.tmp_vmlinux1] Error 1

Why does the pmac code /not/ need a lock? And would it be harmless if it
was locked too?

If so, you could just make the mutex available to all powerpc code, and
rename it, and then we wouldn't need all this jiggery pokery just to
take & release a lock.

cheers

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

Re: [PATCH] Make cpu hotplug driver lock part of ppc_md

[Nathan Fontenot]

Michael Ellerman wrote:
> On Tue, 2009-12-22 at 08:45 -0600, Nathan Fontenot wrote:
>> The recently introduced cpu_hotplug_driver_lock used to serialize
>> cpu hotplug operations, namely for the pseries platform, causes a build
>> issue for other platforms.  The base cpu hotplug code attempts
>> to take this lock, but it may not be needed for all platforms.  This patch
>> moves the lock/unlock routines to be part of the ppc_md structure
>> so that platforms needing the lock can take it.  This also makes the
>> previous cpu_hotplug_driver_lock, defined in pseries code, pseries specific.
>>
>> The past failure without this patch was seen when building pmac and may
>> be present in other platform builds.  The error is included below for reference.
>>
>> drivers/built-in.o: In function `.store_online':
>> cpu.c:(.ref.text+0xf5c): undefined reference to `.cpu_hotplug_driver_lock'
>> cpu.c:(.ref.text+0xfc8): undefined reference to `.cpu_hotplug_driver_unlock'
>> make: *** [.tmp_vmlinux1] Error 1
> 
> Why does the pmac code /not/ need a lock? And would it be harmless if it
> was locked too?

The intention of the cpu_hotplug_driver_locks to add additional serialization
during cpu hotplug operations.  For pseries this is used during DLPAR of cpu
operations so that cpu hotplug actions cannot be initiated whiloe a DLPAR
operation is in flight.  For example, during DLPAR add we take the lock while
acquiring the cpu from firmware and updating the device tree with the new
cpu information, after which we hotplug add the cpu to the system.  

There is nothing harmless about taking the lock on all platforms, I was just
trying to avoid taking the lock if the additional serialization is not needed.

> 
> If so, you could just make the mutex available to all powerpc code, and
> rename it, and then we wouldn't need all this jiggery pokery just to
> take & release a lock.

I can make the lock available to all powerpc code and not go through the
ppc_md struct, it makes no difference to me personally.  Of course this would
make all that fun pokery jiggery go away :)

-Nathan

Re: [PATCH] Make cpu hotplug driver lock part of ppc_md

[Michael Ellerman]

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

On Wed, 2009-12-23 at 08:48 -0600, Nathan Fontenot wrote:
> Michael Ellerman wrote:
> > On Tue, 2009-12-22 at 08:45 -0600, Nathan Fontenot wrote:
> >> The recently introduced cpu_hotplug_driver_lock used to serialize
> >> cpu hotplug operations, namely for the pseries platform, causes a build
> >> issue for other platforms.  The base cpu hotplug code attempts
> >> to take this lock, but it may not be needed for all platforms.  This patch
> >> moves the lock/unlock routines to be part of the ppc_md structure
> >> so that platforms needing the lock can take it.  This also makes the
> >> previous cpu_hotplug_driver_lock, defined in pseries code, pseries specific.
> >>
> >> The past failure without this patch was seen when building pmac and may
> >> be present in other platform builds.  The error is included below for reference.
> >>
> >> drivers/built-in.o: In function `.store_online':
> >> cpu.c:(.ref.text+0xf5c): undefined reference to `.cpu_hotplug_driver_lock'
> >> cpu.c:(.ref.text+0xfc8): undefined reference to `.cpu_hotplug_driver_unlock'
> >> make: *** [.tmp_vmlinux1] Error 1
> > 
> > Why does the pmac code /not/ need a lock? And would it be harmless if it
> > was locked too?
> 
> The intention of the cpu_hotplug_driver_locks to add additional serialization
> during cpu hotplug operations.  For pseries this is used during DLPAR of cpu
> operations so that cpu hotplug actions cannot be initiated whiloe a DLPAR
> operation is in flight.  For example, during DLPAR add we take the lock while
> acquiring the cpu from firmware and updating the device tree with the new
> cpu information, after which we hotplug add the cpu to the system.  

Right.

> There is nothing harmless about taking the lock on all platforms, I was just
> trying to avoid taking the lock if the additional serialization is not needed.

"nothing harmless" :)

But I think I know what you mean.

> > 
> > If so, you could just make the mutex available to all powerpc code, and
> > rename it, and then we wouldn't need all this jiggery pokery just to
> > take & release a lock.
> 
> I can make the lock available to all powerpc code and not go through the
> ppc_md struct, it makes no difference to me personally.  Of course this would
> make all that fun pokery jiggery go away :)

I think that would be a nicer result. Sure it adds an extra lock/unlock
on other platforms, but I think that's thoroughly insignificant compared
to a cpu hotplug.

Of course Ben might disagree with me ;)

cheers


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

Re: [PATCH] Make cpu hotplug driver lock part of ppc_md

[Benjamin Herrenschmidt]

> The intention of the cpu_hotplug_driver_locks to add additional serialization
> during cpu hotplug operations.  For pseries this is used during DLPAR of cpu
> operations so that cpu hotplug actions cannot be initiated whiloe a DLPAR
> operation is in flight.  For example, during DLPAR add we take the lock while
> acquiring the cpu from firmware and updating the device tree with the new
> cpu information, after which we hotplug add the cpu to the system.  
> 
> There is nothing harmless about taking the lock on all platforms, I was just
> trying to avoid taking the lock if the additional serialization is not needed.
> 
> > 
> > If so, you could just make the mutex available to all powerpc code, and
> > rename it, and then we wouldn't need all this jiggery pokery just to
> > take & release a lock.
> 
> I can make the lock available to all powerpc code and not go through the
> ppc_md struct, it makes no difference to me personally.  Of course this would
> make all that fun pokery jiggery go away :)

Yeah, Michael is right, just make it global to powerpc, it should make
things simpler.

Cheers,
Ben.

Re: [PATCH] Make cpu hotplug driver lock part of ppc_md

[Nathan Fontenot]

Benjamin Herrenschmidt wrote:
>> The intention of the cpu_hotplug_driver_locks to add additional serialization
>> during cpu hotplug operations.  For pseries this is used during DLPAR of cpu
>> operations so that cpu hotplug actions cannot be initiated whiloe a DLPAR
>> operation is in flight.  For example, during DLPAR add we take the lock while
>> acquiring the cpu from firmware and updating the device tree with the new
>> cpu information, after which we hotplug add the cpu to the system.  
>>
>> There is nothing harmless about taking the lock on all platforms, I was just
>> trying to avoid taking the lock if the additional serialization is not needed.
>>
>>> If so, you could just make the mutex available to all powerpc code, and
>>> rename it, and then we wouldn't need all this jiggery pokery just to
>>> take & release a lock.
>> I can make the lock available to all powerpc code and not go through the
>> ppc_md struct, it makes no difference to me personally.  Of course this would
>> make all that fun pokery jiggery go away :)
> 
> Yeah, Michael is right, just make it global to powerpc, it should make
> things simpler.

Sounds good, I'll get a patch out to do this.

-Nathan