* [PATCH] i2c: xiic: Don't try to handle more interrupt events after error
@ 2023-06-06 18:25 Robert Hancock
2023-06-06 19:24 ` Andi Shyti
2023-07-06 19:33 ` Wolfram Sang
0 siblings, 2 replies; 9+ messages in thread
From: Robert Hancock @ 2023-06-06 18:25 UTC (permalink / raw)
To: Michal Simek, Andi Shyti, Shubhrajyoti Datta, Wolfram Sang,
Marek Vasut
Cc: linux-arm-kernel, linux-i2c, linux-kernel, Robert Hancock
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(+)
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 */
--
2.40.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: xiic: Don't try to handle more interrupt events after error
2023-06-06 18:25 [PATCH] i2c: xiic: Don't try to handle more interrupt events after error Robert Hancock
@ 2023-06-06 19:24 ` Andi Shyti
2023-06-06 19:40 ` Robert Hancock
2023-07-06 19:33 ` Wolfram Sang
1 sibling, 1 reply; 9+ messages in thread
From: Andi Shyti @ 2023-06-06 19:24 UTC (permalink / raw)
To: Robert Hancock
Cc: Michal Simek, Shubhrajyoti Datta, Wolfram Sang, Marek Vasut,
linux-arm-kernel, linux-i2c, linux-kernel
Hi Robert,
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.
what kind of 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>
please add:
Cc: <stable@vger.kernel.org> # v4.3+
> ---
> drivers/i2c/busses/i2c-xiic.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> 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;
why don't we have goto's after every irq evaluation but only
here? Do the issues you mentioned happen olny in this particular
error case?
Thanks,
Andi
> }
> if (pend & XIIC_INTR_RX_FULL_MASK) {
> /* Receive register/FIFO is full */
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: xiic: Don't try to handle more interrupt events after error
2023-06-06 19:24 ` Andi Shyti
@ 2023-06-06 19:40 ` Robert Hancock
2023-06-06 21:20 ` Andi Shyti
0 siblings, 1 reply; 9+ messages in thread
From: Robert Hancock @ 2023-06-06 19:40 UTC (permalink / raw)
To: andi.shyti@kernel.org
Cc: michal.simek@amd.com, shubhraj@xilinx.com, marex@denx.de,
linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org,
linux-kernel@vger.kernel.org, wsa@kernel.org
On Tue, 2023-06-06 at 21:24 +0200, Andi Shyti wrote:
> Hi Robert,
>
> 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.
>
> what kind of issues?
>
The one I ran into is what I alluded to further down, where that
WARN_ON was triggering repeatedly, which ended up flooding the kernel
log and causing the device's watchdog timer to timeout. I'm not sure
what other forms of havoc might ensue, other than "nothing good"..
> > 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>
>
> please add:
>
> Cc: <stable@vger.kernel.org> # v4.3+
>
Can add for a v2 - although with the Fixes tag it would most likely be
picked up for stable anyway..
> > ---
> > drivers/i2c/busses/i2c-xiic.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > 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;
>
> why don't we have goto's after every irq evaluation but only
> here? Do the issues you mentioned happen olny in this particular
> error case?
>
As far as I can tell, yes. For example you could legitimately have
XIIC_INTR_TX_EMPTY_MASK and XIIC_INTR_BNB_MASK both being handled. The
error case is special as we end up resetting the controller, so it
doesn't make sense to try to continue with the rest of the transaction
while also completing it with an error.
> Thanks,
> Andi
>
> > }
> > if (pend & XIIC_INTR_RX_FULL_MASK) {
> > /* Receive register/FIFO is full */
> > --
> > 2.40.1
> >
--
Robert Hancock <robert.hancock@calian.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: xiic: Don't try to handle more interrupt events after error
2023-06-06 19:40 ` Robert Hancock
@ 2023-06-06 21:20 ` Andi Shyti
2023-06-09 15:32 ` wsa
0 siblings, 1 reply; 9+ messages in thread
From: Andi Shyti @ 2023-06-06 21:20 UTC (permalink / raw)
To: Robert Hancock
Cc: michal.simek@amd.com, shubhraj@xilinx.com, marex@denx.de,
linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org,
linux-kernel@vger.kernel.org, wsa@kernel.org
Hi Robert,
On Tue, Jun 06, 2023 at 07:40:15PM +0000, Robert Hancock wrote:
> On Tue, 2023-06-06 at 21:24 +0200, Andi Shyti wrote:
> > Hi Robert,
> >
> > 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.
> >
> > what kind of issues?
> >
>
> The one I ran into is what I alluded to further down, where that
> WARN_ON was triggering repeatedly, which ended up flooding the kernel
> log and causing the device's watchdog timer to timeout. I'm not sure
> what other forms of havoc might ensue, other than "nothing good"..
>
> > > 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>
> >
> > please add:
> >
> > Cc: <stable@vger.kernel.org> # v4.3+
> >
>
> Can add for a v2 - although with the Fixes tag it would most likely be
> picked up for stable anyway..
It's just a courtesy to stable maintainers :)
> > > ---
> > > drivers/i2c/busses/i2c-xiic.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > 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;
> >
> > why don't we have goto's after every irq evaluation but only
> > here? Do the issues you mentioned happen olny in this particular
> > error case?
> >
>
> As far as I can tell, yes. For example you could legitimately have
> XIIC_INTR_TX_EMPTY_MASK and XIIC_INTR_BNB_MASK both being handled. The
> error case is special as we end up resetting the controller, so it
> doesn't make sense to try to continue with the rest of the transaction
> while also completing it with an error.
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?
Thanks a lot,
Andi
> > Thanks,
> > Andi
> >
> > > }
> > > if (pend & XIIC_INTR_RX_FULL_MASK) {
> > > /* Receive register/FIFO is full */
> > > --
> > > 2.40.1
> > >
>
> --
> Robert Hancock <robert.hancock@calian.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: xiic: Don't try to handle more interrupt events after error
2023-06-06 21:20 ` Andi Shyti
@ 2023-06-09 15:32 ` wsa
2023-06-09 21:25 ` Andi Shyti
0 siblings, 1 reply; 9+ messages in thread
From: wsa @ 2023-06-09 15:32 UTC (permalink / raw)
To: Andi Shyti
Cc: Robert Hancock, michal.simek@amd.com, shubhraj@xilinx.com,
marex@denx.de, linux-arm-kernel@lists.infradead.org,
linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 570 bytes --]
> 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?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: xiic: Don't try to handle more interrupt events after error
2023-06-09 15:32 ` wsa
@ 2023-06-09 21:25 ` Andi Shyti
2023-06-27 16:13 ` Robert Hancock
0 siblings, 1 reply; 9+ messages in thread
From: Andi Shyti @ 2023-06-09 21:25 UTC (permalink / raw)
To: wsa@kernel.org, Robert Hancock, michal.simek@amd.com,
shubhraj@xilinx.com, marex@denx.de,
linux-arm-kernel@lists.infradead.org, linux-i2c@vger.kernel.org,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: xiic: Don't try to handle more interrupt events after error
2023-06-09 21:25 ` Andi Shyti
@ 2023-06-27 16:13 ` Robert Hancock
2023-06-27 21:25 ` wsa
0 siblings, 1 reply; 9+ messages in thread
From: Robert Hancock @ 2023-06-27 16:13 UTC (permalink / raw)
To: wsa@kernel.org, linux-kernel@vger.kernel.org, shubhraj@xilinx.com,
michal.simek@amd.com, marex@denx.de, linux-i2c@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, andi.shyti@kernel.org
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.
--
Robert Hancock <robert.hancock@calian.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: xiic: Don't try to handle more interrupt events after error
2023-06-27 16:13 ` Robert Hancock
@ 2023-06-27 21:25 ` wsa
0 siblings, 0 replies; 9+ messages in thread
From: wsa @ 2023-06-27 21:25 UTC (permalink / raw)
To: Robert Hancock
Cc: linux-kernel@vger.kernel.org, shubhraj@xilinx.com,
michal.simek@amd.com, marex@denx.de, linux-i2c@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, andi.shyti@kernel.org
[-- Attachment #1: Type: text/plain, Size: 214 bytes --]
> 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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] i2c: xiic: Don't try to handle more interrupt events after error
2023-06-06 18:25 [PATCH] i2c: xiic: Don't try to handle more interrupt events after error Robert Hancock
2023-06-06 19:24 ` Andi Shyti
@ 2023-07-06 19:33 ` Wolfram Sang
1 sibling, 0 replies; 9+ messages in thread
From: Wolfram Sang @ 2023-07-06 19:33 UTC (permalink / raw)
To: Robert Hancock
Cc: Michal Simek, Andi Shyti, Shubhrajyoti Datta, Marek Vasut,
linux-arm-kernel, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]
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!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-07-06 19:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-06 18:25 [PATCH] i2c: xiic: Don't try to handle more interrupt events after error Robert Hancock
2023-06-06 19:24 ` Andi Shyti
2023-06-06 19:40 ` Robert Hancock
2023-06-06 21:20 ` Andi Shyti
2023-06-09 15:32 ` wsa
2023-06-09 21:25 ` Andi Shyti
2023-06-27 16:13 ` Robert Hancock
2023-06-27 21:25 ` wsa
2023-07-06 19:33 ` Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).