Re: [PATCH 05/11] ppc/pnv: make pnv_ics_get() use the chip8->phbs[] array

[Frederic Barrat]

On 13/06/2022 17:44, Daniel Henrique Barboza wrote:
> The function is working today by getting all the child objects of the
> chip, interacting with each of them to check whether the child is a PHB,
> and then doing what needs to be done.
>
> We have all the chip PHBs in the phbs[] array so interacting with all
> child objects is unneeded.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.ibm.com>
> ---
>   hw/ppc/pnv.c | 30 ++++++++++++++----------------
>   1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 40e0cbd84d..05a8d5034f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1944,41 +1944,39 @@ typedef struct ForeachPhb3Args {
>       ICSState *ics;
>   } ForeachPhb3Args;
>   
> -static int pnv_ics_get_child(Object *child, void *opaque)
> +static void pnv_ics_get_phb_ics(PnvPHB3 *phb3, ForeachPhb3Args *args)
>   {
> -    ForeachPhb3Args *args = opaque;
> -    PnvPHB3 *phb3 = (PnvPHB3 *) object_dynamic_cast(child, TYPE_PNV_PHB3);
> +    if (ics_valid_irq(&phb3->lsis, args->irq)) {
> +        args->ics = &phb3->lsis;
> +    }
>   
> -    if (phb3) {
> -        if (ics_valid_irq(&phb3->lsis, args->irq)) {
> -            args->ics = &phb3->lsis;
> -        }
> -        if (ics_valid_irq(ICS(&phb3->msis), args->irq)) {
> -            args->ics = ICS(&phb3->msis);
> -        }
> +    if (ics_valid_irq(ICS(&phb3->msis), args->irq)) {
> +        args->ics = ICS(&phb3->msis);
>       }
> -    return args->ics ? 1 : 0;
>   }


It seems that we could gain in readability by dropping the 
ForeachPhb3Args structure completely.
The 'irq' member can just be an input argument to the function instead 
of the full structure.
The 'ics' member is no longer needed, it can be the returned value of 
the function (instead of void)

  Fred


>   
>   static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
>   {
>       PnvMachineState *pnv = PNV_MACHINE(xi);
>       ForeachPhb3Args args = { irq, NULL };
> -    int i;
> +    int i, j;
>   
>       for (i = 0; i < pnv->num_chips; i++) {
> -        PnvChip *chip = pnv->chips[i];
>           Pnv8Chip *chip8 = PNV8_CHIP(pnv->chips[i]);
>   
>           if (ics_valid_irq(&chip8->psi.ics, irq)) {
>               return &chip8->psi.ics;
>           }
>   
> -        object_child_foreach(OBJECT(chip), pnv_ics_get_child, &args);
> -        if (args.ics) {
> -            return args.ics;
> +        for (j = 0; j < chip8->num_phbs; j++) {
> +            pnv_ics_get_phb_ics(&chip8->phbs[j], &args);
> +
> +            if (args.ics) {
> +                return args.ics;
> +            }
>           }
>       }
> +
>       return NULL;
>   }
>