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

["Cédric Le Goater" <clg@kaod.org>]

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.