| Message ID | 20240315171836.343830-2-jthies@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: typec: ucsi: Check capabilities before discovery | expand |
On 15/03/2024 18:18, 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. Additionally, > move 8 bits from reserved_1 to features in the ucsi_capability struct. This > makes the field 16 bits, still 8 short of the 24 bits allocated for it in > UCSI v3.0, but it will not overflow because UCSI only defines 14 bits in > bmOptionalFeatures. > > Fixes: 38ca416597b0 ("usb: typec: ucsi: Register cables based on GET_CABLE_PROPERTY") > Link: https://lore.kernel.org/linux-usb/44e8142f-d9b3-487b-83fe-39deadddb492@linaro.org > 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. > > 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; LGTM Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-QRD Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD Thanks, Neil
On Fri, Mar 15, 2024 at 05:18:35PM +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. Additionally, > move 8 bits from reserved_1 to features in the ucsi_capability struct. This > makes the field 16 bits, still 8 short of the 24 bits allocated for it in > UCSI v3.0, but it will not overflow because UCSI only defines 14 bits in > bmOptionalFeatures. > > Fixes: 38ca416597b0 ("usb: typec: ucsi: Register cables based on GET_CABLE_PROPERTY") > Link: https://lore.kernel.org/linux-usb/44e8142f-d9b3-487b-83fe-39deadddb492@linaro.org > Suggested-by: Neil Armstrong <neil.armstrong@linaro.org> > Signed-off-by: Jameson Thies <jthies@google.com> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > Confirmed a device which supports GET_PD_MESSAGE, GET_CABLE_PROPERTY and > GET_ALTERNATE_MODES still requested identity and cable information. > > 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
Hi Greg, On 15/03/2024 18:18, 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. Additionally, > move 8 bits from reserved_1 to features in the ucsi_capability struct. This > makes the field 16 bits, still 8 short of the 24 bits allocated for it in > UCSI v3.0, but it will not overflow because UCSI only defines 14 bits in > bmOptionalFeatures. > > Fixes: 38ca416597b0 ("usb: typec: ucsi: Register cables based on GET_CABLE_PROPERTY") > Link: https://lore.kernel.org/linux-usb/44e8142f-d9b3-487b-83fe-39deadddb492@linaro.org > Suggested-by: Neil Armstrong <neil.armstrong@linaro.org> > Signed-off-by: Jameson Thies <jthies@google.com> Could you queue this for v6.9-rc2 ? So far most of the recent Qualcomm boards are waiting this fix to be unbroken. Thanks, Neil > --- > Confirmed a device which supports GET_PD_MESSAGE, GET_CABLE_PROPERTY and > GET_ALTERNATE_MODES still requested identity and cable information. > > 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;
On Fri, Mar 15, 2024 at 05:18:35PM +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. Additionally, > move 8 bits from reserved_1 to features in the ucsi_capability struct. This > makes the field 16 bits, still 8 short of the 24 bits allocated for it in > UCSI v3.0, but it will not overflow because UCSI only defines 14 bits in > bmOptionalFeatures. > > Fixes: 38ca416597b0 ("usb: typec: ucsi: Register cables based on GET_CABLE_PROPERTY") > Link: https://lore.kernel.org/linux-usb/44e8142f-d9b3-487b-83fe-39deadddb492@linaro.org > Suggested-by: Neil Armstrong <neil.armstrong@linaro.org> > Signed-off-by: Jameson Thies <jthies@google.com> Thanks you, Jameson, this resolve the regression I ran into on qcs6490-rb3gen2 as well. Tested-by: Bjorn Andersson <quic_bjorande@quicinc.com> # QCS6490 Rb3Gen2 Regards, Bjorn
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. Additionally, move 8 bits from reserved_1 to features in the ucsi_capability struct. This makes the field 16 bits, still 8 short of the 24 bits allocated for it in UCSI v3.0, but it will not overflow because UCSI only defines 14 bits in bmOptionalFeatures. Fixes: 38ca416597b0 ("usb: typec: ucsi: Register cables based on GET_CABLE_PROPERTY") Link: https://lore.kernel.org/linux-usb/44e8142f-d9b3-487b-83fe-39deadddb492@linaro.org 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. drivers/usb/typec/ucsi/ucsi.c | 34 +++++++++++++++++++++------------- drivers/usb/typec/ucsi/ucsi.h | 5 +++-- 2 files changed, 24 insertions(+), 15 deletions(-)