Cédric Le Goater <c...@kaod.org> writes: > On 11/15/21 16:57, Markus Armbruster wrote: >> Philippe Mathieu-Daudé <f4...@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 <arm...@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(). I can't judge whether incompatible change is okay here.