Re: [Qemu-devel] [RFC PATCH v2 1/3] hw/vfio/sysbus-fdt: refactoring

[Eric Auger <eric.auger@linaro.org>]

Hi Baptiste,

On 12/22/2014 05:23 PM, Baptiste Reynal wrote:
> Creates set_interrupts_fdt_node and set_regions_fdt_node
> for code reusability.
> 
> Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  hw/arm/sysbus-fdt.c | 102 +++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 73 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
> index 15bb50c..f6ff8a7 100644
> --- a/hw/arm/sysbus-fdt.c
> +++ b/hw/arm/sysbus-fdt.c
> @@ -58,6 +58,76 @@ typedef struct NodeCreationPair {
>  /* Device Specific Code */
>  
>  /**
> + * set_interrupts_fdt_node
> + *
> + * Helper function, generate interrupts property for a given node
> + */
> +static int set_interrupts_fdt_node(char *nodename, SysBusDevice *sbdev,
> +        void *opaque, int type, int flags)
> +{
> +    PlatformBusFdtData *data = opaque;
> +    PlatformBusDevice *pbus = data->pbus;
> +    void *fdt = data->fdt;
> +    uint32_t *irq_attr;
> +    uint64_t irq_number;
> +    int i, ret;
> +    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> +    VFIODevice *vbasedev = &vdev->vbasedev;
> +
> +    irq_attr = g_new(uint32_t, vbasedev->num_irqs*3);
> +
> +    for (i = 0; i < vbasedev->num_irqs; i++) {
> +        irq_number = platform_bus_get_irqn(pbus, sbdev , i)
> +                         + data->irq_start;
> +        irq_attr[3*i] = cpu_to_be32(type);
> +        irq_attr[3*i+1] = cpu_to_be32(irq_number);
> +        irq_attr[3*i+2] = cpu_to_be32(flags);
> +    }
> +
> +   ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
> +                     irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
> +
> +    g_free(irq_attr);
> +
> +   return ret;
> +}
> +
> +/**
> + * set_regions_fdt_node
> + *
> + * Helper function, generate reg property for a given node
> + */
> +static int set_regions_fdt_node(char *nodename, SysBusDevice *sbdev,
> +        void *opaque)
> +{
> +    PlatformBusFdtData *data = opaque;
> +    PlatformBusDevice *pbus = data->pbus;
> +    void *fdt = data->fdt;
> +    uint64_t *reg_attr;
> +    uint64_t mmio_base;
> +    int i, ret;
> +    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> +    VFIODevice *vbasedev = &vdev->vbasedev;
> +
> +    reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
> +
> +    for (i = 0; i < vbasedev->num_regions; i++) {
> +        mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
> +        reg_attr[4*i] = 1;
> +        reg_attr[4*i+1] = mmio_base;
> +        reg_attr[4*i+2] = 1;
> +        reg_attr[4*i+3] = memory_region_size(&vdev->regions[i]->mem);
> +    }
> +
> +    ret = qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "reg",
> +                     vbasedev->num_regions*2, reg_attr);
> +
> +    g_free(reg_attr);
> +
> +    return ret;
> +}
> +
> +/**
>   * add_calxeda_midway_xgmac_fdt_node
>   *
>   * Generates a very simple node with following properties:
> @@ -66,16 +136,12 @@ typedef struct NodeCreationPair {
>  static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
>  {
>      PlatformBusFdtData *data = opaque;
> -    PlatformBusDevice *pbus = data->pbus;
>      void *fdt = data->fdt;
>      const char *parent_node = data->pbus_node_name;
>      int compat_str_len;
>      char *nodename;
> -    int i, ret;
> -    uint32_t *irq_attr;
> -    uint64_t *reg_attr;
> +    int ret;
>      uint64_t mmio_base;
> -    uint64_t irq_number;
>      VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
>      VFIODevice *vbasedev = &vdev->vbasedev;
>      Object *obj = OBJECT(sbdev);
> @@ -92,35 +158,15 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
>      qemu_fdt_setprop(fdt, nodename, "compatible",
>                            vdev->compat, compat_str_len);
>  
> -    reg_attr = g_new(uint64_t, vbasedev->num_regions*4);
> -
> -    for (i = 0; i < vbasedev->num_regions; i++) {
> -        mmio_base = platform_bus_get_mmio_addr(pbus, sbdev, i);
> -        reg_attr[4*i] = 1;
> -        reg_attr[4*i+1] = mmio_base;
> -        reg_attr[4*i+2] = 1;
> -        reg_attr[4*i+3] = memory_region_size(&vdev->regions[i]->mem);
> -    }
> +    ret = set_regions_fdt_node(nodename, sbdev, opaque);
>  
> -    ret = qemu_fdt_setprop_sized_cells_from_array(fdt, nodename, "reg",
> -                     vbasedev->num_regions*2, reg_attr);
>      if (ret < 0) {
>          error_report("could not set reg property of node %s", nodename);
>          goto fail;
>      }
>  
> -    irq_attr = g_new(uint32_t, vbasedev->num_irqs*3);
> +    ret = set_interrupts_fdt_node(nodename, sbdev, opaque, 0, 0x4);
>  
> -    for (i = 0; i < vbasedev->num_irqs; i++) {
> -        irq_number = platform_bus_get_irqn(pbus, sbdev , i)
> -                         + data->irq_start;
> -        irq_attr[3*i] = cpu_to_be32(0);
> -        irq_attr[3*i+1] = cpu_to_be32(irq_number);
> -        irq_attr[3*i+2] = cpu_to_be32(0x4);
> -    }
> -
> -   ret = qemu_fdt_setprop(fdt, nodename, "interrupts",
> -                     irq_attr, vbasedev->num_irqs*3*sizeof(uint32_t));
>      if (ret < 0) {
>          error_report("could not set interrupts property of node %s",
>                       nodename);
> @@ -128,8 +174,6 @@ static int add_calxeda_midway_xgmac_fdt_node(SysBusDevice *sbdev, void *opaque)
>      }
>  
>      g_free(nodename);
> -    g_free(irq_attr);
> -    g_free(reg_attr);
>  
>      return 0;
I think we should also take into account intermediate ret values.
Otherwise looks sensible to me.

Also you properly handled irq_attr and reg_attr dealloc here as opposed
to what I did in that version ;-)

maybe the title of your commit may be more explicit?

Best Regards

Eric
>  
>