| Message ID | 20230606182558.1301413-1-robert.hancock@calian.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: xiic: Don't try to handle more interrupt events after error | expand |
> I think the patch is correct and I will ack it: > > Acked-by: Andi Shyti <andi.shyti@kernel.org> > > I think, though, that this needs a proper fix and testing, in > order to cover all the possible combinations. The scenario you > highlighted is indeed one, but not only, potential situation that > could arise. > > Can I just ask you to write a bit more in the comment to > highlight the possible failure? I tend to apply it to for-current because it improves the situation. Further improvements could be made incrementally? D'accord everyone?
Hi Wolfram, On Fri, Jun 09, 2023 at 05:32:42PM +0200, wsa@kernel.org wrote: > > > I think the patch is correct and I will ack it: > > > > Acked-by: Andi Shyti <andi.shyti@kernel.org> > > > > I think, though, that this needs a proper fix and testing, in > > order to cover all the possible combinations. The scenario you > > highlighted is indeed one, but not only, potential situation that > > could arise. > > > > Can I just ask you to write a bit more in the comment to > > highlight the possible failure? > > I tend to apply it to for-current because it improves the situation. > Further improvements could be made incrementally? D'accord everyone? > OK with that! Thanks, Andi
On Fri, 2023-06-09 at 23:25 +0200, Andi Shyti wrote: > Hi Wolfram, > > On Fri, Jun 09, 2023 at 05:32:42PM +0200, wsa@kernel.org wrote: > > > > > I think the patch is correct and I will ack it: > > > > > > Acked-by: Andi Shyti <andi.shyti@kernel.org> > > > > > > I think, though, that this needs a proper fix and testing, in > > > order to cover all the possible combinations. The scenario you > > > highlighted is indeed one, but not only, potential situation that > > > could arise. > > > > > > Can I just ask you to write a bit more in the comment to > > > highlight the possible failure? > > > > I tend to apply it to for-current because it improves the > > situation. > > Further improvements could be made incrementally? D'accord > > everyone? > > > > OK with that! > > Thanks, > Andi Just checking on this patch, was it merged? It shows accepted in Patchwork, but I'm not seeing it in the Git tree.
> Just checking on this patch, was it merged? It shows accepted in > Patchwork, but I'm not seeing it in the Git tree. Sorry, it seems I forgot to apply it? I will make sure it will go in this merge window now.
On Tue, Jun 06, 2023 at 12:25:58PM -0600, Robert Hancock wrote: > In xiic_process, it is possible that error events such as arbitration > lost or TX error can be raised in conjunction with other interrupt flags > such as TX FIFO empty or bus not busy. Error events result in the > controller being reset and the error returned to the calling request, > but the function could potentially try to keep handling the other > events, such as by writing more messages into the TX FIFO. Since the > transaction has already failed, this is not helpful and will just cause > issues. > > This problem has been present ever since: > > commit 7f9906bd7f72 ("i2c: xiic: Service all interrupts in isr") > > which allowed non-error events to be handled after errors, but became > more obvious after: > > commit 743e227a8959 ("i2c: xiic: Defer xiic_wakeup() and > __xiic_start_xfer() in xiic_process()") > > which reworked the code to add a WARN_ON which triggers if both the > xfer_more and wakeup_req flags were set, since this combination is > not supposed to happen, but was occurring in this scenario. > > Skip further interrupt handling after error flags are detected to avoid > this problem. > > Fixes: 7f9906bd7f72 ("i2c: xiic: Service all interrupts in isr") > Signed-off-by: Robert Hancock <robert.hancock@calian.com> Applied to for-current, thanks!
diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index 8a3d9817cb41..ee6edc963dea 100644 --- a/drivers/i2c/busses/i2c-xiic.c +++ b/drivers/i2c/busses/i2c-xiic.c @@ -721,6 +721,8 @@ static irqreturn_t xiic_process(int irq, void *dev_id) wakeup_req = 1; wakeup_code = STATE_ERROR; } + /* don't try to handle other events */ + goto out; } if (pend & XIIC_INTR_RX_FULL_MASK) { /* Receive register/FIFO is full */
In xiic_process, it is possible that error events such as arbitration lost or TX error can be raised in conjunction with other interrupt flags such as TX FIFO empty or bus not busy. Error events result in the controller being reset and the error returned to the calling request, but the function could potentially try to keep handling the other events, such as by writing more messages into the TX FIFO. Since the transaction has already failed, this is not helpful and will just cause issues. This problem has been present ever since: commit 7f9906bd7f72 ("i2c: xiic: Service all interrupts in isr") which allowed non-error events to be handled after errors, but became more obvious after: commit 743e227a8959 ("i2c: xiic: Defer xiic_wakeup() and __xiic_start_xfer() in xiic_process()") which reworked the code to add a WARN_ON which triggers if both the xfer_more and wakeup_req flags were set, since this combination is not supposed to happen, but was occurring in this scenario. Skip further interrupt handling after error flags are detected to avoid this problem. Fixes: 7f9906bd7f72 ("i2c: xiic: Service all interrupts in isr") Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- drivers/i2c/busses/i2c-xiic.c | 2 ++ 1 file changed, 2 insertions(+)