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
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 <[email protected]>
Signed-off-by: Jameson Thies <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Jameson Thies <[email protected]>
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
>
Hi Jameson,
On Thu, Mar 14, 2024 at 4:55 PM Jameson Thies <[email protected]> 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
Hey Benson,
thank you for the feedback. I'll follow up with a version 2 of this
patch addressing these comments.
Thanks,
Jameson