On Tue, May 19, 2020 at 8:14 AM Markus Armbruster <arm...@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 <alist...@alistair23.me> > Signed-off-by: Markus Armbruster <arm...@redhat.com>
This looks ok. I haven't tested it though. Acked-by: Alistair Francis <alistair.fran...@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 > >