[PATCH v2] usb: typec: tcpci: pass correct rx_type to tcpm_pd_receive()

[PATCH v2] usb: typec: tcpci: pass correct rx_type to tcpm_pd_receive()

[Xu Yang]

From: Xu Yang <xu.yang_2@nxp.com>

Previously, tcpci_irq() always passed TCPC_TX_SOP as the receive type
to tcpm_pd_receive(), ignoring the actual frame type reported by the
TCPC_RX_BUF_FRAME_TYPE register.

Cache the TCPC_RX_DETECT register value in rx_type_mask variable. When
a PD messageis received, read TCPC_RX_BUF_FRAME_TYPE register and handle
the message only if its frame type is enabled in mask.

The TCPC_RX_BUF_FRAME_TYPE register records the received message type,
which has a 1:1 mapping to enum tcpm_transmit_type.

Fixes: fb7ff25ae433 ("usb: typec: tcpm: add discover identity support for SOP'")
Cc: stable@vger.kernel.org
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v2:
 - add fix tag
 - check return value when get RX SOP type
 - pass all possible RX message type to tcpm_pd_receive()
 - cache TCPC_RX_DETECT to filter out unallowed RX messages as suggested by Badhri
---
 drivers/usb/typec/tcpm/tcpci.c | 12 +++++++++++-
 include/linux/usb/tcpci.h      |  1 +
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index 7ac7000b2d13..6717ac914c6a 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -38,6 +38,7 @@ struct tcpci {
 
 	struct regmap *regmap;
 	unsigned int alert_mask;
+	unsigned int rx_type_mask;
 
 	bool controls_vbus;
 
@@ -488,6 +489,8 @@ static int tcpci_set_pd_rx(struct tcpc_dev *tcpc, bool enable)
 		if (tcpci->data->cable_comm_capable)
 			reg |= TCPC_RX_DETECT_SOP1;
 	}
+
+	tcpci->rx_type_mask = reg;
 	ret = regmap_write(tcpci->regmap, TCPC_RX_DETECT, reg);
 	if (ret < 0)
 		return ret;
@@ -749,6 +752,7 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
 	if (status & TCPC_ALERT_RX_STATUS) {
 		struct pd_message msg;
 		unsigned int cnt, payload_cnt;
+		enum tcpm_transmit_type rx_type;
 		u16 header;
 
 		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
@@ -773,10 +777,16 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
 			regmap_raw_read(tcpci->regmap, TCPC_RX_DATA,
 					&msg.payload, payload_cnt);
 
+		ret = regmap_read(tcpci->regmap, TCPC_RX_BUF_FRAME_TYPE, &rx_type);
+		if (ret)
+			return ret;
+
 		/* Read complete, clear RX status alert bit */
 		tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);
 
-		tcpm_pd_receive(tcpci->port, &msg, TCPC_TX_SOP);
+		rx_type &= TCPC_RX_BUF_FRAME_TYPE_MASK;
+		if (tcpci->rx_type_mask & BIT(rx_type))
+			tcpm_pd_receive(tcpci->port, &msg, rx_type);
 	}
 
 	if (tcpci->data->vbus_vsafe0v && (status & TCPC_ALERT_EXTENDED_STATUS)) {
diff --git a/include/linux/usb/tcpci.h b/include/linux/usb/tcpci.h
index f7f5cfbdef12..9b46a6bc762c 100644
--- a/include/linux/usb/tcpci.h
+++ b/include/linux/usb/tcpci.h
@@ -144,6 +144,7 @@
 #define TCPC_RX_BUF_FRAME_TYPE		0x31
 #define TCPC_RX_BUF_FRAME_TYPE_SOP	0
 #define TCPC_RX_BUF_FRAME_TYPE_SOP1	1
+#define TCPC_RX_BUF_FRAME_TYPE_MASK	GENMASK(2, 0)
 #define TCPC_RX_HDR			0x32
 #define TCPC_RX_DATA			0x34 /* through 0x4f */
 
-- 
2.34.1

Re: [PATCH v2] usb: typec: tcpci: pass correct rx_type to tcpm_pd_receive()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Returning a negative error code from `regmap_read()` directly in an IRQ thread function skips the interrupt acknowledgment, causing an interrupt storm.
- [High] Flawed 1:1 enum and bit mapping misclassifies Cable Reset messages as Hard Reset messages.
- [Medium] Data race on `tcpci->rx_type_mask` between workqueue and IRQ context.
- [Low] Passing an enum pointer to `regmap_read()` violates strict typing and breaks the build.

Pre-existing issues:
- [High] Uninitialized stack variables are used after potentially failed I2C reads.
--

--- Patch [1]: [PATCH v2] usb: typec: tcpci: pass correct rx_type to tcpm_pd_receive() ---
commit f5e85aadc576c9a21ace1e250d65e3ad2d0b7a00
Author: Xu Yang <xu.yang_2@nxp.com>
usb: typec: tcpci: pass correct rx_type to tcpm_pd_receive()

This patch updates the TCPCI driver to dynamically determine the RX frame type
instead of hardcoding it to TCPC_TX_SOP. It caches the TCPC_RX_DETECT register
and verifies incoming messages against this mask before passing them to the
TCPM core.

> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -488,6 +489,8 @@ static int tcpci_set_pd_rx(struct tcpc_dev *tcpc, bool enable)
>  		if (tcpci->data->cable_comm_capable)
>  			reg |= TCPC_RX_DETECT_SOP1;
>  	}
> +
> +	tcpci->rx_type_mask = reg;

[Severity: Medium]
Could this assignment to tcpci->rx_type_mask race with the read in tcpci_irq()?
This variable is written in process context here and read in interrupt context,
but lacks locking or READ_ONCE/WRITE_ONCE annotations, which might allow
compiler optimizations to reorder or cache the variable unexpectedly.

>  	ret = regmap_write(tcpci->regmap, TCPC_RX_DETECT, reg);

[ ... ]

> @@ -749,6 +752,7 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  	if (status & TCPC_ALERT_RX_STATUS) {
>  		struct pd_message msg;
>  		unsigned int cnt, payload_cnt;
> +		enum tcpm_transmit_type rx_type;
>  		u16 header;
>  
>  		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);

[Severity: High]
This is a pre-existing issue, but is it safe to use cnt unconditionally?
If the I2C read fails here, cnt remains uninitialized. Could this lead to
processing garbage data as USB PD messages and corrupting the state machine?

>  		/*
>  		 * 'cnt' corresponds to READABLE_BYTE_COUNT in section 4.4.14
>  		 * of the TCPCI spec [Rev 2.0 Ver 1.0 October 2017] and is

[ ... ]

> @@ -773,10 +777,16 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  			regmap_raw_read(tcpci->regmap, TCPC_RX_DATA,
>  					&msg.payload, payload_cnt);
>  
> +		ret = regmap_read(tcpci->regmap, TCPC_RX_BUF_FRAME_TYPE, &rx_type);

[Severity: Low]
Does passing an enum pointer to regmap_read() violate strict typing?
regmap_read() expects an unsigned int *, but &rx_type is of type
enum tcpm_transmit_type *. Would this trigger -Wincompatible-pointer-types
warnings and fail the build on modern kernels?

> +		if (ret)
> +			return ret;

[Severity: High]
Does returning a negative error code directly from this IRQ thread function
skip the interrupt acknowledgment?

tcpci_irq()
    if (ret)
        return ret;
    ...
    tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);

Since the TCPC alert pin is level-triggered, would leaving the alert bit
unacknowledged cause the interrupt to remain asserted and result in an
infinite IRQ storm? Also, is returning a negative integer a valid irqreturn_t
value?

> +
>  		/* Read complete, clear RX status alert bit */
>  		tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);
>  
> -		tcpm_pd_receive(tcpci->port, &msg, TCPC_TX_SOP);
> +		rx_type &= TCPC_RX_BUF_FRAME_TYPE_MASK;
> +		if (tcpci->rx_type_mask & BIT(rx_type))
> +			tcpm_pd_receive(tcpci->port, &msg, rx_type);

[Severity: High]
Does the 1:1 mapping between TCPC_RX_BUF_FRAME_TYPE and enum tcpm_transmit_type
hold true?

The TCPCI spec maps TCPC_RX_BUF_FRAME_TYPE value 5 to 'Cable Reset', but
TCPC_RX_DETECT bit 5 is 'Hard Reset'. Because the driver enables bit 5 for Hard
Resets, receiving a Cable Reset would evaluate as allowed, and pass rx_type 5
to TCPM.

In enum tcpm_transmit_type, 5 is TCPC_TX_HARD_RESET. Would the TCPM state
machine then process a 0-byte hardware packet as an invalid Hard Reset RX
message instead of a Cable Reset, corrupting the protocol state?

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723104614.3717623-1-xu.yang_2@oss.nxp.com?part=1