completely broken IICBUS_IVAR_NOSTOP [Was: Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined?]

[Andriy Gapon]

A lot of time has passed but the problem is still there.
The "nostop" thing is completely broken.
I have recently thought about this issue a little bit again and now I think that
IVARs are not suitable for nostop property.  Simply put, there is no bus (in the
"newbus" sense) where such an IVAR could be configured for a child.

I see two possible solutions.

1. Make "nostop" a property of an iicbus instance.  We can keep the current
getter and setter, but they won't be IVAR accessors any more.  Also, target
devices for the calls to the functions would need to be adjusted.

2. Since this property hasn't found a wider use and remains specific to the
intel drm driver, we could just subclass iicbb and put the quirk into the
subclassed driver.


On 23/03/2018 11:56, Andriy Gapon wrote:
> 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