Page MenuHomeFreeBSD

first step towards enforcing must-succeed semantics for bus accessors
ClosedPublic

Authored by avg on May 29 2019, 9:54 AM.

Details

Summary

Unlike BUS_READ_IVAR / BUS_WRITE_IVAR, bus accessors do not have a
return code. It is assumed that there is a tight coupling between a bus
driver and a driver for a device on the bus with respect to instance
variables that the bus defines for its children. So, the driver is
supposed to have only valid accesses to the variables and, thus, the
accessors must always succeed.

Of course, programming errors sometimes happen. At present, such errors
go completely unnoticed. The idea of this change is to start catching
them. As a first step, there will be a warning about a failed accessor
call. This is to give developers a heads-up. I plan to add a panic
call some time later, so that the warning is harder to ignore.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

avg created this revision.May 29 2019, 9:54 AM
cem accepted this revision.May 29 2019, 9:03 PM

Is it worth a ratelimit or do none of these show up often in practice? panic is inherently ratelimiting ๐Ÿ˜

LGTM.

This revision is now accepted and ready to land.May 29 2019, 9:03 PM
ian accepted this revision.May 29 2019, 10:11 PM
ian added a subscriber: ian.

I think this is a good idea, especially if these turn into KASSERT after some initial period for flushing out existing offenders.

imp accepted this revision.May 29 2019, 10:15 PM

I'd be tempted to make them be PANIC rather than just a printf, which makes them better as a KASSERT which will compile away into nothing for non-debug kernels.

jhb added a comment.May 29 2019, 10:31 PM

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

avg added a comment.Jun 5 2019, 1:03 PM

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.

avg added a comment.Jun 5 2019, 1:12 PM

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.

This revision was automatically updated to reflect the committed changes.
cem added a comment.Jun 5 2019, 1:59 PM
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.

imp added a comment.Jun 5 2019, 3:30 PM
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.

jhb added a comment.Jun 5 2019, 7:44 PM
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.

avg added a comment.Jun 19 2019, 3:08 PM

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!

cem added a comment.Jun 19 2019, 5:28 PM
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.

avg added a comment.Jun 26 2019, 6:11 AM
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? :-)

imp added a comment.Jun 26 2019, 3:09 PM

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?

avg added a comment.Jun 26 2019, 5:18 PM

That's a good point, Warner.
Thanks!