Re: [PATCH 1/2] usb: typec: tcpci: add check code for tcpci/regmap_read/write()

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Aug 28, 2023 at 05:30:34PM +0800, Xu Yang wrote:
> The return value from tcpci/regmap_read/write() must be checked to get
> rid of the bad influence of other modules.

What do you mean by "other modules" here?

> This will add check code for
> all of the rest read/write() callbacks and will show error when failed
> to get ALERT register.
> 
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
>  drivers/usb/typec/tcpm/tcpci.c | 36 +++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index 0ee3e6e29bb1..698d00b7fce9 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c
> @@ -657,21 +657,30 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  	int ret;
>  	unsigned int raw;
>  
> -	tcpci_read16(tcpci, TCPC_ALERT, &status);
> +	ret = tcpci_read16(tcpci, TCPC_ALERT, &status);
> +	if (ret < 0) {
> +		dev_err(tcpci->dev, "ALERT read failed\n");

You are printing something in an irq handler, are you sure you want to
do that?  What happens if this is very noisy?

> +		return ret;
> +	}
>  
>  	/*
>  	 * Clear alert status for everything except RX_STATUS, which shouldn't
>  	 * be cleared until we have successfully retrieved message.
>  	 */
> -	if (status & ~TCPC_ALERT_RX_STATUS)
> -		tcpci_write16(tcpci, TCPC_ALERT,
> +	if (status & ~TCPC_ALERT_RX_STATUS) {
> +		ret = tcpci_write16(tcpci, TCPC_ALERT,
>  			      status & ~TCPC_ALERT_RX_STATUS);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	if (status & TCPC_ALERT_CC_STATUS)
>  		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 (ret < 0)
> +			return ret;
>  		/*
>  		 * If power status mask has been reset, then the TCPC
>  		 * has reset.
> @@ -687,7 +696,9 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  		unsigned int cnt, payload_cnt;
>  		u16 header;
>  
> -		regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> +		ret = regmap_read(tcpci->regmap, TCPC_RX_BYTE_CNT, &cnt);
> +		if (ret < 0)
> +			return ret;
>  		/*
>  		 * '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
> @@ -699,18 +710,25 @@ 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 < 0)
> +			return ret;
>  		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,
> +		if (payload_cnt > 0) {
> +			ret = regmap_raw_read(tcpci->regmap, TCPC_RX_DATA,
>  					&msg.payload, payload_cnt);
> +			if (ret < 0)
> +				return ret;
> +		}
>  
>  		/* Read complete, clear RX status alert bit */
> -		tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);
> +		ret = tcpci_write16(tcpci, TCPC_ALERT, TCPC_ALERT_RX_STATUS);
> +		if (ret < 0)
> +			return ret;

For all of these, if an error happens, you are not able to unwind from
the previous actions that the irq handler took.

Are you sure this is ok?  How was this tested?

thanks,

greg k-h