Re: [PATCH 13/20] spapr: Eliminate SpaprIrq:get_nodename method

[Philippe Mathieu-Daudé]

On 9/25/19 8:45 AM, David Gibson wrote:
> This method is used to determine the name of the irq backend's node in the
> device tree, so that we can find its phandle (after SLOF may have modified
> it from the phandle we initially gave it).
> 
> But, in the two cases the only difference between the node name is the
> presence of a unit address.  Searching for a node name without considering
> unit address is standard practice for the device tree, and
> fdt_subnode_offset() will do exactly that.
> 
> So, the method is unnecessary.
> 
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/ppc/spapr_irq.c         | 25 +++----------------------
>  include/hw/ppc/spapr_irq.h |  1 -
>  2 files changed, 3 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 038bfffff4..79167ccc68 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -211,11 +211,6 @@ static void spapr_irq_reset_xics(SpaprMachineState 
> *spapr, Error **errp)
>      }
>  }
>  
> -static const char *spapr_irq_get_nodename_xics(SpaprMachineState *spapr)
> -{
> -    return XICS_NODENAME;
> -}
> -
>  static void spapr_irq_init_kvm_xics(SpaprMachineState *spapr, Error **errp)
>  {
>      if (kvm_enabled()) {
> @@ -237,7 +232,6 @@ SpaprIrq spapr_irq_xics = {
>      .post_load   = spapr_irq_post_load_xics,
>      .reset       = spapr_irq_reset_xics,
>      .set_irq     = spapr_irq_set_irq_xics,
> -    .get_nodename = spapr_irq_get_nodename_xics,
>      .init_kvm    = spapr_irq_init_kvm_xics,
>  };
>  
> @@ -362,11 +356,6 @@ static void spapr_irq_set_irq_xive(void *opaque, int 
> irq, int val)
>      }
>  }
>  
> -static const char *spapr_irq_get_nodename_xive(SpaprMachineState *spapr)
> -{
> -    return spapr->xive->nodename;
> -}
> -
>  static void spapr_irq_init_kvm_xive(SpaprMachineState *spapr, Error **errp)
>  {
>      if (kvm_enabled()) {
> @@ -393,7 +382,6 @@ SpaprIrq spapr_irq_xive = {
>      .post_load   = spapr_irq_post_load_xive,
>      .reset       = spapr_irq_reset_xive,
>      .set_irq     = spapr_irq_set_irq_xive,
> -    .get_nodename = spapr_irq_get_nodename_xive,
>      .init_kvm    = spapr_irq_init_kvm_xive,
>  };
>  
> @@ -538,11 +526,6 @@ static void spapr_irq_set_irq_dual(void *opaque, int 
> irq, int val)
>      spapr_irq_current(spapr)->set_irq(spapr, irq, val);
>  }
>  
> -static const char *spapr_irq_get_nodename_dual(SpaprMachineState *spapr)
> -{
> -    return spapr_irq_current(spapr)->get_nodename(spapr);
> -}
> -
>  /*
>   * Define values in sync with the XIVE and XICS backend
>   */
> @@ -560,7 +543,6 @@ SpaprIrq spapr_irq_dual = {
>      .post_load   = spapr_irq_post_load_dual,
>      .reset       = spapr_irq_reset_dual,
>      .set_irq     = spapr_irq_set_irq_dual,
> -    .get_nodename = spapr_irq_get_nodename_dual,
>      .init_kvm    = NULL, /* should not be used */
>  };
>  
> @@ -697,13 +679,13 @@ void spapr_irq_reset(SpaprMachineState *spapr, Error 
> **errp)
>  
>  int spapr_irq_get_phandle(SpaprMachineState *spapr, void *fdt, Error **errp)
>  {
> -    const char *nodename = spapr->irq->get_nodename(spapr);
> +    const char *nodename = "interrupt-controller";
>      int offset, phandle;
>  
>      offset = fdt_subnode_offset(fdt, 0, nodename);
>      if (offset < 0) {
> -        error_setg(errp, "Can't find node \"%s\": %s", nodename,
> -                   fdt_strerror(offset));
> +        error_setg(errp, "Can't find node \"%s\": %s",
> +                   nodename, fdt_strerror(offset));
>          return -1;
>      }
>  
> @@ -787,6 +769,5 @@ SpaprIrq spapr_irq_xics_legacy = {
>      .post_load   = spapr_irq_post_load_xics,
>      .reset       = spapr_irq_reset_xics,
>      .set_irq     = spapr_irq_set_irq_xics,
> -    .get_nodename = spapr_irq_get_nodename_xics,
>      .init_kvm    = spapr_irq_init_kvm_xics,
>  };
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index a4e790ef60..9b60378e28 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -52,7 +52,6 @@ typedef struct SpaprIrq {
>      int (*post_load)(SpaprMachineState *spapr, int version_id);
>      void (*reset)(SpaprMachineState *spapr, Error **errp);
>      void (*set_irq)(void *opaque, int srcno, int val);
> -    const char *(*get_nodename)(SpaprMachineState *spapr);

Another Yay!

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

>      void (*init_kvm)(SpaprMachineState *spapr, Error **errp);
>  } SpaprIrq;
>  
>