Re: [PATCH 12/19] usb: typec: tcpci: don't act on unread alerts

[Amit Sunil Dhamne <amitsd@google.com>]

Hi,

On 10/27/26 7:57 AM, Greg Kroah-Hartman wrote:
> From: Josef Bacik <josef@toxicpanda.com>
> 
> tcpci_irq() ignores the result of nearly all its register reads.
> 
> If the first read of TCPC_ALERT fails, "status" is an unwritten variable,
> and its bits decide which events are reported to the port manager: among
> them a hard reset, which drops VBUS. If the read at the end of the function
> fails, "status" keeps the alerts which have just been handled: they are
> handled again, for as long as the read fails.
> 
> In the receive branch the byte count, the header and the payload are not
> checked: a count made of stack content trips the WARN_ON(), and a message
> made of stack content is handed to tcpm_pd_receive(). A failed read of the
> frame type returns a negative errno as irqreturn_t, with RX_STATUS still
> set.
> 
> The handler must not simply return in these cases. Nine in-tree boards
> connect ALERT# to an edge triggered interrupt: as long as an alert is
> pending the line stays low, and there is no further edge. That is what the
> read at the end is for.
> 
> So read the alert register through a helper which tries three times and
> reports "no alert" if that fails: after a failed first read the function
> falls through to the read at the end. Drop a message which cannot be read,
> but clear RX_STATUS, to release the line. Don't take an unread power status
> mask for a reset of the TCPC. Return IRQ_HANDLED if any round has found an
> alert, not only the first one.
> 

nit: This commit is quite verbose. It would be better if it were more 
succinct and to the point. For example, the last paragraph narrates the 
code rather than stating the intent.


> Fixes: 74e656d6b055 ("staging: typec: Type-C Port Controller Interface driver (tcpci)")
> Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
> Fixes: b691a07c5f64 ("usb: typec: tcpci: pass correct rx_type to tcpm_pd_receive()")
> Cc: Badhri Jagan Sridharan <badhri@google.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: stable <stable@kernel.org>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>   drivers/usb/typec/tcpm/tcpci.c | 64 +++++++++++++++++++++++++++++++-----------
>   1 file changed, 47 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index 6717ac914c6a..26c62216a813 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -715,17 +715,34 @@ static int tcpci_init(struct tcpc_dev *tcpc)
>   	return 0;
>   }
>   
> +/*
> + * An edge triggered ALERT# does not fire again while an alert is pending, so
> + * don't give up after one failed transfer. No alert if it cannot be read.
> + */
> +static u16 tcpci_read_alert(struct tcpci *tcpci)
> +{
> +	u16 status;
> +	int i;
> +
> +	for (i = 0; i < 3; i++)
> +		if (!tcpci_read16(tcpci, TCPC_ALERT, &status))
> +			return status;

Retrying immediately on a transient bus failure is unlikely to help. Add 
a small delay before retrying. Please keep in mind the Type-C/PD 
deadlines when selecting a delay value.

> +

Please add some error log here to indicate consecutive alert read failures.

> +	return 0;
> +}
> +
>   irqreturn_t tcpci_irq(struct tcpci *tcpci)
>   {
>   	u16 status;
>   	int ret;
> -	int irq_ret;
> +	int irq_ret = 0;
>   	unsigned int raw;
>   
> -	tcpci_read16(tcpci, TCPC_ALERT, &status);
> -	irq_ret = status & tcpci->alert_mask;
> +	status = tcpci_read_alert(tcpci);
>   
>   process_status:
> +	irq_ret |= status & tcpci->alert_mask;
> +
>   	/*
>   	 * Clear alert status for everything except RX_STATUS, which shouldn't
>   	 * be cleared until we have successfully retrieved message.
> @@ -738,12 +755,12 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>   		tcpm_cc_change(tcpci->port);
>   
>   	if (status & TCPC_ALERT_POWER_STATUS) {
> -		regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
> +		ret = regmap_read(tcpci->regmap, TCPC_POWER_STATUS_MASK, &raw);
>   		/*
>   		 * If power status mask has been reset, then the TCPC
>   		 * has reset.
>   		 */
> -		if (raw == 0xff)
> +		if (!ret && raw == 0xff)
>   			tcpm_tcpc_reset(tcpci->port);
>   		else
>   			tcpm_vbus_change(tcpci->port);
> @@ -755,7 +772,10 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>   		enum tcpm_transmit_type rx_type;
>   		u16 header;
>   
> -		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> +		ret = regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> +		if (ret)
> +			goto rx_done;
> +
>   		/*
>   		 * '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
> @@ -767,26 +787,36 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>   		else
>   			payload_cnt = 0;
>   
> -		tcpci_read16(tcpci, TCPC_RX_HDR, &header);
> +		ret = tcpci_read16(tcpci, TCPC_RX_HDR, &header);
> +		if (ret)
> +			goto rx_done;
> +
>   		msg.header = cpu_to_le16(header);
>   
>   		if (WARN_ON(payload_cnt > sizeof(msg.payload)))
>   			payload_cnt = sizeof(msg.payload);
>   
> -		if (payload_cnt > 0)
> -			regmap_raw_read(tcpci->regmap, TCPC_RX_DATA,
> -					&msg.payload, payload_cnt);
> +		if (payload_cnt > 0) {
> +			ret = regmap_raw_read(tcpci->regmap, TCPC_RX_DATA,
> +					      &msg.payload, payload_cnt);
> +			if (ret)
> +				goto rx_done;
> +		}
>   
>   		ret = regmap_read(tcpci->regmap, TCPC_RX_BUF_FRAME_TYPE, &rx_type);
> -		if (ret)
> -			return ret;
>   
> -		/* Read complete, clear RX status alert bit */
> +rx_done:

Worth logging when you're dropping RX packets (due to ret!=0) given that 
the tcpc has already sent a GoodCRC.

Regards,
Amit