linux-kernel - Re: [PATCH] i2c-at91: fix data-loss issue

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Mon, 16 Apr 2012 11:27:29 +0200
From:	Hubert Feurstein <h.feurstein@...il.com>
To:	"Voss, Nikolaus" <N.Voss@...nmann.de>
Cc:	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"nicolas.ferre@...el.com" <nicolas.ferre@...el.com>,
	"ben-linux@...ff.org" <ben-linux@...ff.org>,
	"balbi@...com" <balbi@...com>,
	"rmallon@...il.com" <rmallon@...il.com>
Subject: Re: [PATCH] i2c-at91: fix data-loss issue

Am 16. April 2012 09:30 schrieb Voss, Nikolaus <N.Voss@...nmann.de>:
> 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-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/