| Message ID | 20240314235554.90079-2-jthies@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: Check capabilities before discovery | expand |
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 >
Hey Benson, thank you for the feedback. I'll follow up with a version 2 of this patch addressing these comments. Thanks, Jameson
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;
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(-)