Re: [Xen-devel] [PATCH 13/34] xen/arm: gic: Introduce GIC_SGI_MAX

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 26 Mar 2014, Julien Grall wrote:
> 
> 
> On 26/03/14 09:03, Ian Campbell wrote:
>  > Does ASSERT(sgi < GIC_SGI_MAX) not compiler without warnings?
> 
> I reworked this patch (see below):
> 
> commit a07ef8994c10ae45c0fa42040e802006f7a510c6
> Author: Julien Grall <julien.grall@xxxxxxxxxx>
> Date:   Wed Mar 19 20:51:51 2014 +0000
> 
>     xen/arm: gic: Introduce GIC_SGI_MAX
>     
>     All the functions that send an SGI takes an enum. Therefore checking 
> everytime
>     if the value is in the range is not correct.
>     
>     Introduce GIC_SGI_MAX to check the enum will never reach more than 16 
> values and
>     use it to check if the developper will use a wrong SGI by mistake.
>     
>     This is fix the compilation with Clang 3.5:
>     
>     gic.c:515:15: error: comparison of constant 16 with expression of type 
> 'enum gic_sgi' is always true 
> [-Werror,-Wtautological-constant-out-of-range-compare]
>        ASSERT(sgi < 16); /* There are only 16 SGIs */
>               ~~~ ^ ~~
>     xen/xen/include/xen/lib.h:43:26: note: expanded from macro 'ASSERT'
>         do { if ( unlikely(!(p)) ) assert_failed(#p); } while (0)
>                              ^
>     xen/xen/include/xen/compiler.h:11:41: note: expanded from macro 'unlikely'
>      #define unlikely(x)   __builtin_expect((x),0)
>     
>     Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
>     Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
>     Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
>     Cc: Tim Deegan <tim@xxxxxxx>

I think that this is fine.

Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

>     ---
>         Changes in v2:
>             - Replace ASSERT(sgi != GIC_SGI_MAX) by ASSERT(sgi < GIC_SGI_MAX)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index b03f2c2..2b9cdc5 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -473,7 +473,8 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi 
> sgi)
>      unsigned int mask = 0;
>      cpumask_t online_mask;
>  
> -    ASSERT(sgi < 16); /* There are only 16 SGIs */
> +    BUILD_BUG_ON(GIC_SGI_MAX >= 16);
> +    ASSERT(sgi < GIC_SGI_MAX);
>  
>      cpumask_and(&online_mask, cpumask, &cpu_online_map);
>      mask = gic_cpu_mask(&online_mask);
> @@ -493,7 +494,7 @@ void send_SGI_one(unsigned int cpu, enum gic_sgi sgi)
>  
>  void send_SGI_self(enum gic_sgi sgi)
>  {
> -    ASSERT(sgi < 16); /* There are only 16 SGIs */
> +    ASSERT(sgi < GIC_SGI_MAX);
>  
>      dsb(sy);
>  
> @@ -503,7 +504,7 @@ void send_SGI_self(enum gic_sgi sgi)
>  
>  void send_SGI_allbutself(enum gic_sgi sgi)
>  {
> -   ASSERT(sgi < 16); /* There are only 16 SGIs */
> +   ASSERT(sgi < GIC_SGI_MAX);
>  
>     dsb(sy);
>  
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 76586ab..a4e513f 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -198,6 +198,8 @@ enum gic_sgi {
>      GIC_SGI_EVENT_CHECK = 0,
>      GIC_SGI_DUMP_STATE  = 1,
>      GIC_SGI_CALL_FUNCTION = 2,
> +    /* GIC_SGI_MAX must be the last type of the enum */
> +    GIC_SGI_MAX,
>  };
>  extern void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi);
>  extern void send_SGI_one(unsigned int cpu, enum gic_sgi sgi);
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel