I'd just go ahead and go for the panic now (via a KASSERT) rather than the printf personally.

I would like to commit this change as is now. And then change printf to KASSERT in a week rather than in a month.
Hope that no one would object.

Also, another thing that I realized is that this defensive code won't help much if a bus defines BUS1_IVAR_X and a child device requests BUS2_IVAR_Y when BUS1_IVAR_X == BUS2_IVAR_Y.
BUS_READ_IVAR / BUS_WRITE_IVAR may succeed in that case but the result could be meaningless.

The only idea I have is to start gradually introducing a pattern where a first value of an ivar enum is some randomly chosen number.
E.g.:

enum {
        IICBUS_IVAR_ADDR = 100500,      /* Address or base address */
        IICBUS_IVAR_NOSTOP,             /* nostop defaults */
};

enum smbus_ivars {
    SMBUS_IVAR_ADDR = 100600,           /* slave address of the device */
};

That might help to minimize a chance of an accidental ivar value clash.
But changing this for existing ivars would break the ABI.
So, I guess, that we could start doing this only for new bus devices.

In D20458#443358, @avg wrote:

I would like to commit this change as is now. And then change printf to KASSERT in a week rather than in a month.
Hope that no one would object.

Ok by me.

In D20458#443359, @avg wrote:

Also, another thing that I realized is that this defensive code won't help much if a bus defines BUS1_IVAR_X and a child device requests BUS2_IVAR_Y when BUS1_IVAR_X == BUS2_IVAR_Y.
BUS_READ_IVAR / BUS_WRITE_IVAR may succeed in that case but the result could be meaningless.

The only idea I have is to start gradually introducing a pattern where a first value of an ivar enum is some randomly chosen number.
E.g.:

enum {
        IICBUS_IVAR_ADDR = 100500,      /* Address or base address */
        IICBUS_IVAR_NOSTOP,             /* nostop defaults */
};

enum smbus_ivars {
    SMBUS_IVAR_ADDR = 100600,           /* slave address of the device */
};

That might help to minimize a chance of an accidental ivar value clash.
But changing this for existing ivars would break the ABI.
So, I guess, that we could start doing this only for new bus devices.

Could do something cute like compile time hash of svn/git rev + bus + var name? Would require some makefile support or a c++1x constexpr compilation unit with extern C visibility (depending on how much we care to detect this). Obviously svn/git revision would be frozen at stable branch. Maybe too cute for little benefit.

In D20458#443364, @cem wrote:

In D20458#443358, @avg wrote:

I would like to commit this change as is now. And then change printf to KASSERT in a week rather than in a month.
Hope that no one would object.

Ok by me.

In D20458#443359, @avg wrote:

Also, another thing that I realized is that this defensive code won't help much if a bus defines BUS1_IVAR_X and a child device requests BUS2_IVAR_Y when BUS1_IVAR_X == BUS2_IVAR_Y.
BUS_READ_IVAR / BUS_WRITE_IVAR may succeed in that case but the result could be meaningless.

The only idea I have is to start gradually introducing a pattern where a first value of an ivar enum is some randomly chosen number.
E.g.:

enum {
        IICBUS_IVAR_ADDR = 100500,      /* Address or base address */
        IICBUS_IVAR_NOSTOP,             /* nostop defaults */
};

enum smbus_ivars {
    SMBUS_IVAR_ADDR = 100600,           /* slave address of the device */
};

That might help to minimize a chance of an accidental ivar value clash.
But changing this for existing ivars would break the ABI.
So, I guess, that we could start doing this only for new bus devices.

Could do something cute like compile time hash of svn/git rev + bus + var name? Would require some makefile support or a c++1x constexpr compilation unit with extern C visibility (depending on how much we care to detect this). Obviously svn/git revision would be frozen at stable branch. Maybe too cute for little benefit.

I'm in the way too little benefit for way too much pain camp here.

The IVARS of different busses isn't an issue, in general. It's only an issue when we do subclassing in the tree. So far, there's CardBus and IIC/SMB as the only two instances I'm aware of. While it's true some coordination is needed, doing cute compile time tricks is well beyond overkill.

In D20458#443359, @avg wrote:

Also, another thing that I realized is that this defensive code won't help much if a bus defines BUS1_IVAR_X and a child device requests BUS2_IVAR_Y when BUS1_IVAR_X == BUS2_IVAR_Y.
BUS_READ_IVAR / BUS_WRITE_IVAR may succeed in that case but the result could be meaningless.

The only idea I have is to start gradually introducing a pattern where a first value of an ivar enum is some randomly chosen number.
E.g.:

enum {
        IICBUS_IVAR_ADDR = 100500,      /* Address or base address */
        IICBUS_IVAR_NOSTOP,             /* nostop defaults */
};

enum smbus_ivars {
    SMBUS_IVAR_ADDR = 100600,           /* slave address of the device */
};

That might help to minimize a chance of an accidental ivar value clash.
But changing this for existing ivars would break the ABI.
So, I guess, that we could start doing this only for new bus devices.

You could do it in HEAD. This is definitely an old issue. ACPI uses a starting base of 100 for its IVARs so that it doesn't clash and so you can "decorate" an existing bus with ACPI IVARs. My thought many years ago was to extend the ACPI model by assigning unique ranges to each bus. This would give a cleaner (IMO) way to determine if a given device_t is a PCI device by seeing if has valid PCI IVARs for example as opposed to the current approach which is to see if it the parent device has the "pci" devclass.

I encountered a technical problem trying to replace the printf with KASSERT.
bus.h does not include systm.h and there are multiple places where bus.h is included before systm.h (or systm.h not included at all).
I am not looking forward to hear from Bruce Evans about yet another namespace pollution, but I am not sure what would be the best here.
Some options:

1. use expanded version of KASSERT, that is, panic under ifdef INVARIANTS
2. include systm.h from bus.h, despite
3. fix all places that include bus.h to include systm.h as well

Opinions, advice?
Thanks!

In D20458#447283, @avg wrote:

I encountered a technical problem trying to replace the printf with KASSERT.
bus.h does not include systm.h and there are multiple places where bus.h is included before systm.h (or systm.h not included at all).

We could create a _systm.h which can be included from bus.h without quite as much as pollution as all of systm.h. It's still some pollution but not as severe. I've wanted systm.h stuff in headers before.

We could instead just require systm.h be included at the top, like param.h. This option is not super appealing to me.

Whatever is needed from systm.h could be moved to another header? This probably breaks some consumers, but not all.

In D20458#447325, @cem wrote:

We could create a _systm.h which can be included from bus.h without quite as much as pollution as all of systm.h. It's still some pollution but not as severe. I've wanted systm.h stuff in headers before.

We could instead just require systm.h be included at the top, like param.h. This option is not super appealing to me.

Whatever is needed from systm.h could be moved to another header? This probably breaks some consumers, but not all.

Conrad, these are good suggestions. Thank you!
But I am still a bit stuck as I cannot pick the one option. The paradox of choice is strong with me.

As a side note, I thought that style(9) already required systm.h to be one of the top includes. But it does not seem to be the case. I see that many source files follow that convention and that's probably where I got the impression. Unfortunately for me, just as many files do not follow that "rule", so introducing it and fixing all the files is just not worth it.

_systm.h would probably be the easiest solution.
Can anyone bless it? :-)

I'm not sure I see the need for _system.h, at least for this. sys/bus.h isn't a standards defined thing, so name space pollution is not an issue, right?

That's a good point, Warner.
Thanks!