[PATCH v2 0/6] usb: typec: ucsi: Minor improvements

[PATCH v2 0/6] usb: typec: ucsi: Minor improvements

[Heikki Krogerus]

Hi,

The first three patches remove potential problems. I prepared
especially the second one as a protection against similar issues that
I had to fix earlier. The rest is just cleanups.

What changed since v1:

In this version I refactored the code so that UCSI_MAX_DATA_LENGTH()
is introduced in the first patch where it is used.

I also added one more patch, the first one, where I remove the unused
fields from struct ucsi_connector_status. Those fields are beyond the
MAX_DATA_LENGTH (16 bytes) with the older UCSI versions, so without
removing them, the code would fail on older systems after the read
truncation is removed in the second patch. Thanks for reporting that
Siva.

So please test these if you have time.

The first version of these patches:
https://lore.kernel.org/linux-usb/20240815085726.2865482-1-heikki.krogerus@linux.intel.com/

thanks,


Heikki Krogerus (6):
  usb: typec: ucsi: Remove unused fields from struct
    ucsi_connector_status
  usb: typec: ucsi: Don't truncate the reads
  usb: typec: ucsi: Only assign the identity structure if the PPM
    supports it
  usb: typec: ucsi: Common function for the GET_PD_MESSAGE command
  usb: typec: ucsi: Call CANCEL from single location
  usb: typec: ucsi: Remove useless error check from ucsi_read_error()

 drivers/usb/typec/ucsi/ucsi.c | 126 ++++++++++------------------------
 drivers/usb/typec/ucsi/ucsi.h |  41 ++---------
 2 files changed, 41 insertions(+), 126 deletions(-)

-- 
2.43.0

[PATCH v2 1/6] usb: typec: ucsi: Remove unused fields from struct ucsi_connector_status

[Heikki Krogerus]

The new fields are valid only with the new UCSI versions.
They are at offsets that go beyond the MAX_DATA_LENGTH (16
bytes) with the older UCSI versions. That has not caused any
problems before because nothing uses those new fields yet.
Because they are not used yet, dropping them for now.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.h | 27 ++-------------------------
 1 file changed, 2 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 57129f3c0814..7bc132b59027 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -344,35 +344,12 @@ struct ucsi_connector_status {
 #define   UCSI_CONSTAT_PARTNER_TYPE_AUDIO	6
 	u32 request_data_obj;
 
-	u8 pwr_status[3];
-#define UCSI_CONSTAT_BC_STATUS(_p_)		((_p_[0]) & GENMASK(1, 0))
+	u8 pwr_status;
+#define UCSI_CONSTAT_BC_STATUS(_p_)		((_p_) & GENMASK(1, 0))
 #define   UCSI_CONSTAT_BC_NOT_CHARGING		0
 #define   UCSI_CONSTAT_BC_NOMINAL_CHARGING	1
 #define   UCSI_CONSTAT_BC_SLOW_CHARGING		2
 #define   UCSI_CONSTAT_BC_TRICKLE_CHARGING	3
-#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_)	(((_p_[0]) & GENMASK(5, 2)) >> 2)
-#define   UCSI_CONSTAT_CAP_PWR_LOWERED		0
-#define   UCSI_CONSTAT_CAP_PWR_BUDGET_LIMIT	1
-#define UCSI_CONSTAT_PROVIDER_PD_VERSION_OPER_MODE(_p_)	\
-	((get_unaligned_le32(_p_) & GENMASK(21, 6)) >> 6)
-#define UCSI_CONSTAT_ORIENTATION(_p_)		(((_p_[2]) & GENMASK(6, 6)) >> 6)
-#define   UCSI_CONSTAT_ORIENTATION_DIRECT	0
-#define   UCSI_CONSTAT_ORIENTATION_FLIPPED	1
-#define UCSI_CONSTAT_SINK_PATH_STATUS(_p_)	(((_p_[2]) & GENMASK(7, 7)) >> 7)
-#define   UCSI_CONSTAT_SINK_PATH_DISABLED	0
-#define   UCSI_CONSTAT_SINK_PATH_ENABLED	1
-	u8 pwr_readings[9];
-#define UCSI_CONSTAT_REV_CURR_PROT_STATUS(_p_)	((_p_[0]) & 0x1)
-#define UCSI_CONSTAT_PWR_READING_VALID(_p_)	(((_p_[0]) & GENMASK(1, 1)) >> 1)
-#define UCSI_CONSTAT_CURRENT_SCALE(_p_)		(((_p_[0]) & GENMASK(4, 2)) >> 2)
-#define UCSI_CONSTAT_PEAK_CURRENT(_p_) \
-	((get_unaligned_le32(_p_) & GENMASK(20, 5)) >> 5)
-#define UCSI_CONSTAT_AVG_CURRENT(_p_) \
-	((get_unaligned_le32(&(_p_)[2]) & GENMASK(20, 5)) >> 5)
-#define UCSI_CONSTAT_VOLTAGE_SCALE(_p_) \
-	((get_unaligned_le16(&(_p_)[4]) & GENMASK(8, 5)) >> 5)
-#define UCSI_CONSTAT_VOLTAGE_READING(_p_) \
-	((get_unaligned_le32(&(_p_)[5]) & GENMASK(16, 1)) >> 1)
 } __packed;
 
 /*
-- 
2.43.0

[PATCH v2 2/6] usb: typec: ucsi: Don't truncate the reads

[Heikki Krogerus]

That may silently corrupt the data. Instead, failing attempts
to read more than the interface can handle.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 8 ++------
 drivers/usb/typec/ucsi/ucsi.h | 2 ++
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 4039851551c1..96ef099a6f84 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -99,12 +99,8 @@ static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
 
 	*cci = 0;
 
-	/*
-	 * Below UCSI 2.0, MESSAGE_IN was limited to 16 bytes. Truncate the
-	 * reads here.
-	 */
-	if (ucsi->version <= UCSI_VERSION_1_2)
-		size = clamp(size, 0, 16);
+	if (size > UCSI_MAX_DATA_LENGTH(ucsi))
+		return -EINVAL;
 
 	ret = ucsi->ops->sync_control(ucsi, command);
 	if (ret)
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 7bc132b59027..5e3c6cb822c8 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -412,6 +412,8 @@ struct ucsi {
 #define UCSI_DELAY_DEVICE_PDOS	BIT(1)	/* Reading PDOs fails until the parter is in PD mode */
 };
 
+#define UCSI_MAX_DATA_LENGTH(u) (((u)->version < UCSI_VERSION_2_0) ? 0x10 : 0xff)
+
 #define UCSI_MAX_SVID		5
 #define UCSI_MAX_ALTMODES	(UCSI_MAX_SVID * 6)
 
-- 
2.43.0

[PATCH v2 3/6] usb: typec: ucsi: Only assign the identity structure if the PPM supports it

[Heikki Krogerus]

This will make sure that the identity sysfs attribute files
are kept hidden if the UCSI interface doesn't support
reading the USB Power Delivery messages.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 96ef099a6f84..1f6e3f0d25c1 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -979,7 +979,8 @@ static int ucsi_register_cable(struct ucsi_connector *con)
 		break;
 	}
 
-	desc.identity = &con->cable_identity;
+	if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE)
+		desc.identity = &con->cable_identity;
 	desc.active = !!(UCSI_CABLE_PROP_FLAG_ACTIVE_CABLE &
 			 con->cable_prop.flags);
 	desc.pd_revision = UCSI_CABLE_PROP_FLAG_PD_MAJOR_REV_AS_BCD(
@@ -1058,7 +1059,8 @@ static int ucsi_register_partner(struct ucsi_connector *con)
 	if (pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD)
 		ucsi_register_device_pdos(con);
 
-	desc.identity = &con->partner_identity;
+	if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE)
+		desc.identity = &con->partner_identity;
 	desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD;
 	desc.pd_revision = UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags);
 
-- 
2.43.0

[PATCH v2 4/6] usb: typec: ucsi: Common function for the GET_PD_MESSAGE command

[Heikki Krogerus]

So far that command was only used to read the response to
the Discover Identity Request, but it is handled with two
separate functions, which is not really necessary. Squashing
the command execution into a single function. That function
can now also be used to read other messages on top of the
Request Identity response.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 92 ++++++++++-------------------------
 drivers/usb/typec/ucsi/ucsi.h | 12 -----
 2 files changed, 27 insertions(+), 77 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 1f6e3f0d25c1..470c9532b4f2 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -748,104 +748,66 @@ static struct usb_power_delivery_capabilities *ucsi_get_pd_caps(struct ucsi_conn
 							&pd_caps);
 }
 
-static int ucsi_read_identity(struct ucsi_connector *con, u8 recipient,
-			      u8 offset, u8 bytes, void *resp)
+static int ucsi_get_pd_message(struct ucsi_connector *con, u8 recipient,
+			       size_t bytes, void *data, u8 type)
 {
-	struct ucsi *ucsi = con->ucsi;
+	size_t len = min(bytes, UCSI_MAX_DATA_LENGTH(con->ucsi));
 	u64 command;
+	u8 offset;
 	int ret;
 
-	command = UCSI_COMMAND(UCSI_GET_PD_MESSAGE) |
-	    UCSI_CONNECTOR_NUMBER(con->num);
-	command |= UCSI_GET_PD_MESSAGE_RECIPIENT(recipient);
-	command |= UCSI_GET_PD_MESSAGE_OFFSET(offset);
-	command |= UCSI_GET_PD_MESSAGE_BYTES(bytes);
-	command |= UCSI_GET_PD_MESSAGE_TYPE(UCSI_GET_PD_MESSAGE_TYPE_IDENTITY);
-
-	ret = ucsi_send_command(ucsi, command, resp, bytes);
-	if (ret < 0)
-		dev_err(ucsi->dev, "UCSI_GET_PD_MESSAGE failed (%d)\n", ret);
-
-	return ret;
-}
-
-static int ucsi_get_identity(struct ucsi_connector *con, u8 recipient,
-			      struct usb_pd_identity *id)
-{
-	struct ucsi *ucsi = con->ucsi;
-	struct ucsi_pd_message_disc_id resp = {};
-	int ret;
-
-	if (ucsi->version < UCSI_VERSION_2_0) {
-		/*
-		 * Before UCSI v2.0, MESSAGE_IN is 16 bytes which cannot fit
-		 * the 28 byte identity response including the VDM header.
-		 * First request the VDM header, ID Header VDO, Cert Stat VDO
-		 * and Product VDO.
-		 */
-		ret = ucsi_read_identity(con, recipient, 0, 0x10, &resp);
-		if (ret < 0)
-			return ret;
+	for (offset = 0; offset < bytes; offset += len) {
+		len = min(len, bytes - offset);
 
+		command = UCSI_COMMAND(UCSI_GET_PD_MESSAGE) | UCSI_CONNECTOR_NUMBER(con->num);
+		command |= UCSI_GET_PD_MESSAGE_RECIPIENT(recipient);
+		command |= UCSI_GET_PD_MESSAGE_OFFSET(offset);
+		command |= UCSI_GET_PD_MESSAGE_BYTES(len);
+		command |= UCSI_GET_PD_MESSAGE_TYPE(type);
 
-		/* Then request Product Type VDO1 through Product Type VDO3. */
-		ret = ucsi_read_identity(con, recipient, 0x10, 0xc,
-					 &resp.vdo[0]);
-		if (ret < 0)
-			return ret;
-
-	} else {
-		/*
-		 * In UCSI v2.0 and after, MESSAGE_IN is large enough to request
-		 * the large enough to request the full Discover Identity
-		 * response at once.
-		 */
-		ret = ucsi_read_identity(con, recipient, 0x0, 0x1c, &resp);
+		ret = ucsi_send_command(con->ucsi, command, data + offset, len);
 		if (ret < 0)
 			return ret;
 	}
 
-	id->id_header = resp.id_header;
-	id->cert_stat = resp.cert_stat;
-	id->product = resp.product;
-	id->vdo[0] = resp.vdo[0];
-	id->vdo[1] = resp.vdo[1];
-	id->vdo[2] = resp.vdo[2];
 	return 0;
 }
 
 static int ucsi_get_partner_identity(struct ucsi_connector *con)
 {
+	u32 vdo[7] = {};
 	int ret;
 
-	ret = ucsi_get_identity(con, UCSI_RECIPIENT_SOP,
-				 &con->partner_identity);
+	ret = ucsi_get_pd_message(con, UCSI_RECIPIENT_SOP, sizeof(vdo), vdo,
+				  UCSI_GET_PD_MESSAGE_TYPE_IDENTITY);
 	if (ret < 0)
 		return ret;
 
+	/* VDM Header is not part of struct usb_pd_identity, so dropping it. */
+	con->partner_identity = *(struct usb_pd_identity *)&vdo[1];
+
 	ret = typec_partner_set_identity(con->partner);
-	if (ret < 0) {
-		dev_err(con->ucsi->dev, "Failed to set partner identity (%d)\n",
-			ret);
-	}
+	if (ret < 0)
+		dev_err(con->ucsi->dev, "Failed to set partner identity (%d)\n", ret);
 
 	return ret;
 }
 
 static int ucsi_get_cable_identity(struct ucsi_connector *con)
 {
+	u32 vdo[7] = {};
 	int ret;
 
-	ret = ucsi_get_identity(con, UCSI_RECIPIENT_SOP_P,
-				 &con->cable_identity);
+	ret = ucsi_get_pd_message(con, UCSI_RECIPIENT_SOP_P, sizeof(vdo), vdo,
+				  UCSI_GET_PD_MESSAGE_TYPE_IDENTITY);
 	if (ret < 0)
 		return ret;
 
+	con->cable_identity = *(struct usb_pd_identity *)&vdo[1];
+
 	ret = typec_cable_set_identity(con->cable);
-	if (ret < 0) {
-		dev_err(con->ucsi->dev, "Failed to set cable identity (%d)\n",
-			ret);
-	}
+	if (ret < 0)
+		dev_err(con->ucsi->dev, "Failed to set cable identity (%d)\n", ret);
 
 	return ret;
 }
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 5e3c6cb822c8..c8c87377909d 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -352,18 +352,6 @@ struct ucsi_connector_status {
 #define   UCSI_CONSTAT_BC_TRICKLE_CHARGING	3
 } __packed;
 
-/*
- * Data structure filled by PPM in response to GET_PD_MESSAGE command with the
- * Response Message Type set to Discover Identity Response.
- */
-struct ucsi_pd_message_disc_id {
-	u32 vdm_header;
-	u32 id_header;
-	u32 cert_stat;
-	u32 product;
-	u32 vdo[3];
-} __packed;
-
 /* -------------------------------------------------------------------------- */
 
 struct ucsi_debugfs_entry {
-- 
2.43.0

[PATCH v2 5/6] usb: typec: ucsi: Call CANCEL from single location

[Heikki Krogerus]

The command cancellation can be done right after detecting
that the PPM is busy. There is no need to do it separately
in ucsi_read_error() and ucsi_send_command_common().

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 470c9532b4f2..64fe59e05b4f 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -111,7 +111,7 @@ static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
 		return ret;
 
 	if (*cci & UCSI_CCI_BUSY)
-		return -EBUSY;
+		return ucsi_run_command(ucsi, UCSI_CANCEL, cci, NULL, 0, false) ?: -EBUSY;
 
 	if (!(*cci & UCSI_CCI_COMMAND_COMPLETE))
 		return -EIO;
@@ -144,15 +144,7 @@ static int ucsi_read_error(struct ucsi *ucsi, u8 connector_num)
 	int ret;
 
 	command = UCSI_GET_ERROR_STATUS | UCSI_CONNECTOR_NUMBER(connector_num);
-	ret = ucsi_run_command(ucsi, command, &cci,
-			       &error, sizeof(error), false);
-
-	if (cci & UCSI_CCI_BUSY) {
-		ret = ucsi_run_command(ucsi, UCSI_CANCEL, &cci, NULL, 0, false);
-
-		return ret ? ret : -EBUSY;
-	}
-
+	ret = ucsi_run_command(ucsi, command, &cci, &error, sizeof(error), false);
 	if (ret < 0)
 		return ret;
 
@@ -234,9 +226,8 @@ static int ucsi_send_command_common(struct ucsi *ucsi, u64 cmd,
 	mutex_lock(&ucsi->ppm_lock);
 
 	ret = ucsi_run_command(ucsi, cmd, &cci, data, size, conn_ack);
-	if (cci & UCSI_CCI_BUSY)
-		ret = ucsi_run_command(ucsi, UCSI_CANCEL, &cci, NULL, 0, false) ?: -EBUSY;
-	else if (cci & UCSI_CCI_ERROR)
+
+	if (cci & UCSI_CCI_ERROR)
 		ret = ucsi_read_error(ucsi, connector_num);
 
 	mutex_unlock(&ucsi->ppm_lock);
-- 
2.43.0

[PATCH v2 6/6] usb: typec: ucsi: Remove useless error check from ucsi_read_error()

[Heikki Krogerus]

If the GET_ERROR_STATUS command fails, ucsi_read_error() can
not reach the condition where the CCI error bit is checked,
because ucsi_run_command() has already checked that bit and
returned an error.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 64fe59e05b4f..927007230cb8 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -148,9 +148,6 @@ static int ucsi_read_error(struct ucsi *ucsi, u8 connector_num)
 	if (ret < 0)
 		return ret;
 
-	if (cci & UCSI_CCI_ERROR)
-		return -EIO;
-
 	switch (error) {
 	case UCSI_ERROR_INCOMPATIBLE_PARTNER:
 		return -EOPNOTSUPP;
-- 
2.43.0

Re: [PATCH v2 2/6] usb: typec: ucsi: Don't truncate the reads

[Abhishek Pandit-Subedi]

On Fri, Aug 16, 2024 at 6:59 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> That may silently corrupt the data. Instead, failing attempts
> to read more than the interface can handle.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 8 ++------
>  drivers/usb/typec/ucsi/ucsi.h | 2 ++
>  2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 4039851551c1..96ef099a6f84 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -99,12 +99,8 @@ static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
>
>         *cci = 0;
>
> -       /*
> -        * Below UCSI 2.0, MESSAGE_IN was limited to 16 bytes. Truncate the
> -        * reads here.
> -        */
> -       if (ucsi->version <= UCSI_VERSION_1_2)
> -               size = clamp(size, 0, 16);
> +       if (size > UCSI_MAX_DATA_LENGTH(ucsi))
> +               return -EINVAL;
>
>         ret = ucsi->ops->sync_control(ucsi, command);
>         if (ret)
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 7bc132b59027..5e3c6cb822c8 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -412,6 +412,8 @@ struct ucsi {
>  #define UCSI_DELAY_DEVICE_PDOS BIT(1)  /* Reading PDOs fails until the parter is in PD mode */
>  };
>
> +#define UCSI_MAX_DATA_LENGTH(u) (((u)->version < UCSI_VERSION_2_0) ? 0x10 : 0xff)
> +
>  #define UCSI_MAX_SVID          5
>  #define UCSI_MAX_ALTMODES      (UCSI_MAX_SVID * 6)
>
> --
> 2.43.0
>
>

Looking through the list of commands that changed between UCSI1.2 and
UCSI2+, only GET_CONNECTOR_STATUS seems to have a size that exceeds 16
bytes (128 bits). As long as we treat that structure specially, this
change should be fine.

Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Re: [PATCH v2 1/6] usb: typec: ucsi: Remove unused fields from struct ucsi_connector_status

[Abhishek Pandit-Subedi]

On Fri, Aug 16, 2024 at 6:59 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> The new fields are valid only with the new UCSI versions.
> They are at offsets that go beyond the MAX_DATA_LENGTH (16
> bytes) with the older UCSI versions. That has not caused any
> problems before because nothing uses those new fields yet.
> Because they are not used yet, dropping them for now.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/typec/ucsi/ucsi.h | 27 ++-------------------------
>  1 file changed, 2 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 57129f3c0814..7bc132b59027 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -344,35 +344,12 @@ struct ucsi_connector_status {
>  #define   UCSI_CONSTAT_PARTNER_TYPE_AUDIO      6
>         u32 request_data_obj;
>
> -       u8 pwr_status[3];
> -#define UCSI_CONSTAT_BC_STATUS(_p_)            ((_p_[0]) & GENMASK(1, 0))
> +       u8 pwr_status;
> +#define UCSI_CONSTAT_BC_STATUS(_p_)            ((_p_) & GENMASK(1, 0))
>  #define   UCSI_CONSTAT_BC_NOT_CHARGING         0
>  #define   UCSI_CONSTAT_BC_NOMINAL_CHARGING     1
>  #define   UCSI_CONSTAT_BC_SLOW_CHARGING                2
>  #define   UCSI_CONSTAT_BC_TRICKLE_CHARGING     3
> -#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_)   (((_p_[0]) & GENMASK(5, 2)) >> 2)
> -#define   UCSI_CONSTAT_CAP_PWR_LOWERED         0
> -#define   UCSI_CONSTAT_CAP_PWR_BUDGET_LIMIT    1
> -#define UCSI_CONSTAT_PROVIDER_PD_VERSION_OPER_MODE(_p_)        \
> -       ((get_unaligned_le32(_p_) & GENMASK(21, 6)) >> 6)
> -#define UCSI_CONSTAT_ORIENTATION(_p_)          (((_p_[2]) & GENMASK(6, 6)) >> 6)
> -#define   UCSI_CONSTAT_ORIENTATION_DIRECT      0
> -#define   UCSI_CONSTAT_ORIENTATION_FLIPPED     1
> -#define UCSI_CONSTAT_SINK_PATH_STATUS(_p_)     (((_p_[2]) & GENMASK(7, 7)) >> 7)
> -#define   UCSI_CONSTAT_SINK_PATH_DISABLED      0
> -#define   UCSI_CONSTAT_SINK_PATH_ENABLED       1
> -       u8 pwr_readings[9];
> -#define UCSI_CONSTAT_REV_CURR_PROT_STATUS(_p_) ((_p_[0]) & 0x1)
> -#define UCSI_CONSTAT_PWR_READING_VALID(_p_)    (((_p_[0]) & GENMASK(1, 1)) >> 1)
> -#define UCSI_CONSTAT_CURRENT_SCALE(_p_)                (((_p_[0]) & GENMASK(4, 2)) >> 2)
> -#define UCSI_CONSTAT_PEAK_CURRENT(_p_) \
> -       ((get_unaligned_le32(_p_) & GENMASK(20, 5)) >> 5)
> -#define UCSI_CONSTAT_AVG_CURRENT(_p_) \
> -       ((get_unaligned_le32(&(_p_)[2]) & GENMASK(20, 5)) >> 5)
> -#define UCSI_CONSTAT_VOLTAGE_SCALE(_p_) \
> -       ((get_unaligned_le16(&(_p_)[4]) & GENMASK(8, 5)) >> 5)
> -#define UCSI_CONSTAT_VOLTAGE_READING(_p_) \
> -       ((get_unaligned_le32(&(_p_)[5]) & GENMASK(16, 1)) >> 1)
>  } __packed;
>
>  /*
> --
> 2.43.0
>
>

I'm working on a patch series that depends on the sink path status so
I would prefer we don't remove it:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5784952

Since GET_CONNECTOR_STATUS is the only command that exceeds 16 bytes
for MESSAGE_IN, can we just add a wrapper that checks the UCSI version
for that command only and limits the size sent to ucsi_run_command?

Re: [PATCH v2 3/6] usb: typec: ucsi: Only assign the identity structure if the PPM supports it

[Abhishek Pandit-Subedi]

On Fri, Aug 16, 2024 at 6:59 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> This will make sure that the identity sysfs attribute files
> are kept hidden if the UCSI interface doesn't support
> reading the USB Power Delivery messages.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 96ef099a6f84..1f6e3f0d25c1 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -979,7 +979,8 @@ static int ucsi_register_cable(struct ucsi_connector *con)
>                 break;
>         }
>
> -       desc.identity = &con->cable_identity;
> +       if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE)
> +               desc.identity = &con->cable_identity;
>         desc.active = !!(UCSI_CABLE_PROP_FLAG_ACTIVE_CABLE &
>                          con->cable_prop.flags);
>         desc.pd_revision = UCSI_CABLE_PROP_FLAG_PD_MAJOR_REV_AS_BCD(
> @@ -1058,7 +1059,8 @@ static int ucsi_register_partner(struct ucsi_connector *con)
>         if (pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD)
>                 ucsi_register_device_pdos(con);
>
> -       desc.identity = &con->partner_identity;
> +       if (con->ucsi->cap.features & UCSI_CAP_GET_PD_MESSAGE)
> +               desc.identity = &con->partner_identity;
>         desc.usb_pd = pwr_opmode == UCSI_CONSTAT_PWR_OPMODE_PD;
>         desc.pd_revision = UCSI_CONCAP_FLAG_PARTNER_PD_MAJOR_REV_AS_BCD(con->cap.flags);
>
> --
> 2.43.0
>
>

Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Re: [PATCH v2 1/6] usb: typec: ucsi: Remove unused fields from struct ucsi_connector_status

[Heikki Krogerus]

Hi Abhishek,

On Sun, Aug 18, 2024 at 05:02:28PM -0700, Abhishek Pandit-Subedi wrote:
> On Fri, Aug 16, 2024 at 6:59 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > The new fields are valid only with the new UCSI versions.
> > They are at offsets that go beyond the MAX_DATA_LENGTH (16
> > bytes) with the older UCSI versions. That has not caused any
> > problems before because nothing uses those new fields yet.
> > Because they are not used yet, dropping them for now.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/usb/typec/ucsi/ucsi.h | 27 ++-------------------------
> >  1 file changed, 2 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> > index 57129f3c0814..7bc132b59027 100644
> > --- a/drivers/usb/typec/ucsi/ucsi.h
> > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > @@ -344,35 +344,12 @@ struct ucsi_connector_status {
> >  #define   UCSI_CONSTAT_PARTNER_TYPE_AUDIO      6
> >         u32 request_data_obj;
> >
> > -       u8 pwr_status[3];
> > -#define UCSI_CONSTAT_BC_STATUS(_p_)            ((_p_[0]) & GENMASK(1, 0))
> > +       u8 pwr_status;
> > +#define UCSI_CONSTAT_BC_STATUS(_p_)            ((_p_) & GENMASK(1, 0))
> >  #define   UCSI_CONSTAT_BC_NOT_CHARGING         0
> >  #define   UCSI_CONSTAT_BC_NOMINAL_CHARGING     1
> >  #define   UCSI_CONSTAT_BC_SLOW_CHARGING                2
> >  #define   UCSI_CONSTAT_BC_TRICKLE_CHARGING     3
> > -#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_)   (((_p_[0]) & GENMASK(5, 2)) >> 2)
> > -#define   UCSI_CONSTAT_CAP_PWR_LOWERED         0
> > -#define   UCSI_CONSTAT_CAP_PWR_BUDGET_LIMIT    1
> > -#define UCSI_CONSTAT_PROVIDER_PD_VERSION_OPER_MODE(_p_)        \
> > -       ((get_unaligned_le32(_p_) & GENMASK(21, 6)) >> 6)
> > -#define UCSI_CONSTAT_ORIENTATION(_p_)          (((_p_[2]) & GENMASK(6, 6)) >> 6)
> > -#define   UCSI_CONSTAT_ORIENTATION_DIRECT      0
> > -#define   UCSI_CONSTAT_ORIENTATION_FLIPPED     1
> > -#define UCSI_CONSTAT_SINK_PATH_STATUS(_p_)     (((_p_[2]) & GENMASK(7, 7)) >> 7)
> > -#define   UCSI_CONSTAT_SINK_PATH_DISABLED      0
> > -#define   UCSI_CONSTAT_SINK_PATH_ENABLED       1
> > -       u8 pwr_readings[9];
> > -#define UCSI_CONSTAT_REV_CURR_PROT_STATUS(_p_) ((_p_[0]) & 0x1)
> > -#define UCSI_CONSTAT_PWR_READING_VALID(_p_)    (((_p_[0]) & GENMASK(1, 1)) >> 1)
> > -#define UCSI_CONSTAT_CURRENT_SCALE(_p_)                (((_p_[0]) & GENMASK(4, 2)) >> 2)
> > -#define UCSI_CONSTAT_PEAK_CURRENT(_p_) \
> > -       ((get_unaligned_le32(_p_) & GENMASK(20, 5)) >> 5)
> > -#define UCSI_CONSTAT_AVG_CURRENT(_p_) \
> > -       ((get_unaligned_le32(&(_p_)[2]) & GENMASK(20, 5)) >> 5)
> > -#define UCSI_CONSTAT_VOLTAGE_SCALE(_p_) \
> > -       ((get_unaligned_le16(&(_p_)[4]) & GENMASK(8, 5)) >> 5)
> > -#define UCSI_CONSTAT_VOLTAGE_READING(_p_) \
> > -       ((get_unaligned_le32(&(_p_)[5]) & GENMASK(16, 1)) >> 1)
> >  } __packed;
> >
> >  /*
> > --
> > 2.43.0
> >
> >
> 
> I'm working on a patch series that depends on the sink path status so
> I would prefer we don't remove it:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5784952
> 
> Since GET_CONNECTOR_STATUS is the only command that exceeds 16 bytes
> for MESSAGE_IN, can we just add a wrapper that checks the UCSI version
> for that command only and limits the size sent to ucsi_run_command?

It's always "just this one command" :). It's actually not only the
GET_CONNECTOR_STATUS command in this case - at least GET_PD_MESSAGE
can also exceed 16 bytes - but even if it was, it would still not be
okay to simply guard the read. You would also have to check the
version with every access of those extended fields, and that's where
it's basically guaranteed to fail. Somebody will access those extended
fields unconditionally without anybody noticing sooner or later, and
that's why they can't be part of this data structure.

So there needs to be a completely separate data structure for the
extended version.

struct ucsi_connector_status_extended {
        u8 status[16];
        u8 extended[3];
} __packed;

Something like that. But that of course should not be introduced
before there is an actual user for it.

thanks,

-- 
heikki

Re: [PATCH v2 1/6] usb: typec: ucsi: Remove unused fields from struct ucsi_connector_status

[Abhishek Pandit-Subedi]

On Mon, Aug 19, 2024 at 4:11 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Abhishek,
>
> On Sun, Aug 18, 2024 at 05:02:28PM -0700, Abhishek Pandit-Subedi wrote:
> > On Fri, Aug 16, 2024 at 6:59 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > The new fields are valid only with the new UCSI versions.
> > > They are at offsets that go beyond the MAX_DATA_LENGTH (16
> > > bytes) with the older UCSI versions. That has not caused any
> > > problems before because nothing uses those new fields yet.
> > > Because they are not used yet, dropping them for now.
> > >
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > ---
> > >  drivers/usb/typec/ucsi/ucsi.h | 27 ++-------------------------
> > >  1 file changed, 2 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> > > index 57129f3c0814..7bc132b59027 100644
> > > --- a/drivers/usb/typec/ucsi/ucsi.h
> > > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > > @@ -344,35 +344,12 @@ struct ucsi_connector_status {
> > >  #define   UCSI_CONSTAT_PARTNER_TYPE_AUDIO      6
> > >         u32 request_data_obj;
> > >
> > > -       u8 pwr_status[3];
> > > -#define UCSI_CONSTAT_BC_STATUS(_p_)            ((_p_[0]) & GENMASK(1, 0))
> > > +       u8 pwr_status;
> > > +#define UCSI_CONSTAT_BC_STATUS(_p_)            ((_p_) & GENMASK(1, 0))
> > >  #define   UCSI_CONSTAT_BC_NOT_CHARGING         0
> > >  #define   UCSI_CONSTAT_BC_NOMINAL_CHARGING     1
> > >  #define   UCSI_CONSTAT_BC_SLOW_CHARGING                2
> > >  #define   UCSI_CONSTAT_BC_TRICKLE_CHARGING     3
> > > -#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_)   (((_p_[0]) & GENMASK(5, 2)) >> 2)
> > > -#define   UCSI_CONSTAT_CAP_PWR_LOWERED         0
> > > -#define   UCSI_CONSTAT_CAP_PWR_BUDGET_LIMIT    1
> > > -#define UCSI_CONSTAT_PROVIDER_PD_VERSION_OPER_MODE(_p_)        \
> > > -       ((get_unaligned_le32(_p_) & GENMASK(21, 6)) >> 6)
> > > -#define UCSI_CONSTAT_ORIENTATION(_p_)          (((_p_[2]) & GENMASK(6, 6)) >> 6)
> > > -#define   UCSI_CONSTAT_ORIENTATION_DIRECT      0
> > > -#define   UCSI_CONSTAT_ORIENTATION_FLIPPED     1
> > > -#define UCSI_CONSTAT_SINK_PATH_STATUS(_p_)     (((_p_[2]) & GENMASK(7, 7)) >> 7)
> > > -#define   UCSI_CONSTAT_SINK_PATH_DISABLED      0
> > > -#define   UCSI_CONSTAT_SINK_PATH_ENABLED       1
> > > -       u8 pwr_readings[9];
> > > -#define UCSI_CONSTAT_REV_CURR_PROT_STATUS(_p_) ((_p_[0]) & 0x1)
> > > -#define UCSI_CONSTAT_PWR_READING_VALID(_p_)    (((_p_[0]) & GENMASK(1, 1)) >> 1)
> > > -#define UCSI_CONSTAT_CURRENT_SCALE(_p_)                (((_p_[0]) & GENMASK(4, 2)) >> 2)
> > > -#define UCSI_CONSTAT_PEAK_CURRENT(_p_) \
> > > -       ((get_unaligned_le32(_p_) & GENMASK(20, 5)) >> 5)
> > > -#define UCSI_CONSTAT_AVG_CURRENT(_p_) \
> > > -       ((get_unaligned_le32(&(_p_)[2]) & GENMASK(20, 5)) >> 5)
> > > -#define UCSI_CONSTAT_VOLTAGE_SCALE(_p_) \
> > > -       ((get_unaligned_le16(&(_p_)[4]) & GENMASK(8, 5)) >> 5)
> > > -#define UCSI_CONSTAT_VOLTAGE_READING(_p_) \
> > > -       ((get_unaligned_le32(&(_p_)[5]) & GENMASK(16, 1)) >> 1)
> > >  } __packed;
> > >
> > >  /*
> > > --
> > > 2.43.0
> > >
> > >
> >
> > I'm working on a patch series that depends on the sink path status so
> > I would prefer we don't remove it:
> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5784952
> >
> > Since GET_CONNECTOR_STATUS is the only command that exceeds 16 bytes
> > for MESSAGE_IN, can we just add a wrapper that checks the UCSI version
> > for that command only and limits the size sent to ucsi_run_command?
>
> It's always "just this one command" :). It's actually not only the
> GET_CONNECTOR_STATUS command in this case - at least GET_PD_MESSAGE
> can also exceed 16 bytes - but even if it was, it would still not be
> okay to simply guard the read. You would also have to check the
> version with every access of those extended fields, and that's where
> it's basically guaranteed to fail. Somebody will access those extended
> fields unconditionally without anybody noticing sooner or later, and
> that's why they can't be part of this data structure.

So this kind of points out a fundamental question to UCSI 1.2 vs
UCSI2+ in this driver: should we be doing a single driver that checks
the UCSI version before doing things or have two separate drivers?

I'm in favor of a single driver approach as I think there are a lot of
commonalities between the different UCSI versions. I think zero-ing
out the extended data on reads should be sufficient to support both
versions from the same code-base.
The alternative of creating a separate driver comes with more problems
-- do we fork for ucsi2 AND ucsi3? ucsi3 adds additional commands
(i.e. set_usb) and, in the case of get_pd_message, adds additional
behavior to existing commands (Get_Revision).

If your main worry is that people will unconditionally use the new
bits, why don't we simply change the macros from UCSI_CONSTAT to
UCSI_CONSTAT_v2 to indicate they need a revision bump? We can make
similar changes to other macros + enums to indicate the minimum UCSI
version that added those values.

>
> So there needs to be a completely separate data structure for the
> extended version.
>
> struct ucsi_connector_status_extended {
>         u8 status[16];
>         u8 extended[3];
> } __packed;
>
> Something like that. But that of course should not be introduced
> before there is an actual user for it.
>
> thanks,
>
> --
> heikki

Re: [PATCH v2 1/6] usb: typec: ucsi: Remove unused fields from struct ucsi_connector_status

[Heikki Krogerus]

On Mon, Aug 19, 2024 at 04:23:56PM -0700, Abhishek Pandit-Subedi wrote:
> On Mon, Aug 19, 2024 at 4:11 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi Abhishek,
> >
> > On Sun, Aug 18, 2024 at 05:02:28PM -0700, Abhishek Pandit-Subedi wrote:
> > > On Fri, Aug 16, 2024 at 6:59 AM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > The new fields are valid only with the new UCSI versions.
> > > > They are at offsets that go beyond the MAX_DATA_LENGTH (16
> > > > bytes) with the older UCSI versions. That has not caused any
> > > > problems before because nothing uses those new fields yet.
> > > > Because they are not used yet, dropping them for now.
> > > >
> > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > ---
> > > >  drivers/usb/typec/ucsi/ucsi.h | 27 ++-------------------------
> > > >  1 file changed, 2 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> > > > index 57129f3c0814..7bc132b59027 100644
> > > > --- a/drivers/usb/typec/ucsi/ucsi.h
> > > > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > > > @@ -344,35 +344,12 @@ struct ucsi_connector_status {
> > > >  #define   UCSI_CONSTAT_PARTNER_TYPE_AUDIO      6
> > > >         u32 request_data_obj;
> > > >
> > > > -       u8 pwr_status[3];
> > > > -#define UCSI_CONSTAT_BC_STATUS(_p_)            ((_p_[0]) & GENMASK(1, 0))
> > > > +       u8 pwr_status;
> > > > +#define UCSI_CONSTAT_BC_STATUS(_p_)            ((_p_) & GENMASK(1, 0))
> > > >  #define   UCSI_CONSTAT_BC_NOT_CHARGING         0
> > > >  #define   UCSI_CONSTAT_BC_NOMINAL_CHARGING     1
> > > >  #define   UCSI_CONSTAT_BC_SLOW_CHARGING                2
> > > >  #define   UCSI_CONSTAT_BC_TRICKLE_CHARGING     3
> > > > -#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_)   (((_p_[0]) & GENMASK(5, 2)) >> 2)
> > > > -#define   UCSI_CONSTAT_CAP_PWR_LOWERED         0
> > > > -#define   UCSI_CONSTAT_CAP_PWR_BUDGET_LIMIT    1
> > > > -#define UCSI_CONSTAT_PROVIDER_PD_VERSION_OPER_MODE(_p_)        \
> > > > -       ((get_unaligned_le32(_p_) & GENMASK(21, 6)) >> 6)
> > > > -#define UCSI_CONSTAT_ORIENTATION(_p_)          (((_p_[2]) & GENMASK(6, 6)) >> 6)
> > > > -#define   UCSI_CONSTAT_ORIENTATION_DIRECT      0
> > > > -#define   UCSI_CONSTAT_ORIENTATION_FLIPPED     1
> > > > -#define UCSI_CONSTAT_SINK_PATH_STATUS(_p_)     (((_p_[2]) & GENMASK(7, 7)) >> 7)
> > > > -#define   UCSI_CONSTAT_SINK_PATH_DISABLED      0
> > > > -#define   UCSI_CONSTAT_SINK_PATH_ENABLED       1
> > > > -       u8 pwr_readings[9];
> > > > -#define UCSI_CONSTAT_REV_CURR_PROT_STATUS(_p_) ((_p_[0]) & 0x1)
> > > > -#define UCSI_CONSTAT_PWR_READING_VALID(_p_)    (((_p_[0]) & GENMASK(1, 1)) >> 1)
> > > > -#define UCSI_CONSTAT_CURRENT_SCALE(_p_)                (((_p_[0]) & GENMASK(4, 2)) >> 2)
> > > > -#define UCSI_CONSTAT_PEAK_CURRENT(_p_) \
> > > > -       ((get_unaligned_le32(_p_) & GENMASK(20, 5)) >> 5)
> > > > -#define UCSI_CONSTAT_AVG_CURRENT(_p_) \
> > > > -       ((get_unaligned_le32(&(_p_)[2]) & GENMASK(20, 5)) >> 5)
> > > > -#define UCSI_CONSTAT_VOLTAGE_SCALE(_p_) \
> > > > -       ((get_unaligned_le16(&(_p_)[4]) & GENMASK(8, 5)) >> 5)
> > > > -#define UCSI_CONSTAT_VOLTAGE_READING(_p_) \
> > > > -       ((get_unaligned_le32(&(_p_)[5]) & GENMASK(16, 1)) >> 1)
> > > >  } __packed;
> > > >
> > > >  /*
> > > > --
> > > > 2.43.0
> > > >
> > > >
> > >
> > > I'm working on a patch series that depends on the sink path status so
> > > I would prefer we don't remove it:
> > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5784952
> > >
> > > Since GET_CONNECTOR_STATUS is the only command that exceeds 16 bytes
> > > for MESSAGE_IN, can we just add a wrapper that checks the UCSI version
> > > for that command only and limits the size sent to ucsi_run_command?
> >
> > It's always "just this one command" :). It's actually not only the
> > GET_CONNECTOR_STATUS command in this case - at least GET_PD_MESSAGE
> > can also exceed 16 bytes - but even if it was, it would still not be
> > okay to simply guard the read. You would also have to check the
> > version with every access of those extended fields, and that's where
> > it's basically guaranteed to fail. Somebody will access those extended
> > fields unconditionally without anybody noticing sooner or later, and
> > that's why they can't be part of this data structure.
> 
> So this kind of points out a fundamental question to UCSI 1.2 vs
> UCSI2+ in this driver: should we be doing a single driver that checks
> the UCSI version before doing things or have two separate drivers?

Nobody is proposing a driver split.

> I'm in favor of a single driver approach as I think there are a lot of
> commonalities between the different UCSI versions. I think zero-ing
> out the extended data on reads should be sufficient to support both
> versions from the same code-base.

Unfortunately 0 is a valid value also in this case.

> The alternative of creating a separate driver comes with more problems
> -- do we fork for ucsi2 AND ucsi3? ucsi3 adds additional commands
> (i.e. set_usb) and, in the case of get_pd_message, adds additional
> behavior to existing commands (Get_Revision).

Again, nobody is proposing a driver split. I don't know where did you
get this idea.

> If your main worry is that people will unconditionally use the new
> bits, why don't we simply change the macros from UCSI_CONSTAT to
> UCSI_CONSTAT_v2 to indicate they need a revision bump? We can make
> similar changes to other macros + enums to indicate the minimum UCSI
> version that added those values.

Simply naming a macro is not enough. A macro is fine, but it must have
the means to check the version and fail if it does not match.

We have a golden opportunity here to do exactly that. In most cases
only fields are extended, which is much more difficult situation, but
in this case we actually have completely new fields that extend the
data structure. That makes it possible to use the size like I'm doing
in patch 3/5 which guarantees that driver fails if those extended
fields are accessed when they are not available. That is exactly what
we want.

Otherwise accessing those fields when they are not available will
create the issues silently, most likely in form of a horrible user
experience: the cable works only if you plug it one way but not the
other because something thinks the orientation field is valid, or the
screen may simply be black. There are no error messages in dmesg, so
from kernel PoW everything seems to be working as it should. This is
not what we want. What we want is a spectacular failure if something
like that happens.

That failure will give us these two things:

1. It pin points the root cause of these issues.
2. If forces us to actually fix the issue (these are usually not
   considered as critical issues unfortunately).

These "silent" issues are really common and they always take a lot of
time to debug. I'm not going to waste this opportunity to make them a
bit less "silent" in this case.

thanks,

-- 
heikki

Re: [PATCH v2 1/6] usb: typec: ucsi: Remove unused fields from struct ucsi_connector_status

[Abhishek Pandit-Subedi]

On Tue, Aug 20, 2024 at 6:12 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Mon, Aug 19, 2024 at 04:23:56PM -0700, Abhishek Pandit-Subedi wrote:
> > On Mon, Aug 19, 2024 at 4:11 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > Hi Abhishek,
> > >
> > > On Sun, Aug 18, 2024 at 05:02:28PM -0700, Abhishek Pandit-Subedi wrote:
> > > > On Fri, Aug 16, 2024 at 6:59 AM Heikki Krogerus
> > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > >
> > > > > The new fields are valid only with the new UCSI versions.
> > > > > They are at offsets that go beyond the MAX_DATA_LENGTH (16
> > > > > bytes) with the older UCSI versions. That has not caused any
> > > > > problems before because nothing uses those new fields yet.
> > > > > Because they are not used yet, dropping them for now.
> > > > >
> > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > ---
> > > > >  drivers/usb/typec/ucsi/ucsi.h | 27 ++-------------------------
> > > > >  1 file changed, 2 insertions(+), 25 deletions(-)
> > > > >
> > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> > > > > index 57129f3c0814..7bc132b59027 100644
> > > > > --- a/drivers/usb/typec/ucsi/ucsi.h
> > > > > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > > > > @@ -344,35 +344,12 @@ struct ucsi_connector_status {
> > > > >  #define   UCSI_CONSTAT_PARTNER_TYPE_AUDIO      6
> > > > >         u32 request_data_obj;
> > > > >
> > > > > -       u8 pwr_status[3];
> > > > > -#define UCSI_CONSTAT_BC_STATUS(_p_)            ((_p_[0]) & GENMASK(1, 0))
> > > > > +       u8 pwr_status;
> > > > > +#define UCSI_CONSTAT_BC_STATUS(_p_)            ((_p_) & GENMASK(1, 0))
> > > > >  #define   UCSI_CONSTAT_BC_NOT_CHARGING         0
> > > > >  #define   UCSI_CONSTAT_BC_NOMINAL_CHARGING     1
> > > > >  #define   UCSI_CONSTAT_BC_SLOW_CHARGING                2
> > > > >  #define   UCSI_CONSTAT_BC_TRICKLE_CHARGING     3
> > > > > -#define UCSI_CONSTAT_PROVIDER_CAP_LIMIT(_p_)   (((_p_[0]) & GENMASK(5, 2)) >> 2)
> > > > > -#define   UCSI_CONSTAT_CAP_PWR_LOWERED         0
> > > > > -#define   UCSI_CONSTAT_CAP_PWR_BUDGET_LIMIT    1
> > > > > -#define UCSI_CONSTAT_PROVIDER_PD_VERSION_OPER_MODE(_p_)        \
> > > > > -       ((get_unaligned_le32(_p_) & GENMASK(21, 6)) >> 6)
> > > > > -#define UCSI_CONSTAT_ORIENTATION(_p_)          (((_p_[2]) & GENMASK(6, 6)) >> 6)
> > > > > -#define   UCSI_CONSTAT_ORIENTATION_DIRECT      0
> > > > > -#define   UCSI_CONSTAT_ORIENTATION_FLIPPED     1
> > > > > -#define UCSI_CONSTAT_SINK_PATH_STATUS(_p_)     (((_p_[2]) & GENMASK(7, 7)) >> 7)
> > > > > -#define   UCSI_CONSTAT_SINK_PATH_DISABLED      0
> > > > > -#define   UCSI_CONSTAT_SINK_PATH_ENABLED       1
> > > > > -       u8 pwr_readings[9];
> > > > > -#define UCSI_CONSTAT_REV_CURR_PROT_STATUS(_p_) ((_p_[0]) & 0x1)
> > > > > -#define UCSI_CONSTAT_PWR_READING_VALID(_p_)    (((_p_[0]) & GENMASK(1, 1)) >> 1)
> > > > > -#define UCSI_CONSTAT_CURRENT_SCALE(_p_)                (((_p_[0]) & GENMASK(4, 2)) >> 2)
> > > > > -#define UCSI_CONSTAT_PEAK_CURRENT(_p_) \
> > > > > -       ((get_unaligned_le32(_p_) & GENMASK(20, 5)) >> 5)
> > > > > -#define UCSI_CONSTAT_AVG_CURRENT(_p_) \
> > > > > -       ((get_unaligned_le32(&(_p_)[2]) & GENMASK(20, 5)) >> 5)
> > > > > -#define UCSI_CONSTAT_VOLTAGE_SCALE(_p_) \
> > > > > -       ((get_unaligned_le16(&(_p_)[4]) & GENMASK(8, 5)) >> 5)
> > > > > -#define UCSI_CONSTAT_VOLTAGE_READING(_p_) \
> > > > > -       ((get_unaligned_le32(&(_p_)[5]) & GENMASK(16, 1)) >> 1)
> > > > >  } __packed;
> > > > >
> > > > >  /*
> > > > > --
> > > > > 2.43.0
> > > > >
> > > > >
> > > >
> > > > I'm working on a patch series that depends on the sink path status so
> > > > I would prefer we don't remove it:
> > > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5784952
> > > >
> > > > Since GET_CONNECTOR_STATUS is the only command that exceeds 16 bytes
> > > > for MESSAGE_IN, can we just add a wrapper that checks the UCSI version
> > > > for that command only and limits the size sent to ucsi_run_command?
> > >
> > > It's always "just this one command" :). It's actually not only the
> > > GET_CONNECTOR_STATUS command in this case - at least GET_PD_MESSAGE
> > > can also exceed 16 bytes - but even if it was, it would still not be
> > > okay to simply guard the read. You would also have to check the
> > > version with every access of those extended fields, and that's where
> > > it's basically guaranteed to fail. Somebody will access those extended
> > > fields unconditionally without anybody noticing sooner or later, and
> > > that's why they can't be part of this data structure.
> >
> > So this kind of points out a fundamental question to UCSI 1.2 vs
> > UCSI2+ in this driver: should we be doing a single driver that checks
> > the UCSI version before doing things or have two separate drivers?
>
> Nobody is proposing a driver split.
>
> > I'm in favor of a single driver approach as I think there are a lot of
> > commonalities between the different UCSI versions. I think zero-ing
> > out the extended data on reads should be sufficient to support both
> > versions from the same code-base.
>
> Unfortunately 0 is a valid value also in this case.
>
> > The alternative of creating a separate driver comes with more problems
> > -- do we fork for ucsi2 AND ucsi3? ucsi3 adds additional commands
> > (i.e. set_usb) and, in the case of get_pd_message, adds additional
> > behavior to existing commands (Get_Revision).
>
> Again, nobody is proposing a driver split. I don't know where did you
> get this idea.

Well you did revert a backwards compatible structure :)

>
> > If your main worry is that people will unconditionally use the new
> > bits, why don't we simply change the macros from UCSI_CONSTAT to
> > UCSI_CONSTAT_v2 to indicate they need a revision bump? We can make
> > similar changes to other macros + enums to indicate the minimum UCSI
> > version that added those values.
>
> Simply naming a macro is not enough. A macro is fine, but it must have
> the means to check the version and fail if it does not match.
>
> We have a golden opportunity here to do exactly that. In most cases
> only fields are extended, which is much more difficult situation, but
> in this case we actually have completely new fields that extend the
> data structure. That makes it possible to use the size like I'm doing
> in patch 3/5 which guarantees that driver fails if those extended
> fields are accessed when they are not available. That is exactly what
> we want.
>
> Otherwise accessing those fields when they are not available will
> create the issues silently, most likely in form of a horrible user
> experience: the cable works only if you plug it one way but not the
> other because something thinks the orientation field is valid, or the
> screen may simply be black. There are no error messages in dmesg, so
> from kernel PoW everything seems to be working as it should. This is
> not what we want. What we want is a spectacular failure if something
> like that happens.
>
> That failure will give us these two things:
>
> 1. It pin points the root cause of these issues.
> 2. If forces us to actually fix the issue (these are usually not
>    considered as critical issues unfortunately).
>
> These "silent" issues are really common and they always take a lot of
> time to debug. I'm not going to waste this opportunity to make them a
> bit less "silent" in this case.

You have me convinced on the "failing loudly" part but I'm still
confused about the "how".

Making sure we always check versions to access the bits makes me think
we need wrappers on casting to the rightly versioned connector status.
Should we be versioning access for everything that's not in UCSI 1.2
then?

Example:

struct ucsi_connector_status_raw {
  u8 bytes[19];
};

struct ucsi_connector_status_v1 {
  ...
};

struct ucsi_connector_status_v2 {
  ...
};

struct ucsi_connector_status_v1* get_connector_status_v1(struct
ucsi_connector *con) {
  return (struct ucsi_connector_status_v1 *)con->raw_status;
}

struct ucsi_connector_status_v2* get_connector_status_v2(struct
ucsi_connector *con) {
  return con->ucsi->version >= UCSI_VERSION_2_0 ? (struct
ucsi_connector_status_v2 *)&con->raw_status : NULL;
}

/* Read all bits supported by the current version. */
int ucsi_read_connector_status(struct ucsi_connector *con, struct
ucsi_connector_status_raw *raw_conn_status);

>
> thanks,
>
> --
> heikki

Re: [PATCH v2 1/6] usb: typec: ucsi: Remove unused fields from struct ucsi_connector_status

[Heikki Krogerus]

Hi Abhishek,

I'm sorry to keep you waiting.

> You have me convinced on the "failing loudly" part but I'm still
> confused about the "how".
> 
> Making sure we always check versions to access the bits makes me think
> we need wrappers on casting to the rightly versioned connector status.
> Should we be versioning access for everything that's not in UCSI 1.2
> then?
> 
> Example:
> 
> struct ucsi_connector_status_raw {
>   u8 bytes[19];
> };
> 
> struct ucsi_connector_status_v1 {
>   ...
> };
> 
> struct ucsi_connector_status_v2 {
>   ...
> };
> 
> struct ucsi_connector_status_v1* get_connector_status_v1(struct
> ucsi_connector *con) {
>   return (struct ucsi_connector_status_v1 *)con->raw_status;
> }
> 
> struct ucsi_connector_status_v2* get_connector_status_v2(struct
> ucsi_connector *con) {
>   return con->ucsi->version >= UCSI_VERSION_2_0 ? (struct
> ucsi_connector_status_v2 *)&con->raw_status : NULL;
> }
> 
> /* Read all bits supported by the current version. */
> int ucsi_read_connector_status(struct ucsi_connector *con, struct
> ucsi_connector_status_raw *raw_conn_status);

I'll take a look at this next week. Right now I have to focus on
other tasks.

Br,

-- 
heikki

Re: [PATCH v2 1/6] usb: typec: ucsi: Remove unused fields from struct ucsi_connector_status

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

On Thu, Aug 22, 2024 at 02:24:17PM +0300, Heikki Krogerus wrote:
> Hi Abhishek,
> 
> I'm sorry to keep you waiting.
> 
> > You have me convinced on the "failing loudly" part but I'm still
> > confused about the "how".
> > 
> > Making sure we always check versions to access the bits makes me think
> > we need wrappers on casting to the rightly versioned connector status.
> > Should we be versioning access for everything that's not in UCSI 1.2
> > then?
> > 
> > Example:
> > 
> > struct ucsi_connector_status_raw {
> >   u8 bytes[19];
> > };
> > 
> > struct ucsi_connector_status_v1 {
> >   ...
> > };
> > 
> > struct ucsi_connector_status_v2 {
> >   ...
> > };
> > 
> > struct ucsi_connector_status_v1* get_connector_status_v1(struct
> > ucsi_connector *con) {
> >   return (struct ucsi_connector_status_v1 *)con->raw_status;
> > }
> > 
> > struct ucsi_connector_status_v2* get_connector_status_v2(struct
> > ucsi_connector *con) {
> >   return con->ucsi->version >= UCSI_VERSION_2_0 ? (struct
> > ucsi_connector_status_v2 *)&con->raw_status : NULL;
> > }
> > 
> > /* Read all bits supported by the current version. */
> > int ucsi_read_connector_status(struct ucsi_connector *con, struct
> > ucsi_connector_status_raw *raw_conn_status);
> 
> I'll take a look at this next week. Right now I have to focus on
> other tasks.

Sorry again for the delay. Today I sketched the idea that I have. I
think this problem would be the easiest to handle with field specific
helpers (see attached).

But at the same time I would like to get rid of all command specific
data structures. I've been planning that for some time now. We can do
that with a macro like that UCSI_FIELD(). The problem with those
structures is that if the fields in the structure don't align nicely
(like in case of GET_CONNECTOR_STATUS), we have to come up with custom
members, and that is not ideal at all. There are probable other
problems too.

I did not convert anything yet, I'll prepare a more complete RFC
tomorrow, but you should be able to get the idea from this.

thanks,

-- 
heikki

[-- Attachment #2: ucsi_field.diff --]
[-- Type: text/plain, Size: 3829 bytes --]

diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index c8c87377909d..972d7b364a3d 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -4,6 +4,7 @@
 #define __DRIVER_USB_TYPEC_UCSI_H
 
 #include <linux/bitops.h>
+#include <linux/bitfield.h>
 #include <linux/completion.h>
 #include <linux/device.h>
 #include <linux/power_supply.h>
@@ -352,6 +353,27 @@ struct ucsi_connector_status {
 #define   UCSI_CONSTAT_BC_TRICKLE_CHARGING	3
 } __packed;
 
+#define UCSI_CONNECTOR_CHANGE(_con)		UCSI_FIELD(&(_con)->status, 0, 16)
+#define UCSI_CONNECTOR_PWR_OPMODE(_con)		UCSI_FIELD(&(_con)->status, 16, 3)
+#define UCSI_CONNECTOR_CONNECTED(_con)		UCSI_FIELD(&(_con)->status, 19, 1)
+#define UCSI_CONNECTOR_PWR_DIR(_con)		UCSI_FIELD(&(_con)->status, 20, 1)
+#define UCSI_CONNECTOR_PARTNER_FLAGS(_con)	UCSI_FIELD(&(_con)->status, 21, 8)
+#define UCSI_CONNECTOR_PARTNER_TYPE(_con)	UCSI_FIELD(&(_con)->status, 29, 3)
+#define UCSI_CONNECTOR_RDO(_con)		UCSI_FIELD(&(_con)->status, 32, 32)
+#define UCSI_CONNECTOR_BC_STATUS(_con)		UCSI_FIELD(&(_con)->status, 64, 2)
+#define UCSI_CONNECTOR_PD_STATUS(_con)		UCSI_FIELD(&(_con)->status, 70, 16)
+
+#define UCSI_FIELD(data, offset, size)					\
+	({								\
+		u8 m = ((offset) % 8);					\
+		FIELD_GET((GENMASK((m + ((size) - 1)), m)),		\
+			  (*((u32 *)(&((u8 *)data)[((offset) / 8)]))));	\
+	})
+
+#define UCSI_FIELD_SAFE(ucsi, data, offset, size)			\
+	if (!WARN_ON(((offset) / 8) >= UCSI_MAX_DATA_LENGTH(ucsi)))	\
+		UCSI_FIELD(data, offset, size)
+
 /* -------------------------------------------------------------------------- */
 
 struct ucsi_debugfs_entry {
@@ -513,4 +535,73 @@ static inline void ucsi_debugfs_unregister(struct ucsi *ucsi) { }
 #define USB_TYPEC_NVIDIA_VLINK_DP_VDO	0x1
 #define USB_TYPEC_NVIDIA_VLINK_DBG_VDO	0x3
 
+/* -------------------------------------------------------------------------- */
+
+static __always_inline enum typec_orientation ucsi_orientation(struct ucsi_connector *con)
+{
+	if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0))
+		return TYPEC_ORIENTATION_NONE;
+	if (!UCSI_CONNECTOR_CONNECTED(con))
+		return TYPEC_ORIENTATION_NONE;
+	if (UCSI_FIELD(con, 86, 1))
+		return TYPEC_ORIENTATION_REVERSE;
+	return TYPEC_ORIENTATION_NORMAL;
+}
+
+static __always_inline int ucsi_sink_path_status(struct ucsi_connector *con)
+{
+	if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0))
+		return -EINVAL;
+	return UCSI_FIELD(con, 87, 1);
+}
+
+static inline int ucsi_reverse_current_protection_status(struct ucsi_connector *con)
+{
+	if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0))
+		return -EINVAL;
+	return UCSI_FIELD(con, 88, 1);
+}
+
+static inline int ucsi_power_reading_ready(struct ucsi_connector *con)
+{
+	if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0))
+		return -EINVAL;
+	return UCSI_FIELD(con, 89, 1);
+}
+
+static inline int ucsi_current_scale(struct ucsi_connector *con)
+{
+	if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0))
+		return -EINVAL;
+	return UCSI_FIELD(con, 90, 3) * 5; /* mA */
+}
+
+static inline int ucsi_peak_current(struct ucsi_connector *con)
+{
+	if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0))
+		return -EINVAL;
+	return UCSI_FIELD(con, 93, 16);
+}
+
+static inline int ucsi_average_current(struct ucsi_connector *con)
+{
+	if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0))
+		return -EINVAL;
+	return UCSI_FIELD(con, 109, 16);
+}
+
+static inline int ucsi_voltage_scale(struct ucsi_connector *con)
+{
+	if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0))
+		return -EINVAL;
+	return UCSI_FIELD(con, 125, 4) * 5; /* mV */
+}
+
+static inline int ucsi_voltage_reading(struct ucsi_connector *con)
+{
+	if (WARN_ON(con->ucsi->version < UCSI_VERSION_2_0))
+		return -EINVAL;
+	return UCSI_FIELD(con, 129, 16);
+}
+
 #endif /* __DRIVER_USB_TYPEC_UCSI_H */