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

[Alexey Charkov <alchark@flipper.net>]

On Mon, Nov 2, 2026 at 4:54 PM <sashiko-bot@kernel.org> wrote:
>
> 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?

That is exactly the case, and I have just sent a patch [1] dropping
Get_Source_Cap_Extended from the queue separately, as they are only
tangentially related and can be applied in any order, and the current
broken behavior remains broken until both patches are applied.

[1] https://lore.kernel.org/all/20261102-fusb302-getsourcecap-v1-1-26b5bfea4b2b@flipper.net/

Best regards,
Alexey