From nobody Sun Nov 24 05:28:50 2024
X-Original-To: arch@mlmmj.nyi.freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1])
by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Xwy7M30vlz5dWN6
for <arch@mlmmj.nyi.freebsd.org>; Sun, 24 Nov 2024 05:29:03 +0000 (UTC)
(envelope-from wlosh@bsdimp.com)
Received: from mail-pl1-x632.google.com (mail-pl1-x632.google.com [IPv6:2607:f8b0:4864:20::632])
(using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)
key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256
client-signature RSA-PSS (2048 bits) client-digest SHA256)
(Client CN "smtp.gmail.com", Issuer "WR4" (verified OK))
by mx1.freebsd.org (Postfix) with ESMTPS id 4Xwy7M0gJ4z4XHM
for <arch@freebsd.org>; Sun, 24 Nov 2024 05:29:03 +0000 (UTC)
(envelope-from wlosh@bsdimp.com)
Authentication-Results: mx1.freebsd.org;
none
Received: by mail-pl1-x632.google.com with SMTP id d9443c01a7336-21200c749bfso33038815ad.1
for <arch@freebsd.org>; Sat, 23 Nov 2024 21:29:02 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=bsdimp-com.20230601.gappssmtp.com; s=20230601; t=1732426141; x=1733030941; darn=freebsd.org;
h=cc:to:subject:message-id:date:from:in-reply-to:references
:mime-version:from:to:cc:subject:date:message-id:reply-to;
bh=9iLUfXBaPQwdtnIOce+wn2F1HbbhCC06E82NAKz1emc=;
b=Qz71U19eQcPQoMSUa1CCCRTMEUhlQO22qbm+gr09OvDg6HtwtXpY1XewOd9Vrhb/4I
mJychj9b23L2dI62KnAost9yHtLMh3YFIMJYJNWhe5smweLXmHhCNSFoGmkUinFXgxMK
fcWrbKmSm3qW0g1OnywY7X/en7aYQHb4slJkIJwpXUvFTnzjwEz43KA3P+hGoXoM0DYY
nh+dtXAJreUt7FrMXexxa+YoU6icBe4Jm4ijQ+gFOdWIREboATfYWx8Zbjon7vZ6jXfM
a9jLYH3ZpDrQY/yedVyfYaOtjS37bzmLUSHLYD+oJqOA4+dSD2Rg229phkn+F2uShHYG
gTaQ==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20230601; t=1732426141; x=1733030941;
h=cc:to:subject:message-id:date:from:in-reply-to:references
:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id
:reply-to;
bh=9iLUfXBaPQwdtnIOce+wn2F1HbbhCC06E82NAKz1emc=;
b=b46tFA2mADw+aPOnm36LmyGVdXMExphjZP5NMYxEC8ey4ujmuQe335R+cUlO68C8Gk
nC1uXsowd9H503zsoj4bi/VpGwB6hk4hYkxmoACjq1Op6WVCQis/2x5TyaIweG2nuYDF
gkxxgrWDurv58vZxq2xZgWUqRy6oqQeGWAcz9RxsxWhXRvNLivjqros9OZoWu7wyfFJa
A/5JtsjcG/aQbt8Zgtj27lqcIxl7ZQfX/g5aMK7GvAT8e+wdvsZbgG1EY8M0SA9bx7f+
KxCvNl7kdFISinLaKlygS5eiDQZztOiBejWATzAgfF4GAMZG5AED2JrW0uu4RKyr7bBu
BGHw==
X-Gm-Message-State: AOJu0YwKLd7ZeceOzeLaqk/Pb4k1PkAxFt13wZSIozP5YIr8UDJrB7Xg
s7vjWJJFraBraDUFQqX6tIvmCaJfIbwsAT79ic7NkCAGZCU5hpjMly2hIQvZTBx28bQ9wtsYChA
l83Gy4m3VQIHcylLAwK82IJXJnA3dz6FnoihaJ9MCIIf9mhv+zhmVaw==
X-Gm-Gg: ASbGncu9e/eQbZEwBjP3uGlThdH0zhFU+bq/RGVJdhRAeCN7B5zzrU8zZGwsycLMsey
4595MCa7NKPtDmMcMEr55zyUq8u56iIs=
X-Google-Smtp-Source: AGHT+IHIjwjOmwtuEJEdSIej3PPNhtaycVvsg/SyWb+JqrTz5EihHIsUn1HDx759BLB6sTPfQ09FgudL3RelS38GGtg=
X-Received: by 2002:a17:902:ecd2:b0:20c:895d:b41c with SMTP id
d9443c01a7336-2129f27eeccmr122635075ad.41.1732426141463; Sat, 23 Nov 2024
21:29:01 -0800 (PST)
List-Id: Discussion related to FreeBSD architecture <freebsd-arch.freebsd.org>
List-Archive: https://lists.freebsd.org/archives/freebsd-arch
List-Help: <mailto:freebsd-arch+help@freebsd.org>
List-Post: <mailto:freebsd-arch@freebsd.org>
List-Subscribe: <mailto:freebsd-arch+subscribe@freebsd.org>
List-Unsubscribe: <mailto:freebsd-arch+unsubscribe@freebsd.org>
Sender: owner-freebsd-arch@FreeBSD.org
MIME-Version: 1.0
References: <202411232114.4ANLEGaj077161@critter.freebsd.dk>
In-Reply-To: <202411232114.4ANLEGaj077161@critter.freebsd.dk>
From: Warner Losh <imp@bsdimp.com>
Date: Sat, 23 Nov 2024 22:28:50 -0700
Message-ID: <CANCZdfrkdbsaonxPrp_QwoEtpx1ieeQSo6D1aynZErysx1t6wA@mail.gmail.com>
Subject: Re: Locking inconsistencies in I2C land
To: Poul-Henning Kamp <phk@phk.freebsd.dk>
Cc: arch@freebsd.org
Content-Type: multipart/alternative; boundary="000000000000c316750627a1e474"
X-Rspamd-Pre-Result: action=no action;
module=replies;
Message is reply to one we originated
X-Spamd-Result: default: False [-4.00 / 15.00];
REPLY(-4.00)[];
ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]
X-Rspamd-Queue-Id: 4Xwy7M0gJ4z4XHM
X-Spamd-Bar: ----
--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--