Re: ensuring a machine's buses have unique names

[Peter Maydell]

On Thu, 26 Aug 2021 at 15:08, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> > What's the right way to ensure that when a machine has multiple
> > buses of the same type (eg multiple i2c controllers, multiple
> > sd card controllers) they all get assigned unique names so that
> > the user can use '-device ...,bus=some-name' to put a device on a
> > specific bus?

> Another one used to be isapc.  It's not anymore.  I believe it's due to
>
> commit 61de36761b565a4138d8ad7ec75489ab28fe84b6
> Author: Alexander Graf <agraf@csgraf.de>
> Date:   Thu Feb 6 16:08:15 2014 +0100
>
>     qdev: Keep global allocation counter per bus

> Note that the automatic bus numbers depend on the order in which board
> code creates devices.  Too implicit and fragile for my taste.  But it's
> been working well enough.

I had a bit of a look into this. I think the problem here is that
we created a family of easy-to-misuse APIs and then misused them...

The qbus_create() and qbus_create_inplace() functions both take
a 'const char *name' argument. If they're passed in NULL then
they create an automatically-uniquified name (as per the commit
above). If they're passed in a non-NULL string then they use
it as-is, whether it's unique in the system or not. We then
typically wrap qbus_create() in a bus-specific creation function
(examples are scsi_bus_new(), ide_bus_new(), i2c_init_bus()).
Mostly those creation functions also take a 'name' argument and
pass it through. ide_bus_new() is an interesting exception which
does not take a name argument.

The easy-to-misuse part is that now we have a set of functions
that look like you should pass them in a name (and where there's
plenty of code in the codebase that passes in a name) but where
that's the wrong thing unless you're a board model and are
picking a guaranteed unique name, or you're an odd special case
like virtio-scsi. (virtio-scsi is the one caller of scsi_bus_new()
that passes in something other than NULL.) In particular for
code which is implementing a device that is a whatever-controller,
creating a whatever-bus and specifying a name is almost always
going to be wrong, because as soon as some machine creates two
of these whatever-controllers it has non-unique bus names.

It looks like IDE buses are OK because ide_bus_new() takes no
name argument, and SCSI buses are OK because the callers
correctly pass in NULL, but almost all the "minor" buses
(SD, I2C, ipack, aux...) have a lot of incorrect naming of
buses in controller models.

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.
 * drop the 'name' argument for buses like ide that don't
   actually have any callsites that need to pass a name
 * split into foo_bus_new() and foo_bus_new_named() so that
   the "easy default" doesn't pass a name, and there's at least
   a place to put a doc comment explaining that the name passed
   into the _named() version should be unique ??
 * something else ?

thanks
-- PMM