Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 12 Apr 2022 14:42:11 -0700
From:   Nathan Chancellor <nathan@kernel.org>
To:     Tom Rix <trix@redhat.com>
Cc:     heikki.krogerus@linux.intel.com, gregkh@linuxfoundation.org,
        ndesaulniers@google.com, sven@svenpeter.dev, alyssa@rosenzweig.io,
        marcan@marcan.st, martink@posteo.de, saranya.gopal@intel.com,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        llvm@lists.linux.dev
Subject: Re: [PATCH] usb: typec: tipd: improve handling of failures in
 interrupt handlers
Message-ID: <YlXyM7e+Bkqa3HCZ@dev-arch.thelio-3990X>
References: <20220409130013.1474412-1-trix@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220409130013.1474412-1-trix@redhat.com>
Precedence: bulk

Hi Tom,

Thanks for the patch!

On Sat, Apr 09, 2022 at 09:00:13AM -0400, Tom Rix wrote:
> clang static analysis reports this representative issue
> core.c:516:6: warning: Branch condition evaluates
>   to a garbage value
>   if (event)
>       ^~~~~
> 
> In cd321x_interrupt(), a successful call to
> tps6598x_read64() is the only way event is set,
> and if a failure happens the irq should not be
> reported as handled.
> 
> Instead of initializing event, rework the
> usage of ret by initializing it to IRQ_NONE
> and then setting it when event is known to
> be not zero.  This removes the if-statement
> before the return.
> 
> tps6598x_interrupt() is similar.
> 
> Fixes: 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery controllers")

I am not sure this fixes tag is accurate. At that point in time,
tps6598x_interrupt() did not have any use of event1 or event2 that was
uninitialized.

I think

Fixes: c7260e29dd20 ("usb: typec: tipd: Add short-circuit for no irqs")
Fixes: 45188f27b3d0 ("usb: typec: tipd: Add support for Apple CD321X")

is a more accurate set, as these changes made it possible for the event
variables to be used uninitialized.

> Signed-off-by: Tom Rix <trix@redhat.com>

I found one issue below. With that addressed, feel free to carry
forward:

Reviewed-by: Nathan Chancellor <nathan@kernel.org>

> ---
>  drivers/usb/typec/tipd/core.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 16b4560216ba..88a20cc15da4 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -478,12 +478,11 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>  	struct tps6598x *tps = data;
>  	u64 event;
>  	u32 status;
> -	int ret;
> +	int ret = IRQ_NONE;
>  
>  	mutex_lock(&tps->lock);
>  
> -	ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event);
> -	if (ret) {
> +	if (tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event)) {
>  		dev_err(tps->dev, "%s: failed to read events\n", __func__);
>  		goto err_unlock;
>  	}
> @@ -492,6 +491,8 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>  	if (!event)
>  		goto err_unlock;
>  
> +	ret = IRQ_HANDLED;
> +
>  	if (!tps6598x_read_status(tps, &status))
>  		goto err_clear_ints;
>  
> @@ -513,9 +514,7 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>  err_unlock:
>  	mutex_unlock(&tps->lock);
>  
> -	if (event)
> -		return IRQ_HANDLED;
> -	return IRQ_NONE;
> +	return ret;
>  }
>  
>  static irqreturn_t tps6598x_interrupt(int irq, void *data)
> @@ -524,13 +523,12 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  	u64 event1;
>  	u64 event2;
>  	u32 status;
> -	int ret;
> +	int ret = IRQ_NONE;
>  
>  	mutex_lock(&tps->lock);
>  
> -	ret = tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event1);
> -	ret |= tps6598x_read64(tps, TPS_REG_INT_EVENT2, &event2);
> -	if (ret) {
> +	if (tps6598x_read64(tps, TPS_REG_INT_EVENT1, &event1) ||
> +	    tps6598x_read64(tps, TPS_REG_INT_EVENT2, &event2)) {

This change is incorrect. If the first tps6598x_read64() call succeeds,
then the second tps6598x_read64() will not be called, which would leave
event2 uninitialized. This should be a bitwise OR so that both calls to
tps6598x_read64() occur.

>  		dev_err(tps->dev, "%s: failed to read events\n", __func__);
>  		goto err_unlock;
>  	}
> @@ -539,6 +537,8 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  	if (!(event1 | event2))
>  		goto err_unlock;
>  
> +	ret = IRQ_HANDLED;
> +
>  	if (!tps6598x_read_status(tps, &status))
>  		goto err_clear_ints;
>  
> @@ -561,9 +561,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  err_unlock:
>  	mutex_unlock(&tps->lock);
>  
> -	if (event1 | event2)
> -		return IRQ_HANDLED;
> -	return IRQ_NONE;
> +	return ret;
>  }
>  
>  static int tps6598x_check_mode(struct tps6598x *tps)
> -- 
> 2.27.0
> 
>