Hi,
This is the RFC series I promised [1].
I'm sorry it took this long to prepare these. I had to concentrate on
other task for a while.
Let me know if you still see any problems.
[1] https://lore.kernel.org/linux-usb/YKT3oEt%[email protected]/
thanks,
Heikki Krogerus (7):
usb: typec: ucsi: Always cancel the command if PPM reports BUSY
condition
usb: typec: ucsi: Don't stop alt mode registration on busy condition
usb: typec: ucsi: Add poll worker for alternate modes
usb: typec: ucsi: acpi: Reduce the command completion timeout
usb: typec: ucsi: Process every connector change as unique connector
state
usb: typec: ucsi: Filter out spurious events
usb: typec: ucsi: Read the PDOs in separate work
drivers/usb/typec/ucsi/ucsi.c | 317 ++++++++++++++++-------------
drivers/usb/typec/ucsi/ucsi.h | 3 +-
drivers/usb/typec/ucsi/ucsi_acpi.c | 2 +-
3 files changed, 183 insertions(+), 139 deletions(-)
--
2.30.2
If the PPM tells it's busy, we can now simply try again.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 366c8a468bc18..a8e0e31dcddf5 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -437,8 +437,11 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
command |= UCSI_GET_ALTMODE_OFFSET(i);
len = ucsi_send_command(con->ucsi, command, alt, sizeof(alt));
- if (len <= 0)
+ if (len <= 0) {
+ if (len == -EBUSY)
+ continue;
return len;
+ }
/*
* This code is requesting one alt mode at a time, but some PPMs
--
2.30.2
This makes it possible to execute next command immediately
after the busy condition.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 4e1973fbdf0dc..366c8a468bc18 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -128,8 +128,10 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
if (ret)
return ret;
- if (cci & UCSI_CCI_BUSY)
+ if (cci & UCSI_CCI_BUSY) {
+ ucsi->ops->async_write(ucsi, UCSI_CANCEL, NULL, 0);
return -EBUSY;
+ }
if (!(cci & UCSI_CCI_COMMAND_COMPLETE))
return -EIO;
--
2.30.2
This will change the Connector Change event handler function
so that it will only read the connector status and store it
as a unique state, queue a job where it's actually
processed, and then acknowledge the event immediately. That
routine will not do anything else from now on.
That will make sure we don't loose any of the reported
connector states even if they are reported while the driver
is still processing the previous ones.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.c | 177 ++++++++++++----------------------
drivers/usb/typec/ucsi/ucsi.h | 2 -
2 files changed, 61 insertions(+), 118 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index d41147b3b6e8a..a4123f77d1f16 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -49,11 +49,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));
+ mutex_lock(&ucsi->ppm_lock);
+ ret = ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
+ mutex_unlock(&ucsi->ppm_lock);
+
+ return ret;
}
static int ucsi_exec_command(struct ucsi *ucsi, u64 command);
@@ -729,118 +734,26 @@ static void ucsi_partner_change(struct ucsi_connector *con)
ucsi_partner_task(con, ucsi_check_altmodes, 30);
}
-static void ucsi_handle_connector_change(struct work_struct *work)
+struct ucsi_con_event {
+ struct work_struct work;
+ struct ucsi_connector *con;
+ struct ucsi_connector_status status;
+};
+
+static void ucsi_connector_work(struct work_struct *work)
{
- struct ucsi_connector *con = container_of(work, struct ucsi_connector,
- work);
+ struct ucsi_con_event *event = container_of(work, struct ucsi_con_event, work);
+ struct ucsi_connector *con = event->con;
struct ucsi *ucsi = con->ucsi;
- struct ucsi_connector_status pre_ack_status;
- struct ucsi_connector_status post_ack_status;
enum typec_role role;
enum usb_role u_role = USB_ROLE_NONE;
- u16 inferred_changes;
- u16 changed_flags;
- u64 command;
int ret;
mutex_lock(&con->lock);
- /*
- * Some/many PPMs have an issue where all fields in the change bitfield
- * are cleared when an ACK is send. This will causes any change
- * between GET_CONNECTOR_STATUS and ACK to be lost.
- *
- * We work around this by re-fetching the connector status afterwards.
- * We then infer any changes that we see have happened but that may not
- * be represented in the change bitfield.
- *
- * Also, even though we don't need to know the currently supported alt
- * modes, we run the GET_CAM_SUPPORTED command to ensure the PPM does
- * not get stuck in case it assumes we do.
- * Always do this, rather than relying on UCSI_CONSTAT_CAM_CHANGE to be
- * set in the change bitfield.
- *
- * We end up with the following actions:
- * 1. UCSI_GET_CONNECTOR_STATUS, store result, update unprocessed_changes
- * 2. UCSI_GET_CAM_SUPPORTED, discard result
- * 3. ACK connector change
- * 4. UCSI_GET_CONNECTOR_STATUS, store result
- * 5. Infere lost changes by comparing UCSI_GET_CONNECTOR_STATUS results
- * 6. If PPM reported a new change, then restart in order to ACK
- * 7. Process everything as usual.
- *
- * We may end up seeing a change twice, but we can only miss extremely
- * short transitional changes.
- */
-
- /* 1. First UCSI_GET_CONNECTOR_STATUS */
- command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
- ret = ucsi_send_command(ucsi, command, &pre_ack_status,
- sizeof(pre_ack_status));
- if (ret < 0) {
- dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n",
- __func__, ret);
- goto out_unlock;
- }
- con->unprocessed_changes |= pre_ack_status.change;
-
- /* 2. Run UCSI_GET_CAM_SUPPORTED and discard the result. */
- command = UCSI_GET_CAM_SUPPORTED;
- command |= UCSI_CONNECTOR_NUMBER(con->num);
- ucsi_send_command(con->ucsi, command, NULL, 0);
-
- /* 3. ACK connector change */
- ret = ucsi_acknowledge_connector_change(ucsi);
- clear_bit(EVENT_PENDING, &ucsi->flags);
- if (ret) {
- dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
- goto out_unlock;
- }
-
- /* 4. Second UCSI_GET_CONNECTOR_STATUS */
- command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
- ret = ucsi_send_command(ucsi, command, &post_ack_status,
- sizeof(post_ack_status));
- if (ret < 0) {
- dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n",
- __func__, ret);
- goto out_unlock;
- }
-
- /* 5. Inferre any missing changes */
- changed_flags = pre_ack_status.flags ^ post_ack_status.flags;
- inferred_changes = 0;
- if (UCSI_CONSTAT_PWR_OPMODE(changed_flags) != 0)
- inferred_changes |= UCSI_CONSTAT_POWER_OPMODE_CHANGE;
-
- if (changed_flags & UCSI_CONSTAT_CONNECTED)
- inferred_changes |= UCSI_CONSTAT_CONNECT_CHANGE;
-
- if (changed_flags & UCSI_CONSTAT_PWR_DIR)
- inferred_changes |= UCSI_CONSTAT_POWER_DIR_CHANGE;
-
- if (UCSI_CONSTAT_PARTNER_FLAGS(changed_flags) != 0)
- inferred_changes |= UCSI_CONSTAT_PARTNER_CHANGE;
-
- if (UCSI_CONSTAT_PARTNER_TYPE(changed_flags) != 0)
- inferred_changes |= UCSI_CONSTAT_PARTNER_CHANGE;
-
- /* Mask out anything that was correctly notified in the later call. */
- inferred_changes &= ~post_ack_status.change;
- if (inferred_changes)
- dev_dbg(ucsi->dev, "%s: Inferred changes that would have been lost: 0x%04x\n",
- __func__, inferred_changes);
-
- con->unprocessed_changes |= inferred_changes;
-
- /* 6. If PPM reported a new change, then restart in order to ACK */
- if (post_ack_status.change)
- goto out_unlock;
-
- /* 7. Continue as if nothing happened */
- con->status = post_ack_status;
- con->status.change = con->unprocessed_changes;
- con->unprocessed_changes = 0;
+ trace_ucsi_connector_change(con->num, &event->status);
+ con->status = event->status;
+ kfree(event);
role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR);
@@ -898,17 +811,49 @@ static void ucsi_handle_connector_change(struct work_struct *work)
if (con->status.change & UCSI_CONSTAT_PARTNER_CHANGE)
ucsi_partner_change(con);
- trace_ucsi_connector_change(con->num, &con->status);
+ mutex_unlock(&con->lock);
+}
-out_unlock:
- if (test_and_clear_bit(EVENT_PENDING, &ucsi->flags)) {
- schedule_work(&con->work);
- mutex_unlock(&con->lock);
- return;
+/*
+ * We can not read the connector status in ucsi_connector_change() function
+ * below because there may be already a command pending. This work is scheduled
+ * separately only because of that.
+ *
+ * This function must finish fast so we do not loose the next events. Every
+ * event will have a separate job queued for it in the connector specific
+ * workqueue. That way the next event can be generated safely before the
+ * previous ones are fully processed.
+ */
+static void ucsi_handle_connector_change(struct work_struct *work)
+{
+ struct ucsi_connector *con = container_of(work, struct ucsi_connector, work);
+ struct ucsi_connector_status status;
+ struct ucsi_con_event *event;
+ u64 command;
+ int ret;
+
+ command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
+ ret = ucsi_send_command(con->ucsi, command, &status, sizeof(status));
+ if (ret < 0) {
+ dev_err(con->ucsi->dev, "GET_CONNECTOR_STATUS failed (%d)\n", ret);
+ goto out_ack;
}
- clear_bit(EVENT_PROCESSING, &ucsi->flags);
- mutex_unlock(&con->lock);
+ event = kzalloc(sizeof(*event), GFP_KERNEL);
+ if (!event)
+ goto out_ack;
+
+ INIT_WORK(&event->work, ucsi_connector_work);
+ event->status = status;
+ event->con = con;
+ queue_work(con->wq, &event->work);
+
+out_ack:
+ clear_bit(EVENT_PENDING, &con->ucsi->flags);
+
+ ret = ucsi_acknowledge_connector_change(con->ucsi);
+ if (ret)
+ dev_err(con->ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
}
/**
@@ -925,10 +870,10 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num)
return;
}
- set_bit(EVENT_PENDING, &ucsi->flags);
+ if (test_and_set_bit(EVENT_PENDING, &ucsi->flags))
+ return;
- if (!test_and_set_bit(EVENT_PROCESSING, &ucsi->flags))
- schedule_work(&con->work);
+ schedule_work(&con->work);
}
EXPORT_SYMBOL_GPL(ucsi_connector_change);
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index d10b8c24435af..280f1e1bda2c9 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -300,7 +300,6 @@ struct ucsi {
#define EVENT_PENDING 0
#define COMMAND_PENDING 1
#define ACK_PENDING 2
-#define EVENT_PROCESSING 3
};
#define UCSI_MAX_SVID 5
@@ -327,7 +326,6 @@ struct ucsi_connector {
struct typec_capability typec_cap;
- u16 unprocessed_changes;
struct ucsi_connector_status status;
struct ucsi_connector_capability cap;
struct power_supply *psy;
--
2.30.2
The huge delay was there to workaround a problem where the
firmware did not report that if it was busy with the
alternate mode commands. Now that the alternate modes are
polled, the delay can be dropped.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/typec/ucsi/ucsi_acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 04976435ad736..6771f05e32c29 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -78,7 +78,7 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi, unsigned int offset,
if (ret)
goto out_clear_bit;
- if (!wait_for_completion_timeout(&ua->complete, 60 * HZ))
+ if (!wait_for_completion_timeout(&ua->complete, HZ))
ret = -ETIMEDOUT;
out_clear_bit:
--
2.30.2
Polling also the PDOs, just like the alt modes.
After this ucsi_handle_connector_change() doesn't execute
any commands.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index ce80a433ef9db..bd39fe2cb1d0b 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -575,7 +575,7 @@ static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
command |= UCSI_GET_PDOS_SRC_PDOS;
ret = ucsi_send_command(ucsi, command, pdos + offset,
num_pdos * sizeof(u32));
- if (ret < 0)
+ if (ret < 0 && ret != -ETIMEDOUT)
dev_err(ucsi->dev, "UCSI_GET_PDOS failed (%d)\n", ret);
if (ret == 0 && offset == 0)
dev_warn(ucsi->dev, "UCSI_GET_PDOS returned 0 bytes\n");
@@ -583,26 +583,30 @@ static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner,
return ret;
}
-static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
+static int ucsi_get_src_pdos(struct ucsi_connector *con)
{
int ret;
/* UCSI max payload means only getting at most 4 PDOs at a time */
ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
if (ret < 0)
- return;
+ return ret;
con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
if (con->num_pdos < UCSI_MAX_PDOS)
- return;
+ return 0;
/* get the remaining PDOs, if any */
ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
if (ret < 0)
- return;
+ return ret;
con->num_pdos += ret / sizeof(u32);
+
+ ucsi_port_psy_changed(con);
+
+ return 0;
}
static int ucsi_check_altmodes(struct ucsi_connector *con)
@@ -630,7 +634,7 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
case UCSI_CONSTAT_PWR_OPMODE_PD:
con->rdo = con->status.request_data_obj;
typec_set_pwr_opmode(con->port, TYPEC_PWR_MODE_PD);
- ucsi_get_src_pdos(con, 1);
+ ucsi_partner_task(con, ucsi_get_src_pdos, 20);
break;
case UCSI_CONSTAT_PWR_OPMODE_TYPEC1_5:
con->rdo = 0;
@@ -758,10 +762,8 @@ static void ucsi_connector_work(struct work_struct *work)
role = !!(con->status.flags & UCSI_CONSTAT_PWR_DIR);
if (con->status.change & UCSI_CONSTAT_POWER_OPMODE_CHANGE ||
- con->status.change & UCSI_CONSTAT_POWER_LEVEL_CHANGE) {
+ con->status.change & UCSI_CONSTAT_POWER_LEVEL_CHANGE)
ucsi_pwr_opmode_change(con);
- ucsi_port_psy_changed(con);
- }
if (con->status.change & UCSI_CONSTAT_POWER_DIR_CHANGE) {
typec_set_pwr_role(con->port, role);
@@ -1170,9 +1172,9 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
if (con->status.flags & UCSI_CONSTAT_CONNECTED) {
typec_set_pwr_role(con->port,
!!(con->status.flags & UCSI_CONSTAT_PWR_DIR));
- ucsi_pwr_opmode_change(con);
ucsi_register_partner(con);
ucsi_port_psy_changed(con);
+ ucsi_pwr_opmode_change(con);
}
con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);
--
2.30.2
WIP.
The alternate modes are now checked by polling them. That
make's it possible to drop the huge delay for command
completion timeout that we now have at least in the UCSI
ACPI driver. The delay was causing other problems as
the driver can't do anything for the connector until the
command completes.
In practice this is a "better" workaround for the problem
where the firmware (PPM) does not return BUSY when it should
with some commands.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.c | 104 +++++++++++++++++++++++++++++++---
drivers/usb/typec/ucsi/ucsi.h | 1 +
2 files changed, 97 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index a8e0e31dcddf5..d41147b3b6e8a 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -191,6 +191,62 @@ int ucsi_resume(struct ucsi *ucsi)
EXPORT_SYMBOL_GPL(ucsi_resume);
/* -------------------------------------------------------------------------- */
+struct ucsi_work {
+ struct work_struct work;
+ unsigned int count;
+ struct ucsi_connector *con;
+ int (*cb)(struct ucsi_connector *);
+};
+
+static void ucsi_poll_worker(struct work_struct *work)
+{
+ struct ucsi_work *uwork = container_of(work, struct ucsi_work, work);
+ struct ucsi_connector *con = uwork->con;
+ int ret;
+
+ mutex_lock(&con->lock);
+
+ if (!con->partner) {
+ mutex_unlock(&con->lock);
+ kfree(uwork);
+ return;
+ }
+
+ ret = uwork->cb(con);
+
+ if (uwork->count-- && (ret == -EBUSY || ret == -ETIMEDOUT))
+ queue_work(con->wq, &uwork->work);
+ else
+ kfree(uwork);
+
+ mutex_unlock(&con->lock);
+}
+
+static int ucsi_partner_task(struct ucsi_connector *con,
+ int (*cb)(struct ucsi_connector *),
+ int retries)
+{
+ struct ucsi_work *uwork;
+
+ if (!con->partner)
+ return 0;
+
+ uwork = kzalloc(sizeof(*uwork), GFP_KERNEL);
+ if (!uwork)
+ return -ENOMEM;
+
+ INIT_WORK(&uwork->work, ucsi_poll_worker);
+ uwork->count = retries;
+ uwork->con = con;
+ uwork->cb = cb;
+
+ queue_work(con->wq, &uwork->work);
+
+ return 0;
+}
+
+/* -------------------------------------------------------------------------- */
+
void ucsi_altmode_update_active(struct ucsi_connector *con)
{
const struct typec_altmode *altmode = NULL;
@@ -544,6 +600,25 @@ static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
con->num_pdos += ret / sizeof(u32);
}
+static int ucsi_check_altmodes(struct ucsi_connector *con)
+{
+ int ret;
+
+ ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP);
+ if (ret && ret != -ETIMEDOUT)
+ dev_err(con->ucsi->dev,
+ "con%d: failed to register partner alt modes (%d)\n",
+ con->num, ret);
+
+ /* Ignoring the errors in this case. */
+ if (con->partner_altmode[0]) {
+ ucsi_altmode_update_active(con);
+ return 0;
+ }
+
+ return ret;
+}
+
static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
{
switch (UCSI_CONSTAT_PWR_OPMODE(con->status.flags)) {
@@ -651,14 +726,7 @@ static void ucsi_partner_change(struct ucsi_connector *con)
dev_err(con->ucsi->dev, "con:%d: failed to set usb role:%d\n",
con->num, u_role);
- /* Can't rely on Partner Flags field. Always checking the alt modes. */
- ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP);
- if (ret)
- dev_err(con->ucsi->dev,
- "con%d: failed to register partner alternate modes\n",
- con->num);
- else
- ucsi_altmode_update_active(con);
+ ucsi_partner_task(con, ucsi_check_altmodes, 30);
}
static void ucsi_handle_connector_change(struct work_struct *work)
@@ -1046,8 +1114,18 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
enum typec_accessory *accessory = cap->accessory;
enum usb_role u_role = USB_ROLE_NONE;
u64 command;
+ char *name;
int ret;
+ name = kasprintf(GFP_KERNEL, "%s-con%d", dev_name(ucsi->dev), con->num);
+ if (!name)
+ return -ENOMEM;
+
+ con->wq = create_singlethread_workqueue(name);
+ kfree(name);
+ if (!con->wq)
+ return -ENOMEM;
+
INIT_WORK(&con->work, ucsi_handle_connector_change);
init_completion(&con->complete);
mutex_init(&con->lock);
@@ -1183,6 +1261,12 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
fwnode_handle_put(cap->fwnode);
out_unlock:
mutex_unlock(&con->lock);
+
+ if (ret && con->wq) {
+ destroy_workqueue(con->wq);
+ con->wq = NULL;
+ }
+
return ret;
}
@@ -1253,6 +1337,8 @@ static int ucsi_init(struct ucsi *ucsi)
ucsi_unregister_partner(con);
ucsi_unregister_altmodes(con, UCSI_RECIPIENT_CON);
ucsi_unregister_port_psy(con);
+ if (con->wq)
+ destroy_workqueue(con->wq);
typec_unregister_port(con->port);
con->port = NULL;
}
@@ -1374,6 +1460,8 @@ void ucsi_unregister(struct ucsi *ucsi)
ucsi_unregister_altmodes(&ucsi->connector[i],
UCSI_RECIPIENT_CON);
ucsi_unregister_port_psy(&ucsi->connector[i]);
+ if (ucsi->connector[i].wq)
+ destroy_workqueue(ucsi->connector[i].wq);
typec_unregister_port(ucsi->connector[i].port);
}
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index cee666790907e..d10b8c24435af 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -317,6 +317,7 @@ struct ucsi_connector {
struct mutex lock; /* port lock */
struct work_struct work;
struct completion complete;
+ struct workqueue_struct *wq;
struct typec_port *port;
struct typec_partner *partner;
--
2.30.2
Ignoring events without changes.
Signed-off-by: Heikki Krogerus <[email protected]>
---
drivers/usb/typec/ucsi/ucsi.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index a4123f77d1f16..ce80a433ef9db 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -839,6 +839,11 @@ static void ucsi_handle_connector_change(struct work_struct *work)
goto out_ack;
}
+ if (!status.change) {
+ dev_dbg(con->ucsi->dev, "con%d: spurious event\n", con->num);
+ goto out_ack;
+ }
+
event = kzalloc(sizeof(*event), GFP_KERNEL);
if (!event)
goto out_ack;
--
2.30.2
Hi Heikki,
On Mon, 2021-06-07 at 16:14 +0300, Heikki Krogerus wrote:
> This is the RFC series I promised [1].
Cool.
> I'm sorry it took this long to prepare these. I had to concentrate on
> other task for a while.
>
> Let me know if you still see any problems.
Hmm, I am not sure this got better. I applied the patchset on top of of
the 5.12.9 Fedora 34 kernel. On the machine in question (X1 Carbon 8),
I see the online state getting stuck at 1 occasionally. This can happen
for example when quickly plugging and unplugging a USB-C charger.
Benjamin
> [1]
> https://lore.kernel.org/linux-usb/YKT3oEt%[email protected]/
>
> thanks,
>
> Heikki Krogerus (7):
> usb: typec: ucsi: Always cancel the command if PPM reports BUSY
> condition
> usb: typec: ucsi: Don't stop alt mode registration on busy
> condition
> usb: typec: ucsi: Add poll worker for alternate modes
> usb: typec: ucsi: acpi: Reduce the command completion timeout
> usb: typec: ucsi: Process every connector change as unique
> connector
> state
> usb: typec: ucsi: Filter out spurious events
> usb: typec: ucsi: Read the PDOs in separate work
>
> drivers/usb/typec/ucsi/ucsi.c | 317 ++++++++++++++++-----------
> --
> drivers/usb/typec/ucsi/ucsi.h | 3 +-
> drivers/usb/typec/ucsi/ucsi_acpi.c | 2 +-
> 3 files changed, 183 insertions(+), 139 deletions(-)
>
On Mon, Jun 07, 2021 at 10:09:58PM +0200, Benjamin Berg wrote:
> Hi Heikki,
>
> On Mon, 2021-06-07 at 16:14 +0300, Heikki Krogerus wrote:
> > This is the RFC series I promised [1].
>
> Cool.
>
> > I'm sorry it took this long to prepare these. I had to concentrate on
> > other task for a while.
> >
> > Let me know if you still see any problems.
>
> Hmm, I am not sure this got better. I applied the patchset on top of of
> the 5.12.9 Fedora 34 kernel. On the machine in question (X1 Carbon 8),
> I see the online state getting stuck at 1 occasionally. This can happen
> for example when quickly plugging and unplugging a USB-C charger.
Please check does the partner device get removed. What do you have
under /sys/class/typec after that happens?
thanks,
--
heikki
On Tue, Jun 08, 2021 at 09:42:09AM +0300, Heikki Krogerus wrote:
> On Mon, Jun 07, 2021 at 10:09:58PM +0200, Benjamin Berg wrote:
> > Hi Heikki,
> >
> > On Mon, 2021-06-07 at 16:14 +0300, Heikki Krogerus wrote:
> > > This is the RFC series I promised [1].
> >
> > Cool.
> >
> > > I'm sorry it took this long to prepare these. I had to concentrate on
> > > other task for a while.
> > >
> > > Let me know if you still see any problems.
> >
> > Hmm, I am not sure this got better. I applied the patchset on top of of
> > the 5.12.9 Fedora 34 kernel. On the machine in question (X1 Carbon 8),
> > I see the online state getting stuck at 1 occasionally. This can happen
> > for example when quickly plugging and unplugging a USB-C charger.
>
> Please check does the partner device get removed. What do you have
> under /sys/class/typec after that happens?
Oh yes. Could you also share the trace output when that happens?
cd /sys/kernel/debug/tracing
echo 1 > events/ucsi/enable
# now reproduce the issue
cat trace > ucsi.trace
thanks,
--
heikki
Hello!
On 07.06.2021 16:14, Heikki Krogerus wrote:
> If the PPM tells it's busy, we can now simply try again.
>
> Signed-off-by: Heikki Krogerus <[email protected]>
> ---
> drivers/usb/typec/ucsi/ucsi.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 366c8a468bc18..a8e0e31dcddf5 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -437,8 +437,11 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
> command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
> command |= UCSI_GET_ALTMODE_OFFSET(i);
> len = ucsi_send_command(con->ucsi, command, alt, sizeof(alt));
Could insert your check here, to reduce the indentation...
> - if (len <= 0)
> + if (len <= 0) {
> + if (len == -EBUSY)
> + continue;
> return len;
> + }
>
> /*
> * This code is requesting one alt mode at a time, but some PPMs
MBR, Sergei
On Tue, Jun 08, 2021 at 12:31:45PM +0300, Sergei Shtylyov wrote:
> Hello!
>
> On 07.06.2021 16:14, Heikki Krogerus wrote:
>
> > If the PPM tells it's busy, we can now simply try again.
> >
> > Signed-off-by: Heikki Krogerus <[email protected]>
> > ---
> > drivers/usb/typec/ucsi/ucsi.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index 366c8a468bc18..a8e0e31dcddf5 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -437,8 +437,11 @@ static int ucsi_register_altmodes(struct ucsi_connector *con, u8 recipient)
> > command |= UCSI_GET_ALTMODE_CONNECTOR_NUMBER(con->num);
> > command |= UCSI_GET_ALTMODE_OFFSET(i);
> > len = ucsi_send_command(con->ucsi, command, alt, sizeof(alt));
>
> Could insert your check here, to reduce the indentation...
Sure thing.
> > - if (len <= 0)
> > + if (len <= 0) {
> > + if (len == -EBUSY)
> > + continue;
> > return len;
> > + }
> > /*
> > * This code is requesting one alt mode at a time, but some PPMs
thanks,
--
heikki
On Tue, 2021-06-08 at 09:54 +0300, Heikki Krogerus wrote:
> On Tue, Jun 08, 2021 at 09:42:09AM +0300, Heikki Krogerus wrote:
> > Please check does the partner device get removed. What do you have
> > under /sys/class/typec after that happens?
>
> Oh yes. Could you also share the trace output when that happens?
>
> cd /sys/kernel/debug/tracing
> echo 1 > events/ucsi/enable
> # now reproduce the issue
> cat trace > ucsi.trace
So, the partner device is still there when this happens (see below). I
also only see a single event in the trace for the fast plug/unplug
case:
kworker/u16:8-1771 [003] .... 18848.872145: ucsi_connector_change: port1 status: change=4a04, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
The typec port/partner states is:
port1-partner/accessory_mode:none
port1-partner/supports_usb_power_delivery:no
port1-partner/uevent:DEVTYPE=typec_partner
port1-partner/usb_power_delivery_revision:0.0
and
port0/data_role:host [device]
port0/power_operation_mode:default
port0/power_role:source [sink]
port0/supported_accessory_modes:none
port0/uevent:DEVTYPE=typec_port
port0/uevent:TYPEC_PORT=port0
port0/usb_power_delivery_revision:2.0
port0/usb_typec_revision:1.0
port0/vconn_source:no
port1/data_role:host [device]
port1/power_operation_mode:3.0A
port1/power_role:source [sink]
port1/supported_accessory_modes:none
port1/uevent:DEVTYPE=typec_port
port1/uevent:TYPEC_PORT=port1
port1/usb_power_delivery_revision:2.0
port1/usb_typec_revision:1.0
port1/vconn_source:no
Note that for a normal plug I am usually getting a second event. This
second event is occasionally missing though:
kworker/u16:38-1800 [001] .... 19522.325885: ucsi_connector_change: port1 status: change=4a04, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
kworker/u16:38-1800 [004] .... 19522.552613: ucsi_connector_change: port1 status: change=0044, opmode=3, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
and a working unplug consistently looks like:
kworker/u16:8-1771 [003] .... 19670.020085: ucsi_connector_change: port1 status: change=4804, opmode=0, connected=0, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=0
Benjamin
On Wed, Jun 09, 2021 at 03:18:04PM +0300, Heikki Krogerus wrote:
> On Wed, Jun 09, 2021 at 02:26:00PM +0300, Heikki Krogerus wrote:
> > On Tue, Jun 08, 2021 at 09:32:01PM +0200, Benjamin Berg wrote:
> > > On Tue, 2021-06-08 at 09:54 +0300, Heikki Krogerus wrote:
> > > > On Tue, Jun 08, 2021 at 09:42:09AM +0300, Heikki Krogerus wrote:
> > > > > Please check does the partner device get removed. What do you have
> > > > > under /sys/class/typec after that happens?
> > > >
> > > > Oh yes. Could you also share the trace output when that happens?
> > > >
> > > > ??????? cd /sys/kernel/debug/tracing
> > > > ??????? echo 1 > events/ucsi/enable
> > > > ??????? # now reproduce the issue
> > > > ??????? cat trace > ucsi.trace
> > >
> > > So, the partner device is still there when this happens (see below). I
> > > also only see a single event in the trace for the fast plug/unplug
> > > case:
> > > kworker/u16:8-1771 [003] .... 18848.872145: ucsi_connector_change: port1 status: change=4a04, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
> >
> > OK. Sorry I had to double check because you were only talking about
> > the psy online state.
> >
> > Can you now try this HACK on top of these patches:
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > index bd39fe2cb1d0b..99f072700ce7f 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.c
> > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > @@ -843,7 +843,8 @@ static void ucsi_handle_connector_change(struct work_struct *work)
> >
> > if (!status.change) {
> > dev_dbg(con->ucsi->dev, "con%d: spurious event\n", con->num);
> > - goto out_ack;
> > + /* XXX Force connection check. */
> > + status.change = UCSI_CONSTAT_CONNECT_CHANGE;
> > }
> >
> > event = kzalloc(sizeof(*event), GFP_KERNEL);
>
> No, that's not enough. Sorry.
>
> I'm trying to get a confirmation on my suspecion that we do always
> actually get an event from the EC firmware, but we just end up
> filtering it out in this case because we are too slow in the driver. I
> have an idea what could be done about that, but I need to test if that
> really is the case.
>
> I'll prepare a new version out of this entire series.
Actually, it's easier if you could just test this attached patch on
top of this series. It makes sure the every single event is
considered. I'm sorry about the hassle.
thanks,
--
heikki
On Tue, Jun 08, 2021 at 09:32:01PM +0200, Benjamin Berg wrote:
> On Tue, 2021-06-08 at 09:54 +0300, Heikki Krogerus wrote:
> > On Tue, Jun 08, 2021 at 09:42:09AM +0300, Heikki Krogerus wrote:
> > > Please check does the partner device get removed. What do you have
> > > under /sys/class/typec after that happens?
> >
> > Oh yes. Could you also share the trace output when that happens?
> >
> > ??????? cd /sys/kernel/debug/tracing
> > ??????? echo 1 > events/ucsi/enable
> > ??????? # now reproduce the issue
> > ??????? cat trace > ucsi.trace
>
> So, the partner device is still there when this happens (see below). I
> also only see a single event in the trace for the fast plug/unplug
> case:
> kworker/u16:8-1771 [003] .... 18848.872145: ucsi_connector_change: port1 status: change=4a04, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
OK. Sorry I had to double check because you were only talking about
the psy online state.
Can you now try this HACK on top of these patches:
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index bd39fe2cb1d0b..99f072700ce7f 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -843,7 +843,8 @@ static void ucsi_handle_connector_change(struct work_struct *work)
if (!status.change) {
dev_dbg(con->ucsi->dev, "con%d: spurious event\n", con->num);
- goto out_ack;
+ /* XXX Force connection check. */
+ status.change = UCSI_CONSTAT_CONNECT_CHANGE;
}
event = kzalloc(sizeof(*event), GFP_KERNEL);
thanks,
--
heikki
On Wed, Jun 09, 2021 at 02:26:00PM +0300, Heikki Krogerus wrote:
> On Tue, Jun 08, 2021 at 09:32:01PM +0200, Benjamin Berg wrote:
> > On Tue, 2021-06-08 at 09:54 +0300, Heikki Krogerus wrote:
> > > On Tue, Jun 08, 2021 at 09:42:09AM +0300, Heikki Krogerus wrote:
> > > > Please check does the partner device get removed. What do you have
> > > > under /sys/class/typec after that happens?
> > >
> > > Oh yes. Could you also share the trace output when that happens?
> > >
> > > ??????? cd /sys/kernel/debug/tracing
> > > ??????? echo 1 > events/ucsi/enable
> > > ??????? # now reproduce the issue
> > > ??????? cat trace > ucsi.trace
> >
> > So, the partner device is still there when this happens (see below). I
> > also only see a single event in the trace for the fast plug/unplug
> > case:
> > kworker/u16:8-1771 [003] .... 18848.872145: ucsi_connector_change: port1 status: change=4a04, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
>
> OK. Sorry I had to double check because you were only talking about
> the psy online state.
>
> Can you now try this HACK on top of these patches:
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index bd39fe2cb1d0b..99f072700ce7f 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -843,7 +843,8 @@ static void ucsi_handle_connector_change(struct work_struct *work)
>
> if (!status.change) {
> dev_dbg(con->ucsi->dev, "con%d: spurious event\n", con->num);
> - goto out_ack;
> + /* XXX Force connection check. */
> + status.change = UCSI_CONSTAT_CONNECT_CHANGE;
> }
>
> event = kzalloc(sizeof(*event), GFP_KERNEL);
No, that's not enough. Sorry.
I'm trying to get a confirmation on my suspecion that we do always
actually get an event from the EC firmware, but we just end up
filtering it out in this case because we are too slow in the driver. I
have an idea what could be done about that, but I need to test if that
really is the case.
I'll prepare a new version out of this entire series.
thanks,
--
heikki
On Wed, 2021-06-09 at 15:56 +0300, Heikki Krogerus wrote:
> On Wed, Jun 09, 2021 at 03:18:04PM +0300, Heikki Krogerus wrote:
> >
> > I'm trying to get a confirmation on my suspecion that we do always
> > actually get an event from the EC firmware, but we just end up
> > filtering it out in this case because we are too slow in the driver.
> > I
> > have an idea what could be done about that, but I need to test if
> > that
> > really is the case.
> >
> > I'll prepare a new version out of this entire series.
>
> Actually, it's easier if you could just test this attached patch on
> top of this series. It makes sure the every single event is
> considered. I'm sorry about the hassle.
No worries! I probably should have included some more information
earlier (i.e. enabling the debug print for spurious events).
With the patch, I am seeing the following on plug
kworker/u16:1-6847 [002] .... 1375.485010: ucsi_connector_change: port1 status: change=4a04, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
kworker/u16:2-6848 [006] .... 1375.561811: ucsi_connector_change: port1 status: change=4000, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
kworker/u16:2-6848 [007] .... 1375.634275: ucsi_connector_change: port1 status: change=4000, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
kworker/u16:2-6848 [003] .... 1375.743161: ucsi_connector_change: port1 status: change=4000, opmode=3, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
and unplug
kworker/u16:1-6847 [005] .... 1394.062501: ucsi_connector_change: port1 status: change=4804, opmode=0, connected=0, sourcing=0, partner_flags=0, partner_type=0, request_data_obj=00000000, BC status=0
kworker/u16:1-6847 [005] .... 1394.161612: ucsi_connector_change: port1 status: change=4000, opmode=0, connected=0, sourcing=0, partner_flags=0, partner_type=0, request_data_obj=00000000, BC status=0
kworker/u16:1-6847 [005] .... 1394.251503: ucsi_connector_change: port1 status: change=4000, opmode=0, connected=0, sourcing=0, partner_flags=0, partner_type=0, request_data_obj=00000000, BC status=0
where all but the first event are spurious events. I believe that in
the above spurious event with the change to opmode=3, the PPM should be
reporting change=0004 (i.e. UCSI_CONSTAT_POWER_OPMODE_CHANGE).
Occasionally I also see the following on plug. Note the non-spurious
event with change=0040 (UCSI_CONSTAT_POWER_LEVEL_CHANGE) right before
the event where opmode changes.
kworker/u16:11-2201 [001] .... 3240.124431: ucsi_connector_change: port1 status: change=4a04, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
kworker/u16:3-7469 [003] .... 3240.222799: ucsi_connector_change: port1 status: change=4000, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
kworker/u16:3-7469 [003] .... 3240.325946: ucsi_connector_change: port1 status: change=0040, opmode=5, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
kworker/u16:3-7469 [003] .... 3240.423503: ucsi_connector_change: port1 status: change=4000, opmode=3, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
kworker/u16:3-7469 [003] .... 3240.861986: ucsi_connector_change: port1 status: change=4000, opmode=3, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
kworker/u16:3-7469 [007] .... 3240.999048: ucsi_connector_change: port1 status: change=4000, opmode=3, connected=1, sourcing=0, partner_flags=1, partner_type=1, request_data_obj=1304b12c, BC status=1
My thought when I first ran into the issue was that the PPM simply
resets the change bitfield on ACK, effectively discarding any changes
that happened after the last GET_CONNECTOR_STATUS call. I believed to
have confirmed this by inserting an msleep in between.
However, I have to admit that I have never really looked for
alternative explanations.
Benjamin
On Wed, Jun 09, 2021 at 07:39:41PM +0200, Benjamin Berg wrote:
> My thought when I first ran into the issue was that the PPM simply
> resets the change bitfield on ACK, effectively discarding any changes
> that happened after the last GET_CONNECTOR_STATUS call. I believed to
> have confirmed this by inserting an msleep in between.
> However, I have to admit that I have never really looked for
> alternative explanations.
Thanks a lot for testing these. I now have pretty good idea about the
problem. The problem is not what you though it is, but the driver is
also not too slow like I suspected.
I'm quite certain now that the PPM simply does not create the event at
all in this case, regardless of what the driver does. We do need to
workaround that problem, but I think we can do it in a better way. I
have an idea for that.
thanks,
--
heikki