Re: [PATCH] usb: typec: fusb302: Drain the RX FIFO on each PD interrupt

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: typec: fusb302: unacknowledged messages passed to tcpm_pd_receive()
--

commit c6b027a56fce877cc0a50df66b78e46a0dbd88cb
Author: Alexey Charkov <alchark@flipper.net>

usb: typec: fusb302: Drain the RX FIFO on each PD interrupt

This commit adds fusb302_pd_read_messages() to drain all messages from the
80-byte RX FIFO on each TX_SUCCESS or GCRCSENT interrupt until
FUSB_REG_STATUS1_RX_EMPTY is set. This prevents the driver from falling one
message behind when the hardware queues unacknowledged messages without
raising an interrupt or when multiple messages arrive in quick succession.

> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index 3319f6a2b0c9b..6841db172e117 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -1481,6 +1481,36 @@ static int fusb302_pd_read_message(struct fusb302_chip *chip,
>  	return ret;
>  }
>  
> +/* The 80-byte RX FIFO holds at most 11 messages without data objects */
> +#define FUSB302_RX_FIFO_MAX_MSGS	11
> +
> +static int fusb302_pd_read_messages(struct fusb302_chip *chip)
> +{
> +	struct pd_message pd_msg;
> +	u8 status1;
> +	int ret;
> +	int i;
> +
> +	/*
> +	 * Drain the whole RX FIFO rather than one message per interrupt:
> +	 * messages the chip does not acknowledge raise no interrupt, and
> +	 * reading one message per event would then leave every later read
> +	 * returning the message queued before the one that raised it.
> +	 */
> +	for (i = 0; i < FUSB302_RX_FIFO_MAX_MSGS; i++) {
> +		ret = fusb302_i2c_read(chip, FUSB_REG_STATUS1, &status1);
> +		if (ret < 0)
> +			return ret;
> +		if (status1 & FUSB_REG_STATUS1_RX_EMPTY)
> +			return 0;
> +		ret = fusb302_pd_read_message(chip, &pd_msg);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
When a PD 3.0 partner sends PD_CTRL_GET_SOURCE_CAP_EXT (type 17, 0x11, with
len == 0), the FUSB302 hardware decodes only the low four bits of the message
type (0x11 & 0xf == PD_CTRL_GOOD_CRC) and places the message and its retries
into the RX FIFO without sending a GoodCRC on the CC line or raising an
interrupt.

After those retries time out, the partner sends PD_CTRL_SOFT_RESET, which the
FUSB302 acknowledges with GoodCRC, raising FUSB_REG_INTERRUPTB_GCRCSENT. When
fusb302_irq_work() -> fusb302_pd_read_messages() drains the RX FIFO, it calls
fusb302_pd_read_message() for the unacknowledged PD_CTRL_GET_SOURCE_CAP_EXT
queued ahead of PD_CTRL_SOFT_RESET:

drivers/usb/typec/tcpm/fusb302.c:fusb302_pd_read_message() {
    ...
	if ((!len) && (pd_header_type_le(msg->header) == PD_CTRL_GOOD_CRC))
		tcpm_pd_transmit_complete(chip->tcpm_port, TCPC_TX_SUCCESS);
	else
		tcpm_pd_receive(chip->tcpm_port, msg, TCPC_TX_SOP);
    ...
}

Because pd_header_type_le(msg->header) checks all five bits,
fusb302_pd_read_message() forwards the unacknowledged
PD_CTRL_GET_SOURCE_CAP_EXT to tcpm_pd_receive().

In tcpm_pd_rx_handler() -> tcpm_pd_ctrl_request() -> tcpm_pd_handle_msg() ->
tcpm_queue_message(), TCPM updates port->rx_msgid and sets
port->queued_message = PD_MSG_CTRL_NOT_SUPP:

drivers/usb/typec/tcpm/tcpm.c:tcpm_pd_ctrl_request() {
    ...
	case PD_CTRL_GET_SOURCE_CAP_EXT:
	case PD_CTRL_GET_STATUS:
	case PD_CTRL_FR_SWAP:
	case PD_CTRL_GET_PPS_STATUS:
	case PD_CTRL_GET_COUNTRY_CODES:
		/* Currently not supported */
		tcpm_pd_handle_msg(port,
				   port->negotiated_rev < PD_REV30 ?
				   PD_MSG_CTRL_REJECT :
				   PD_MSG_CTRL_NOT_SUPP,
				   NONE_AMS);
		break;
    ...
}

When the subsequent PD_CTRL_SOFT_RESET is processed, tcpm_pd_ctrl_request()
calls tcpm_set_state(port, SOFT_RESET, 0) without clearing
port->queued_message. Then, in tcpm_state_machine_work(), TCPM sends the
queued PD_MSG_CTRL_NOT_SUPP before run_state_machine() runs the SOFT_RESET
state to send PD_CTRL_ACCEPT:

drivers/usb/typec/tcpm/tcpm.c:tcpm_state_machine_work() {
    ...
	if (port->queued_message && tcpm_send_queued_message(port))
		goto done;
    ...
}

Doesn't forwarding unacknowledged control messages (where !len and the low
four bits of the type equal PD_CTRL_GOOD_CRC, but the full type is not
PD_CTRL_GOOD_CRC) to tcpm_pd_receive() cause TCPM to transmit a spurious
PD_CTRL_NOT_SUPP on the wire while the partner is waiting for PD_CTRL_ACCEPT
in response to its Soft_Reset?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-fusb302-drain-rxfifo-v1-1-bd9b232ab345@flipper.net?part=1