Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <eebb4180ca4aa6d9f9dc488d7d91199de5731b5c.camel@redhat.com>
Subject: Re: [PATCH] usb: typec: ucsi: Clear pending after acking connector
 change
From:   Benjamin Berg <bberg@redhat.com>
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Mon, 17 May 2021 14:36:50 +0200
In-Reply-To: <YKI/XT8qpZDjDuqs@kuha.fi.intel.com>
References: <20210516040953.622409-1-bjorn.andersson@linaro.org>
         <YKI/XT8qpZDjDuqs@kuha.fi.intel.com>
Content-Type: multipart/signed; micalg="pgp-sha256";
        protocol="application/pgp-signature"; boundary="=-VzXmzjqYSVJOfJgFmYWm"
User-Agent: Evolution 3.40.0 (3.40.0-1.fc34) 
MIME-Version: 1.0
Precedence: bulk


--=-VzXmzjqYSVJOfJgFmYWm
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, 2021-05-17 at 13:03 +0300, Heikki Krogerus wrote:
> On Sat, May 15, 2021 at 09:09:53PM -0700, Bjorn Andersson wrote:
> > It's possible that the interrupt handler for the UCSI driver
> > signals a
> > connector changes after the handler clears the PENDING bit, but
> > before
> > it has sent the acknowledge request. The result is that the handler
> > is
> > invoked yet again, to ack the same connector change.
> >=20
> > At least some versions of the Qualcomm UCSI firmware will not
> > handle the
> > second - "spurious" - acknowledgment gracefully. So make sure to
> > not
> > clear the pending flag untuntil the changeil the change is
> > acknowledged.
> >=20
> > Any connector changes coming in after the acknowledgment, that
> > would
> > have the pending flag incorrectly cleared, would afaict be covered
> > by
> > the subsequent connector status check.
> >=20
> > Fixes: 217504a05532 ("usb: typec: ucsi: Work around PPM losing
> > change information")
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>=20
> I'm OK with this if Bejamin does not see any problems with it. I'll
> wait for his comments before giving my reviewed-by tag.

Interesting problem. I wonder if we can detect this situation and
restart the worker instead of sending the ACK.

As for the patch, I do believe it is safe. I agree with the assessment
in the commit message, that the subsequent connector check will detect
any race condition and recovers from it.

Acked-By: Benjamin Berg <bberg@redhat.com>

Benjamin

> That workaround (commit 217504a05532) is unfortunately too fragile.
> I'm going to now separate the processing of the connector state from
> the event handler (interrupt handler). That way we should be fairly
> sure we don't loose any of the connector states even if an event is
> generated while we are still in the middle of processing the previous
> one(s), and at the same time be sure that we also don't confuse the
> firmware.
> So the event handler shall after that only read the connector status,
> schedule the unique job where it's processed and ACK the event.
> Nothing else.
>=20
> > ---
> > =C2=A0drivers/usb/typec/ucsi/ucsi.c | 2 +-
> > =C2=A01 file changed, 1 insertion(+), 1 deletion(-)
> >=20
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c
> > b/drivers/usb/typec/ucsi/ucsi.c
> > index 282c3c825c13..f451ce0132a9 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -694,8 +694,8 @@ static void ucsi_handle_connector_change(struct
> > work_struct *work)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ucsi_send_command(con->=
ucsi, command, NULL, 0);
> > =C2=A0
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0/* 3. ACK connector cha=
nge */
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0clear_bit(EVENT_PENDING, &uc=
si->flags);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0ret =3D ucsi_acknowledg=
e_connector_change(ucsi);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0clear_bit(EVENT_PENDING, &uc=
si->flags);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (ret) {
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0dev_err(ucsi->dev, "%s: ACK failed (%d)", __func=
__,
> > ret);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0goto out_unlock;
> > --=20
> > 2.29.2
>=20
> thanks,
>=20


--=-VzXmzjqYSVJOfJgFmYWm
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: This is a digitally signed message part
Content-Transfer-Encoding: 7bit

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEED2NO4vMS33W8E4AFq6ZWhpmFY3AFAmCiY2IACgkQq6ZWhpmF
Y3BAPQ//ZABRXpjcFB9x/Z3GiFyO+PHGDqyRi1FfVJh+7DZABPkC2zsWvXCqF0CR
EmS3YllK7h1IIPh3CChElO6yO2GdbJl+cf+rW6nlgWT9GK1yHIbddbigAlyy8gtd
gBauXkN9nYHvL7VsOnYoFdM/morETSHCsvN71QVbRYz8Lvtr50KkCHxRiFFRiTb/
V7KtEfPTfh0UNssQJVf47SzBpVPfxu+k/IhLe+GCAFwRg4s9HQEQ+v3q3YGxHogl
CV6uAxEmMuAazD+YIVSSr4TwNH/nX5w6q+/LiehiYxH2PbQfsRWm73WsQR2ZQA/N
W634QmgCx18ZKg/qvbXynOyuCku/fd7W3ZTwDMmN2P1ZgYx4FJ5jl6IZwo4rpmFc
+2y2wPmZ6SHvbqHXc+lNiYuzAleHawHJ00zEG64Gg7zrNyvLEvuL6Y3dLraRwa+I
342e4ZIWi99Xbga2R2mqs05UqQeY+d92tX4mFq3yD/N+WwjviFBsjLSgaPoix/8q
JpdTi18Pzt5t55ls5vi9G0jwDNw8TllJ9HFkrEiIsMEqwRcMvvoAKuS1vG7EN8Kr
4Ykzid+/l4zo/QKxBh1HfTdL3/ogY4j1GOcbCYI3oEoLRt9SgnIzHMDeATpmMk/U
1HhbBzLx72t1QmkBxxobt0f1+Gp6yNrLAzqzwfH1SwqZr19w5rI=
=B0Yk
-----END PGP SIGNATURE-----

--=-VzXmzjqYSVJOfJgFmYWm--