Re: [RFC PATCH] usb: typec: ucsi: ack connector change after all tasks finish

From: Christian Ehrhardt <lk@c--e.de>
To: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
Cc: heikki.krogerus@linux.intel.com, gregkh@linuxfoundation.org,
	neil.armstrong@linaro.org, quic_prashk@quicinc.com,
	dmitry.baryshkov@linaro.org, fabrice.gasnier@foss.st.com,
	saranya.gopal@intel.com, linux-usb@vger.kernel.org
Subject: Re: [RFC PATCH] usb: typec: ucsi: ack connector change after all tasks finish
Date: Thu, 28 Mar 2024 23:40:09 +0100	[thread overview]
Message-ID: <ZgXxyWsdA7YML3mR@cae.in-ulm.de> (raw)
In-Reply-To: <ynrqweb7hhfkrlvjr6suajq3jpgi4sqexz44qt4chekce7phiw@cyofo73xztdg>

Hi,

On Thu, Mar 28, 2024 at 03:42:40PM +0000, Diogo Ivo wrote:
> On Wed, Mar 27, 2024 at 05:06:38PM +0100, Christian Ehrhardt wrote:
> > On Wed, Mar 27, 2024 at 12:39:04PM +0000, Diogo Ivo wrote:
> > > This fixes a problem with some LG Gram laptops where the PPM sometimes
> > > notifies the OPM with the Connector Change Indicator field set in the
> > > CCI after an ACK_CC_CI command is sent,causing the UCSI stack to check
> > > the connector status indefinitely since the EVENT_PENDING bit is already
> > > cleared. This causes an interrupt storm and an artificial high load on
> > > these platforms.
> > 
> > If the PPM does this for a connector change ACK_CC_CI command it is
> > IMHO violating the spec (unless there is a _new_ event).
> 
> Yes, the problem is exactly that the PPM in these laptops is really not
> conformant with the spec and moving the command change ACK_CC_CI to the
> end circumvented the problems in the PPM. If [1] is the way to go then
> we need some sort of quirk for these devices and I'll have to dig
> deeper.

Just to make this clear: This is not my call to make.

> > When I saw this type of loops the connector change indicator was set
> > in response to an ACK_CC_CI command for a command (sent by a different
> > thread for a different connector) between clearing the EVENT_PENDING
> > bit and acquiring the PPM lock.
> > 
> > Can you test if the changes that already are in usb-linus are
> > sufficient to fix your issues?
> 
> I am seeing these problems when addressing one connector only, so other
> threads for other connectors do not play a role here. I have tested the
> latest usb-linus with and without your early ack patch set [1] on top
> and the issue is still not fixed.

There are legitimate reaons why the connector change indicator
is set in response to a command:
- If the condition was reported previously it is sticky until
  cleared.
- Something else changed on the connector.

For a more complicated device that I have here, there are five
different connector change events after plugging it in.

I'd like to understand why you run into a loop here.
Printing the completed command (if any) and the CCI in
ucsi_acpi_notify() and the details of the connector status in
ucsi_handle_connector_change() could shed some light on this.

> [1]: https://lore.kernel.org/linux-usb/20240327224554.1772525-1-lk@c--e.de/

Best regards   Christian