[1/2] xen/lib: Introduce printk_once() and replace some opencoded examples
diff mbox series

Message ID 1558119524-318-1-git-send-email-andrew.cooper3@citrix.com
State New, archived
Headers show
Series
  • [1/2] xen/lib: Introduce printk_once() and replace some opencoded examples
Related show

Commit Message

Andrew Cooper May 17, 2019, 6:58 p.m. UTC
Reflow the ZynqMP message for grepability, and fix the omission of a newline.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/cpuerrata.c               | 18 ++----------------
 xen/arch/arm/platforms/xilinx-zynqmp.c |  9 ++-------
 xen/include/xen/lib.h                  | 11 +++++++++++
 3 files changed, 15 insertions(+), 23 deletions(-)

Comments

Jan Beulich May 20, 2019, 8:24 a.m. UTC | #1
>>> On 17.05.19 at 20:58, <andrew.cooper3@citrix.com> wrote:
> Reflow the ZynqMP message for grepability, and fix the omission of a newline.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with two remarks:

> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -105,6 +105,17 @@ debugtrace_printk(const char *fmt, ...) {}
>  #define _p(_x) ((void *)(unsigned long)(_x))
>  extern void printk(const char *format, ...)
>      __attribute__ ((format (printf, 1, 2)));
> +
> +#define printk_once(fmt, args...)               \
> +({                                              \
> +    static bool __read_mostly once_;            \
> +    if ( unlikely(!once_) )                     \
> +    {                                           \
> +        once_ = true;                           \
> +        printk(fmt, ## args);                   \
> +    }                                           \
> +})

Just like Linux we accept this having a small race window, i.e.
it not truly being "once" in unlikely cases. I think it would be
worthwhile to make this intention explicit in the commit
message.

Unlike Linux'es the macro doesn't have a "return value". Is
this intentional? I view this as particularly useful for the
WARN_ONCE() sort-of counterpart, but I have to admit I can't
immediately see a good use here, so I'm largely curious.
(The 2 uses I could find in x86-specific code in Linux look
to me like they'd better be WARN_ONCE().)

Jan
Julien Grall May 31, 2019, 5:20 p.m. UTC | #2
Hi Andrew,

On 17/05/2019 19:58, Andrew Cooper wrote:
> Reflow the ZynqMP message for grepability, and fix the omission of a newline.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Regardless Jan's comment:

Reviewed-by: Julien Grall <julien.grall@arm.com>

I will let Jan/You to commit the patch.

Cheers,

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>   xen/arch/arm/cpuerrata.c               | 18 ++----------------
>   xen/arch/arm/platforms/xilinx-zynqmp.c |  9 ++-------
>   xen/include/xen/lib.h                  | 11 +++++++++++
>   3 files changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index 4431b24..8904939 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -336,18 +336,11 @@ static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
>       switch ( ssbd_state )
>       {
>       case ARM_SSBD_FORCE_DISABLE:
> -    {
> -        static bool once = true;
> -
> -        if ( once )
> -            printk("%s disabled from command-line\n", entry->desc);
> -        once = false;
> +        printk_once("%s disabled from command-line\n", entry->desc);
>   
>           arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 0, NULL);
>           required = false;
> -
>           break;
> -    }
>   
>       case ARM_SSBD_RUNTIME:
>           if ( required )
> @@ -359,18 +352,11 @@ static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
>           break;
>   
>       case ARM_SSBD_FORCE_ENABLE:
> -    {
> -        static bool once = true;
> -
> -        if ( once )
> -            printk("%s forced from command-line\n", entry->desc);
> -        once = false;
> +        printk_once("%s forced from command-line\n", entry->desc);
>   
>           arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
>           required = true;
> -
>           break;
> -    }
>   
>       default:
>           ASSERT_UNREACHABLE();
> diff --git a/xen/arch/arm/platforms/xilinx-zynqmp.c b/xen/arch/arm/platforms/xilinx-zynqmp.c
> index 08e3e11..3060d79 100644
> --- a/xen/arch/arm/platforms/xilinx-zynqmp.c
> +++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
> @@ -35,14 +35,9 @@ static bool zynqmp_smc(struct cpu_user_regs *regs)
>        */
>       if ( !cpus_have_const_cap(ARM_SMCCC_1_1) )
>       {
> -        static bool once = true;
> +        printk_once(XENLOG_WARNING
> +                    "ZynqMP firmware Error: no SMCCC 1.1 support. Disabling firmware calls\n");
>   
> -        if ( once )
> -        {
> -            printk(XENLOG_WARNING "ZynqMP firmware Error: no SMCCC 1.1 "
> -                   "support. Disabling firmware calls.");
> -            once = false;
> -        }
>           return false;
>       }
>       return zynqmp_eemi(regs);
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index 91ed56c..ce231c5 100644
> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -105,6 +105,17 @@ debugtrace_printk(const char *fmt, ...) {}
>   #define _p(_x) ((void *)(unsigned long)(_x))
>   extern void printk(const char *format, ...)
>       __attribute__ ((format (printf, 1, 2)));
> +
> +#define printk_once(fmt, args...)               \
> +({                                              \
> +    static bool __read_mostly once_;            \
> +    if ( unlikely(!once_) )                     \
> +    {                                           \
> +        once_ = true;                           \
> +        printk(fmt, ## args);                   \
> +    }                                           \
> +})
> +
>   extern void guest_printk(const struct domain *d, const char *format, ...)
>       __attribute__ ((format (printf, 2, 3)));
>   extern void noreturn panic(const char *format, ...)
>

Patch
diff mbox series

diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index 4431b24..8904939 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -336,18 +336,11 @@  static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
     switch ( ssbd_state )
     {
     case ARM_SSBD_FORCE_DISABLE:
-    {
-        static bool once = true;
-
-        if ( once )
-            printk("%s disabled from command-line\n", entry->desc);
-        once = false;
+        printk_once("%s disabled from command-line\n", entry->desc);
 
         arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 0, NULL);
         required = false;
-
         break;
-    }
 
     case ARM_SSBD_RUNTIME:
         if ( required )
@@ -359,18 +352,11 @@  static bool has_ssbd_mitigation(const struct arm_cpu_capabilities *entry)
         break;
 
     case ARM_SSBD_FORCE_ENABLE:
-    {
-        static bool once = true;
-
-        if ( once )
-            printk("%s forced from command-line\n", entry->desc);
-        once = false;
+        printk_once("%s forced from command-line\n", entry->desc);
 
         arm_smccc_1_1_smc(ARM_SMCCC_ARCH_WORKAROUND_2_FID, 1, NULL);
         required = true;
-
         break;
-    }
 
     default:
         ASSERT_UNREACHABLE();
diff --git a/xen/arch/arm/platforms/xilinx-zynqmp.c b/xen/arch/arm/platforms/xilinx-zynqmp.c
index 08e3e11..3060d79 100644
--- a/xen/arch/arm/platforms/xilinx-zynqmp.c
+++ b/xen/arch/arm/platforms/xilinx-zynqmp.c
@@ -35,14 +35,9 @@  static bool zynqmp_smc(struct cpu_user_regs *regs)
      */
     if ( !cpus_have_const_cap(ARM_SMCCC_1_1) )
     {
-        static bool once = true;
+        printk_once(XENLOG_WARNING
+                    "ZynqMP firmware Error: no SMCCC 1.1 support. Disabling firmware calls\n");
 
-        if ( once )
-        {
-            printk(XENLOG_WARNING "ZynqMP firmware Error: no SMCCC 1.1 "
-                   "support. Disabling firmware calls.");
-            once = false;
-        }
         return false;
     }
     return zynqmp_eemi(regs);
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 91ed56c..ce231c5 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -105,6 +105,17 @@  debugtrace_printk(const char *fmt, ...) {}
 #define _p(_x) ((void *)(unsigned long)(_x))
 extern void printk(const char *format, ...)
     __attribute__ ((format (printf, 1, 2)));
+
+#define printk_once(fmt, args...)               \
+({                                              \
+    static bool __read_mostly once_;            \
+    if ( unlikely(!once_) )                     \
+    {                                           \
+        once_ = true;                           \
+        printk(fmt, ## args);                   \
+    }                                           \
+})
+
 extern void guest_printk(const struct domain *d, const char *format, ...)
     __attribute__ ((format (printf, 2, 3)));
 extern void noreturn panic(const char *format, ...)