Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined?
avg at FreeBSD.org
Fri Mar 23 10:02:53 UTC 2018
On 18/03/2018 16:30, Ian Lepore wrote:
> Now for the bad news: don't use it. It doesn't work. It's 100% a bug
> in the code that maybe kinda-sorta seemed to work for whoever added it,
> because accidentally the right garbage was on the stack in the local
> nostop var. The generic transfer code doesn't check that the accessor
> failed so it ends up using stack garbage for nostop. The reason
> there's g'teed to be no such ivar is because the code is asking the
> wrong device, and it doesn't even have a handle to the right child
> device to get the info it wants.
I think that there never was an intention to make "nostop" a property of an i2c
slave, a child of an iicbus device. I think that instead it was supposed to be
a property of the iicbus's parent device, an actual i2c adapter driver.
I guess that the reason that "nostop" became an ivar in iicbus was an incorrect
understanding of how a "bit-banger" device (something implementing iicbb_if),
iicbb device and iicbus device are connected. I think that I was among the
reviewers and I probably had a bit of confusion back then.
It seems that the only place where iicbus_get_nostop() is used is
iicbus_transfer_gen(). iicbus_transfer_gen is used in several i2c drivers as an
iicbus_transfer method. it's also used in iicbb, thinly wrapped by
So, iicbus_get_nostop() actually translates to a call to BUS_READ_IVAR(parent,
device, 1, &v) where I already substituted one for IICBUS_IVAR_NOSTOP.
Here we can see that while the accessor functions lok quite nice they get
expanded to generic calls without much context. So, whether that
BUS_READ_IVAR() call succeeds and what value it gets depends on whether the
parent bus defines an ivar with a value of 1 and what value that ivar might have
for the driver device. Many buses define at least a couple of ivars.
So, a return value of iicbus_get_nostop() could be consistent for a particular
driver while still being arbitrary. But it can be a piece of stack garbage too,
just as you said.
The only place where iicbus_set_nostop() is used is intel_iicbb_attach().
In that case the ivar is "set" on a wrong device at all (the bit-banger, not iicbb).
This definitely needs to be fixed / reworked.
Perhaps nostop should become a new interface in iicbus_if and iicbb_if...
More information about the freebsd-hackers