Re: [PATCH 7/7] MIPS: GIC: Fix GICBIS macro

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

Hello.

On 07/17/2014 12:20 PM, Markos Chandras wrote:

> From: Jeffrey Deans <jeffrey.deans@imgtec.com>

> The GICBIS macro could update the GIC registers incorrectly, depending
> on the data value passed in:

> * Bits were only OR'd into the register data, so register fields could
>    not be cleared.

> * Bits were OR'd into the register data without masking the data to the
>    correct field width, corrupting adjacent bits.

> Signed-off-by: Jeffrey Deans <jeffrey.deans@imgtec.com>
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> ---
>   arch/mips/include/asm/gic.h | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)

> diff --git a/arch/mips/include/asm/gic.h b/arch/mips/include/asm/gic.h
> index 8b30befd99d6..3f20b2111d56 100644
> --- a/arch/mips/include/asm/gic.h
> +++ b/arch/mips/include/asm/gic.h
> @@ -43,18 +43,17 @@
>   #ifdef GICISBYTELITTLEENDIAN
>   #define GICREAD(reg, data)	((data) = (reg), (data) = le32_to_cpu(data))
>   #define GICWRITE(reg, data)	((reg) = cpu_to_le32(data))
> -#define GICBIS(reg, bits)			\
> -	({unsigned int data;			\
> -		GICREAD(reg, data);		\
> -		data |= bits;			\
> -		GICWRITE(reg, data);		\
> -	})
> -
>   #else
>   #define GICREAD(reg, data)	((data) = (reg))
>   #define GICWRITE(reg, data)	((reg) = (data))
> -#define GICBIS(reg, bits)	((reg) |= (bits))
>   #endif
> +#define GICBIS(reg, mask, bits)			\
> +	do { u32 data;				\
> +		GICREAD((reg), data);		\

    Why () only around 'reg', not around 'data'?

> +		data &= ~(mask);		\
> +		data |= ((bits) & (mask));	\

    Outer () not needed.

> +		GICWRITE((reg), data);		\

    Again, why no () around 'data'?

> +	} while (0)

WBR, Sergei