Re: [RFC PATCH 3/4] xen/arm: GICv3: address MISRA C:2012 Rule 15.2

[Julien Grall <julien@xxxxxxx>]

Hi Nicola,

On 07/11/2023 10:33, Nicola Vetrini wrote:
> The backwards jump due to the "goto retry;" statement
> can be transformed into a loop, without losing much in terms
> of readability.
>
> Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> ---
> This specific patch was provided by Stefano, I just added the
> commit message.
If that's the case, then Stefano should be the Author (at the moment 
this is you).
> ---
>   xen/arch/arm/gic-v3-its.c | 81 ++++++++++++++++++++-------------------
>   1 file changed, 42 insertions(+), 39 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
> index 8afcd9783bc8..4ba3f869ddf2 100644
> --- a/xen/arch/arm/gic-v3-its.c
> +++ b/xen/arch/arm/gic-v3-its.c
> @@ -341,6 +341,7 @@ static int its_map_baser(void __iomem *basereg, uint64_t 
> regc,
>       unsigned int pagesz = 2;    /* try 64K pages first, then go down. */
>       unsigned int table_size;
>       void *buffer;
> +    bool retry = false;
retry is false so...

>   
>       attr  = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
>       attr |= GIC_BASER_CACHE_SameAsInner << 
> GITS_BASER_OUTER_CACHEABILITY_SHIFT;
> @@ -351,55 +352,57 @@ static int its_map_baser(void __iomem *basereg, uint64_t 
> regc,
>        * it back and see what sticks (page size, cacheability and shareability
>        * attributes), retrying if necessary.
>        */
> -retry:
> -    table_size = ROUNDUP(nr_items * entry_size,
> -                         BIT(BASER_PAGE_BITS(pagesz), UL));
> -    /* The BASE registers support at most 256 pages. */
> -    table_size = min(table_size, 256U << BASER_PAGE_BITS(pagesz));
> +    while ( retry )
... you will never end in the loop. I also think that name 'retry' is 
confusing as the first time is not retry.
It would be clearer if we use a

do {

} while (retry)

That said, I actually prefer the goto version because some of the lines 
within the loop are now over 80 characters and splitting them would make 
the code harder to read.
Below an example, with the new indentation it is just over 80 characters.

if ( (regc & GITS_BASER_SHAREABILITY_MASK) == GIC_BASER_NonShareable )

One possibly would be to move the logic within the loop in a separate 
function.
Cheers,

--
Julien Grall