* [PATCH v1 0/1] usb: typec: ucsi: Check capabilities before discovery
@ 2024-03-14 23:55 Jameson Thies
2024-03-14 23:55 ` [PATCH v1 1/1] usb: typec: ucsi: Check capabilities before cable and identity discovery Jameson Thies
2024-03-15 0:29 ` [PATCH v1 0/1] usb: typec: ucsi: Check capabilities before discovery Prashant Malani
0 siblings, 2 replies; 5+ messages in thread
From: Jameson Thies @ 2024-03-14 23:55 UTC (permalink / raw)
To: heikki.krogerus, linux-usb
Cc: jthies, pmalani, bleung, abhishekpandit, andersson,
dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede,
neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel
Hi Heikki,
This patch updates the UCSI driver to better check the appropriate
capability bit before sending a UCSI command to discovery cable and
identity information.
These have been tested on a the usb-testing branch merged with a
chromeOS 6.8-rc2 kernel. Let me know if you have any questions.
Thanks,
Jameson
Jameson Thies (1):
usb: typec: ucsi: Check capabilities before cable and identity
discovery
drivers/usb/typec/ucsi/ucsi.c | 34 +++++++++++++++++++++-------------
drivers/usb/typec/ucsi/ucsi.h | 5 +++--
2 files changed, 24 insertions(+), 15 deletions(-)
base-commit: d99e42ce6b8341d3f09e22c6706461ec900fe172
--
2.44.0.291.gc1ea87d7ee-goog
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v1 1/1] usb: typec: ucsi: Check capabilities before cable and identity discovery 2024-03-14 23:55 [PATCH v1 0/1] usb: typec: ucsi: Check capabilities before discovery Jameson Thies @ 2024-03-14 23:55 ` Jameson Thies 2024-03-15 0:15 ` Benson Leung 2024-03-15 0:29 ` [PATCH v1 0/1] usb: typec: ucsi: Check capabilities before discovery Prashant Malani 1 sibling, 1 reply; 5+ messages in thread From: Jameson Thies @ 2024-03-14 23:55 UTC (permalink / raw) To: heikki.krogerus, linux-usb Cc: jthies, pmalani, bleung, abhishekpandit, andersson, dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede, neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel Check the UCSI_CAP_GET_PD_MESSAGE bit before sending GET_PD_MESSAGE to discover partner and cable identity, check UCSI_CAP_CABLE_DETAILS before sending GET_CABLE_PROPERTY to discover the cable and check UCSI_CAP_ALT_MODE_DETAILS before registering the a cable plug. Suggested-by: Neil Armstrong <neil.armstrong@linaro.org> Signed-off-by: Jameson Thies <jthies@google.com> --- Confirmed a device which supports GET_PD_MESSAGE, GET_CABLE_PROPERTY and GET_ALTERNATE_MODES still requested identity and cable information. This moves 8 bits from "reserved_1" to "features" in the ucsi_capability struct. Really, this should be 24 bits to reflect the field size in UCSI. But, as of UCSI 3.0, this will not overflow becasue the bmOptionalFeatures description only defines 14 bits. drivers/usb/typec/ucsi/ucsi.c | 34 +++++++++++++++++++++------------- drivers/usb/typec/ucsi/ucsi.h | 5 +++-- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index cf52cb34d2859..958dc82989b60 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -1133,17 +1133,21 @@ static int ucsi_check_cable(struct ucsi_connector *con) if (ret < 0) return ret; - ret = ucsi_get_cable_identity(con); - if (ret < 0) - return ret; + if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE) { + ret = ucsi_get_cable_identity(con); + if (ret < 0) + return ret; + } - ret = ucsi_register_plug(con); - if (ret < 0) - return ret; + if (con->ucsi->cap.features & UCSI_CAP_ALT_MODE_DETAILS) { + ret = ucsi_register_plug(con); + if (ret < 0) + return ret; - ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP_P); - if (ret < 0) - return ret; + ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP_P); + if (ret < 0) + return ret; + } return 0; } @@ -1189,8 +1193,10 @@ static void ucsi_handle_connector_change(struct work_struct *work) ucsi_register_partner(con); ucsi_partner_task(con, ucsi_check_connection, 1, HZ); ucsi_partner_task(con, ucsi_check_connector_capability, 1, HZ); - ucsi_partner_task(con, ucsi_get_partner_identity, 1, HZ); - ucsi_partner_task(con, ucsi_check_cable, 1, HZ); + if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE) + ucsi_partner_task(con, ucsi_get_partner_identity, 1, HZ); + if (con->ucsi->cap.features & UCSI_CAP_CABLE_DETAILS) + ucsi_partner_task(con, ucsi_check_cable, 1, HZ); if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == UCSI_CONSTAT_PWR_OPMODE_PD) @@ -1589,8 +1595,10 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) ucsi_register_partner(con); ucsi_pwr_opmode_change(con); ucsi_port_psy_changed(con); - ucsi_get_partner_identity(con); - ucsi_check_cable(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 */ diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h index 32daf5f586505..0e7c92eb1b227 100644 --- a/drivers/usb/typec/ucsi/ucsi.h +++ b/drivers/usb/typec/ucsi/ucsi.h @@ -206,7 +206,7 @@ struct ucsi_capability { #define UCSI_CAP_ATTR_POWER_OTHER BIT(10) #define UCSI_CAP_ATTR_POWER_VBUS BIT(14) u8 num_connectors; - u8 features; + u16 features; #define UCSI_CAP_SET_UOM BIT(0) #define UCSI_CAP_SET_PDM BIT(1) #define UCSI_CAP_ALT_MODE_DETAILS BIT(2) @@ -215,7 +215,8 @@ struct ucsi_capability { #define UCSI_CAP_CABLE_DETAILS BIT(5) #define UCSI_CAP_EXT_SUPPLY_NOTIFICATIONS BIT(6) #define UCSI_CAP_PD_RESET BIT(7) - u16 reserved_1; +#define UCSI_CAP_GET_PD_MESSAGE BIT(8) + u8 reserved_1; u8 num_alt_modes; u8 reserved_2; u16 bc_version; -- 2.44.0.291.gc1ea87d7ee-goog ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v1 1/1] usb: typec: ucsi: Check capabilities before cable and identity discovery 2024-03-14 23:55 ` [PATCH v1 1/1] usb: typec: ucsi: Check capabilities before cable and identity discovery Jameson Thies @ 2024-03-15 0:15 ` Benson Leung 2024-03-15 16:35 ` Jameson Thies 0 siblings, 1 reply; 5+ messages in thread From: Benson Leung @ 2024-03-15 0:15 UTC (permalink / raw) To: Jameson Thies Cc: heikki.krogerus, linux-usb, pmalani, abhishekpandit, andersson, dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede, neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4715 bytes --] Hi Jameson, On Thu, Mar 14, 2024 at 11:55:54PM +0000, Jameson Thies wrote: > Check the UCSI_CAP_GET_PD_MESSAGE bit before sending GET_PD_MESSAGE to > discover partner and cable identity, check UCSI_CAP_CABLE_DETAILS before > sending GET_CABLE_PROPERTY to discover the cable and check > UCSI_CAP_ALT_MODE_DETAILS before registering the a cable plug. > > Suggested-by: Neil Armstrong <neil.armstrong@linaro.org> > Signed-off-by: Jameson Thies <jthies@google.com> Since Neil pointed out that this error appeared starting in 38ca416597b0, I think it would be appropriate to tag this commit with a Fixes: tag. See here on instructions how to describe a Fixes: https://www.kernel.org/doc/html/v4.10/process/submitting-patches.html#describe-changes > --- > Confirmed a device which supports GET_PD_MESSAGE, GET_CABLE_PROPERTY and > GET_ALTERNATE_MODES still requested identity and cable information. > > This moves 8 bits from "reserved_1" to "features" in the ucsi_capability > struct. Really, this should be 24 bits to reflect the field size in UCSI. > But, as of UCSI 3.0, this will not overflow becasue the bmOptionalFeatures > description only defines 14 bits. Are you sure you wanted to include this information below the --- ? This won't be incorporated into the commit message when this is merged. Thanks, Benson > > drivers/usb/typec/ucsi/ucsi.c | 34 +++++++++++++++++++++------------- > drivers/usb/typec/ucsi/ucsi.h | 5 +++-- > 2 files changed, 24 insertions(+), 15 deletions(-) > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index cf52cb34d2859..958dc82989b60 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -1133,17 +1133,21 @@ static int ucsi_check_cable(struct ucsi_connector *con) > if (ret < 0) > return ret; > > - ret = ucsi_get_cable_identity(con); > - if (ret < 0) > - return ret; > + if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE) { > + ret = ucsi_get_cable_identity(con); > + if (ret < 0) > + return ret; > + } > > - ret = ucsi_register_plug(con); > - if (ret < 0) > - return ret; > + if (con->ucsi->cap.features & UCSI_CAP_ALT_MODE_DETAILS) { > + ret = ucsi_register_plug(con); > + if (ret < 0) > + return ret; > > - ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP_P); > - if (ret < 0) > - return ret; > + ret = ucsi_register_altmodes(con, UCSI_RECIPIENT_SOP_P); > + if (ret < 0) > + return ret; > + } > > return 0; > } > @@ -1189,8 +1193,10 @@ static void ucsi_handle_connector_change(struct work_struct *work) > ucsi_register_partner(con); > ucsi_partner_task(con, ucsi_check_connection, 1, HZ); > ucsi_partner_task(con, ucsi_check_connector_capability, 1, HZ); > - ucsi_partner_task(con, ucsi_get_partner_identity, 1, HZ); > - ucsi_partner_task(con, ucsi_check_cable, 1, HZ); > + if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE) > + ucsi_partner_task(con, ucsi_get_partner_identity, 1, HZ); > + if (con->ucsi->cap.features & UCSI_CAP_CABLE_DETAILS) > + ucsi_partner_task(con, ucsi_check_cable, 1, HZ); > > if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == > UCSI_CONSTAT_PWR_OPMODE_PD) > @@ -1589,8 +1595,10 @@ static int ucsi_register_port(struct ucsi *ucsi, struct ucsi_connector *con) > ucsi_register_partner(con); > ucsi_pwr_opmode_change(con); > ucsi_port_psy_changed(con); > - ucsi_get_partner_identity(con); > - ucsi_check_cable(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 */ > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > index 32daf5f586505..0e7c92eb1b227 100644 > --- a/drivers/usb/typec/ucsi/ucsi.h > +++ b/drivers/usb/typec/ucsi/ucsi.h > @@ -206,7 +206,7 @@ struct ucsi_capability { > #define UCSI_CAP_ATTR_POWER_OTHER BIT(10) > #define UCSI_CAP_ATTR_POWER_VBUS BIT(14) > u8 num_connectors; > - u8 features; > + u16 features; > #define UCSI_CAP_SET_UOM BIT(0) > #define UCSI_CAP_SET_PDM BIT(1) > #define UCSI_CAP_ALT_MODE_DETAILS BIT(2) > @@ -215,7 +215,8 @@ struct ucsi_capability { > #define UCSI_CAP_CABLE_DETAILS BIT(5) > #define UCSI_CAP_EXT_SUPPLY_NOTIFICATIONS BIT(6) > #define UCSI_CAP_PD_RESET BIT(7) > - u16 reserved_1; > +#define UCSI_CAP_GET_PD_MESSAGE BIT(8) > + u8 reserved_1; > u8 num_alt_modes; > u8 reserved_2; > u16 bc_version; > -- > 2.44.0.291.gc1ea87d7ee-goog > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 1/1] usb: typec: ucsi: Check capabilities before cable and identity discovery 2024-03-15 0:15 ` Benson Leung @ 2024-03-15 16:35 ` Jameson Thies 0 siblings, 0 replies; 5+ messages in thread From: Jameson Thies @ 2024-03-15 16:35 UTC (permalink / raw) To: Benson Leung Cc: heikki.krogerus, linux-usb, pmalani, abhishekpandit, andersson, dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede, neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel Hey Benson, thank you for the feedback. I'll follow up with a version 2 of this patch addressing these comments. Thanks, Jameson ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 0/1] usb: typec: ucsi: Check capabilities before discovery 2024-03-14 23:55 [PATCH v1 0/1] usb: typec: ucsi: Check capabilities before discovery Jameson Thies 2024-03-14 23:55 ` [PATCH v1 1/1] usb: typec: ucsi: Check capabilities before cable and identity discovery Jameson Thies @ 2024-03-15 0:29 ` Prashant Malani 1 sibling, 0 replies; 5+ messages in thread From: Prashant Malani @ 2024-03-15 0:29 UTC (permalink / raw) To: Jameson Thies Cc: heikki.krogerus, linux-usb, bleung, abhishekpandit, andersson, dmitry.baryshkov, fabrice.gasnier, gregkh, hdegoede, neil.armstrong, rajaram.regupathy, saranya.gopal, linux-kernel Hi Jameson, On Thu, Mar 14, 2024 at 4:55 PM Jameson Thies <jthies@google.com> wrote: > > Hi Heikki, > > This patch updates the UCSI driver to better check the appropriate > capability bit before sending a UCSI command to discovery cable and > identity information. > > These have been tested on a the usb-testing branch merged with a > chromeOS 6.8-rc2 kernel. Let me know if you have any questions. > > Thanks, > Jameson > > Jameson Thies (1): > usb: typec: ucsi: Check capabilities before cable and identity > discovery Just something to keep in mind for future patches: it's OK to upload single patch submissions without a cover letter. BR, -Prashant ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-03-15 16:35 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-14 23:55 [PATCH v1 0/1] usb: typec: ucsi: Check capabilities before discovery Jameson Thies 2024-03-14 23:55 ` [PATCH v1 1/1] usb: typec: ucsi: Check capabilities before cable and identity discovery Jameson Thies 2024-03-15 0:15 ` Benson Leung 2024-03-15 16:35 ` Jameson Thies 2024-03-15 0:29 ` [PATCH v1 0/1] usb: typec: ucsi: Check capabilities before discovery Prashant Malani
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.