linux-kernel - Re: [PATCH] i2c: at91: fix write transfers by clearing pending interrupt first

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <561D20E7.5070405@lysator.liu.se>
Date:	Tue, 13 Oct 2015 17:19:03 +0200
From:	Peter Rosin <peda@...ator.liu.se>
To:	Ludovic Desroches <ludovic.desroches@...el.com>, wsa@...-dreams.de
Cc:	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, nicolas.ferre@...el.com,
	Cyrille Pitchen <cyrille.pitchen@...el.com>
Subject: Re: [PATCH] i2c: at91: fix write transfers by clearing pending
 interrupt first

On 2015-10-13 16:21, Ludovic Desroches wrote:
> From: Cyrille Pitchen <cyrille.pitchen@...el.com>
> 
> In some cases a NACK interrupt may be pending in the Status Register (SR)
> as a result of a previous transfer. However at91_do_twi_transfer() did not
> read the SR to clear pending interruptions before starting a new transfer.
> Hence a NACK interrupt rose as soon as it was enabled again at the I2C
> controller level, resulting in a wrong sequence of operations and strange
> patterns of behaviour on the I2C bus, such as a clock stretch followed by
> a restart of the transfer.
> 
> This first issue occurred with both DMA and PIO write transfers.
> 
> Also when a NACK error was detected during a PIO write transfer, the
> interrupt handler used to wrongly start a new transfer by writing into the
> Transmit Holding Register (THR). Then the I2C slave was likely to reply
> with a second NACK.
> 
> This second issue is fixed in atmel_twi_interrupt() by handling the TXRDY
> status bit only if both the TXCOMP and NACK status bits are cleared.
> 
> Tested with a at24 eeprom on sama5d36ek board running a linux-4.1-at91
> kernel image. Adapted to linux-next.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@...el.com>
> Fixes: 93563a6a71bb ("i2c: at91: fix a race condition when using the DMA controller")
> Reported-by: Peter Rosin <peda@...ator.liu.se>
> Signed-off-by: Ludovic Desroches <ludovic.desroches@...el.com>
> Cc: stable@...r.kernel.org #4.1

The regression is gone with this patch (vanilla 4.2), thank you!

However, looking at the bus, there are two NACKs after each
successful chunk of memory written by the eeprom driver.

Looking at the eeprom driver, I expect this on the bus:

S 0x50 0x00 "hello there guys" P
S 0x50 NACK P
delay for at least 1 ms (since the eeprom driver has a msleep(1) call).
S 0x50 NACK P
delay for at least 1 ms
...
...
S 0x50 NACK P
delay for at least 1 ms
S 0x50 0x10 "and girls\n" P

This is not what I observe on the bus, most of the time there is a
second NACK immediately following the first. I suspect that it is
the i2c bus driver that somehow confuses itself and reissues the
command for some reason?

But this behavior has been there since the beginning, so it's probably
orthogonal, and killing the data corrupting regression is much more
important to me than fussing over a surplus failed transfer. Hence

Tested-by: Peter Rosin <peda@...ator.liu.se>

Cheers,
Peter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/