linux-kernel - Re: [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

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:	Fri, 13 Apr 2012 12:17:59 +0200
From:	Hubert Feurstein <h.feurstein@...il.com>
To:	Nikolaus Voss <n.voss@...nmann.de>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-i2c@...r.kernel.org, nicolas.ferre@...el.com,
	ben-linux@...ff.org, balbi@...com, rmallon@...il.com
Subject: 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-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/