linux-kernel - AW: [PATCH v5 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]

Message-ID: <D99582E5322435468A77E74BB0039E7B1D1655C047@SRV02.hamburg.garz-fricke.de>
Date:	Wed, 23 Nov 2011 15:21:52 +0100
From:	Carsten Behling <carsten.behling@...z-fricke.com>
To:	"Voss, Nikolaus" <N.Voss@...nmann.de>
CC:	"'linux-i2c@...r.kernel.org'" <linux-i2c@...r.kernel.org>,
	"'linux-arm-kernel@...ts.infradead.org'" 
	<linux-arm-kernel@...ts.infradead.org>,
	"'linux-kernel@...r.kernel.org'" <linux-kernel@...r.kernel.org>
Subject: AW: [PATCH v5 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

Hi,

I try to use the at24 eeprom driver on top of this driver.

This EEPROM (24c32) works with two address bytes.

Writing results in a call to at91_twi_xfer() with num=1.
In this case the internal address register is not used and the address is sent out within the buffer.

Reading results in a call to at91_twi_xfer() with num=2.
In this case the internal address register is used.

However the MSB of the internal address resides in msg->buf[0] and the LSB resides in msg->buf[1] of the first message.

As a result the code:

+		for (i = 0; i < msg->len; ++i) {
+			internal_address |= ((unsigned)msg->buf[i]) << (8 * i);
+			int_addr_flag += AT91_TWI_IADRSZ_1;
+		}
+		at91_twi_write(dev, AT91_TWI_IADR, internal_address);

constructs an internal address in a wrong byte order.

Example: Try to read from address 0x100:

msg[0]->buf[0] = 0x1; 
msg[0]->buf[1] = 0x0;

results in

internal_address = 0x1;

I think it must be:

+		for (i = 0; i < msg->len; ++i) {
+			internal_address |= ((unsigned)msg->buf[msg->len-1-i]) << (8 * i);
+			int_addr_flag += AT91_TWI_IADRSZ_1;
+		}
+		at91_twi_write(dev, AT91_TWI_IADR, internal_address);

... or the at24 eeprom driver constructs the wrong internal address ...

Mit freundlichen Grüßen / Best regards
Carsten Behling

Development Engineer
Garz & Fricke GmbH
Tempowerkring 2, 21079 Hamburg - Germany
Amtsgericht Hamburg HRB 60514
Geschäftsführer: Manfred Garz, Matthias Fricke
Phone: +49 (0) 40 791 899 - 56
Fax:    +49 40 / 791 899 - 39
www.garz-fricke.com 

-----Ursprüngliche Nachricht-----
Von: Voss, Nikolaus [mailto:N.Voss@...nmann.de] 
Gesendet: Mittwoch, 23. November 2011 11:29
An: Carsten Behling
Cc: 'linux-i2c@...r.kernel.org'; 'linux-arm-kernel@...ts.infradead.org'; 'linux-kernel@...r.kernel.org'
Betreff: RE: [PATCH v5 3/4] drivers/i2c/busses/i2c-at91.c: add new driver

Hi,

Carsten Behling wrote on 2011-11-23:

>> this case is already catched in at91_do_twi_transfer():
> 
> Sorry, I did not found this code in your patch.
> (http://www.mail-archive.com/linux-i2c@vger.kernel.org/msg06556.html):
> 
>> +       if (is_read) {
>> +               if (!dev->buf_len)

yes, this won't work for buf_len == 1. It is corrected in
https://lkml.org/lkml/2011/11/18/223 which I held back for some time
as it should have been just a feature extension. I was not aware it
also fixed the buf_len = 1 bug. Sorry for that...

(Explanation: in the first implementation I immediately decremented
buf_len, so buf_len == 1 could not occur. Later I removed that but
did not fully fold it into the base patch.)

Thanks,
Niko

--
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/