Re: [PATCH v2 02/13] hw: Replace trivial drive_get_next() by drive_get()

[Alistair Francis]

On Thu, Nov 18, 2021 at 2:42 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> drive_get_next() is basically a bad idea.  It returns the "next" block
> backend of a certain interface type.  "Next" means bus=0,unit=N, where
> subsequent calls count N up from zero, per interface type.
>
> This lets you define unit numbers implicitly by execution order.  If the
> order changes, or new calls appear "in the middle", unit numbers change.
> ABI break.  Hard to spot in review.
>
> A number of machines connect just one backend with drive_get_next().
> Change them to use drive_get() directly.  This makes the (zero) unit
> number explicit in the code.
>
> Cc: Beniamino Galvani <b.galvani@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Subbaraya Sundeep <sundeep.lkml@gmail.com>
> Cc: Niek Linnenbank <nieklinnenbank@gmail.com>
> Cc: Andrew Baumann <Andrew.Baumann@microsoft.com>
> Cc: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
> Cc: Jean-Christophe Dubois <jcd@tribudubois.net>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>
> Cc: Bin Meng <bin.meng@windriver.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Michael Tokarev <mjt@tls.msk.ru>
> Cc: Laurent Vivier <laurent@vivier.eu>
> Cc: qemu-arm@nongnu.org
> Cc: qemu-riscv@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/cubieboard.c        | 2 +-
>  hw/arm/integratorcp.c      | 2 +-
>  hw/arm/msf2-som.c          | 2 +-
>  hw/arm/orangepi.c          | 2 +-
>  hw/arm/raspi.c             | 2 +-
>  hw/arm/realview.c          | 2 +-
>  hw/arm/sabrelite.c         | 2 +-
>  hw/misc/sifive_u_otp.c     | 2 +-
>  hw/riscv/microchip_pfsoc.c | 2 +-
>  hw/riscv/sifive_u.c        | 2 +-
>  hw/sparc64/niagara.c       | 2 +-
>  11 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
> index 294ba5de6e..5e3372a3c7 100644
> --- a/hw/arm/cubieboard.c
> +++ b/hw/arm/cubieboard.c
> @@ -81,7 +81,7 @@ static void cubieboard_init(MachineState *machine)
>      }
>
>      /* Retrieve SD bus */
> -    di = drive_get_next(IF_SD);
> +    di = drive_get(IF_SD, 0, 0);
>      blk = di ? blk_by_legacy_dinfo(di) : NULL;
>      bus = qdev_get_child_bus(DEVICE(a10), "sd-bus");
>
> diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
> index 16e8985953..b109ece3ae 100644
> --- a/hw/arm/integratorcp.c
> +++ b/hw/arm/integratorcp.c
> @@ -649,7 +649,7 @@ static void integratorcp_init(MachineState *machine)
>                            qdev_get_gpio_in_named(icp, ICP_GPIO_MMC_WPROT, 
> 0));
>      qdev_connect_gpio_out_named(dev, "card-inserted", 0,
>                            qdev_get_gpio_in_named(icp, ICP_GPIO_MMC_CARDIN, 
> 0));
> -    dinfo = drive_get_next(IF_SD);
> +    dinfo = drive_get(IF_SD, 0, 0);
>      if (dinfo) {
>          DeviceState *card;
>
> diff --git a/hw/arm/msf2-som.c b/hw/arm/msf2-som.c
> index 396e8b9913..d9f881690e 100644
> --- a/hw/arm/msf2-som.c
> +++ b/hw/arm/msf2-som.c
> @@ -45,7 +45,7 @@ static void emcraft_sf2_s2s010_init(MachineState *machine)
>      DeviceState *spi_flash;
>      MSF2State *soc;
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
> -    DriveInfo *dinfo = drive_get_next(IF_MTD);
> +    DriveInfo *dinfo = drive_get(IF_MTD, 0, 0);
>      qemu_irq cs_line;
>      BusState *spi_bus;
>      MemoryRegion *sysmem = get_system_memory();
> diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
> index 0cf9895ce7..e796382236 100644
> --- a/hw/arm/orangepi.c
> +++ b/hw/arm/orangepi.c
> @@ -85,7 +85,7 @@ static void orangepi_init(MachineState *machine)
>      qdev_realize(DEVICE(h3), NULL, &error_abort);
>
>      /* Retrieve SD bus */
> -    di = drive_get_next(IF_SD);
> +    di = drive_get(IF_SD, 0, 0);
>      blk = di ? blk_by_legacy_dinfo(di) : NULL;
>      bus = qdev_get_child_bus(DEVICE(h3), "sd-bus");
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index 146d35382b..b4dd6c1e99 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -284,7 +284,7 @@ static void raspi_machine_init(MachineState *machine)
>      qdev_realize(DEVICE(&s->soc), NULL, &error_fatal);
>
>      /* Create and plug in the SD cards */
> -    di = drive_get_next(IF_SD);
> +    di = drive_get(IF_SD, 0, 0);
>      blk = di ? blk_by_legacy_dinfo(di) : NULL;
>      bus = qdev_get_child_bus(DEVICE(&s->soc), "sd-bus");
>      if (bus == NULL) {
> diff --git a/hw/arm/realview.c b/hw/arm/realview.c
> index 1c54316ba3..ddc70b54a5 100644
> --- a/hw/arm/realview.c
> +++ b/hw/arm/realview.c
> @@ -237,7 +237,7 @@ static void realview_init(MachineState *machine,
>          qemu_irq_invert(qdev_get_gpio_in(gpio2, 0)));
>      qdev_connect_gpio_out_named(dev, "card-read-only", 0, mmc_irq[0]);
>      qdev_connect_gpio_out_named(dev, "card-inserted", 0, mmc_irq[1]);
> -    dinfo = drive_get_next(IF_SD);
> +    dinfo = drive_get(IF_SD, 0, 0);
>      if (dinfo) {
>          DeviceState *card;
>
> diff --git a/hw/arm/sabrelite.c b/hw/arm/sabrelite.c
> index 553608e583..cce49aa25c 100644
> --- a/hw/arm/sabrelite.c
> +++ b/hw/arm/sabrelite.c
> @@ -76,7 +76,7 @@ static void sabrelite_init(MachineState *machine)
>              if (spi_bus) {
>                  DeviceState *flash_dev;
>                  qemu_irq cs_line;
> -                DriveInfo *dinfo = drive_get_next(IF_MTD);
> +                DriveInfo *dinfo = drive_get(IF_MTD, 0, 0);
>
>                  flash_dev = qdev_new("sst25vf016b");
>                  if (dinfo) {
> diff --git a/hw/misc/sifive_u_otp.c b/hw/misc/sifive_u_otp.c
> index 18aa0bd55d..5d5a8c8a90 100644
> --- a/hw/misc/sifive_u_otp.c
> +++ b/hw/misc/sifive_u_otp.c
> @@ -209,7 +209,7 @@ static void sifive_u_otp_realize(DeviceState *dev, Error 
> **errp)
>                            TYPE_SIFIVE_U_OTP, SIFIVE_U_OTP_REG_SIZE);
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mmio);
>
> -    dinfo = drive_get_next(IF_NONE);
> +    dinfo = drive_get(IF_NONE, 0, 0);
>      if (dinfo) {
>          int ret;
>          uint64_t perm;
> diff --git a/hw/riscv/microchip_pfsoc.c b/hw/riscv/microchip_pfsoc.c
> index 57d779fb55..d1d065efbc 100644
> --- a/hw/riscv/microchip_pfsoc.c
> +++ b/hw/riscv/microchip_pfsoc.c
> @@ -458,7 +458,7 @@ static void 
> microchip_icicle_kit_machine_init(MachineState *machine)
>      target_ulong firmware_end_addr, kernel_start_addr;
>      uint64_t kernel_entry;
>      uint32_t fdt_load_addr;
> -    DriveInfo *dinfo = drive_get_next(IF_SD);
> +    DriveInfo *dinfo = drive_get(IF_SD, 0, 0);
>
>      /* Sanity check on RAM size */
>      if (machine->ram_size < mc->default_ram_size) {
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index a4ecadaf12..aa74e67889 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -672,7 +672,7 @@ static void sifive_u_machine_init(MachineState *machine)
>
>      /* Connect an SPI flash to SPI0 */
>      flash_dev = qdev_new("is25wp256");
> -    dinfo = drive_get_next(IF_MTD);
> +    dinfo = drive_get(IF_MTD, 0, 0);
>      if (dinfo) {
>          qdev_prop_set_drive_err(flash_dev, "drive",
>                                  blk_by_legacy_dinfo(dinfo),
> diff --git a/hw/sparc64/niagara.c b/hw/sparc64/niagara.c
> index f3e42d0326..ccad2c43a3 100644
> --- a/hw/sparc64/niagara.c
> +++ b/hw/sparc64/niagara.c
> @@ -98,7 +98,7 @@ static void add_rom_or_fail(const char *file, const hwaddr 
> addr)
>  static void niagara_init(MachineState *machine)
>  {
>      NiagaraBoardState *s = g_new(NiagaraBoardState, 1);
> -    DriveInfo *dinfo = drive_get_next(IF_PFLASH);
> +    DriveInfo *dinfo = drive_get(IF_PFLASH, 0, 0);
>      MemoryRegion *sysmem = get_system_memory();
>
>      /* init CPUs */
> --
> 2.31.1
>
>