* [PATCH v6] usb: typec: ucsi: Get connector status after enable notifications
@ 2025-12-05 7:07 Hsin-Te Yuan
2025-12-05 10:51 ` Heikki Krogerus
2025-12-17 13:53 ` Greg Kroah-Hartman
0 siblings, 2 replies; 3+ messages in thread
From: Hsin-Te Yuan @ 2025-12-05 7:07 UTC (permalink / raw)
To: Heikki Krogerus, Greg Kroah-Hartman, Guenter Roeck
Cc: linux-usb, linux-kernel, stable, Hsin-Te Yuan
Originally, the notification for connector change will be enabled after
the first read of the connector status. Therefore, if the event happens
during this window, it will be missing and make the status unsynced.
Get the connector status only after enabling the notification for
connector change to ensure the status is synced.
Fixes: c1b0bc2dabfa ("usb: typec: Add support for UCSI interface")
Cc: stable@vger.kernel.org # v4.13+
Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
---
Changes in v6:
- Free the locks in error path.
- Link to v5: https://lore.kernel.org/r/20251205-ucsi-v5-1-488eb89bc9b8@chromium.org
Changes in v5:
- Hold the lock of each connector during the initialization to avoid
race condition between initialization and other event handler
- Add Fixes tag
- Link to v4: https://lore.kernel.org/r/20251125-ucsi-v4-1-8c94568ddaa5@chromium.org
Changes in v4:
- Handle a single connector in ucsi_init_port() and call it in a loop
- Link to v3: https://lore.kernel.org/r/20251121-ucsi-v3-1-b1047ca371b8@chromium.org
Changes in v3:
- Seperate the status checking part into a new function called
ucsi_init_port() and call it after enabling the notifications
- Link to v2: https://lore.kernel.org/r/20251118-ucsi-v2-1-d314d50333e2@chromium.org
Changes in v2:
- Remove unnecessary braces.
- Link to v1: https://lore.kernel.org/r/20251117-ucsi-v1-1-1dcbc5ea642b@chromium.org
---
drivers/usb/typec/ucsi/ucsi.c | 131 +++++++++++++++++++++++-------------------
1 file changed, 73 insertions(+), 58 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 3f568f790f39b0271667e80816270274b8dd3008..3a0471fa4cc980c0512bc71776e3984e6cd2cdb7 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1560,11 +1560,70 @@ static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con)
return NULL;
}
+static void ucsi_init_port(struct ucsi *ucsi, struct ucsi_connector *con)
+{
+ enum usb_role u_role = USB_ROLE_NONE;
+ int ret;
+
+ /* Get the status */
+ ret = ucsi_get_connector_status(con, false);
+ if (ret) {
+ dev_err(ucsi->dev, "con%d: failed to get status\n", con->num);
+ return;
+ }
+
+ if (ucsi->ops->connector_status)
+ ucsi->ops->connector_status(con);
+
+ switch (UCSI_CONSTAT(con, PARTNER_TYPE)) {
+ case UCSI_CONSTAT_PARTNER_TYPE_UFP:
+ case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
+ u_role = USB_ROLE_HOST;
+ fallthrough;
+ case UCSI_CONSTAT_PARTNER_TYPE_CABLE:
+ typec_set_data_role(con->port, TYPEC_HOST);
+ break;
+ case UCSI_CONSTAT_PARTNER_TYPE_DFP:
+ u_role = USB_ROLE_DEVICE;
+ typec_set_data_role(con->port, TYPEC_DEVICE);
+ break;
+ default:
+ break;
+ }
+
+ /* Check if there is already something connected */
+ if (UCSI_CONSTAT(con, CONNECTED)) {
+ typec_set_pwr_role(con->port, UCSI_CONSTAT(con, PWR_DIR));
+ ucsi_register_partner(con);
+ ucsi_pwr_opmode_change(con);
+ ucsi_port_psy_changed(con);
+ if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE)
+ ucsi_get_partner_identity(con);
+ if (con->ucsi->cap.features & UCSI_CAP_CABLE_DETAILS)
+ ucsi_check_cable(con);
+ }
+
+ /* Only notify USB controller if partner supports USB data */
+ if (!(UCSI_CONSTAT(con, PARTNER_FLAG_USB)))
+ u_role = USB_ROLE_NONE;
+
+ ret = usb_role_switch_set_role(con->usb_role_sw, u_role);
+ if (ret)
+ dev_err(ucsi->dev, "con:%d: failed to set usb role:%d\n",
+ con->num, u_role);
+
+ if (con->partner && UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) {
+ ucsi_register_device_pdos(con);
+ ucsi_get_src_pdos(con);
+ ucsi_check_altmodes(con);
+ ucsi_check_connector_capability(con);
+ }
+}
+
static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
{
struct typec_capability *cap = &con->typec_cap;
enum typec_accessory *accessory = cap->accessory;
- enum usb_role u_role = USB_ROLE_NONE;
u64 command;
char *name;
int ret;
@@ -1659,62 +1718,6 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
goto out;
}
- /* Get the status */
- ret = ucsi_get_connector_status(con, false);
- if (ret) {
- dev_err(ucsi->dev, "con%d: failed to get status\n", con->num);
- goto out;
- }
-
- if (ucsi->ops->connector_status)
- ucsi->ops->connector_status(con);
-
- switch (UCSI_CONSTAT(con, PARTNER_TYPE)) {
- case UCSI_CONSTAT_PARTNER_TYPE_UFP:
- case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
- u_role = USB_ROLE_HOST;
- fallthrough;
- case UCSI_CONSTAT_PARTNER_TYPE_CABLE:
- typec_set_data_role(con->port, TYPEC_HOST);
- break;
- case UCSI_CONSTAT_PARTNER_TYPE_DFP:
- u_role = USB_ROLE_DEVICE;
- typec_set_data_role(con->port, TYPEC_DEVICE);
- break;
- default:
- break;
- }
-
- /* Check if there is already something connected */
- if (UCSI_CONSTAT(con, CONNECTED)) {
- typec_set_pwr_role(con->port, UCSI_CONSTAT(con, PWR_DIR));
- ucsi_register_partner(con);
- ucsi_pwr_opmode_change(con);
- ucsi_port_psy_changed(con);
- if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE)
- ucsi_get_partner_identity(con);
- if (con->ucsi->cap.features & UCSI_CAP_CABLE_DETAILS)
- ucsi_check_cable(con);
- }
-
- /* Only notify USB controller if partner supports USB data */
- if (!(UCSI_CONSTAT(con, PARTNER_FLAG_USB)))
- u_role = USB_ROLE_NONE;
-
- ret = usb_role_switch_set_role(con->usb_role_sw, u_role);
- if (ret) {
- dev_err(ucsi->dev, "con:%d: failed to set usb role:%d\n",
- con->num, u_role);
- ret = 0;
- }
-
- if (con->partner && UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) {
- ucsi_register_device_pdos(con);
- ucsi_get_src_pdos(con);
- ucsi_check_altmodes(con);
- ucsi_check_connector_capability(con);
- }
-
trace_ucsi_register_port(con->num, con);
out:
@@ -1823,16 +1826,28 @@ static int ucsi_init(struct ucsi *ucsi)
goto err_unregister;
}
+ /* Delay other interactions with each connector until ucsi_init_port is done */
+ for (i = 0; i < ucsi->cap.num_connectors; i++)
+ mutex_lock(&connector[i].lock);
+
/* Enable all supported notifications */
ntfy = ucsi_get_supported_notifications(ucsi);
command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
ret = ucsi_send_command(ucsi, command, NULL, 0);
- if (ret < 0)
+ if (ret < 0) {
+ for (i = 0; i < ucsi->cap.num_connectors; i++)
+ mutex_unlock(&connector[i].lock);
goto err_unregister;
+ }
ucsi->connector = connector;
ucsi->ntfy = ntfy;
+ for (i = 0; i < ucsi->cap.num_connectors; i++) {
+ ucsi_init_port(ucsi, &connector[i]);
+ mutex_unlock(&connector[i].lock);
+ }
+
mutex_lock(&ucsi->ppm_lock);
ret = ucsi->ops->read_cci(ucsi, &cci);
mutex_unlock(&ucsi->ppm_lock);
---
base-commit: 2061f18ad76ecaddf8ed17df81b8611ea88dbddd
change-id: 20251117-ucsi-c2dfe8c006d7
Best regards,
--
Hsin-Te Yuan <yuanhsinte@chromium.org>
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v6] usb: typec: ucsi: Get connector status after enable notifications
2025-12-05 7:07 [PATCH v6] usb: typec: ucsi: Get connector status after enable notifications Hsin-Te Yuan
@ 2025-12-05 10:51 ` Heikki Krogerus
2025-12-17 13:53 ` Greg Kroah-Hartman
1 sibling, 0 replies; 3+ messages in thread
From: Heikki Krogerus @ 2025-12-05 10:51 UTC (permalink / raw)
To: Hsin-Te Yuan
Cc: Greg Kroah-Hartman, Guenter Roeck, linux-usb, linux-kernel,
stable
Fri, Dec 05, 2025 at 03:07:46PM +0800, Hsin-Te Yuan kirjoitti:
> Originally, the notification for connector change will be enabled after
> the first read of the connector status. Therefore, if the event happens
> during this window, it will be missing and make the status unsynced.
>
> Get the connector status only after enabling the notification for
> connector change to ensure the status is synced.
>
> Fixes: c1b0bc2dabfa ("usb: typec: Add support for UCSI interface")
> Cc: stable@vger.kernel.org # v4.13+
> Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> Changes in v6:
> - Free the locks in error path.
> - Link to v5: https://lore.kernel.org/r/20251205-ucsi-v5-1-488eb89bc9b8@chromium.org
>
> Changes in v5:
> - Hold the lock of each connector during the initialization to avoid
> race condition between initialization and other event handler
> - Add Fixes tag
> - Link to v4: https://lore.kernel.org/r/20251125-ucsi-v4-1-8c94568ddaa5@chromium.org
>
> Changes in v4:
> - Handle a single connector in ucsi_init_port() and call it in a loop
> - Link to v3: https://lore.kernel.org/r/20251121-ucsi-v3-1-b1047ca371b8@chromium.org
>
> Changes in v3:
> - Seperate the status checking part into a new function called
> ucsi_init_port() and call it after enabling the notifications
> - Link to v2: https://lore.kernel.org/r/20251118-ucsi-v2-1-d314d50333e2@chromium.org
>
> Changes in v2:
> - Remove unnecessary braces.
> - Link to v1: https://lore.kernel.org/r/20251117-ucsi-v1-1-1dcbc5ea642b@chromium.org
> ---
> drivers/usb/typec/ucsi/ucsi.c | 131 +++++++++++++++++++++++-------------------
> 1 file changed, 73 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 3f568f790f39b0271667e80816270274b8dd3008..3a0471fa4cc980c0512bc71776e3984e6cd2cdb7 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1560,11 +1560,70 @@ static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con)
> return NULL;
> }
>
> +static void ucsi_init_port(struct ucsi *ucsi, struct ucsi_connector *con)
> +{
> + enum usb_role u_role = USB_ROLE_NONE;
> + int ret;
> +
> + /* Get the status */
> + ret = ucsi_get_connector_status(con, false);
> + if (ret) {
> + dev_err(ucsi->dev, "con%d: failed to get status\n", con->num);
> + return;
> + }
> +
> + if (ucsi->ops->connector_status)
> + ucsi->ops->connector_status(con);
> +
> + switch (UCSI_CONSTAT(con, PARTNER_TYPE)) {
> + case UCSI_CONSTAT_PARTNER_TYPE_UFP:
> + case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
> + u_role = USB_ROLE_HOST;
> + fallthrough;
> + case UCSI_CONSTAT_PARTNER_TYPE_CABLE:
> + typec_set_data_role(con->port, TYPEC_HOST);
> + break;
> + case UCSI_CONSTAT_PARTNER_TYPE_DFP:
> + u_role = USB_ROLE_DEVICE;
> + typec_set_data_role(con->port, TYPEC_DEVICE);
> + break;
> + default:
> + break;
> + }
> +
> + /* Check if there is already something connected */
> + if (UCSI_CONSTAT(con, CONNECTED)) {
> + typec_set_pwr_role(con->port, UCSI_CONSTAT(con, PWR_DIR));
> + ucsi_register_partner(con);
> + ucsi_pwr_opmode_change(con);
> + ucsi_port_psy_changed(con);
> + if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE)
> + ucsi_get_partner_identity(con);
> + if (con->ucsi->cap.features & UCSI_CAP_CABLE_DETAILS)
> + ucsi_check_cable(con);
> + }
> +
> + /* Only notify USB controller if partner supports USB data */
> + if (!(UCSI_CONSTAT(con, PARTNER_FLAG_USB)))
> + u_role = USB_ROLE_NONE;
> +
> + ret = usb_role_switch_set_role(con->usb_role_sw, u_role);
> + if (ret)
> + dev_err(ucsi->dev, "con:%d: failed to set usb role:%d\n",
> + con->num, u_role);
> +
> + if (con->partner && UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) {
> + ucsi_register_device_pdos(con);
> + ucsi_get_src_pdos(con);
> + ucsi_check_altmodes(con);
> + ucsi_check_connector_capability(con);
> + }
> +}
> +
> static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
> {
> struct typec_capability *cap = &con->typec_cap;
> enum typec_accessory *accessory = cap->accessory;
> - enum usb_role u_role = USB_ROLE_NONE;
> u64 command;
> char *name;
> int ret;
> @@ -1659,62 +1718,6 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con)
> goto out;
> }
>
> - /* Get the status */
> - ret = ucsi_get_connector_status(con, false);
> - if (ret) {
> - dev_err(ucsi->dev, "con%d: failed to get status\n", con->num);
> - goto out;
> - }
> -
> - if (ucsi->ops->connector_status)
> - ucsi->ops->connector_status(con);
> -
> - switch (UCSI_CONSTAT(con, PARTNER_TYPE)) {
> - case UCSI_CONSTAT_PARTNER_TYPE_UFP:
> - case UCSI_CONSTAT_PARTNER_TYPE_CABLE_AND_UFP:
> - u_role = USB_ROLE_HOST;
> - fallthrough;
> - case UCSI_CONSTAT_PARTNER_TYPE_CABLE:
> - typec_set_data_role(con->port, TYPEC_HOST);
> - break;
> - case UCSI_CONSTAT_PARTNER_TYPE_DFP:
> - u_role = USB_ROLE_DEVICE;
> - typec_set_data_role(con->port, TYPEC_DEVICE);
> - break;
> - default:
> - break;
> - }
> -
> - /* Check if there is already something connected */
> - if (UCSI_CONSTAT(con, CONNECTED)) {
> - typec_set_pwr_role(con->port, UCSI_CONSTAT(con, PWR_DIR));
> - ucsi_register_partner(con);
> - ucsi_pwr_opmode_change(con);
> - ucsi_port_psy_changed(con);
> - if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE)
> - ucsi_get_partner_identity(con);
> - if (con->ucsi->cap.features & UCSI_CAP_CABLE_DETAILS)
> - ucsi_check_cable(con);
> - }
> -
> - /* Only notify USB controller if partner supports USB data */
> - if (!(UCSI_CONSTAT(con, PARTNER_FLAG_USB)))
> - u_role = USB_ROLE_NONE;
> -
> - ret = usb_role_switch_set_role(con->usb_role_sw, u_role);
> - if (ret) {
> - dev_err(ucsi->dev, "con:%d: failed to set usb role:%d\n",
> - con->num, u_role);
> - ret = 0;
> - }
> -
> - if (con->partner && UCSI_CONSTAT(con, PWR_OPMODE) == UCSI_CONSTAT_PWR_OPMODE_PD) {
> - ucsi_register_device_pdos(con);
> - ucsi_get_src_pdos(con);
> - ucsi_check_altmodes(con);
> - ucsi_check_connector_capability(con);
> - }
> -
> trace_ucsi_register_port(con->num, con);
>
> out:
> @@ -1823,16 +1826,28 @@ static int ucsi_init(struct ucsi *ucsi)
> goto err_unregister;
> }
>
> + /* Delay other interactions with each connector until ucsi_init_port is done */
> + for (i = 0; i < ucsi->cap.num_connectors; i++)
> + mutex_lock(&connector[i].lock);
> +
> /* Enable all supported notifications */
> ntfy = ucsi_get_supported_notifications(ucsi);
> command = UCSI_SET_NOTIFICATION_ENABLE | ntfy;
> ret = ucsi_send_command(ucsi, command, NULL, 0);
> - if (ret < 0)
> + if (ret < 0) {
> + for (i = 0; i < ucsi->cap.num_connectors; i++)
> + mutex_unlock(&connector[i].lock);
> goto err_unregister;
> + }
>
> ucsi->connector = connector;
> ucsi->ntfy = ntfy;
>
> + for (i = 0; i < ucsi->cap.num_connectors; i++) {
> + ucsi_init_port(ucsi, &connector[i]);
> + mutex_unlock(&connector[i].lock);
> + }
> +
> mutex_lock(&ucsi->ppm_lock);
> ret = ucsi->ops->read_cci(ucsi, &cci);
> mutex_unlock(&ucsi->ppm_lock);
>
> ---
> base-commit: 2061f18ad76ecaddf8ed17df81b8611ea88dbddd
> change-id: 20251117-ucsi-c2dfe8c006d7
>
> Best regards,
> --
> Hsin-Te Yuan <yuanhsinte@chromium.org>
--
heikki
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v6] usb: typec: ucsi: Get connector status after enable notifications
2025-12-05 7:07 [PATCH v6] usb: typec: ucsi: Get connector status after enable notifications Hsin-Te Yuan
2025-12-05 10:51 ` Heikki Krogerus
@ 2025-12-17 13:53 ` Greg Kroah-Hartman
1 sibling, 0 replies; 3+ messages in thread
From: Greg Kroah-Hartman @ 2025-12-17 13:53 UTC (permalink / raw)
To: Hsin-Te Yuan
Cc: Heikki Krogerus, Guenter Roeck, linux-usb, linux-kernel, stable
On Fri, Dec 05, 2025 at 03:07:46PM +0800, Hsin-Te Yuan wrote:
> Originally, the notification for connector change will be enabled after
> the first read of the connector status. Therefore, if the event happens
> during this window, it will be missing and make the status unsynced.
>
> Get the connector status only after enabling the notification for
> connector change to ensure the status is synced.
>
> Fixes: c1b0bc2dabfa ("usb: typec: Add support for UCSI interface")
> Cc: stable@vger.kernel.org # v4.13+
> Signed-off-by: Hsin-Te Yuan <yuanhsinte@chromium.org>
> ---
> Changes in v6:
> - Free the locks in error path.
> - Link to v5: https://lore.kernel.org/r/20251205-ucsi-v5-1-488eb89bc9b8@chromium.org
This does not apply to 6.19-rc1 at all, please rebase and resend.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-12-17 13:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-05 7:07 [PATCH v6] usb: typec: ucsi: Get connector status after enable notifications Hsin-Te Yuan
2025-12-05 10:51 ` Heikki Krogerus
2025-12-17 13:53 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).