* [PATCH] usb: typec: tipd: improve handling of failures in interrupt handlers
@ 2022-04-09 13:00 Tom Rix
2022-04-12 21:42 ` Nathan Chancellor
2022-04-21 16:24 ` Greg KH
0 siblings, 2 replies; 3+ messages in thread
From: Tom Rix @ 2022-04-09 13:00 UTC (permalink / raw)
To: heikki.krogerus, gregkh, nathan, ndesaulniers, sven, alyssa,
marcan, martink, saranya.gopal
Cc: linux-usb, linux-kernel, llvm, Tom Rix
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")
Signed-off-by: Tom Rix <trix@redhat.com>
---
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)) {
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] usb: typec: tipd: improve handling of failures in interrupt handlers
2022-04-09 13:00 [PATCH] usb: typec: tipd: improve handling of failures in interrupt handlers Tom Rix
@ 2022-04-12 21:42 ` Nathan Chancellor
2022-04-21 16:24 ` Greg KH
1 sibling, 0 replies; 3+ messages in thread
From: Nathan Chancellor @ 2022-04-12 21:42 UTC (permalink / raw)
To: Tom Rix
Cc: heikki.krogerus, gregkh, ndesaulniers, sven, alyssa, marcan,
martink, saranya.gopal, linux-usb, linux-kernel, llvm
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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] usb: typec: tipd: improve handling of failures in interrupt handlers
2022-04-09 13:00 [PATCH] usb: typec: tipd: improve handling of failures in interrupt handlers Tom Rix
2022-04-12 21:42 ` Nathan Chancellor
@ 2022-04-21 16:24 ` Greg KH
1 sibling, 0 replies; 3+ messages in thread
From: Greg KH @ 2022-04-21 16:24 UTC (permalink / raw)
To: Tom Rix
Cc: heikki.krogerus, nathan, ndesaulniers, sven, alyssa, marcan,
martink, saranya.gopal, linux-usb, linux-kernel, llvm
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.
Please use the full 72 columns.
>
> 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.
So the code today is correct, but clang is wrong? We don't need to do
anything then...
> tps6598x_interrupt() is similar.
This line makes no sense, sorry.
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-04-21 16:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-09 13:00 [PATCH] usb: typec: tipd: improve handling of failures in interrupt handlers Tom Rix
2022-04-12 21:42 ` Nathan Chancellor
2022-04-21 16:24 ` Greg KH
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.