Re: [PATCH 10/11] hw/sd: Remove unused legacy functions, stop killing mammoths

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

+Markus for commit 007d1dbf725 ("sd: Hide the qdev-but-not-quite thing
created by sd_init()").

On 28/1/25 11:45, Peter Maydell wrote:
> The sdcard_legacy.h header defines function prototypes for the "legacy"
> SD card API, which was used by non-qdevified SD controller models.
> We've now converted the only remaining non-qdev SD controller, so
> we can drop the legacy API.
> 
> Entirely unused functions:
>    sd_init(), sd_set_cb(), sd_enable()
> 
> Functions which now become static inside sd.c (they are the
> underlying implementations of methods on SDCardClass):
>    sd_do_command(), sd_write_byte(), sd_read_byte()
> 
> Removal of sd_init() means that we can also remove the
> me_no_qdev_me_kill_mammoth_with_rocks flag, the codepaths that were
> only reachable when it was set, and the inserted_cb and readonly_cb
> qemu_irq lines that went with that.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/sd/sdcard_legacy.h | 50 -----------------------
>   hw/sd/omap_mmc.c              |  2 +-
>   hw/sd/sd.c                    | 77 ++++-------------------------------
>   3 files changed, 9 insertions(+), 120 deletions(-)
>   delete mode 100644 include/hw/sd/sdcard_legacy.h
> 
> diff --git a/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h
> deleted file mode 100644
> index 0dc38895551..00000000000
> --- a/include/hw/sd/sdcard_legacy.h
> +++ /dev/null
> @@ -1,50 +0,0 @@
> -/*
> - * SD Memory Card emulation (deprecated legacy API)
> - *
> - * Copyright (c) 2006 Andrzej Zaborowski  <balrog@zabor.org>
> - *
> - * Redistribution and use in source and binary forms, with or without
> - * modification, are permitted provided that the following conditions
> - * are met:
> - *
> - * 1. Redistributions of source code must retain the above copyright
> - *    notice, this list of conditions and the following disclaimer.
> - * 2. Redistributions in binary form must reproduce the above copyright
> - *    notice, this list of conditions and the following disclaimer in
> - *    the documentation and/or other materials provided with the
> - *    distribution.
> - *
> - * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS''
> - * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> - * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> - * PARTICULAR PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR
> - * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> - * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> - * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> - * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
> - * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> - */
> -#ifndef HW_SDCARD_LEGACY_H
> -#define HW_SDCARD_LEGACY_H
> -
> -#include "hw/sd/sd.h"
> -
> -/* Legacy functions to be used only by non-qdevified callers */
> -SDState *sd_init(BlockBackend *blk, bool is_spi);
> -int sd_do_command(SDState *card, SDRequest *request, uint8_t *response);
> -void sd_write_byte(SDState *card, uint8_t value);
> -uint8_t sd_read_byte(SDState *card);
> -void sd_set_cb(SDState *card, qemu_irq readonly, qemu_irq insert);
> -
> -/* sd_enable should not be used -- it is only used on the nseries boards,
> - * where it is part of a broken implementation of the MMC card slot switch
> - * (there should be two card slots which are multiplexed to a single MMC
> - * controller, but instead we model it with one card and controller and
> - * disable the card when the second slot is selected, so it looks like the
> - * second slot is always empty).
> - */
> -void sd_enable(SDState *card, bool enable);
> -
> -#endif /* HW_SDCARD_LEGACY_H */
> diff --git a/hw/sd/omap_mmc.c b/hw/sd/omap_mmc.c
> index c1598135f44..d31456ad236 100644
> --- a/hw/sd/omap_mmc.c
> +++ b/hw/sd/omap_mmc.c
> @@ -25,7 +25,7 @@
>   #include "hw/irq.h"
>   #include "hw/sysbus.h"
>   #include "hw/arm/omap.h"
> -#include "hw/sd/sdcard_legacy.h"
> +#include "hw/sd/sd.h"

This hunk belongs to patch #4 "hw/sd/omap_mmc: Convert to SDBus API",
otherwise:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>   
>   typedef struct OMAPMMCState {
>       SysBusDevice parent_obj;
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index f781fd1641d..74bb7f39bbf 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -39,7 +39,6 @@
>   #include "hw/registerfields.h"
>   #include "system/block-backend.h"
>   #include "hw/sd/sd.h"
> -#include "hw/sd/sdcard_legacy.h"
>   #include "migration/vmstate.h"
>   #include "qapi/error.h"
>   #include "qemu/bitmap.h"
> @@ -120,10 +119,6 @@ typedef struct SDProto {
>   struct SDState {
>       DeviceState parent_obj;
>   
> -    /* If true, created by sd_init() for a non-qdevified caller */
> -    /* TODO purge them with fire */
> -    bool me_no_qdev_me_kill_mammoth_with_rocks;
> -
>       /* SD Memory Card Registers */
>       uint32_t ocr;
>       uint8_t scr[8];
> @@ -177,8 +172,6 @@ struct SDState {
>       uint32_t data_offset;
>       size_t data_size;
>       uint8_t data[512];
> -    qemu_irq readonly_cb;
> -    qemu_irq inserted_cb;
>       QEMUTimer *ocr_power_timer;
>       bool enable;
>       uint8_t dat_lines;
> @@ -892,17 +885,10 @@ static void sd_cardchange(void *opaque, bool load, Error **errp)
>           trace_sdcard_ejected();
>       }
>   
> -    if (sd->me_no_qdev_me_kill_mammoth_with_rocks) {
> -        qemu_set_irq(sd->inserted_cb, inserted);
> -        if (inserted) {
> -            qemu_set_irq(sd->readonly_cb, readonly);
> -        }
> -    } else {
> -        sdbus = SD_BUS(qdev_get_parent_bus(dev));
> -        sdbus_set_inserted(sdbus, inserted);
> -        if (inserted) {
> -            sdbus_set_readonly(sdbus, readonly);
> -        }
> +    sdbus = SD_BUS(qdev_get_parent_bus(dev));
> +    sdbus_set_inserted(sdbus, inserted);
> +    if (inserted) {
> +        sdbus_set_readonly(sdbus, readonly);
>       }
>   }
>   
> @@ -1000,48 +986,6 @@ static const VMStateDescription sd_vmstate = {
>       },
>   };
>   
> -/* Legacy initialization function for use by non-qdevified callers */
> -SDState *sd_init(BlockBackend *blk, bool is_spi)
> -{
> -    Object *obj;
> -    DeviceState *dev;
> -    SDState *sd;
> -    Error *err = NULL;
> -
> -    obj = object_new(is_spi ? TYPE_SD_CARD_SPI : TYPE_SD_CARD);
> -    dev = DEVICE(obj);
> -    if (!qdev_prop_set_drive_err(dev, "drive", blk, &err)) {
> -        error_reportf_err(err, "sd_init failed: ");
> -        return NULL;
> -    }
> -
> -    /*
> -     * Realizing the device properly would put it into the QOM
> -     * composition tree even though it is not plugged into an
> -     * appropriate bus.  That's a no-no.  Hide the device from
> -     * QOM/qdev, and call its qdev realize callback directly.
> -     */
> -    object_ref(obj);
> -    object_unparent(obj);
> -    sd_realize(dev, &err);
> -    if (err) {
> -        error_reportf_err(err, "sd_init failed: ");
> -        return NULL;
> -    }
> -
> -    sd = SD_CARD(dev);
> -    sd->me_no_qdev_me_kill_mammoth_with_rocks = true;
> -    return sd;
> -}
> -
> -void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
> -{
> -    sd->readonly_cb = readonly;
> -    sd->inserted_cb = insert;
> -    qemu_set_irq(readonly, sd->blk ? !blk_is_writable(sd->blk) : 0);
> -    qemu_set_irq(insert, sd->blk ? blk_is_inserted(sd->blk) : 0);
> -}
> -
>   static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
>   {
>       trace_sdcard_read_block(addr, len);
> @@ -2196,8 +2140,8 @@ static bool cmd_valid_while_locked(SDState *sd, unsigned cmd)
>       return cmd_class == 0 || cmd_class == 7;
>   }
>   
> -int sd_do_command(SDState *sd, SDRequest *req,
> -                  uint8_t *response) {
> +static int sd_do_command(SDState *sd, SDRequest *req,
> +                         uint8_t *response) {
>       int last_state;
>       sd_rsp_type_t rtype;
>       int rsplen;
> @@ -2349,7 +2293,7 @@ static bool sd_generic_read_byte(SDState *sd, uint8_t *value)
>       return false;
>   }
>   
> -void sd_write_byte(SDState *sd, uint8_t value)
> +static void sd_write_byte(SDState *sd, uint8_t value)
>   {
>       int i;
>   
> @@ -2478,7 +2422,7 @@ void sd_write_byte(SDState *sd, uint8_t value)
>       }
>   }
>   
> -uint8_t sd_read_byte(SDState *sd)
> +static uint8_t sd_read_byte(SDState *sd)
>   {
>       /* TODO: Append CRCs */
>       const uint8_t dummy_byte = 0x00;
> @@ -2561,11 +2505,6 @@ static bool sd_data_ready(SDState *sd)
>       return sd->state == sd_sendingdata_state;
>   }
>   
> -void sd_enable(SDState *sd, bool enable)
> -{
> -    sd->enable = enable;
> -}
> -
>   static const SDProto sd_proto_spi = {
>       .name = "SPI",
>       .cmd = {