This small series contains two general bugfixes to ucsi_acpi
and a quirk to make the UCSI controller on various Dell laptops
work. The changes can be applied idependantly but all three
are required to fix the Dell issues.
For details on the general bugfixes please refer to the individual
commit messages.
The UCSI interface on a Dell Latitude 5431 stops working after
the first async event with:
GET_CONNECTOR_STATUS failed (-110)
The core problem is that when sending the ACK_CC_CI command to
clear the connector status changed condition the PPM expects us
to send anothr ack for the command completion condition. However,
the UCSI spec states that no ack for the command completion is
required when the command is ACK_CC_CI (or PPM_RESET).
There are various reports that suggest that several Dell laptops
are affected by this problem. E.g. the kernel bugzilla has this
report which is most likely an instance of this bug:
https://bugzilla.kernel.org/show_bug.cgi?id=216426
This led me to the somewhat bold conclusion that the quirk should
probably be applied to on Dell systems.
To mitigate potential problems from this dell quirk includes a
probe mechanism that detect the need for the quirk once and we
only deviate from the UCSI spec if the quirk is actually required.
Changes in v2 from v1:
- Add a second general bugfix.
- Remove module parmater and generic quirk infrastructure.
- Implement quirk directly in ucsi_acpi.c
- Add probe logic to reliably detect the need for the quirk
Christian A. Ehrhardt (3):
usb: ucsi: Add missing ppm_lock
usb: ucsi_acpi: Fix command completion handling
usb: ucsi_acpi: Quirk to ack a connector change ack cmd
drivers/usb/typec/ucsi/ucsi.c | 2 +
drivers/usb/typec/ucsi/ucsi_acpi.c | 86 +++++++++++++++++++++++++++---
2 files changed, 81 insertions(+), 7 deletions(-)
--
2.40.1
Calling ->sync_write must be done while holding the PPM lock as the
mailbox logic does not support concurrent commands.
Thus protect the only call to ucsi_acknowledge_connector_change
with the PPM lock as it calls ->sync_write. All other calls to
->sync_write already happen under the PPM lock.
Signed-off-by: Christian A. Ehrhardt <[email protected]>
---
NOTE: This is not a theoretical issue. I've seen problems resulting
from the missing lock on real hardware.
drivers/usb/typec/ucsi/ucsi.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 61b64558f96c..8f9dff993b3d 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -935,7 +935,9 @@ static void ucsi_handle_connector_change(struct work_struct *work)
clear_bit(EVENT_PENDING, &con->ucsi->flags);
+ mutex_lock(&ucsi->ppm_lock);
ret = ucsi_acknowledge_connector_change(ucsi);
+ mutex_unlock(&ucsi->ppm_lock);
if (ret)
dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
--
2.40.1
On Tue, Jan 16, 2024 at 11:40:39PM +0100, Christian A. Ehrhardt wrote:
> Calling ->sync_write must be done while holding the PPM lock as the
> mailbox logic does not support concurrent commands.
>
> Thus protect the only call to ucsi_acknowledge_connector_change
> with the PPM lock as it calls ->sync_write. All other calls to
> ->sync_write already happen under the PPM lock.
>
> Signed-off-by: Christian A. Ehrhardt <[email protected]>
> ---
> NOTE: This is not a theoretical issue. I've seen problems resulting
> from the missing lock on real hardware.
What commit id does this fix?
Should it be cc: stable?
thanks,
greg k-h
On Wed, Jan 17, 2024 at 06:44:40AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 16, 2024 at 11:40:39PM +0100, Christian A. Ehrhardt wrote:
> > Calling ->sync_write must be done while holding the PPM lock as the
> > mailbox logic does not support concurrent commands.
> >
> > Thus protect the only call to ucsi_acknowledge_connector_change
> > with the PPM lock as it calls ->sync_write. All other calls to
> > ->sync_write already happen under the PPM lock.
> >
> > Signed-off-by: Christian A. Ehrhardt <[email protected]>
> > ---
> > NOTE: This is not a theoretical issue. I've seen problems resulting
> > from the missing lock on real hardware.
>
> What commit id does this fix?
It's hard to tell (due to rewrites, logic and API changes). After
digging a bit more I think it is at least a theoretical issues since
the introduction of partner tasks.
I'll wait a bit for additional feedback and fix this and other issues
noticed by your patch bot (sorry for those) in the next iteration.
> Should it be cc: stable?
Not sure. The race is triggered much more ofter after the quirk added
in patch 3/3, so this may not be a practical issue before that.
I'll add the tag in the next iteration, though.
thanks Christian