Date: Wed, 29 May 2019 15:01:55 -0600 From: Ian Lepore <ian@freebsd.org> To: Andriy Gapon <avg@FreeBSD.org>, FreeBSD Hackers <freebsd-hackers@freebsd.org>, Konstantin Belousov <kib@freebsd.org> Subject: Re: completely broken IICBUS_IVAR_NOSTOP [Was: Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined?] Message-ID: <dab5ec57017aa7596a8ad975728794ad19e91d1c.camel@freebsd.org> In-Reply-To: <edebfae9-b678-7e0a-4598-097e5428b32d@FreeBSD.org> References: <CANC_bnOVAb7R%2Bc_8x66Gz5A1nNjJQ8NXpkaF4A85N0gnEjyO%2Bw@mail.gmail.com> <1521383420.99081.87.camel@freebsd.org> <0b0dee4b-e958-e25c-72d9-1ca296495802@FreeBSD.org> <edebfae9-b678-7e0a-4598-097e5428b32d@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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... > > > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?dab5ec57017aa7596a8ad975728794ad19e91d1c.camel>