Re: [PATCH] i2c-at91: fix data-loss issue
Am 16. April 2012 09:30 schrieb Voss, Nikolaus :
> Hubert Feurstein wrote on 2012-04-13:
>> In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
>> pending at the same time. In this case TXCOMP is handled but NOT RXRDY
>> which causes a data-loss on the current transfer
>
> Right, this is definitely a bug and must be corrected. Part of my
> motivation for exclusively or-ing the irq bits was not reading/
> writing beyond the buffer because of (still) pending bits despite
> of an already finished transfer, so I gave TXCOMP the highest prio.
>
> Because of other reasons, write_next_byte() already checks this and
> does nothing if all data already has been written. My suggestion is
> to have read_next_byte() do this check too, as I don't trust the
> hardware to reset RXRDY _immediately_ after reading.
Adding a check in read_next_byte() would be good just for safety.
>
>> @@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void
>> *dev_id)
>> {
>> struct at91_twi_dev *dev = dev_id;
>> const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
>> - const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
>> + unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
>> +
>> + irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
>
> The above line should be unnecessary as no more than those interrupts
> are enabled anyway. Any special reason for this?
No special reason for this.
>
>> + if (!irqstatus)
>> + return IRQ_NONE;
>> +
>> + if (irqstatus & AT91_TWI_RXRDY)
>> + at91_twi_read_next_byte(dev);
>> +
>> + if (irqstatus & AT91_TWI_TXRDY)
>> + at91_twi_write_next_byte(dev);
>
> I would like to exclusively or TXRDY and RXRDY as those really should
> not be active at the same time. Keeps the decision tree lean ;-).
I agree, it should be save to xor at least those two.
>
>> @@ -189,6 +193,10 @@ static int
>> at91_do_twi_transfer(struct at91_twi_dev *dev) if (dev->msg->flags &
>> I2C_M_RD) { unsigned start_flags = AT91_TWI_START;
>> + /* clear any pending data */
>> + (void)at91_twi_read(dev, AT91_TWI_SR);
>> + (void)at91_twi_read(dev, AT91_TWI_RHR);
>
> I would like to modify this, as this is a partial fix for the above bug
> which should already be fully fixed by the modified isr.
> I fear subtle data-loss if we make (partial) tabula rasa before each
> transfer. I'd rather add an assertion to check if the corresponding
> irqs are active as an indication for a driver/hw-bug.
You also can add both, print an error/warning if the state in SR is
not as expected and then add the two recovery lines.
>
> I'll repost the driver with your fix on positive feedback from you.
> Thanks for tracking this down.
>
> Ben, is there any chance to get this driver into next?
>
> Niko
>
>
Hubert
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c-at91: fix data-loss issue
In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
pending at the same time. In this case TXCOMP is handled but NOT RXRDY
which causes a data-loss on the current transfer. As a consequence also the
next transfer will be corrupt, because old data is pending in RHR.
To make sure that we do not start with old data in any case, SR and RHR is
read empty before initiating a new transfer.
Signed-off-by: Hubert Feurstein
---
This patch applies on top of [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add
new driver
drivers/i2c/busses/i2c-at91.c | 23 ---
1 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 40c39a2..295835f 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void
*dev_id)
{
struct at91_twi_dev *dev = dev_id;
const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
- const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
+ unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
+
+ irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
+ if (!irqstatus)
+ return IRQ_NONE;
+
+ if (irqstatus & AT91_TWI_RXRDY)
+ at91_twi_read_next_byte(dev);
+
+ if (irqstatus & AT91_TWI_TXRDY)
+ at91_twi_write_next_byte(dev);
if (irqstatus & AT91_TWI_TXCOMP) {
at91_disable_twi_interrupts(dev);
dev->transfer_status = status;
complete(&dev->cmd_complete);
- } else if (irqstatus & AT91_TWI_RXRDY) {
- at91_twi_read_next_byte(dev);
- } else if (irqstatus & AT91_TWI_TXRDY) {
- at91_twi_write_next_byte(dev);
- } else {
- return IRQ_NONE;
}
return IRQ_HANDLED;
@@ -189,6 +193,10 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
if (dev->msg->flags & I2C_M_RD) {
unsigned start_flags = AT91_TWI_START;
+ /* clear any pending data */
+ (void)at91_twi_read(dev, AT91_TWI_SR);
+ (void)at91_twi_read(dev, AT91_TWI_RHR);
+
/* if only one byte is to be read, immediately stop transfer */
if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
start_flags |= AT91_TWI_STOP;
@@ -259,6 +267,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct
i2c_msg *msg, int num)
internal_address |= addr << (8 * i);
int_addr_flag += AT91_TWI_IADRSZ_1;
}
+
at91_twi_write(dev, AT91_TWI_IADR, internal_address);
}
--
1.7.8.3
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
Hi Niko,
I'm using this driver since a while a now in my system
(soc:at91sam9m10; kernel:v3.2.14) and it works quite well. But
occasionally it happens that wrong data is read from my devices. I've
traced down the issue to the way how you do the interrupt handling. In
your code you do not consider that both status-flags (TXCOMP and
RXRDY) may be pending at the same time. So you handle the TXCOMP but
NOT the RXRDY which will cause a data-loss on the current transfer. As
a consequence also the next transfer will be corrupt, because you
start with old data in RHR. So I would suggest the following changes:
static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
{
struct at91_twi_dev *dev = dev_id;
const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
if (irqstatus & AT91_TWI_RXRDY) {
at91_twi_read_next_byte(dev);
irqstatus &= ~AT91_TWI_RXRDY;
}
if (irqstatus & AT91_TWI_TXRDY) {
at91_twi_write_next_byte(dev);
irqstatus &= ~AT91_TWI_TXRDY;
}
if (irqstatus & AT91_TWI_TXCOMP) {
at91_disable_twi_interrupts(dev);
dev->transfer_status = status;
complete(&dev->cmd_complete);
irqstatus &= ~AT91_TWI_TXCOMP;
}
if (irqstatus) {
/* There should be no pending interrupt anymore. *)
return IRQ_NONE;
}
return IRQ_HANDLED;
}
To make sure that we do not start with old data in any case, I would
suggest to read SR and RHR before initiating a new transfer.
static int at91_do_twi_transfer(struct at91_twi_dev *dev)
{
int ret;
dev_dbg(dev->dev, "transfer: %s %d bytes.\n",
(dev->msg->flags & I2C_M_RD) ? "read" : "write", dev->buf_len);
INIT_COMPLETION(dev->cmd_complete);
if (dev->msg->flags & I2C_M_RD) {
unsigned start_flags = AT91_TWI_START;
/* clear any pending data */
(void)at91_twi_read(dev, AT91_TWI_SR);
(void)at91_twi_read(dev, AT91_TWI_RHR);
/* if only one byte is to be read, immediately stop transfer */
if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
start_flags |= AT91_TWI_STOP;
at91_twi_write(dev, AT91_TWI_CR, start_flags);
[snip]
}
Hubert
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/5] AT91: replace broken TWI driver i2c-at91.c
Hi,
I've tested this driver on a 2.6.38 kernel with several i2c clients
(temp-sensor, audio-codec, touchscreen-controller, w1-bridge,
io-expanders) and works without problems. SoC: at91sam9g45
Because of the 2.6.38 kernel, I had to skip "[PATCH v7 2/5] Replace
clk_lookup.con_id with clk_lookup.dev_id entries for twi clk" and used
instead: at91_clock_associate("twi0_clk", &at91sam9g45_twi0_device.dev,
"twi_clk");
Best Regards
Hubert
On 2011-11-23 16:35, Nikolaus Voss wrote:
> The old driver has two main deficencies:
> i) No repeated start (Sr) condiction is possible, this makes it unusable
> e.g. for most SMBus transfers.
> ii) I/O was done with polling/busy waiting what caused over-/underruns
> even at light system loads and clock speeds.
>
> The new driver overcomes these deficencies and in addition allows for
> more than one TWI interface.
>
Tested-by: Hubert Feurstein
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] drivers/i2c/busses/i2c-at91.c: fix brokeness
Hi, sorry, but your patch still does not apply. You fixed the tab issue, but it seems that there are still some bad whitespaces left which corrupt your patch. Regards Hubert -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
