Re: [PATCH v5 6/8] hw/i2c: parent slaves created with i2c_slave_create_simple

[Markus Armbruster <armbru@redhat.com>]

Emmanuel Blot <emmanuel.blot@free.fr> writes:

> Slaves created with i2c_slave_create_simple() were left unparented and
> showed up under /machine/unattached with no stable QOM path. Add each
> slave as a QOM child of its bus, named after its I2C address, so it has
> a deterministic and addressable QOM path.

Could use an example.

Doesn't tell which machines are affected.

To find affected machines, I captured output of "info qom-tree" by running

    $ echo -e 'info qom-tree\nq' | qemu-system-TARGET -M MACHINE -S -display none -monitor stdio

for all targets and all their machine types.

This fails on my box for a number of machines: misses some image file,
needs Xen, ...  These are

    an5206
    boston
    boston-aia
    canon-a1100
    leon3_generic
    loongson3-virt
    mcf5208evb
    microchip-icicle-kit
    niagara
    nitro
    q800
    sx1
    sx1-v1
    xenfv-4.2
    xenpv
    xenpvh

Testing them manually would be possible.  Not by me.

Of the ones that run, output differs for

    anacapa-bmc
    ast1030-evb
    ast1040-evb
    ast1060-evb
    ast2500-evb
    ast2600-evb
    ast2700a1-evb
    ast2700a2-evb
    ast2700fc
    bletchley-bmc
    bpim2u
    catalina-bmc
    cubieboard
    fby35-bmc
    fuji-bmc
    g220a-bmc
    gb200nvl-bmc
    kudo-bmc
    lm3s811evb
    mpc8544ds
    npcm750-evb
    npcm845-evb
    palmetto-bmc
    powernv10-rainier
    ppce500
    quanta-gsj
    quanta-q71l-bmc
    rainier-bmc
    realview-eb
    realview-eb-mpcore
    realview-pb-a8
    realview-pbx-a9
    romulus-bmc
    sam460ex
    supermicro-x11spi-bmc
    supermicrox11-bmc
    tiogapass-bmc
    tt-atlantis
    versatileab
    versatilepb
    vexpress-a15
    vexpress-a9
    witherspoon-bmc
    yosemitev2-bmc

Looking for one with a short diff, I found realview-eb:

         /device[26] (versatile_i2c)
           /arm_sbcon_i2c[0] (memory-region)
           /i2c (i2c-bus)
    -    /device[27] (ds1338)
    +        /0x68 (ds1338)
         /device[2] (realview_gic)
           /gic (arm_gic)
             /gic_cpu[0] (memory-region)

The QOM path of this ds1338 device changes from

    /machine/unattached/device[27]

to

   /machine/unattached/device[26]/0x68

The new path still isn't stable, because the parent's path isn't.  This
caveat should be noted in the commit message.

An example of successful stabilization is tt-atlantis, where the QOM
paths of ds1338 and tmp105 change from

    /machine/unattached/device[21]
    /machine/unattached/device[22]

to

    /machine/i2c[0]/i2c-bus/0x6f
    /machine/i2c[4]/i2c-bus/0x48

> Signed-off-by: Emmanuel Blot <emmanuel.blot@free.fr>
> ---
>  hw/i2c/core.c        | 3 +++
>  include/hw/i2c/i2c.h | 7 +++++--
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
> index 54f6bdca88..0bbce45d48 100644
> --- a/hw/i2c/core.c
> +++ b/hw/i2c/core.c
> @@ -382,6 +382,9 @@ I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr)
>  {
>      I2CSlave *dev = i2c_slave_new(name, addr);
>  
> +    g_autofree char *childname = g_strdup_printf("0x%02x", addr);

Is the hex address a satisfactory child name?

> +    object_property_add_child(OBJECT(bus), childname, OBJECT(dev));
> +
>      i2c_slave_realize_and_unref(dev, bus, &error_abort);
>  
>      return dev;
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index dd5930f4b5..dc557bbf3f 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -166,13 +166,16 @@ bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
>  I2CSlave *i2c_slave_new(const char *name, uint8_t addr);
>  
>  /**
> - * Create and realize an I2C slave device on the heap.
> + * Create and realize an I2C slave device on the heap, add the device as a
> + * child of its parent bus.
> + *
>   * @bus: I2C bus to put it on
>   * @name: I2C slave device type name
>   * @addr: I2C address of the slave when put on a bus
>   *
>   * Create the device state structure, initialize it, put it on the
> - * specified @bus, and drop the reference to it (the device is realized).
> + * specified @bus, parent it, and drop the reference to it (the device is
> + * realized).
>   */
>  I2CSlave *i2c_slave_create_simple(I2CBus *bus, const char *name, uint8_t addr);