i2c: at91: handle TXRDY interrupt spam
diff mbox series

Message ID 1556005008-6318-1-git-send-email-raagjadav@gmail.com
State Superseded
Headers show
Series
  • i2c: at91: handle TXRDY interrupt spam
Related show

Commit Message

Raag Jadav April 23, 2019, 7:36 a.m. UTC
Performing i2c write operation while SDA or SCL line is held
or grounded by slave device, we go into infinite at91_twi_write_next_byte
loop with TXRDY interrupt spam.

Signed-off-by: Raag Jadav <raagjadav@gmail.com>
---
 drivers/i2c/busses/i2c-at91.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Ludovic Desroches April 29, 2019, 9 a.m. UTC | #1
Hello Raag,

On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> External E-Mail
> 
> 
> Performing i2c write operation while SDA or SCL line is held
> or grounded by slave device, we go into infinite at91_twi_write_next_byte
> loop with TXRDY interrupt spam.

Sorry but I am not sure to have the full picture, the controller is in
slave or master mode?

SVREAD is only used in slave mode. When SVREAD is set, it means that a read
access is performed and your issue concerns the write operation.

Regards

Ludovic

> 
> Signed-off-by: Raag Jadav <raagjadav@gmail.com>
> ---
>  drivers/i2c/busses/i2c-at91.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 3f3e8b3..b2f5fdb 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -72,6 +72,7 @@
>  #define	AT91_TWI_TXCOMP		BIT(0)	/* Transmission Complete */
>  #define	AT91_TWI_RXRDY		BIT(1)	/* Receive Holding Register Ready */
>  #define	AT91_TWI_TXRDY		BIT(2)	/* Transmit Holding Register Ready */
> +#define	AT91_TWI_SVREAD		BIT(3)	/* Slave Read */
>  #define	AT91_TWI_OVRE		BIT(6)	/* Overrun Error */
>  #define	AT91_TWI_UNRE		BIT(7)	/* Underrun Error */
>  #define	AT91_TWI_NACK		BIT(8)	/* Not Acknowledged */
> @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
>  		at91_disable_twi_interrupts(dev);
>  		complete(&dev->cmd_complete);
>  	} else if (irqstatus & AT91_TWI_TXRDY) {
> -		at91_twi_write_next_byte(dev);
> +		if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> +			at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> +		else
> +			at91_twi_write_next_byte(dev);
>  	}
>  
>  	/* catch error flags */
> -- 
> 2.7.4
> 
>
Raag Jadav April 29, 2019, 10:33 p.m. UTC | #2
On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
> Hello Raag,
> 
> On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> > External E-Mail
> > 
> > 
> > Performing i2c write operation while SDA or SCL line is held
> > or grounded by slave device, we go into infinite at91_twi_write_next_byte
> > loop with TXRDY interrupt spam.
> 
> Sorry but I am not sure to have the full picture, the controller is in
> slave or master mode?
> 
> SVREAD is only used in slave mode. When SVREAD is set, it means that a read
> access is performed and your issue concerns the write operation.
> 
> Regards
> 
> Ludovic

Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
Couldn't think of a better way to handle such strange behaviour.
Any suggestions would be appreciated.

Cheers,
Raag

> 
> > 
> > Signed-off-by: Raag Jadav <raagjadav@gmail.com>
> > ---
> >  drivers/i2c/busses/i2c-at91.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> > index 3f3e8b3..b2f5fdb 100644
> > --- a/drivers/i2c/busses/i2c-at91.c
> > +++ b/drivers/i2c/busses/i2c-at91.c
> > @@ -72,6 +72,7 @@
> >  #define	AT91_TWI_TXCOMP		BIT(0)	/* Transmission Complete */
> >  #define	AT91_TWI_RXRDY		BIT(1)	/* Receive Holding Register Ready */
> >  #define	AT91_TWI_TXRDY		BIT(2)	/* Transmit Holding Register Ready */
> > +#define	AT91_TWI_SVREAD		BIT(3)	/* Slave Read */
> >  #define	AT91_TWI_OVRE		BIT(6)	/* Overrun Error */
> >  #define	AT91_TWI_UNRE		BIT(7)	/* Underrun Error */
> >  #define	AT91_TWI_NACK		BIT(8)	/* Not Acknowledged */
> > @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> >  		at91_disable_twi_interrupts(dev);
> >  		complete(&dev->cmd_complete);
> >  	} else if (irqstatus & AT91_TWI_TXRDY) {
> > -		at91_twi_write_next_byte(dev);
> > +		if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> > +			at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> > +		else
> > +			at91_twi_write_next_byte(dev);
> >  	}
> >  
> >  	/* catch error flags */
> > -- 
> > 2.7.4
> > 
> >
Ludovic Desroches May 2, 2019, 2:01 p.m. UTC | #3
On Tue, Apr 30, 2019 at 04:03:32AM +0530, Raag Jadav wrote:
> External E-Mail
> 
> 
> On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
> > Hello Raag,
> > 
> > On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> > > External E-Mail
> > > 
> > > 
> > > Performing i2c write operation while SDA or SCL line is held
> > > or grounded by slave device, we go into infinite at91_twi_write_next_byte
> > > loop with TXRDY interrupt spam.
> > 
> > Sorry but I am not sure to have the full picture, the controller is in
> > slave or master mode?
> > 
> > SVREAD is only used in slave mode. When SVREAD is set, it means that a read
> > access is performed and your issue concerns the write operation.
> > 
> > Regards
> > 
> > Ludovic
> 
> Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
> TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
> Couldn't think of a better way to handle such strange behaviour.
> Any suggestions would be appreciated.

I have the confirmation that you can't rely on the SVREAD flag when in
master mode. This flag should always have the same value.

I am trying to understand what could lead to your situation. Can you
give me more details. What kind of device it is? What does lead to this
situation? Does it happen randomly or not?

Regards

Ludovic

> 
> Cheers,
> Raag
> 
> > 
> > > 
> > > Signed-off-by: Raag Jadav <raagjadav@gmail.com>
> > > ---
> > >  drivers/i2c/busses/i2c-at91.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> > > index 3f3e8b3..b2f5fdb 100644
> > > --- a/drivers/i2c/busses/i2c-at91.c
> > > +++ b/drivers/i2c/busses/i2c-at91.c
> > > @@ -72,6 +72,7 @@
> > >  #define	AT91_TWI_TXCOMP		BIT(0)	/* Transmission Complete */
> > >  #define	AT91_TWI_RXRDY		BIT(1)	/* Receive Holding Register Ready */
> > >  #define	AT91_TWI_TXRDY		BIT(2)	/* Transmit Holding Register Ready */
> > > +#define	AT91_TWI_SVREAD		BIT(3)	/* Slave Read */
> > >  #define	AT91_TWI_OVRE		BIT(6)	/* Overrun Error */
> > >  #define	AT91_TWI_UNRE		BIT(7)	/* Underrun Error */
> > >  #define	AT91_TWI_NACK		BIT(8)	/* Not Acknowledged */
> > > @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> > >  		at91_disable_twi_interrupts(dev);
> > >  		complete(&dev->cmd_complete);
> > >  	} else if (irqstatus & AT91_TWI_TXRDY) {
> > > -		at91_twi_write_next_byte(dev);
> > > +		if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> > > +			at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> > > +		else
> > > +			at91_twi_write_next_byte(dev);
> > >  	}
> > >  
> > >  	/* catch error flags */
> > > -- 
> > > 2.7.4
> > > 
> > > 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Raag Jadav May 3, 2019, 11:58 p.m. UTC | #4
On Thu, May 02, 2019 at 04:01:16PM +0200, Ludovic Desroches wrote:
> On Tue, Apr 30, 2019 at 04:03:32AM +0530, Raag Jadav wrote:
> > External E-Mail
> > 
> > 
> > On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
> > > Hello Raag,
> > > 
> > > On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> > > > External E-Mail
> > > > 
> > > > 
> > > > Performing i2c write operation while SDA or SCL line is held
> > > > or grounded by slave device, we go into infinite at91_twi_write_next_byte
> > > > loop with TXRDY interrupt spam.
> > > 
> > > Sorry but I am not sure to have the full picture, the controller is in
> > > slave or master mode?
> > > 
> > > SVREAD is only used in slave mode. When SVREAD is set, it means that a read
> > > access is performed and your issue concerns the write operation.
> > > 
> > > Regards
> > > 
> > > Ludovic
> > 
> > Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
> > TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
> > Couldn't think of a better way to handle such strange behaviour.
> > Any suggestions would be appreciated.
> 
> I have the confirmation that you can't rely on the SVREAD flag when in
> master mode. This flag should always have the same value.
> 
> I am trying to understand what could lead to your situation. Can you
> give me more details. What kind of device it is? What does lead to this
> situation? Does it happen randomly or not?

One of the sama5d2 based board I worked on, was having trouble complete its boot
because of a faulty i2c device, which was randomly holding down the SDA line
on i2c write operation, not allowing the controller to complete its transmission,
causing a massive TXRDY interrupt spam, ultimately hanging the processor.

Another strange observation was that SVREAD was being set in the status register
along with TXRDY, every time I reproduced the issue.
You can reproduce it by simply grounding the SDA line and performing i2c write
on the bus.

Note that NACK, LOCK or TXCOMP are never set as the transmission never completes.
I'm not sure why slave bits are being set in master mode,
but it's been working reliably for me.

This patch doesn't recover the SDA line. It just prevents the processor from
getting hanged in case of i2c bus lockup.

Cheers,
Raag

> 
> Regards
> 
> Ludovic
> 
> > 
> > Cheers,
> > Raag
> > 
> > > 
> > > > 
> > > > Signed-off-by: Raag Jadav <raagjadav@gmail.com>
> > > > ---
> > > >  drivers/i2c/busses/i2c-at91.c | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> > > > index 3f3e8b3..b2f5fdb 100644
> > > > --- a/drivers/i2c/busses/i2c-at91.c
> > > > +++ b/drivers/i2c/busses/i2c-at91.c
> > > > @@ -72,6 +72,7 @@
> > > >  #define	AT91_TWI_TXCOMP		BIT(0)	/* Transmission Complete */
> > > >  #define	AT91_TWI_RXRDY		BIT(1)	/* Receive Holding Register Ready */
> > > >  #define	AT91_TWI_TXRDY		BIT(2)	/* Transmit Holding Register Ready */
> > > > +#define	AT91_TWI_SVREAD		BIT(3)	/* Slave Read */
> > > >  #define	AT91_TWI_OVRE		BIT(6)	/* Overrun Error */
> > > >  #define	AT91_TWI_UNRE		BIT(7)	/* Underrun Error */
> > > >  #define	AT91_TWI_NACK		BIT(8)	/* Not Acknowledged */
> > > > @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> > > >  		at91_disable_twi_interrupts(dev);
> > > >  		complete(&dev->cmd_complete);
> > > >  	} else if (irqstatus & AT91_TWI_TXRDY) {
> > > > -		at91_twi_write_next_byte(dev);
> > > > +		if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> > > > +			at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> > > > +		else
> > > > +			at91_twi_write_next_byte(dev);
> > > >  	}
> > > >  
> > > >  	/* catch error flags */
> > > > -- 
> > > > 2.7.4
> > > > 
> > > > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
Eugen Hristev May 6, 2019, 8:19 a.m. UTC | #5
On 04.05.2019 02:58, Raag Jadav wrote:

> On Thu, May 02, 2019 at 04:01:16PM +0200, Ludovic Desroches wrote:
>> On Tue, Apr 30, 2019 at 04:03:32AM +0530, Raag Jadav wrote:
>>> External E-Mail
>>>
>>>
>>> On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
>>>> Hello Raag,
>>>>
>>>> On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
>>>>> External E-Mail
>>>>>
>>>>>
>>>>> Performing i2c write operation while SDA or SCL line is held
>>>>> or grounded by slave device, we go into infinite at91_twi_write_next_byte
>>>>> loop with TXRDY interrupt spam.
>>>>
>>>> Sorry but I am not sure to have the full picture, the controller is in
>>>> slave or master mode?
>>>>
>>>> SVREAD is only used in slave mode. When SVREAD is set, it means that a read
>>>> access is performed and your issue concerns the write operation.
>>>>
>>>> Regards
>>>>
>>>> Ludovic
>>>
>>> Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
>>> TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
>>> Couldn't think of a better way to handle such strange behaviour.
>>> Any suggestions would be appreciated.
>>
>> I have the confirmation that you can't rely on the SVREAD flag when in
>> master mode. This flag should always have the same value.
>>
>> I am trying to understand what could lead to your situation. Can you
>> give me more details. What kind of device it is? What does lead to this
>> situation? Does it happen randomly or not?
> 
> One of the sama5d2 based board I worked on, was having trouble complete its boot
> because of a faulty i2c device, which was randomly holding down the SDA line
> on i2c write operation, not allowing the controller to complete its transmission,
> causing a massive TXRDY interrupt spam, ultimately hanging the processor.
> 
> Another strange observation was that SVREAD was being set in the status register
> along with TXRDY, every time I reproduced the issue.
> You can reproduce it by simply grounding the SDA line and performing i2c write
> on the bus.
> 
> Note that NACK, LOCK or TXCOMP are never set as the transmission never completes.
> I'm not sure why slave bits are being set in master mode,
> but it's been working reliably for me.
> 
> This patch doesn't recover the SDA line. It just prevents the processor from
> getting hanged in case of i2c bus lockup.

Hello,

I have noticed the same hanging at some points... In my case it is 
because of this patch:

commit e8f39e9fc0e0b7bce24922da925af820bacb8ef8
Author: David Engraf <david.engraf@sysgo.com>
Date:   Thu Apr 26 11:53:14 2018 +0200


diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index bfd1fdf..3f3e8b3 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -518,8 +518,16 @@ static irqreturn_t atmel_twi_interrupt(int irq, 
void *dev_id)
          * the RXRDY interrupt first in order to not keep garbage data 
in the
          * Receive Holding Register for the next transfer.
          */
-       if (irqstatus & AT91_TWI_RXRDY)
-               at91_twi_read_next_byte(dev);
+       if (irqstatus & AT91_TWI_RXRDY) {
+               /*
+                * Read all available bytes at once by polling RXRDY 
usable w/
+                * and w/o FIFO. With FIFO enabled we could also read 
RXFL and
+                * avoid polling RXRDY.
+                */
+               do {
+                       at91_twi_read_next_byte(dev);
+               } while (at91_twi_read(dev, AT91_TWI_SR) & AT91_TWI_RXRDY);
+       }


In my opinion having a do/while with an exit condition relying solely on 
a bit read from hardware is unacceptable in IRQ context - kernel can 
hang here.
A timeout would be a solution...

For me, reverting this patch solves hanging issues.

Hope this helps,

Eugen

> 
> Cheers,
> Raag
> 
>>
>> Regards
>>
>> Ludovic
>>
>>>
>>> Cheers,
>>> Raag
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Raag Jadav <raagjadav@gmail.com>
>>>>> ---
>>>>>   drivers/i2c/busses/i2c-at91.c | 6 +++++-
>>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
>>>>> index 3f3e8b3..b2f5fdb 100644
>>>>> --- a/drivers/i2c/busses/i2c-at91.c
>>>>> +++ b/drivers/i2c/busses/i2c-at91.c
>>>>> @@ -72,6 +72,7 @@
>>>>>   #define	AT91_TWI_TXCOMP		BIT(0)	/* Transmission Complete */
>>>>>   #define	AT91_TWI_RXRDY		BIT(1)	/* Receive Holding Register Ready */
>>>>>   #define	AT91_TWI_TXRDY		BIT(2)	/* Transmit Holding Register Ready */
>>>>> +#define	AT91_TWI_SVREAD		BIT(3)	/* Slave Read */
>>>>>   #define	AT91_TWI_OVRE		BIT(6)	/* Overrun Error */
>>>>>   #define	AT91_TWI_UNRE		BIT(7)	/* Underrun Error */
>>>>>   #define	AT91_TWI_NACK		BIT(8)	/* Not Acknowledged */
>>>>> @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
>>>>>   		at91_disable_twi_interrupts(dev);
>>>>>   		complete(&dev->cmd_complete);
>>>>>   	} else if (irqstatus & AT91_TWI_TXRDY) {
>>>>> -		at91_twi_write_next_byte(dev);
>>>>> +		if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
>>>>> +			at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
>>>>> +		else
>>>>> +			at91_twi_write_next_byte(dev);
>>>>>   	}
>>>>>   
>>>>>   	/* catch error flags */
>>>>> -- 
>>>>> 2.7.4
>>>>>
>>>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
>
Raag Jadav May 6, 2019, 4:02 p.m. UTC | #6
On Mon, May 06, 2019 at 08:19:01AM +0000, Eugen.Hristev@microchip.com wrote:
> 
> 
> On 04.05.2019 02:58, Raag Jadav wrote:
> 
> > On Thu, May 02, 2019 at 04:01:16PM +0200, Ludovic Desroches wrote:
> >> On Tue, Apr 30, 2019 at 04:03:32AM +0530, Raag Jadav wrote:
> >>> External E-Mail
> >>>
> >>>
> >>> On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
> >>>> Hello Raag,
> >>>>
> >>>> On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> >>>>> External E-Mail
> >>>>>
> >>>>>
> >>>>> Performing i2c write operation while SDA or SCL line is held
> >>>>> or grounded by slave device, we go into infinite at91_twi_write_next_byte
> >>>>> loop with TXRDY interrupt spam.
> >>>>
> >>>> Sorry but I am not sure to have the full picture, the controller is in
> >>>> slave or master mode?
> >>>>
> >>>> SVREAD is only used in slave mode. When SVREAD is set, it means that a read
> >>>> access is performed and your issue concerns the write operation.
> >>>>
> >>>> Regards
> >>>>
> >>>> Ludovic
> >>>
> >>> Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
> >>> TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
> >>> Couldn't think of a better way to handle such strange behaviour.
> >>> Any suggestions would be appreciated.
> >>
> >> I have the confirmation that you can't rely on the SVREAD flag when in
> >> master mode. This flag should always have the same value.
> >>
> >> I am trying to understand what could lead to your situation. Can you
> >> give me more details. What kind of device it is? What does lead to this
> >> situation? Does it happen randomly or not?
> > 
> > One of the sama5d2 based board I worked on, was having trouble complete its boot
> > because of a faulty i2c device, which was randomly holding down the SDA line
> > on i2c write operation, not allowing the controller to complete its transmission,
> > causing a massive TXRDY interrupt spam, ultimately hanging the processor.
> > 
> > Another strange observation was that SVREAD was being set in the status register
> > along with TXRDY, every time I reproduced the issue.
> > You can reproduce it by simply grounding the SDA line and performing i2c write
> > on the bus.
> > 
> > Note that NACK, LOCK or TXCOMP are never set as the transmission never completes.
> > I'm not sure why slave bits are being set in master mode,
> > but it's been working reliably for me.
> > 
> > This patch doesn't recover the SDA line. It just prevents the processor from
> > getting hanged in case of i2c bus lockup.
> 
> Hello,
> 
> I have noticed the same hanging at some points... In my case it is 
> because of this patch:
> 
> commit e8f39e9fc0e0b7bce24922da925af820bacb8ef8
> Author: David Engraf <david.engraf@sysgo.com>
> Date:   Thu Apr 26 11:53:14 2018 +0200
> 
> 
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index bfd1fdf..3f3e8b3 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -518,8 +518,16 @@ static irqreturn_t atmel_twi_interrupt(int irq, 
> void *dev_id)
>           * the RXRDY interrupt first in order to not keep garbage data 
> in the
>           * Receive Holding Register for the next transfer.
>           */
> -       if (irqstatus & AT91_TWI_RXRDY)
> -               at91_twi_read_next_byte(dev);
> +       if (irqstatus & AT91_TWI_RXRDY) {
> +               /*
> +                * Read all available bytes at once by polling RXRDY 
> usable w/
> +                * and w/o FIFO. With FIFO enabled we could also read 
> RXFL and
> +                * avoid polling RXRDY.
> +                */
> +               do {
> +                       at91_twi_read_next_byte(dev);
> +               } while (at91_twi_read(dev, AT91_TWI_SR) & AT91_TWI_RXRDY);
> +       }
> 
> 
> In my opinion having a do/while with an exit condition relying solely on 
> a bit read from hardware is unacceptable in IRQ context - kernel can 
> hang here.
> A timeout would be a solution...
> 
> For me, reverting this patch solves hanging issues.
> 
> Hope this helps,
> 
> Eugen

Thank you for your input, but my issue concerns i2c write operation.
I haven't noticed any issue with i2c read yet.
But given the same scenario, it could be true for RXRDY as well.

Cheers,
Raag

> 
> > 
> > Cheers,
> > Raag
> > 
> >>
> >> Regards
> >>
> >> Ludovic
> >>
> >>>
> >>> Cheers,
> >>> Raag
> >>>
> >>>>
> >>>>>
> >>>>> Signed-off-by: Raag Jadav <raagjadav@gmail.com>
> >>>>> ---
> >>>>>   drivers/i2c/busses/i2c-at91.c | 6 +++++-
> >>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> >>>>> index 3f3e8b3..b2f5fdb 100644
> >>>>> --- a/drivers/i2c/busses/i2c-at91.c
> >>>>> +++ b/drivers/i2c/busses/i2c-at91.c
> >>>>> @@ -72,6 +72,7 @@
> >>>>>   #define	AT91_TWI_TXCOMP		BIT(0)	/* Transmission Complete */
> >>>>>   #define	AT91_TWI_RXRDY		BIT(1)	/* Receive Holding Register Ready */
> >>>>>   #define	AT91_TWI_TXRDY		BIT(2)	/* Transmit Holding Register Ready */
> >>>>> +#define	AT91_TWI_SVREAD		BIT(3)	/* Slave Read */
> >>>>>   #define	AT91_TWI_OVRE		BIT(6)	/* Overrun Error */
> >>>>>   #define	AT91_TWI_UNRE		BIT(7)	/* Underrun Error */
> >>>>>   #define	AT91_TWI_NACK		BIT(8)	/* Not Acknowledged */
> >>>>> @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> >>>>>   		at91_disable_twi_interrupts(dev);
> >>>>>   		complete(&dev->cmd_complete);
> >>>>>   	} else if (irqstatus & AT91_TWI_TXRDY) {
> >>>>> -		at91_twi_write_next_byte(dev);
> >>>>> +		if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> >>>>> +			at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> >>>>> +		else
> >>>>> +			at91_twi_write_next_byte(dev);
> >>>>>   	}
> >>>>>   
> >>>>>   	/* catch error flags */
> >>>>> -- 
> >>>>> 2.7.4
> >>>>>
> >>>>>
> >>>
> >>> _______________________________________________
> >>> linux-arm-kernel mailing list
> >>> linux-arm-kernel@lists.infradead.org
> >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>>
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> >
Ludovic Desroches May 14, 2019, 8:42 a.m. UTC | #7
On Sat, May 04, 2019 at 05:28:51AM +0530, Raag Jadav wrote:
> On Thu, May 02, 2019 at 04:01:16PM +0200, Ludovic Desroches wrote:
> > On Tue, Apr 30, 2019 at 04:03:32AM +0530, Raag Jadav wrote:
> > > External E-Mail
> > > 
> > > 
> > > On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
> > > > Hello Raag,
> > > > 
> > > > On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> > > > > External E-Mail
> > > > > 
> > > > > 
> > > > > Performing i2c write operation while SDA or SCL line is held
> > > > > or grounded by slave device, we go into infinite at91_twi_write_next_byte
> > > > > loop with TXRDY interrupt spam.
> > > > 
> > > > Sorry but I am not sure to have the full picture, the controller is in
> > > > slave or master mode?
> > > > 
> > > > SVREAD is only used in slave mode. When SVREAD is set, it means that a read
> > > > access is performed and your issue concerns the write operation.
> > > > 
> > > > Regards
> > > > 
> > > > Ludovic
> > > 
> > > Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
> > > TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
> > > Couldn't think of a better way to handle such strange behaviour.
> > > Any suggestions would be appreciated.
> > 
> > I have the confirmation that you can't rely on the SVREAD flag when in
> > master mode. This flag should always have the same value.
> > 
> > I am trying to understand what could lead to your situation. Can you
> > give me more details. What kind of device it is? What does lead to this
> > situation? Does it happen randomly or not?
> 
> One of the sama5d2 based board I worked on, was having trouble complete its boot
> because of a faulty i2c device, which was randomly holding down the SDA line
> on i2c write operation, not allowing the controller to complete its transmission,
> causing a massive TXRDY interrupt spam, ultimately hanging the processor.
> 
> Another strange observation was that SVREAD was being set in the status register
> along with TXRDY, every time I reproduced the issue.
> You can reproduce it by simply grounding the SDA line and performing i2c write
> on the bus.

Thanks for the details, I'll discussed it with hw guys but expect some
dealy as I'll be off next 2 weeks.

Regards

Ludovic

> 
> Note that NACK, LOCK or TXCOMP are never set as the transmission never completes.
> I'm not sure why slave bits are being set in master mode,
> but it's been working reliably for me.
> 
> This patch doesn't recover the SDA line. It just prevents the processor from
> getting hanged in case of i2c bus lockup.
> 
> Cheers,
> Raag
> 
> > 
> > Regards
> > 
> > Ludovic
> > 
> > > 
> > > Cheers,
> > > Raag
> > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Raag Jadav <raagjadav@gmail.com>
> > > > > ---
> > > > >  drivers/i2c/busses/i2c-at91.c | 6 +++++-
> > > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> > > > > index 3f3e8b3..b2f5fdb 100644
> > > > > --- a/drivers/i2c/busses/i2c-at91.c
> > > > > +++ b/drivers/i2c/busses/i2c-at91.c
> > > > > @@ -72,6 +72,7 @@
> > > > >  #define	AT91_TWI_TXCOMP		BIT(0)	/* Transmission Complete */
> > > > >  #define	AT91_TWI_RXRDY		BIT(1)	/* Receive Holding Register Ready */
> > > > >  #define	AT91_TWI_TXRDY		BIT(2)	/* Transmit Holding Register Ready */
> > > > > +#define	AT91_TWI_SVREAD		BIT(3)	/* Slave Read */
> > > > >  #define	AT91_TWI_OVRE		BIT(6)	/* Overrun Error */
> > > > >  #define	AT91_TWI_UNRE		BIT(7)	/* Underrun Error */
> > > > >  #define	AT91_TWI_NACK		BIT(8)	/* Not Acknowledged */
> > > > > @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> > > > >  		at91_disable_twi_interrupts(dev);
> > > > >  		complete(&dev->cmd_complete);
> > > > >  	} else if (irqstatus & AT91_TWI_TXRDY) {
> > > > > -		at91_twi_write_next_byte(dev);
> > > > > +		if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> > > > > +			at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> > > > > +		else
> > > > > +			at91_twi_write_next_byte(dev);
> > > > >  	}
> > > > >  
> > > > >  	/* catch error flags */
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > > > 
> > > 
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > >
Ludovic Desroches May 14, 2019, 8:52 a.m. UTC | #8
On Mon, May 06, 2019 at 10:19:01AM +0200, Eugen Hristev - M18282 wrote:
> 
> 
> On 04.05.2019 02:58, Raag Jadav wrote:
> 
> > On Thu, May 02, 2019 at 04:01:16PM +0200, Ludovic Desroches wrote:
> >> On Tue, Apr 30, 2019 at 04:03:32AM +0530, Raag Jadav wrote:
> >>> External E-Mail
> >>>
> >>>
> >>> On Mon, Apr 29, 2019 at 11:00:05AM +0200, Ludovic Desroches wrote:
> >>>> Hello Raag,
> >>>>
> >>>> On Tue, Apr 23, 2019 at 01:06:48PM +0530, Raag Jadav wrote:
> >>>>> External E-Mail
> >>>>>
> >>>>>
> >>>>> Performing i2c write operation while SDA or SCL line is held
> >>>>> or grounded by slave device, we go into infinite at91_twi_write_next_byte
> >>>>> loop with TXRDY interrupt spam.
> >>>>
> >>>> Sorry but I am not sure to have the full picture, the controller is in
> >>>> slave or master mode?
> >>>>
> >>>> SVREAD is only used in slave mode. When SVREAD is set, it means that a read
> >>>> access is performed and your issue concerns the write operation.
> >>>>
> >>>> Regards
> >>>>
> >>>> Ludovic
> >>>
> >>> Yes, even though the datasheet suggests that SVREAD is irrelevant in master mode,
> >>> TXRDY and SVREAD are the only ones being set in status register upon reproducing the issue.
> >>> Couldn't think of a better way to handle such strange behaviour.
> >>> Any suggestions would be appreciated.
> >>
> >> I have the confirmation that you can't rely on the SVREAD flag when in
> >> master mode. This flag should always have the same value.
> >>
> >> I am trying to understand what could lead to your situation. Can you
> >> give me more details. What kind of device it is? What does lead to this
> >> situation? Does it happen randomly or not?
> > 
> > One of the sama5d2 based board I worked on, was having trouble complete its boot
> > because of a faulty i2c device, which was randomly holding down the SDA line
> > on i2c write operation, not allowing the controller to complete its transmission,
> > causing a massive TXRDY interrupt spam, ultimately hanging the processor.
> > 
> > Another strange observation was that SVREAD was being set in the status register
> > along with TXRDY, every time I reproduced the issue.
> > You can reproduce it by simply grounding the SDA line and performing i2c write
> > on the bus.
> > 
> > Note that NACK, LOCK or TXCOMP are never set as the transmission never completes.
> > I'm not sure why slave bits are being set in master mode,
> > but it's been working reliably for me.
> > 
> > This patch doesn't recover the SDA line. It just prevents the processor from
> > getting hanged in case of i2c bus lockup.
> 
> Hello,
> 
> I have noticed the same hanging at some points... In my case it is 
> because of this patch:
> 
> commit e8f39e9fc0e0b7bce24922da925af820bacb8ef8
> Author: David Engraf <david.engraf@sysgo.com>
> Date:   Thu Apr 26 11:53:14 2018 +0200
> 

Good to know.

> 
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index bfd1fdf..3f3e8b3 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -518,8 +518,16 @@ static irqreturn_t atmel_twi_interrupt(int irq, 
> void *dev_id)
>           * the RXRDY interrupt first in order to not keep garbage data 
> in the
>           * Receive Holding Register for the next transfer.
>           */
> -       if (irqstatus & AT91_TWI_RXRDY)
> -               at91_twi_read_next_byte(dev);
> +       if (irqstatus & AT91_TWI_RXRDY) {
> +               /*
> +                * Read all available bytes at once by polling RXRDY 
> usable w/
> +                * and w/o FIFO. With FIFO enabled we could also read 
> RXFL and
> +                * avoid polling RXRDY.
> +                */
> +               do {
> +                       at91_twi_read_next_byte(dev);
> +               } while (at91_twi_read(dev, AT91_TWI_SR) & AT91_TWI_RXRDY);
> +       }
> 
> 
> In my opinion having a do/while with an exit condition relying solely on 
> a bit read from hardware is unacceptable in IRQ context - kernel can 
> hang here.
> A timeout would be a solution...

You're right with a faulty hardware it can lead to disaster. As you
mentionned issues with this patch, the end of loop condition is not good
as it can stay true indefinitely.

For sure a timeout is a solution but its value can be controversial.
Maybe there is a better combination of flags to check in the status
register. I'll see this point too.

Regards

Ludovic

> 
> For me, reverting this patch solves hanging issues.
> 
> Hope this helps,
> 
> Eugen
> 
> > 
> > Cheers,
> > Raag
> > 
> >>
> >> Regards
> >>
> >> Ludovic
> >>
> >>>
> >>> Cheers,
> >>> Raag
> >>>
> >>>>
> >>>>>
> >>>>> Signed-off-by: Raag Jadav <raagjadav@gmail.com>
> >>>>> ---
> >>>>>   drivers/i2c/busses/i2c-at91.c | 6 +++++-
> >>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> >>>>> index 3f3e8b3..b2f5fdb 100644
> >>>>> --- a/drivers/i2c/busses/i2c-at91.c
> >>>>> +++ b/drivers/i2c/busses/i2c-at91.c
> >>>>> @@ -72,6 +72,7 @@
> >>>>>   #define	AT91_TWI_TXCOMP		BIT(0)	/* Transmission Complete */
> >>>>>   #define	AT91_TWI_RXRDY		BIT(1)	/* Receive Holding Register Ready */
> >>>>>   #define	AT91_TWI_TXRDY		BIT(2)	/* Transmit Holding Register Ready */
> >>>>> +#define	AT91_TWI_SVREAD		BIT(3)	/* Slave Read */
> >>>>>   #define	AT91_TWI_OVRE		BIT(6)	/* Overrun Error */
> >>>>>   #define	AT91_TWI_UNRE		BIT(7)	/* Underrun Error */
> >>>>>   #define	AT91_TWI_NACK		BIT(8)	/* Not Acknowledged */
> >>>>> @@ -571,7 +572,10 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> >>>>>   		at91_disable_twi_interrupts(dev);
> >>>>>   		complete(&dev->cmd_complete);
> >>>>>   	} else if (irqstatus & AT91_TWI_TXRDY) {
> >>>>> -		at91_twi_write_next_byte(dev);
> >>>>> +		if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
> >>>>> +			at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
> >>>>> +		else
> >>>>> +			at91_twi_write_next_byte(dev);
> >>>>>   	}
> >>>>>   
> >>>>>   	/* catch error flags */
> >>>>> -- 
> >>>>> 2.7.4
> >>>>>
> >>>>>
> >>>
> >>> _______________________________________________
> >>> linux-arm-kernel mailing list
> >>> linux-arm-kernel@lists.infradead.org
> >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>>
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> >

Patch
diff mbox series

diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 3f3e8b3..b2f5fdb 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -72,6 +72,7 @@ 
 #define	AT91_TWI_TXCOMP		BIT(0)	/* Transmission Complete */
 #define	AT91_TWI_RXRDY		BIT(1)	/* Receive Holding Register Ready */
 #define	AT91_TWI_TXRDY		BIT(2)	/* Transmit Holding Register Ready */
+#define	AT91_TWI_SVREAD		BIT(3)	/* Slave Read */
 #define	AT91_TWI_OVRE		BIT(6)	/* Overrun Error */
 #define	AT91_TWI_UNRE		BIT(7)	/* Underrun Error */
 #define	AT91_TWI_NACK		BIT(8)	/* Not Acknowledged */
@@ -571,7 +572,10 @@  static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
 		at91_disable_twi_interrupts(dev);
 		complete(&dev->cmd_complete);
 	} else if (irqstatus & AT91_TWI_TXRDY) {
-		at91_twi_write_next_byte(dev);
+		if ((status & AT91_TWI_SVREAD) && (dev->buf_len == 0))
+			at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_TXRDY);
+		else
+			at91_twi_write_next_byte(dev);
 	}
 
 	/* catch error flags */