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

[Markus Armbruster]

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().

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