RE: [PATCH 04/13] powerpc: Use soft_enabled_set api to update paca->soft_enabled

From: Nicholas Piggin

Sent: 16 September 2016 12:59
On Fri, 16 Sep 2016 11:43:13 +0000
David Laight [off-list ref] wrote:
=20

quoted

From: Nicholas Piggin

quoted

Sent: 16 September 2016 10:53
On Thu, 15 Sep 2016 18:31:54 +0530
Madhavan Srinivasan [off-list ref] wrote:

quoted

Force use of soft_enabled_set() wrapper to update paca-soft_enabled
wherever possisble. Also add a new wrapper function, soft_enabled_s=

et_return(),

quoted

quoted

quoted

added to force the paca->soft_enabled updates.

...

quoted

quoted

diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/inclu=

de/asm/hw_irq.h

quoted

quoted

quoted

index 8fad8c24760b..f828b8f8df02 100644

--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h

@@ -53,6 +53,20 @@ static inline notrace void soft_enabled_set(unsi=

gned long enable)

quoted

quoted

quoted

 	: : "r" (enable), "i" (offsetof(struct paca_struct, soft_enabled)=

));

quoted

quoted

quoted

 }

+static inline notrace unsigned long soft_enabled_set_return(unsign=

ed long enable)

quoted

quoted

quoted

+{
+	unsigned long flags;
+
+	asm volatile(
+		"lbz %0,%1(13); stb %2,%1(13)"
+		: "=3Dr" (flags)
+		: "i" (offsetof(struct paca_struct, soft_enabled)),\
+		  "r" (enable)
+		: "memory");
+
+	return flags;
+}

Why do you have the "memory" clobber here while soft_enabled_set() do=

es not?

quoted

I wondered about the missing memory clobber earlier.

Any 'clobber' ought to be restricted to the referenced memory area.
If the structure is only referenced by r13 through 'asm volatile' it is=

n't needed.

=20
Well a clobber (compiler barrier) at some point is needed in irq_disable =

and

irq_enable paths, so we correctly open and close the critical section vs =

interrupts.

I just wonder about these helpers. It might be better to take the clobber=

s out of

there and add barrier(); in callers, which would make it more obvious.

If the memory clobber is needed to synchronise with the rest of the code
rather than just ensuring the compiler doesn't reorder accesses via r13
then I'd add an explicit barrier() somewhere - even if in these helpers.

Potentially the helper wants a memory clobber for the (r13) area
and a separate barrier() to ensure the interrupts are masked for the
right code.
Even if both are together in the same helper.

	David