Hi Christian, sorry for the late reply! On Thu, Mar 28, 2024 at 11:40:09PM +0100, Christian Ehrhardt wrote: > > 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). > > > 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. > > 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. You are correct, my initial conclusions were not the cause of the issue. After digging a bit more with your ACK early patch set [1] applied the initial connector changes in CCI are meaningful: /* Connect charge here */ [88248.531077] ucsi_acpi_notify: CCI: 20000002 [88248.531084] scheduling connector change [88248.531094] COMMAND: 10012 [88248.690705] ucsi_acpi_notify: CCI: 80000902 [88248.705038] ucsi_acpi_notify: CCI: 80000902 [88248.769716] MESSAGE_IN: 0 294024 [88248.769721] COMMAND: 30004 [88248.905117] ucsi_acpi_notify: CCI: 20000002 [88248.905122] scheduling connector change [88248.905238] COMMAND: 10012 [88248.921032] ucsi_acpi_notify: CCI: 20000002 [88249.052045] ucsi_acpi_notify: CCI: 80000902 [88249.094680] ucsi_acpi_notify: CCI: 80000902 [88249.113033] MESSAGE_IN: 1 42851545402b0a44 [88249.113036] COMMAND: 30004 [88249.234930] ucsi_acpi_notify: CCI: 20000000 as the reply to GET_CONNECTOR_STATUS changes. Turns out the problematic command seems to be the GET_PDOS for the source PDOs of the partner. After testing with multiple chargers and hubs we always have this pattern: [88249.235144] COMMAND: 700810010 [88249.431015] ucsi_acpi_notify: CCI: 80001000 [88249.444510] ucsi_acpi_notify: CCI: 80001000 [88249.474162] MESSAGE_IN: 641450004b12c 2d12c0801912c [88249.474164] COMMAND: 20004 [88249.569839] ucsi_acpi_notify: CCI: 20000002 [88249.569843] scheduling connector change [88249.569878] COMMAND: 604810010 [88249.694744] ucsi_acpi_notify: CCI: 80000002 [88249.756603] ucsi_acpi_notify: CCI: 80000002 [88249.772341] MESSAGE_IN: 0 0 [88249.772343] COMMAND: 20004 [88249.796672] ucsi_acpi_notify: CCI: 80000002 [88249.932743] ucsi_acpi_notify: CCI: 20000000 ... [88250.229964] COMMAND: 10012 [88250.341815] ucsi_acpi_notify: CCI: 80000900 [88250.385756] ucsi_acpi_notify: CCI: 80000900 [88250.404292] MESSAGE_IN: 1 42851545402b0060 And the next time we check the partner source PDOS we get the same, [88251.126928] COMMAND: 10012 [88251.316607] ucsi_acpi_notify: CCI: 80000900 [88251.330743] ucsi_acpi_notify: CCI: 80000900 [88251.358123] MESSAGE_IN: 1 42851545402b0000 [88251.358125] COMMAND: 20004 [88251.474957] ucsi_acpi_notify: CCI: 20000000 [88251.475109] COMMAND: 700810010 [88251.636812] ucsi_acpi_notify: CCI: 80001000 [88251.695686] ucsi_acpi_notify: CCI: 80001000 [88251.709026] MESSAGE_IN: 641450004b12c 2d12c0801912c [88251.709028] COMMAND: 20004 [88251.827071] ucsi_acpi_notify: CCI: 20000002 [88251.827075] scheduling connector change [88251.827189] COMMAND: 604810010 [88251.974138] ucsi_acpi_notify: CCI: 80000002 [88252.034895] ucsi_acpi_notify: CCI: 80000002 [88252.054572] ucsi_acpi_notify: CCI: 80000002 [88252.067847] MESSAGE_IN: 0 0 [88252.067849] COMMAND: 20004 [88252.197648] ucsi_acpi_notify: CCI: 20000000 which then leads to the infinite loop. I have checked that we always get the same PDOs and nothing else is changing, leading me to believe that this change is not meaningful. The only thing that is changes are bits 5 and 6 of the GET_CONNECTOR_STATUS reply that follows the GET_PDOS command. Is this a pattern you have seen before, and if so, do you have any recommendations on how to properly address it? My first idea was to revive the ACPI_SUPRESS bit from the Dell quirk and set it for this command from a custom sync_write() callback for these laptops, at the expense of possibly losing information on actual connector changes that occur. On a related note, I noticed that the conditions for us to read the partner PDOs are not based on the "Supported Provider Capabilities Change" bit of the GET_CONNECTOR_STATUS reply as the spec instructs. Is there a reason for this? Lastly, I also noticed that in ucsi_pwr_opmode_change() we call both ucsi_get_src_pdos() and ucsi_register_partner_pdos(), and both read the partner source PDOs, which to my limited understanding of the driver seems like a duplication of both the reading and the storage of the PDOs. Again, my question here is if this is intended, or if we can condense this into a single call. Thank you in advance for your time and insights! Best regards, Diogo [1]: https://lore.kernel.org/linux-usb/20240327224554.1772525-1-lk@xxxxxxx/
