| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aTK5O0PhQ6AD5zrI@kuha>
Date: Fri, 5 Dec 2025 12:51:39 +0200
From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To: Hsin-Te Yuan <yuanhsinte@...omium.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Guenter Roeck <linux@...ck-us.net>, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v6] usb: typec: ucsi: Get connector status after enable
notifications
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@...r.kernel.org # v4.13+
> Signed-off-by: Hsin-Te Yuan <yuanhsinte@...omium.org>
Reviewed-by: Heikki Krogerus <heikki.krogerus@...ux.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@...omium.org>
--
heikki
Powered by blists - more mailing lists