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

[sashiko-bot@kernel.org]

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