Re: ensuring a machine's buses have unique names

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 15 Sept 2021 at 05:28, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> > I'm not sure how best to sort this tangle out. We could:
>> >  * make controller devices pass in NULL as bus name; this
>> >    means that some bus names will change, which is an annoying
>> >    breakage but for these minor bus types we can probably
>> >    get away with it. This brings these buses into line with
>> >    how we've been handling uniqueness for ide and scsi.
>>
>> To gauge the breakage, we need a list of the affected bus names.
>
> Looking through, there are a few single-use or special
> purpose buses I'm going to ignore for now (eg vmbus, or
> the s390 ones). The four big bus types where controllers
> often specify a bus name and override the 'autogenerate
> unique name' handling are pci, ssi, sd, and i2c. (pci mostly
> gets away with it I expect by machines only having one pci
> bus.) Of those, I've gone through i2c. These are all the
> places where we create a specifically-named i2c bus (via
> i2c_init_bus()), together with the affected boards:
>
>    hw/arm/pxa2xx.c
>     - the PXA SoC code creates both the intended-for-use
>       i2c buses (which get auto-names) and also several i2c
>       buses intended for internal board-code use only which
>       are all given the same name "dummy".
>       Boards: connex, verdex, tosa, mainstone, akita, spitz,
>       borzoi, terrier, z2
>    hw/arm/stellaris.c
>     - The i2c controller names its bus "i2c". There is only one i2c
>       controller on these boards, so no name conflicts.
>       Boards: lm3s811evb, lm3s6965evb
>    hw/display/ati.c
>     - The ATI VGA device has an on-board i2c controller which it
>       connects the DDC that holds the EDID information. The bus is
>       always named "ati-vga.ddc", so if you have multiple of this
>       PCI device in the system the buses have the same names.
>    hw/display/sm501.c
>     - Same as ATI, but the bus name is "sm501.i2c"
>    hw/i2c/aspeed_i2c.c
>     - This I2C controller has either 14 or 16 (!) different i2c
>       buses, and it assigns them names "aspeed.i2c.N" for N = 0,1,2,...
>       The board code mostly seems to use these to wire up various
>       on-board i2c devices.
>       Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc,
>       swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb,
>       tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc
>    hw/i2c/bitbang_i2c.c
>     - the "GPIO to I2C bridge" device always names its bus "i2c".
>       Used only on musicpal, which only creates one of these buses.
>       Boards: musicpal
>    hw/i2c/exynos4210_i2c.c
>     - This i2c controller always names its bus "i2c". There are 9
>       of these controllers on the board, so they all have clashing
>       names.
>       Boards: nuri, smdkc210
>    hw/i2c/i2c_mux_pca954x.c
>     - This is an i2c multiplexer. All the child buses are named
>       "i2c-bus". The multiplexer is used by the aspeed and npcm7xx
>       boards. (There's a programmable way to get at individual
>       downstream i2c buses despite the name clash; none of the boards
>       using this multiplexer actually connect any devices downstream of
>       it yet.)
>       Boards: palmetto-bmc, supermicrox11-bmc, ast2500-evb, romulus-bmc,
>       swift-bmc, sonorapass-bmc, witherspoon-bmc, ast2600-evb,
>       tacoma-bmc, g220a-bmc, quanta-q71l-bmc, rainier-bmc,
>       npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc
>    hw/i2c/mpc_i2c.c
>     - This controller always names its bus "i2c". There is only one
>       of these controllers in the machine.
>       Boards: ppce500, mpc8544ds
>    hw/i2c/npcm7xx_smbus.c
>     - This controller always names its bus "i2c-bus". There are multiple
>       controllers on the boards. The name also clashes with the one used
>       by the pca954x muxes on these boards (see above).
>       Boards: npcm750-evb, quanta-gsj, quanta-gbs-bmc, kudo-bmc
>    hw/i2c/pm_smbus.c
>     - This is the PC SMBUS implementation (it is not a QOM device...)
>       The bus is always called "i2c".
>       Boards: haven't worked through; at least all the x86 PC-like
>       boards, I guess
>    hw/i2c/ppc4xx_i2c.c
>     - This controller always names its bus "i2c". The taihu and
>       ref405ep have only one controller, but sam460ex has two which
>       will have non-unique names.
>       Boards: taihu, ref405ep, sam460ex
>    hw/i2c/versatile_i2c.c
>     - This controller always names its bus "i2c". The MPS boards all
>       have multiples of this controller with clashing names; the others
>       have only one controller.
>       Boards: mps2-an385, mps2-an386, mps2-an500, mps2-an511,
>       mps2-an505, mps2-an521, mps3-an524, mps3-an547,
>       realview-eb, realview-eb-mpcore, realview-pb-a8, realview-pbx-a9,
>       versatileab, versatilepb, vexpress-a9, vexpress-a15
>
> In a lot of these cases I suspect the i2c controllers are
> provided either to allow connection of various internal-to-the-board
> devices, or simply so that guest OS bootup code that initializes
> the i2c controller doesn't fall over. However since there's
> nothing stopping users from creating i2c devices themselves
> on the commandline, some people might be doing that.

What are the devices they could add?  Anything they could *reasonably*
want to add?  Breaking "unreasonable" uses could be defendable.

The only spot where we set ->bus_type = TYPE_I2C_BUS is
i2c_slave_class_init().  Therefore, only the concrete subtypes of
TYPE_I2C_SLAVE can go on an i2c bus, which makes sense.  These are:

    TYPE_ADM1272            "adm1272"
    TYPE_AER915             "aer915"
    TYPE_AT24C_EE           "at24c-eeprom"
    TYPE_DS1338             "ds1338"
                            "emc1413"
                            "emc1414"
    TYPE_I2CDDC             "i2c-ddc"
    TYPE_LM8323             "lm8323"
    TYPE_M41T80             "m41t80"
    TYPE_MAX34451           "max34451"
    TYPE_MAX7310            "max7310"
    TYPE_PCA9546            "pca9546"
    TYPE_PCA9548            "pca9548"
    TYPE_PCA9552            "pca9552"
    TYPE_PXA2XX_I2C_SLAVE   "pxa2xx-i2c-slave"
    TYPE_SII9022            "sii9022"
    TYPE_SMBUS_DEVICE       "smbus-device"
    TYPE_SMBUS_EEPROM       "smbus-eeprom"
    TYPE_SMBUS_IPMI         "smbus-ipmi"
    TYPE_SSD0303            "ssd0303"
    TYPE_TMP105             "tmp105"
                            "tmp421"
                            "tmp422"
                            "tmp423"
    TYPE_TOSA_DAC           "tosa_dac"
    TYPE_TWL92230           "twl92230"
    TYPE_WM8750             "wm8750"

Grep hits in:

    hw/arm/aspeed.c
    hw/arm/musicpal.c
    hw/arm/npcm7xx_boards.c
    hw/arm/nseries.c
    hw/arm/pxa2xx.c
    hw/arm/realview.c
    hw/arm/spitz.c
    hw/arm/stellaris.c
    hw/arm/tosa.c
    hw/arm/versatilepb.c
    hw/arm/vexpress.c
    hw/arm/z2.c
    hw/audio/marvell_88w8618.c
    hw/audio/wm8750.c
    hw/display/ati.c
    hw/display/i2c-ddc.c
    hw/display/sii9022.c
    hw/display/sm501.c
    hw/display/ssd0303.c
    hw/display/xlnx_dp.c
    hw/gpio/max7310.c
    hw/i2c/i2c_mux_pca954x.c
    hw/i2c/pmbus_device.c
    hw/i2c/smbus_eeprom.c
    hw/i2c/smbus_slave.c
    hw/input/lm832x.c
    hw/ipmi/smbus_ipmi.c
    hw/misc/pca9552.c
    hw/nvram/eeprom_at24c.c
    hw/ppc/e500.c
    hw/ppc/sam460ex.c
    hw/rtc/ds1338.c
    hw/rtc/m41t80.c
    hw/rtc/twl92230.c
    hw/sensor/adm1272.c
    hw/sensor/emc141x.c
    hw/sensor/max34451.c
    hw/sensor/tmp105.c
    hw/sensor/tmp421.c
    include/hw/audio/wm8750.h
    include/hw/display/i2c-ddc.h
    include/hw/i2c/i2c_mux_pca954x.h
    include/hw/i2c/smbus_slave.h
    include/hw/input/lm832x.h
    include/hw/misc/pca9552.h
    include/hw/sensor/tmp105.h
    tests/qtest/adm1272-test.c
    tests/qtest/ds1338-test.c
    tests/qtest/emc141x-test.c
    tests/qtest/max34451-test.c
    tests/qtest/pca9552-test.c
    tests/qtest/tmp105-test.c

>
> In some of these cases (eg the i2c bus on the ATI VGA driver)
> I suspect the desired behaviour is "unique bus name based on
> a standard template, eg 'ati-vga.ddc.0/1/...'. It sounds like
> we can't do that, though.

We could add yet another case to qbus_init(): if name is non-null, and
bus->parent has a flag set, we use

    "%s.%d", name, bus->parent->num_child_bus.

"Could" doesn not imply "should".

>                           (Also they probably don't want to
> permit users to command-line plug i2c devices into it...)

Then they should massage the bus so it doesn't accept additional
devices.  Didn't you post a patch related to this recently?