| Message ID | 20220409130013.1474412-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: typec: tipd: improve handling of failures in interrupt handlers | expand |
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 > >
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
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)
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(-)