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

On Friday 16 September 2016 05:13 PM, David Laight wrote:

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_set_return(),
added to force the paca->soft_enabled updates.

...

quoted

quoted

diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
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(unsigned long enable)
  	: : "r" (enable), "i" (offsetof(struct paca_struct, soft_enabled)));
  }

+static inline notrace unsigned long soft_enabled_set_return(unsigned long enable)
+{
+	unsigned long flags;
+
+	asm volatile(
+		"lbz %0,%1(13); stb %2,%1(13)"
+		: "=r" (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() does not?

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 isn't needed.
OTOH why not allocate a global register variable to r13 and access through that?

I do see this in asm/paca.h "register struct paca_struct *local_paca 
asm("r13"); "
and __check_irq_replay() in kernel/irq.c do updates the "irq_happened" as
mentioned. But existing helpers in hw_irq update the soft_enabled via
asm volatile and i did the same.

Maddy

	David