Re: [PATCH RFC 2/2] hw: Replace drive_get_next() by drive_get()

[Cédric Le Goater]

On 11/16/21 10:29, Markus Armbruster wrote:
> Cédric Le Goater <clg@kaod.org> writes:
>
> > On 11/15/21 16:57, Markus Armbruster wrote:
> > > Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> > >
> > > > On 11/15/21 13:55, Markus Armbruster 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.
> > > > >
> > > > > Explicit is better than implicit: use drive_get() directly.
> > > > >
> > > > > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > > > > ---
> > > > >    include/sysemu/blockdev.h           |  1 -
> > > > >    blockdev.c                          | 10 ----------
> > > > >    hw/arm/aspeed.c                     | 21 +++++++++++++--------
> > > > >    hw/arm/cubieboard.c                 |  2 +-
> > > > >    hw/arm/imx25_pdk.c                  |  2 +-
> > > > >    hw/arm/integratorcp.c               |  2 +-
> > > > >    hw/arm/mcimx6ul-evk.c               |  2 +-
> > > > >    hw/arm/mcimx7d-sabre.c              |  2 +-
> > > > >    hw/arm/msf2-som.c                   |  2 +-
> > > > >    hw/arm/npcm7xx_boards.c             |  6 +++---
> > > > >    hw/arm/orangepi.c                   |  2 +-
> > > > >    hw/arm/raspi.c                      |  2 +-
> > > > >    hw/arm/realview.c                   |  2 +-
> > > > >    hw/arm/sabrelite.c                  |  2 +-
> > > > >    hw/arm/versatilepb.c                |  4 ++--
> > > > >    hw/arm/vexpress.c                   |  6 +++---
> > > > >    hw/arm/xilinx_zynq.c                | 16 +++++++++-------
> > > > >    hw/arm/xlnx-versal-virt.c           |  3 ++-
> > > > >    hw/arm/xlnx-zcu102.c                |  6 +++---
> > > > >    hw/microblaze/petalogix_ml605_mmu.c |  2 +-
> > > > >    hw/misc/sifive_u_otp.c              |  2 +-
> > > > >    hw/riscv/microchip_pfsoc.c          |  2 +-
> > > > >    hw/sparc64/niagara.c                |  2 +-
> > > > >    23 files changed, 49 insertions(+), 52 deletions(-)
> > > >
> > > > > @@ -435,11 +438,13 @@ static void aspeed_machine_init(MachineState *machine)
> > > > >        }
> > > > >          for (i = 0; i < bmc->soc.sdhci.num_slots; i++) {
> > > > > -        sdhci_attach_drive(&bmc->soc.sdhci.slots[i], drive_get_next(IF_SD));
> > > > > +        sdhci_attach_drive(&bmc->soc.sdhci.slots[i],
> > > > > +                           drive_get(IF_SD, 0, i));
> > > >
> > > > If we put SD on bus #0, ...
> > > >
> > > > >        }
> > > > >          if (bmc->soc.emmc.num_slots) {
> > > > > -        sdhci_attach_drive(&bmc->soc.emmc.slots[0], drive_get_next(IF_SD));
> > > > > +        sdhci_attach_drive(&bmc->soc.emmc.slots[0],
> > > > > +                           drive_get(IF_SD, 0, bmc->soc.sdhci.num_slots));
> > > >
> > > > ... we'd want to put eMMC on bus #1
> > >
> > > Using separate buses for different kinds of devices would be neater, but
> > > it also would be an incompatible change.  This patch keeps existing
> > > bus/unit numbers working.  drive_get_next() can only use bus 0.
> >
> > All Aspeed SoCs have 3 SPI busses, each with multiple CS, and also multiple
> > sdhci controllers with multiple slots.
> >
> > How drives are defined for the aspeed machines can/should be improved.
> > The machine init iterates on the command line drives, attaches the
> > DriveInfo, in the order found, to a m25p80 device model first and then
> > follows on with the SD devices. This is fragile clearly and a bus+id
> > would be most welcome to identify the drive backend.
> >
> > May be this is a prereq for this patchset ?
>
> Such a change will probably be easier to review after this patch,
> because then it's just a matter of changing / dumbing down parameters to
> drive_get().

ok.

> I can't judge whether incompatible change is okay here.

It looks ok to me since you are using the number of possible devices
of the previous controller as an offset for drive_get().

Thanks,

C.