| 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: <aSQtBhxw5KjR-ySZ@kuha>
Date: Mon, 24 Nov 2025 12:01:42 +0200
From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To: Hsin-Te Yuan <yuanhsinte@...omium.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] usb: typec: ucsi: Get connector status after enable
notifications
Fri, Nov 21, 2025 at 08:33:48PM +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
> conenctor change to ensure the status is synced.
>
> Signed-off-by: Hsin-Te Yuan <yuanhsinte@...omium.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 | 124 +++++++++++++++++++++++-------------------
> 1 file changed, 67 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 3f568f790f39b0271667e80816270274b8dd3008..fbda94bfd5ed12ad8cfaedceb47ad15fee271378 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1560,11 +1560,75 @@ static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con)
> return NULL;
> }
>
> +static void ucsi_init_port(struct ucsi *ucsi)
> +{
> + enum usb_role u_role;
> + int ret, i;
> + struct ucsi_connector *con;
> +
> + for (i = 0; i < ucsi->cap.num_connectors; i++) {
I would prefer that we handle a single connector in this function.
Just call it in a loop. That way we may be able to reuse this if
needed.
But if you still prefer it like this, then at least rename it to
ucsi_init_ports (so plural).
thanks,
> + u_role = USB_ROLE_NONE;
> + con = &ucsi->connector[i];
> + /* 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);
> + continue;
> + }
> +
> + 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 +1723,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:
> @@ -1833,6 +1841,8 @@ static int ucsi_init(struct ucsi *ucsi)
> ucsi->connector = connector;
> ucsi->ntfy = ntfy;
>
> + ucsi_init_port(ucsi);
> +
> mutex_lock(&ucsi->ppm_lock);
> ret = ucsi->ops->read_cci(ucsi, &cci);
> mutex_unlock(&ucsi->ppm_lock);
>
> ---
> base-commit: 6a23ae0a96a600d1d12557add110e0bb6e32730c
> change-id: 20251117-ucsi-c2dfe8c006d7
>
> Best regards,
> --
> Hsin-Te Yuan <yuanhsinte@...omium.org>
--
heikki
Powered by blists - more mailing lists