Date: Sat, 23 Nov 2024 22:28:50 -0700 From: Warner Losh <imp@bsdimp.com> To: Poul-Henning Kamp <phk@phk.freebsd.dk> Cc: arch@freebsd.org Subject: Re: Locking inconsistencies in I2C land Message-ID: <CANCZdfrkdbsaonxPrp_QwoEtpx1ieeQSo6D1aynZErysx1t6wA@mail.gmail.com> In-Reply-To: <202411232114.4ANLEGaj077161@critter.freebsd.dk> References: <202411232114.4ANLEGaj077161@critter.freebsd.dk>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000c316750627a1e474
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
On Sat, Nov 23, 2024 at 2:14=E2=80=AFPM Poul-Henning Kamp <phk@phk.freebsd.=
dk>
wrote:
> The locking in I2C land is inconsistent.
>
> For the sake of this explanation, I will concentrate on
> DEVMETHOD(iicbus_transfer), of which we have about a dozen
> implementations, and which seems to be the most used part
> of the KAPI.
>
It's also the newest KAPI...
The first level of locking must ensure that there can only be one
> DEVMETHOD(iicbus_transfer) active on the I2C bus at any time.
>
> Some drivers do this with a mutex or sxlock.
>
> Some drivers do no locking, leaving it to the users to fight it out.
> In particular drivers which implement wire-primitives and leave
> *_transfer() to iicbus_transfer_gen() get no locking.
>
>
> At the second level of locking, some users need to be able to execute
> a sequence of transactions with no other traffic on the bus, and
> preferably also without the bus/slave going away.
>
> This is implemented via iicbus_{request|release}_bus() which call
> the underlying driver using the surprisingly named
> DEVMETHOD(iicbus_callback) to hear if it has an opinion.
>
> But iicbus_{request|release}_bus() is only advisory: Nothing
> prevents other callers to intrude with traffic subject only to
> whatever locking the primary level might provide.
>
It's not supposed to be advisory. The theory, evidently not the
practice, is that you're supposed to grab the bus, do your thing,
and then release the bus. We used to have an API that was each
phase of the I2C transaction: START XFER STOP, etc and you
were supposed to grab the bus to do a transaction. Later, we
added transaction KPI (I think it was Ian, who needed to support
an i.MX6 SoC that had full transactions, but no access to the individual
phases,IIRC), but at the time there was an assumption one wouldn't
necessarily need to snag the bus for that...
I've not looked closely at how things have evolved since then... I suspect
that the initial design, way back when, was that we'd take a lock, do the
toggling, and then release the lock. Then I think that the whole
request/release
were added because that's holding the locks too long... I have a vague
recollection that holding the locks caused a big performance problem on
the old Atmel gear that Ian and I worked on before I left and he went off
to work on the successor i.MX6 product.
> The ichiic driver tries to mitigate with both levels sharing the
> same sxlock.
>
> That only makes matters worse, because the transfer function contains
> this logic:
>
> allocated =3D sx_xlocked(&sc->call_lock) !=3D 0;
> if (!allocated)
> sx_xlock(&sc->call_lock);
> [=E2=80=A6do the job=E2=80=A6]
> if (!allocated)
> sx_unlock(&sc->call_lock);
>
> Which means that as long as /somebody/ holds iic_request_bus(), there
> will be no first level locking for /anybody/.
>
Yes. This sounds like we have bigger issues...
> We also have other equally hard to fix I2C architectual trouble.
>
> For instance our shifting slave addresses one bit to the left, which
> neither FDT, ACPI nor anybody else does, so we cannot use the
> iicbus-methods directly from those contexts, because we have to
> munge the slave addresses one bit to the right first.
>
Yea... In hind sight, that was a big mistake way back when. I don't even
recall why we did that now... I kinda think it was that way when I started
hacking on the stuff, but I don't recall now.
> Most of the trouble comes from I2C migrating from being "I've tied
> some fun chips to my parallel port" to "there is silicon for 24
> I2C controllers in this laptop, and we need to get at least six of
> them working", which is where I'm sitting now.
>
>
> I have a hard time imagining a remedy for these problems which do
> not involve going over every single I2C driver and I2C user in the
> tree and breaking a lot of out-of-tree stuff in the process, and
> at the end we would still have a KAPI designed last century and
> still ill suited for modern hardware and use cases.
>
>
> It looks, to me, like the sensible cure to adopt or design a new
> I2C KAPI, aimed squarly at ACPI/FDT land, and leave the old stuff
> to rot away, for some number of release cycles.
>
> Input ?
It's not a terrible idea. I suspect that a few of the old things will be
updated, but not all. I can't imagine anybody still using the parallel
port or viapm bit-banger. Though the generic GPIO i2c BB likely would
be a candidate to update. We do have half a dozen, at least, controllers
that are used on modern gear. But dozens of i2c device drivers... what's
the plan there? What did you have in mind?
Warner
--000000000000c316750627a1e474
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">=
<div dir=3D"ltr" class=3D"gmail_attr">On Sat, Nov 23, 2024 at 2:14=E2=80=AF=
PM Poul-Henning Kamp <<a href=3D"mailto:phk@phk.freebsd.dk">phk@phk.free=
bsd.dk</a>> wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"m=
argin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left=
:1ex">The locking in I2C land is inconsistent.<br>
<br>
For the sake of this explanation, I will concentrate on<br>
DEVMETHOD(iicbus_transfer), of which we have about a dozen<br>
implementations, and which seems to be the most used part<br>
of the KAPI.<br></blockquote><div><br></div><div>It's also the newest K=
API...</div><div><br></div><blockquote class=3D"gmail_quote" style=3D"margi=
n:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex=
">
The first level of locking must ensure that there can only be one<br>
DEVMETHOD(iicbus_transfer) active on the I2C bus at any time.<br>
<br>
Some drivers do this with a mutex or sxlock.<br>
<br>
Some drivers do no locking, leaving it to the users to fight it out.<br>
In particular drivers which implement wire-primitives and leave<br>
*_transfer() to iicbus_transfer_gen() get no locking.<br>
<br>
<br>
At the second level of locking, some users need to be able to execute<br>
a sequence of transactions with no other traffic on the bus, and<br>
preferably also without the bus/slave going away.<br>
<br>
This is implemented via iicbus_{request|release}_bus() which call<br>
the underlying driver using the surprisingly named<br>
DEVMETHOD(iicbus_callback) to hear if it has an opinion.<br>
<br>
But iicbus_{request|release}_bus() is only advisory:=C2=A0 Nothing<br>
prevents other callers to intrude with traffic subject only to<br>
whatever locking the primary level might provide.<br></blockquote><div><br>=
</div><div>It's not supposed to be advisory. The theory, evidently not =
the</div><div>practice, is that you're supposed to grab the bus, do you=
r thing,</div><div>and then release the bus. We used to have an API that wa=
s each</div><div>phase of the I2C transaction: START XFER STOP, etc and you=
</div><div>were supposed to grab the bus to do a transaction. Later, we</di=
v><div>added transaction KPI (I think it was Ian, who needed to support</di=
v><div>an i.MX6 SoC that had full transactions, but no access to the indivi=
dual</div><div>phases,IIRC), but at the time there was an assumption one wo=
uldn't</div><div>necessarily need to snag the bus for that...</div><div=
><br></div><div>I've not looked closely at how things have evolved sinc=
e then...=C2=A0 I suspect</div><div>that the initial design, way back when,=
was that we'd take a lock, do the</div><div>toggling, and then release=
the lock. Then I think that the whole request/release</div><div>were added=
because that's holding the locks too long... I have a vague</div><div>=
recollection that holding the locks caused a big performance problem on</di=
v><div>the old Atmel gear that Ian and I worked on before I left and he wen=
t off</div><div>to work on the successor i.MX6 product.</div><div>=C2=A0</d=
iv><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;bord=
er-left:1px solid rgb(204,204,204);padding-left:1ex">
The ichiic driver tries to mitigate with both levels sharing the<br>
same sxlock.<br>
<br>
That only makes matters worse, because the transfer function contains<br>
this logic:<br>
<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 allocated =3D sx_xlocked(&sc->call_lock)=
!=3D 0;<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!allocated)<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sx_xlock(&sc-&g=
t;call_lock);<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 [=E2=80=A6do the job=E2=80=A6]<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!allocated)<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 sx_unlock(&sc-&=
gt;call_lock);<br>
<br>
Which means that as long as /somebody/ holds iic_request_bus(), there<br>
will be no first level locking for /anybody/.<br></blockquote><div><br></di=
v><div>Yes. This sounds like we have bigger issues...</div><div>=C2=A0</div=
><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border=
-left:1px solid rgb(204,204,204);padding-left:1ex">
We also have other equally hard to fix I2C architectual trouble.<br>
<br>
For instance our shifting slave addresses one bit to the left, which<br>
neither FDT, ACPI nor anybody else does, so we cannot use the<br>
iicbus-methods directly from those contexts, because we have to<br>
munge the slave addresses one bit to the right first.<br></blockquote><div>=
<br></div><div>Yea... In hind sight, that was a big mistake way back when. =
I don't even</div><div>recall why we did that now...=C2=A0 I kinda thin=
k it was that way when I started</div><div>hacking on the stuff, but I don&=
#39;t recall now.</div><div>=C2=A0</div><blockquote class=3D"gmail_quote" s=
tyle=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);pad=
ding-left:1ex">
Most of the trouble comes from I2C migrating from being "I've tied=
<br>
some fun chips to my parallel port" to "there is silicon for 24<b=
r>
I2C controllers in this laptop, and we need to get at least six of<br>
them working", which is where I'm sitting now.<br>
<br>
<br>
I have a hard time imagining a remedy for these problems which do<br>
not involve going over every single I2C driver and I2C user in the<br>
tree and breaking a lot of out-of-tree stuff in the process, and<br>
at the end we would still have a KAPI designed last century and<br>
still ill suited for modern hardware and use cases.<br>
<br>
<br>
It looks, to me, like the sensible cure to adopt or design a new<br>
I2C KAPI, aimed squarly at ACPI/FDT land, and leave the old stuff<br>
to rot away, for some number of release cycles.<br>
<br>
Input ?</blockquote><div><br></div><div>It's not a terrible idea. I sus=
pect that a few of the old things will be</div><div>updated, but not all. I=
can't imagine anybody still using the parallel</div><div>port or viapm=
=C2=A0bit-banger. Though the generic GPIO i2c BB likely would</div><div>be =
a candidate to update.=C2=A0 We do have half a dozen, at least, controllers=
</div><div>that are used on modern gear. But dozens of i2c device drivers..=
. what's</div><div>the plan there? What did you have in mind?</div><div=
><br></div><div>Warner</div></div></div>
--000000000000c316750627a1e474--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfrkdbsaonxPrp_QwoEtpx1ieeQSo6D1aynZErysx1t6wA>