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

[Ian Lepore]

On Fri, 2019-05-24 at 20:43 +0300, Andriy Gapon wrote:
> 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.
> 

Sorry for the delay in responding to this, I got busy with $work for a
few days.

I think the right thing to do would be to implement iicbus_transfer
directly in the intel_iicbb_driver, then it could do whatever it wants
in terms of ignoring STOP without perturbing other drivers.  But I'm
not sure it's practical to do so unless someone notices a problem,
because I'm not sure how we'd test it otherwise.

-- Ian

> 
> 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...
> > 
> 
>