Re: [PATCH 21/55] ssi: ssi_auto_connect_slaves() never does anything, drop

[Alistair Francis <alistair23@gmail.com>]

On Tue, May 19, 2020 at 8:14 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> ssi_auto_connect_slaves(parent, cs_line, bus) iterates over @parent's
> QOM children @dev of type TYPE_SSI_SLAVE.  It puts these on @bus, and
> sets cs_line[] to qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0).
>
> Suspicious: there is no protection against overrunning cs_line[].
>
> Turns out it's safe because ssi_auto_connect_slaves() never finds any
> such children.  Its called by realize methods of some (but not all)
> devices providing an SSI bus, and gets passed the device.
>
> SSI slave devices are always created with ssi_create_slave_no_init(),
> optionally via ssi_create_slave().  This adds them to their SSI bus.
> It doesn't set their QOM parent.
>
> ssi_create_slave_no_init() is always immediately followed by
> qdev_init_nofail(), with no QOM parent assigned, so
> device_set_realized() puts the device into the /machine/unattached/
> orphanage.  None become QOM children of a device providing an SSI bus.
>
> ssi_auto_connect_slaves() was added in commit b4ae3cfa57 "ssi: Add
> slave autoconnect helper".  I can't see which slaves it was supposed
> to connect back then.
>
> Cc: Alistair Francis <alistair@alistair23.me>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

This looks ok. I haven't tested it though.

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/ssi/ssi.h  |  4 ----
>  hw/ssi/aspeed_smc.c   |  1 -
>  hw/ssi/imx_spi.c      |  2 --
>  hw/ssi/mss-spi.c      |  1 -
>  hw/ssi/ssi.c          | 33 ---------------------------------
>  hw/ssi/xilinx_spi.c   |  1 -
>  hw/ssi/xilinx_spips.c |  4 ----
>  7 files changed, 46 deletions(-)
>
> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> index 1107cb89ee..1725b13c32 100644
> --- a/include/hw/ssi/ssi.h
> +++ b/include/hw/ssi/ssi.h
> @@ -86,10 +86,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
>
>  uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
>
> -/* Automatically connect all children nodes a spi controller as slaves */
> -void ssi_auto_connect_slaves(DeviceState *parent, qemu_irq *cs_lines,
> -                             SSIBus *bus);
> -
>  /* max111x.c */
>  void max111x_set_input(DeviceState *dev, int line, uint8_t value);
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 2edccef2d5..4fab1f5f85 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -1356,7 +1356,6 @@ static void aspeed_smc_realize(DeviceState *dev, Error **errp)
>
>      /* Setup cs_lines for slaves */
>      s->cs_lines = g_new0(qemu_irq, s->num_cs);
> -    ssi_auto_connect_slaves(dev, s->cs_lines, s->spi);
>
>      for (i = 0; i < s->num_cs; ++i) {
>          sysbus_init_irq(sbd, &s->cs_lines[i]);
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index 2dd9a631e1..2f09f15892 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -424,8 +424,6 @@ static void imx_spi_realize(DeviceState *dev, Error **errp)
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
>      sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
>
> -    ssi_auto_connect_slaves(dev, s->cs_lines, s->bus);
> -
>      for (i = 0; i < 4; ++i) {
>          sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->cs_lines[i]);
>      }
> diff --git a/hw/ssi/mss-spi.c b/hw/ssi/mss-spi.c
> index 3050fabb69..b2432c5a13 100644
> --- a/hw/ssi/mss-spi.c
> +++ b/hw/ssi/mss-spi.c
> @@ -376,7 +376,6 @@ static void mss_spi_realize(DeviceState *dev, Error **errp)
>      s->spi = ssi_create_bus(dev, "spi");
>
>      sysbus_init_irq(sbd, &s->irq);
> -    ssi_auto_connect_slaves(dev, &s->cs_line, s->spi);
>      sysbus_init_irq(sbd, &s->cs_line);
>
>      memory_region_init_io(&s->mmio, OBJECT(s), &spi_ops, s,
> diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
> index c6415eb6e3..54106f5ef8 100644
> --- a/hw/ssi/ssi.c
> +++ b/hw/ssi/ssi.c
> @@ -142,36 +142,3 @@ static void ssi_slave_register_types(void)
>  }
>
>  type_init(ssi_slave_register_types)
> -
> -typedef struct SSIAutoConnectArg {
> -    qemu_irq **cs_linep;
> -    SSIBus *bus;
> -} SSIAutoConnectArg;
> -
> -static int ssi_auto_connect_slave(Object *child, void *opaque)
> -{
> -    SSIAutoConnectArg *arg = opaque;
> -    SSISlave *dev = (SSISlave *)object_dynamic_cast(child, TYPE_SSI_SLAVE);
> -    qemu_irq cs_line;
> -
> -    if (!dev) {
> -        return 0;
> -    }
> -
> -    cs_line = qdev_get_gpio_in_named(DEVICE(dev), SSI_GPIO_CS, 0);
> -    qdev_set_parent_bus(DEVICE(dev), BUS(arg->bus));
> -    **arg->cs_linep = cs_line;
> -    (*arg->cs_linep)++;
> -    return 0;
> -}
> -
> -void ssi_auto_connect_slaves(DeviceState *parent, qemu_irq *cs_line,
> -                             SSIBus *bus)
> -{
> -    SSIAutoConnectArg arg = {
> -        .cs_linep = &cs_line,
> -        .bus = bus
> -    };
> -
> -    object_child_foreach(OBJECT(parent), ssi_auto_connect_slave, &arg);
> -}
> diff --git a/hw/ssi/xilinx_spi.c b/hw/ssi/xilinx_spi.c
> index eba7ccd46a..80d1488dc7 100644
> --- a/hw/ssi/xilinx_spi.c
> +++ b/hw/ssi/xilinx_spi.c
> @@ -334,7 +334,6 @@ static void xilinx_spi_realize(DeviceState *dev, Error **errp)
>
>      sysbus_init_irq(sbd, &s->irq);
>      s->cs_lines = g_new0(qemu_irq, s->num_cs);
> -    ssi_auto_connect_slaves(dev, s->cs_lines, s->spi);
>      for (i = 0; i < s->num_cs; ++i) {
>          sysbus_init_irq(sbd, &s->cs_lines[i]);
>      }
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index e76cf290c8..b9371dbf8d 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1270,7 +1270,6 @@ static void xilinx_spips_realize(DeviceState *dev, Error **errp)
>      XilinxSPIPS *s = XILINX_SPIPS(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>      XilinxSPIPSClass *xsc = XILINX_SPIPS_GET_CLASS(s);
> -    qemu_irq *cs;
>      int i;
>
>      DB_PRINT_L(0, "realized spips\n");
> @@ -1297,9 +1296,6 @@ static void xilinx_spips_realize(DeviceState *dev, Error **errp)
>
>      s->cs_lines = g_new0(qemu_irq, s->num_cs * s->num_busses);
>      s->cs_lines_state = g_new0(bool, s->num_cs * s->num_busses);
> -    for (i = 0, cs = s->cs_lines; i < s->num_busses; ++i, cs += s->num_cs) {
> -        ssi_auto_connect_slaves(DEVICE(s), cs, s->spi[i]);
> -    }
>
>      sysbus_init_irq(sbd, &s->irq);
>      for (i = 0; i < s->num_cs * s->num_busses; ++i) {
> --
> 2.21.1
>
>