2024-01-07 00:18:03

by Christian A. Ehrhardt

[permalink] [raw]
Subject: [PATCH 0/4] Make UCSI on Dell Latitude work

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 all Dell systems. However, I'm open to
suggestions on how to proceed with this.

While debugging another issue was found that is present for all systems
but only triggered with the quirk active: The ACK_CC_CI command
to ack the connector status change is sent without any lock which
allows for a race where this command is sent while another command
is currently in progress.

The series consists of 4 changes:
- Add the missing lock around ucsi_acknowledge_connector_change.
This change is a generic bugfix.
- Add infrastructure for quirks and a quirk override module parameter
to the typec_ucsi module. There's at least one other change in the
works that wants something similar here:
https://lore.kernel.org/all/[email protected]/
Additionally, doing the dell quirk is a bit easier in the gereric
UCSI code. The module parameter will allow users to fix things if
the DMI matching goes wrong. Additionally, we can easily ask users
to test different quirks without the need to recompile.
- Actually add the required quirk to ucsi.c.
- Finally, refactor DMI matching in ucsi_acpi.c and enable the new
quirk for all DELL systems. The latter is kind of bold and I'm open
to suggestings on how to proceed here.
I'm CC'ing [email protected] because this seems to be a
list where Dell people are that might be able to provide more
insight on this.

Christian A. Ehrhardt (4):
usb: ucsi: Add quirk infrastructure
usb: ucsi: Add missing ppm_lock
usb: ucsi: Quirk to ack a connector change ack cmd
usb: ucsi: Apply UCSI_ACK_CONNECTOR_CHANGE_ACK_CMD to DELL systems

.../admin-guide/kernel-parameters.txt | 5 +++
drivers/usb/typec/ucsi/ucsi.c | 21 ++++++++-
drivers/usb/typec/ucsi/ucsi.h | 7 ++-
drivers/usb/typec/ucsi/ucsi_acpi.c | 45 ++++++++++++++++---
drivers/usb/typec/ucsi/ucsi_ccg.c | 2 +-
drivers/usb/typec/ucsi/ucsi_glink.c | 2 +-
drivers/usb/typec/ucsi/ucsi_stm32g0.c | 2 +-
7 files changed, 73 insertions(+), 11 deletions(-)

--
2.40.1



2024-01-07 00:18:17

by Christian A. Ehrhardt

[permalink] [raw]
Subject: [PATCH 1/4] usb: ucsi: Add missing ppm_lock

Calling ->sync_write must be done while holding the PPM lock as the
mailbox logic does not support concurrent commands.

Add a missing lock around the only call to
ucsi_acknowledge_connector_change. Additionally, warn in ucsi_acpi.c
if a command is started while the COMMAND_PENDING bit is already set.

Signed-off-by: Christian A. Ehrhardt <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.c | 2 ++
drivers/usb/typec/ucsi/ucsi_acpi.c | 2 ++
2 files changed, 4 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);

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 6bbf490ac401..8062d0a4b523 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -75,6 +75,8 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
int ret;

+ WARN_ON(test_bit(COMMAND_PENDING, &ua->flags));
+
set_bit(COMMAND_PENDING, &ua->flags);

ret = ucsi_acpi_async_write(ucsi, offset, val, val_len);
--
2.40.1


2024-01-07 00:18:27

by Christian A. Ehrhardt

[permalink] [raw]
Subject: [PATCH 2/4] usb: ucsi: Add quirk infrastructure

Allow bus drivers to specify quirks for the UCSI core on
attach. Allow the user to override the quirks on the command
line.

Signed-off-by: Christian A. Ehrhardt <[email protected]>
---
Documentation/admin-guide/kernel-parameters.txt | 5 +++++
drivers/usb/typec/ucsi/ucsi.c | 12 +++++++++++-
drivers/usb/typec/ucsi/ucsi.h | 6 +++++-
drivers/usb/typec/ucsi/ucsi_acpi.c | 2 +-
drivers/usb/typec/ucsi/ucsi_ccg.c | 2 +-
drivers/usb/typec/ucsi/ucsi_glink.c | 2 +-
drivers/usb/typec/ucsi/ucsi_stm32g0.c | 2 +-
7 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 0a6a4b7f7a3b..fd8152dd4450 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6783,6 +6783,11 @@
<port#>,<js1>,<js2>,<js3>,<js4>,<js5>,<js6>,<js7>
See also Documentation/input/devices/joystick-parport.rst

+ typec_ucsi.quirks= [USB]
+ A hex value specifying the quirks to enable for
+ the USB Type-C connector system software interface
+ driver. This overrides auto detected quirks.
+
udbg-immortal [PPC] When debugging early kernel crashes that
happen after console_init() and before a proper
console driver takes over, this boot options might
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 8f9dff993b3d..00b23292f46f 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -36,6 +36,11 @@
*/
#define UCSI_SWAP_TIMEOUT_MS 5000

+/* Override for auto detected quirks. */
+static unsigned int quirk_override = ~0U;
+module_param_named(quirks, quirk_override, uint, S_IRUGO);
+MODULE_PARM_DESC(quirks, "Override quirks for UCSI");
+
static int ucsi_acknowledge_command(struct ucsi *ucsi)
{
u64 ctrl;
@@ -1507,7 +1512,8 @@ EXPORT_SYMBOL_GPL(ucsi_set_drvdata);
* @dev: Device interface to the PPM (Platform Policy Manager)
* @ops: I/O routines
*/
-struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
+struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops,
+ unsigned int quirks)
{
struct ucsi *ucsi;

@@ -1523,6 +1529,10 @@ struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops)
mutex_init(&ucsi->ppm_lock);
ucsi->dev = dev;
ucsi->ops = ops;
+ if (quirk_override != ~0U)
+ ucsi->quirks = quirk_override;
+ else
+ ucsi->quirks = quirks;

return ucsi;
}
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 474315a72c77..5e6d2225c9c8 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -57,7 +57,8 @@ struct ucsi_operations {
struct ucsi_altmode *updated);
};

-struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops);
+struct ucsi *ucsi_create(struct device *dev, const struct ucsi_operations *ops,
+ unsigned int quirks);
void ucsi_destroy(struct ucsi *ucsi);
int ucsi_register(struct ucsi *ucsi);
void ucsi_unregister(struct ucsi *ucsi);
@@ -317,6 +318,9 @@ struct ucsi {
#define EVENT_PENDING 0
#define COMMAND_PENDING 1
#define ACK_PENDING 2
+
+ /* Enabled quirks. */
+ unsigned int quirks;
};

#define UCSI_MAX_SVID 5
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 8062d0a4b523..78a0d13584ad 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -185,7 +185,7 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
if (dmi_check_system(zenbook_dmi_id))
ops = &ucsi_zenbook_ops;

- ua->ucsi = ucsi_create(&pdev->dev, ops);
+ ua->ucsi = ucsi_create(&pdev->dev, ops, 0);
if (IS_ERR(ua->ucsi))
return PTR_ERR(ua->ucsi);

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 449c125f6f87..d491b6547fc3 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -1387,7 +1387,7 @@ static int ucsi_ccg_probe(struct i2c_client *client)
if (uc->info.mode & CCG_DEVINFO_PDPORTS_MASK)
uc->port_num++;

- uc->ucsi = ucsi_create(dev, &ucsi_ccg_ops);
+ uc->ucsi = ucsi_create(dev, &ucsi_ccg_ops, 0);
if (IS_ERR(uc->ucsi))
return PTR_ERR(uc->ucsi);

diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
index db6e248f8208..49587960b6a3 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -318,7 +318,7 @@ static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
init_completion(&ucsi->sync_ack);
mutex_init(&ucsi->lock);

- ucsi->ucsi = ucsi_create(dev, &pmic_glink_ucsi_ops);
+ ucsi->ucsi = ucsi_create(dev, &pmic_glink_ucsi_ops, 0);
if (IS_ERR(ucsi->ucsi))
return PTR_ERR(ucsi->ucsi);

diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
index 93d7806681cf..269caa3b4e84 100644
--- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -641,7 +641,7 @@ static int ucsi_stm32g0_probe(struct i2c_client *client)
init_completion(&g0->complete);
i2c_set_clientdata(client, g0);

- g0->ucsi = ucsi_create(dev, &ucsi_stm32g0_ops);
+ g0->ucsi = ucsi_create(dev, &ucsi_stm32g0_ops, 0);
if (IS_ERR(g0->ucsi))
return PTR_ERR(g0->ucsi);

--
2.40.1


2024-01-07 00:18:51

by Christian A. Ehrhardt

[permalink] [raw]
Subject: [PATCH 3/4] usb: ucsi: Quirk to ack a connector change ack cmd

The PPM on some Dell laptops seems to expect that the ACK_CC_CI
command to clear the connector change notification is in turn
followed by another ACK_CC_CI to acknowledge the ACK_CC_CI command
itself. This is in violation of the UCSI spec that states:

"The only notification that is not acknowledged by the OPM is
the command completion notification for the ACK_CC_CI or the
PPM_RESET command."

Add a quirk to send this ack anyway.

Signed-off-by: Christian A. Ehrhardt <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.c | 7 ++++++-
drivers/usb/typec/ucsi/ucsi.h | 1 +
2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 00b23292f46f..8718836b6aba 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -54,11 +54,16 @@ static int ucsi_acknowledge_command(struct ucsi *ucsi)
static int ucsi_acknowledge_connector_change(struct ucsi *ucsi)
{
u64 ctrl;
+ int ret;

ctrl = UCSI_ACK_CC_CI;
ctrl |= UCSI_ACK_CONNECTOR_CHANGE;

- return ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
+ ret = ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
+ if (ret == 0 && ucsi->quirks & UCSI_ACK_CONNECTOR_CHANGE_ACK_CMD)
+ ret = ucsi_acknowledge_command(ucsi);
+
+ return ret;
}

static int ucsi_exec_command(struct ucsi *ucsi, u64 command);
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 5e6d2225c9c8..9ea2143776cf 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -321,6 +321,7 @@ struct ucsi {

/* Enabled quirks. */
unsigned int quirks;
+#define UCSI_ACK_CONNECTOR_CHANGE_ACK_CMD BIT(0)
};

#define UCSI_MAX_SVID 5
--
2.40.1


2024-01-07 00:25:56

by Christian A. Ehrhardt

[permalink] [raw]
Subject: [PATCH 4/4] usb: ucsi: Apply UCSI_ACK_CONNECTOR_CHANGE_ACK_CMD to Dell systems

Apply the UCSI_ACK_CONNECTOR_CHANGE_ACK_CMD to all Dell systems.

There are various reports that ucsi does not work on Dell systems
with "GET_CONNECTOR_STATUS failed". At least some of these are
most likely due to the need for this quirk.

If the logic is wrong users can still use the new quirk override
for the typec_ucsi module to disable the quirk.

Signed-off-by: Christian A. Ehrhardt <[email protected]>
---
drivers/usb/typec/ucsi/ucsi_acpi.c | 36 +++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 78a0d13584ad..690d5e55bdc4 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -27,6 +27,11 @@ struct ucsi_acpi {
u64 cmd;
};

+struct ucsi_acpi_attach_data {
+ const struct ucsi_operations *ops;
+ unsigned int quirks;
+};
+
static int ucsi_acpi_dsm(struct ucsi_acpi *ua, int func)
{
union acpi_object *obj;
@@ -121,12 +126,30 @@ static const struct ucsi_operations ucsi_zenbook_ops = {
.async_write = ucsi_acpi_async_write
};

-static const struct dmi_system_id zenbook_dmi_id[] = {
+static const struct ucsi_acpi_attach_data ucsi_acpi_default_attach_data = {
+ .ops = &ucsi_acpi_ops,
+ .quirks = 0
+};
+
+static const struct dmi_system_id ucsi_acpi_quirks[] = {
{
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
DMI_MATCH(DMI_PRODUCT_NAME, "ZenBook UX325UA_UM325UA"),
},
+ .driver_data = &(struct ucsi_acpi_attach_data) {
+ .ops = &ucsi_zenbook_ops,
+ .quirks = 0
+ },
+ },
+ {
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+ },
+ .driver_data = &(struct ucsi_acpi_attach_data) {
+ .ops = &ucsi_acpi_ops,
+ .quirks = UCSI_ACK_CONNECTOR_CHANGE_ACK_CMD
+ },
},
{ }
};
@@ -152,7 +175,8 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
static int ucsi_acpi_probe(struct platform_device *pdev)
{
struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
- const struct ucsi_operations *ops = &ucsi_acpi_ops;
+ const struct dmi_system_id *id;
+ const struct ucsi_acpi_attach_data *attach;
struct ucsi_acpi *ua;
struct resource *res;
acpi_status status;
@@ -182,10 +206,12 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
init_completion(&ua->complete);
ua->dev = &pdev->dev;

- if (dmi_check_system(zenbook_dmi_id))
- ops = &ucsi_zenbook_ops;
+ attach = &ucsi_acpi_default_attach_data;
+ id = dmi_first_match(ucsi_acpi_quirks);
+ if (id)
+ attach = id->driver_data;

- ua->ucsi = ucsi_create(&pdev->dev, ops, 0);
+ ua->ucsi = ucsi_create(&pdev->dev, attach->ops, attach->quirks);
if (IS_ERR(ua->ucsi))
return PTR_ERR(ua->ucsi);

--
2.40.1


2024-01-15 07:53:32

by Heikki Krogerus

[permalink] [raw]
Subject: Re: [PATCH 2/4] usb: ucsi: Add quirk infrastructure

On Sun, Jan 07, 2024 at 01:16:59AM +0100, Christian A. Ehrhardt wrote:
> Allow bus drivers to specify quirks for the UCSI core on
> attach. Allow the user to override the quirks on the command
> line.
>
> Signed-off-by: Christian A. Ehrhardt <[email protected]>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 5 +++++
> drivers/usb/typec/ucsi/ucsi.c | 12 +++++++++++-
> drivers/usb/typec/ucsi/ucsi.h | 6 +++++-
> drivers/usb/typec/ucsi/ucsi_acpi.c | 2 +-
> drivers/usb/typec/ucsi/ucsi_ccg.c | 2 +-
> drivers/usb/typec/ucsi/ucsi_glink.c | 2 +-
> drivers/usb/typec/ucsi/ucsi_stm32g0.c | 2 +-
> 7 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 0a6a4b7f7a3b..fd8152dd4450 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6783,6 +6783,11 @@
> <port#>,<js1>,<js2>,<js3>,<js4>,<js5>,<js6>,<js7>
> See also Documentation/input/devices/joystick-parport.rst
>
> + typec_ucsi.quirks= [USB]
> + A hex value specifying the quirks to enable for
> + the USB Type-C connector system software interface
> + driver. This overrides auto detected quirks.

New module parameters are not going to be accepted.

Please just fix the issue with Dell's first like I proposed, and then
you can start thinking about the infra for the quirks.

thanks,


--
heikki

2024-01-15 09:05:55

by Heikki Krogerus

[permalink] [raw]
Subject: Re: [PATCH 4/4] usb: ucsi: Apply UCSI_ACK_CONNECTOR_CHANGE_ACK_CMD to Dell systems

Hi Christian,

On Sun, Jan 07, 2024 at 01:17:01AM +0100, Christian A. Ehrhardt wrote:
> Apply the UCSI_ACK_CONNECTOR_CHANGE_ACK_CMD to all Dell systems.
>
> There are various reports that ucsi does not work on Dell systems
> with "GET_CONNECTOR_STATUS failed". At least some of these are
> most likely due to the need for this quirk.
>
> If the logic is wrong users can still use the new quirk override
> for the typec_ucsi module to disable the quirk.
>
> Signed-off-by: Christian A. Ehrhardt <[email protected]>
> ---
> drivers/usb/typec/ucsi/ucsi_acpi.c | 36 +++++++++++++++++++++++++-----
> 1 file changed, 31 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index 78a0d13584ad..690d5e55bdc4 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -27,6 +27,11 @@ struct ucsi_acpi {
> u64 cmd;
> };
>
> +struct ucsi_acpi_attach_data {
> + const struct ucsi_operations *ops;
> + unsigned int quirks;
> +};
> +
> static int ucsi_acpi_dsm(struct ucsi_acpi *ua, int func)
> {
> union acpi_object *obj;
> @@ -121,12 +126,30 @@ static const struct ucsi_operations ucsi_zenbook_ops = {
> .async_write = ucsi_acpi_async_write
> };
>
> -static const struct dmi_system_id zenbook_dmi_id[] = {
> +static const struct ucsi_acpi_attach_data ucsi_acpi_default_attach_data = {
> + .ops = &ucsi_acpi_ops,
> + .quirks = 0
> +};
> +
> +static const struct dmi_system_id ucsi_acpi_quirks[] = {
> {
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> DMI_MATCH(DMI_PRODUCT_NAME, "ZenBook UX325UA_UM325UA"),
> },
> + .driver_data = &(struct ucsi_acpi_attach_data) {
> + .ops = &ucsi_zenbook_ops,
> + .quirks = 0
> + },
> + },
> + {
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + },
> + .driver_data = &(struct ucsi_acpi_attach_data) {
> + .ops = &ucsi_acpi_ops,
> + .quirks = UCSI_ACK_CONNECTOR_CHANGE_ACK_CMD

Please don't add any more quirk flags like that for single user (you
may never have more than the one user for it). Let's just first handle
this with only Dell's like I proposed.

If there are other platforms that need the same quirk, then we can
start looking at how to share the quirk.

thanks,

--
heikki

2024-01-15 12:03:46

by Christian A. Ehrhardt

[permalink] [raw]
Subject: Re: [PATCH 4/4] usb: ucsi: Apply UCSI_ACK_CONNECTOR_CHANGE_ACK_CMD to Dell systems


Hi Heikki,

On Mon, Jan 15, 2024 at 11:05:36AM +0200, Heikki Krogerus wrote:
> Hi Christian,
>
> On Sun, Jan 07, 2024 at 01:17:01AM +0100, Christian A. Ehrhardt wrote:
> > Apply the UCSI_ACK_CONNECTOR_CHANGE_ACK_CMD to all Dell systems.
> >
> > There are various reports that ucsi does not work on Dell systems
> > with "GET_CONNECTOR_STATUS failed". At least some of these are
> > most likely due to the need for this quirk.
> >
> > If the logic is wrong users can still use the new quirk override
> > for the typec_ucsi module to disable the quirk.
> >
> > Signed-off-by: Christian A. Ehrhardt <[email protected]>
> > ---
> > drivers/usb/typec/ucsi/ucsi_acpi.c | 36 +++++++++++++++++++++++++-----
> > 1 file changed, 31 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> > index 78a0d13584ad..690d5e55bdc4 100644
> > --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> > @@ -27,6 +27,11 @@ struct ucsi_acpi {
> > u64 cmd;
> > };
> >
> > +struct ucsi_acpi_attach_data {
> > + const struct ucsi_operations *ops;
> > + unsigned int quirks;
> > +};
> > +
> > static int ucsi_acpi_dsm(struct ucsi_acpi *ua, int func)
> > {
> > union acpi_object *obj;
> > @@ -121,12 +126,30 @@ static const struct ucsi_operations ucsi_zenbook_ops = {
> > .async_write = ucsi_acpi_async_write
> > };
> >
> > -static const struct dmi_system_id zenbook_dmi_id[] = {
> > +static const struct ucsi_acpi_attach_data ucsi_acpi_default_attach_data = {
> > + .ops = &ucsi_acpi_ops,
> > + .quirks = 0
> > +};
> > +
> > +static const struct dmi_system_id ucsi_acpi_quirks[] = {
> > {
> > .matches = {
> > DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
> > DMI_MATCH(DMI_PRODUCT_NAME, "ZenBook UX325UA_UM325UA"),
> > },
> > + .driver_data = &(struct ucsi_acpi_attach_data) {
> > + .ops = &ucsi_zenbook_ops,
> > + .quirks = 0
> > + },
> > + },
> > + {
> > + .matches = {
> > + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > + },
> > + .driver_data = &(struct ucsi_acpi_attach_data) {
> > + .ops = &ucsi_acpi_ops,
> > + .quirks = UCSI_ACK_CONNECTOR_CHANGE_ACK_CMD
>
> Please don't add any more quirk flags like that for single user (you
> may never have more than the one user for it). Let's just first handle
> this with only Dell's like I proposed.
>
> If there are other platforms that need the same quirk, then we can
> start looking at how to share the quirk.

Ok, thanks for the feedback. Will do.

However, please note that it is not clear if it is all or only some
dells that need this. This is the prime reason why I was looking for
a way to enable/disable the quirk.

regards Christian