Date: Sat, 24 Mar 2018 09:43:19 -0600 From: Ian Lepore <ian@freebsd.org> To: Andriy Gapon <avg@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: <1521906199.51564.19.camel@freebsd.org> In-Reply-To: <0b0dee4b-e958-e25c-72d9-1ca296495802@FreeBSD.org> References: <CANC_bnOVAb7R%2Bc_8x66Gz5A1nNjJQ8NXpkaF4A85N0gnEjyO%2Bw@mail.gmail.com> <1521383420.99081.87.camel@freebsd.org> <0b0dee4b-e958-e25c-72d9-1ca296495802@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 2018-03-23 at 11:56 +0200, 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...
>
I think our whole interface for transfers is conceptually broken. I
would like to just redefine the behavior of the interface. I think what
I want is basically the same thing that nostop ivar was trying to
achieve:
Ignore the existing start/stop flags in the transfer structures.
When you pass an array of transfers, there is one bus START at the
beginning, one bus STOP at the end, and an automatic REPEAT_START
between any two transfers in the array where the slave address or
direction changes. When there are two adjacent transfers in the
array with the same address and direction, that's just one long
continuous flow of bytes -- effectively, it's scatter/gather IO.
As an optimization, we could define an IICBUS_STOP flag that could be
added to any transfer in the array to force a full STOP/START sequence
after that transfer and before the next. That would amount to basically
a minor optimization... it would be identical to just calling transfer
twice. So I'm not even sure it's a good idea.
We might need to leave the current broken interface in place for out-
of-tree code like proprietary drivers, and define a new transfer method
that works the new way.
When I started looking at all existing callers of iicbus_transfer() I
came to the conclusion that almost all of them followed such identical
patterns that I wrote the iicdev_readfrom()/writeto() functions. I
figured I would run through the system converting everything I could to
those new functions, then whatever changes need to be made to the
interface would almost all be in just a couple functions. But that
project bogged down then other things came along and I forgot all about
it.
-- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1521906199.51564.19.camel>