Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined?

[Andriy Gapon]

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.


Oh, indeed.
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
iicbb_transfer().
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...

-- 
Andriy Gapon