Date: Fri, 23 Mar 2018 11:56:16 +0200 From: Andriy Gapon <avg@FreeBSD.org> To: Ian Lepore <ian@freebsd.org>, FreeBSD Hackers <freebsd-hackers@freebsd.org> Cc: Konstantin Belousov <kib@freebsd.org> Subject: Re: Custom I2C and RTC chip drivers: where is iccbus_get_nostop() defined? Message-ID: <0b0dee4b-e958-e25c-72d9-1ca296495802@FreeBSD.org> In-Reply-To: <1521383420.99081.87.camel@freebsd.org> References: <CANC_bnOVAb7R%2Bc_8x66Gz5A1nNjJQ8NXpkaF4A85N0gnEjyO%2Bw@mail.gmail.com> <1521383420.99081.87.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?0b0dee4b-e958-e25c-72d9-1ca296495802>