ssi: ssi_auto_connect_slaves() never does anything, drop

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>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20200610053247.1583243-23-armbru@redhat.com>

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 2edccef2d54d9e2c70eb4d687f9c55f0e9f19fda..4fab1f5f855e0875fb96d40c9796596d5bc6138b 100644 (file)
--- 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 43b2f14dd280062020445c1180cb937194e9cd3c..7f703d8328dce2c87bbff8322db06c854e64f765 100644 (file)
--- 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 3050fabb692152df03da0125cbe28a351f438cab..b2432c5a132837d649a5cd6d3955d43b53b5fbfd 100644 (file)
--- 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 c6415eb6e329ee78f822f67231921e08646f124d..54106f5ef8ff875764140ac9bac6ea952b50db6c 100644 (file)
--- 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 eba7ccd46a7678dddc59e108a36209a3b7ee6ecb..80d1488dc75bfa2bfac8d95336a5b55b4553e702 100644 (file)
--- 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 e76cf290c8f5b80bd1f791a28483cb4ef08310fb..b9371dbf8d76153298a3ba2866de6e14e1481862 100644 (file)
--- 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) {

diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
index 1107cb89ee14d2989acc77e444885bfb9d060403..1725b13c32d166ef8bb42ef06b3602d60753a679 100644 (file)
--- 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);