From 404ced2600ad39ccc0b6add7e4f38721a3c38dab Mon Sep 17 00:00:00 2001 From: "chrisw@vas.sous-sol.org" Date: Wed, 9 Mar 2005 11:53:49 -0800 Subject: [PATCH] [PATCH] i2c-message-flags-fix.patch (needs testing and rationale) --- amd64-oops-on-modprobe.patch | 196 +++++++++++++++++++++++++++++++++++ i2c-message-flags-fix.patch | 97 +++++++++++++++++ 2 files changed, 293 insertions(+) create mode 100644 amd64-oops-on-modprobe.patch create mode 100644 i2c-message-flags-fix.patch diff --git a/amd64-oops-on-modprobe.patch b/amd64-oops-on-modprobe.patch new file mode 100644 index 00000000000..945b830fbf4 --- /dev/null +++ b/amd64-oops-on-modprobe.patch @@ -0,0 +1,196 @@ +Date: Tue, 8 Mar 2005 20:15:04 +0100 +From: Jean Delvare +To: "Randy.Dunlap" +Cc: Daniel Staaf , LKML , + Andrei Mikhailovsky , + Ian Campbell , + Ronald Bultje , + Gerd Knorr +Subject: Re: amd64 2.6.11 oops on modprobe +Lines: 182 + +[Randy Dunlap] +> I've been working on this with Daniel Staaf and Jean Delvare. +> Jean enabled some more/different I2C bit banging code in +> 2.6.11, and that causes callers to use it differently. + +Actually credits go to Ian Campbell for noticing that i2c-algo-bit was +reporting less capabilities than it should have. Fixing this uncovered +the bug in the saa7110 driver as a side effect but still was the right +thing to do (this will let i2c chip drivers use faster i2c transfer +commands wherever possible.) + +> Jean will be proposing a real patch for that obfuscated loop. + +I'd like to add details on what exactly the problems were in the +original code, in the hope it can help make my patch easier to +understand. As a side note, this was by far the crapiest code I read for +the last few months. + +Here's the faulty code, with my comments: + + u8 reg = *data++; + (...) + if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { + struct saa7110 *decoder = i2c_get_clientdata(client); + struct i2c_msg msg; +*1* u8 block_data[54]; + + msg.len = 0; + msg.buf = (char *) block_data; + msg.addr = client->addr; +*2* msg.flags = client->flags; + while (len >= 1) { + msg.len = 0; + block_data[msg.len++] = reg; +*3* while (len-- >= 1 && msg.len < 54) + block_data[msg.len++] = +*4* decoder->reg[reg++] = *data++; +*5* ret = i2c_transfer(client->adapter, &msg, 1); + } + } (...) + +1* This local buffer is not needed. The original buffer (pointed to by +data) is already properly formated for an i2c_transfer message. + +2* i2c message flags and i2c client flags are essentially different +things [1], so this line doesn't make any sense. In fact, it happens +that client->flags contains I2C_CLIENT_ALLOW_USE (0x01) which will be +interpreted as I2C_M_RD as a message flag, resulting in an I2C block +*read* command instead of the expected I2C block write command. So, oops +or not, there is no chance this code would do anything useful [2]. + +3* The oops is here. When len == 0, the test will fail but len will +still be decreased. Because len is unsigned, this will result in a huge +positive value, so the outer while loop will not be exited as expected. +One hundred iterations later the oops occurs. Mental note: mixing while +loop condition with counter decrement sounds like a generally bad idea. +The code here clearly yields for a clean for loop. + +4* Note that reg is not reset in the outer while loop, so it can take +any arbitrarily large value, while decoder->reg is only 54 bytes long. +So there is a potential buffer overrun here. More generally, the code +makes it look like a message of an arbitrarily long length can be +properly handled (split in 54-byte chunks), while several details would +actually make it fail. Also note that sending messages longer than the +number of registers makes no sense anyway, so not only the double while +loop is complex and broken, but it is also totally useless as far as I +can see. + +5* Return value is not properly handled. + +Considering that this code has been in there since 2003-08-21 +(2.6.0-test3-bk9), I find it quite frightening that nobody noticed how +broken it was until yesterday. Another frightening fact is that the +i2c-algo-bit change made the whole Zoran-based family of devices +unusable for the past 3 months or so in -mm and maybe 1.5 month in +-bk/-rc and nobody reported. As far as I know, these boards are rather +popular. The fact that no user of such board actually tested -mm or -rc +for this period of time points out a problem. (And I am not attempting +to revive a flame war of any kind, nor am I blaming anyone in +particular, merely pointing out what I think is a serious problem.) + +So here is my patch: + +Signed-off-by: Chris Wright + +--- linux-2.6.11-bk3/drivers/media/video/saa7110.c.orig Tue Mar 8 10:27:15 2005 ++++ linux-2.6.11-bk3/drivers/media/video/saa7110.c Tue Mar 8 12:02:45 2005 +@@ -58,10 +58,12 @@ + #define SAA7110_MAX_INPUT 9 /* 6 CVBS, 3 SVHS */ + #define SAA7110_MAX_OUTPUT 0 /* its a decoder only */ + +-#define I2C_SAA7110 0x9C /* or 0x9E */ ++#define I2C_SAA7110 0x9C /* or 0x9E */ ++ ++#define SAA7110_NR_REG 0x35 + + struct saa7110 { +- unsigned char reg[54]; ++ unsigned char reg[SAA7110_NR_REG]; + + int norm; + int input; +@@ -95,31 +97,28 @@ + unsigned int len) + { + int ret = -1; +- u8 reg = *data++; ++ u8 reg = *data; /* first register to write to */ + +- len--; ++ /* Sanity check */ ++ if (reg + (len - 1) > SAA7110_NR_REG) ++ return ret; + + /* the saa7110 has an autoincrement function, use it if + * the adapter understands raw I2C */ + if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { + struct saa7110 *decoder = i2c_get_clientdata(client); + struct i2c_msg msg; +- u8 block_data[54]; + +- msg.len = 0; +- msg.buf = (char *) block_data; ++ msg.len = len; ++ msg.buf = (char *) data; + msg.addr = client->addr; +- msg.flags = client->flags; +- while (len >= 1) { +- msg.len = 0; +- block_data[msg.len++] = reg; +- while (len-- >= 1 && msg.len < 54) +- block_data[msg.len++] = +- decoder->reg[reg++] = *data++; +- ret = i2c_transfer(client->adapter, &msg, 1); +- } ++ msg.flags = 0; ++ ret = i2c_transfer(client->adapter, &msg, 1); ++ ++ /* Cache the written data */ ++ memcpy(decoder->reg + reg, data + 1, len - 1); + } else { +- while (len-- >= 1) { ++ for (++data, --len; len; len--) { + if ((ret = saa7110_write(client, reg++, + *data++)) < 0) + break; +@@ -192,7 +191,7 @@ + return 0; + } + +-static const unsigned char initseq[] = { ++static const unsigned char initseq[1 + SAA7110_NR_REG] = { + 0, 0x4C, 0x3C, 0x0D, 0xEF, 0xBD, 0xF2, 0x03, 0x00, + /* 0x08 */ 0xF8, 0xF8, 0x60, 0x60, 0x00, 0x86, 0x18, 0x90, + /* 0x10 */ 0x00, 0x59, 0x40, 0x46, 0x42, 0x1A, 0xFF, 0xDA, + + +I could load the modified driver on my own DC10+ board (just plugged-in +for the tests) without oops. Unfortunately I am not able to test it any +further (crappy motherboad and lack of video source and tv set). + +The idea of the patch is to keep everything simple because there is no +reason to do otherwise, and add some checks to prevent buffer overruns. +I would appreciate if people familiar with this video chip could check +that my code does the correct thing. I would also appreciate if users of +Zoran-based boards could test it and confirm it works OK. + +Thanks. + +[1] Actually I found that there is one common flag between client and +message, but this is a bad idea IMHO and should never be relied upon +outside of i2c-core. + +[2] I found 5 other drivers with the same problem: adv7170, adv7175, +bt819, saa7114, saa7185, all used by Zoran-based boards. I'll post a +separate patch for these drivers. + +-- +Jean Delvare +- +To unsubscribe from this list: send the line "unsubscribe linux-kernel" in +the body of a message to majordomo@vger.kernel.org +More majordomo info at http://vger.kernel.org/majordomo-info.html +Please read the FAQ at http://www.tux.org/lkml/ + diff --git a/i2c-message-flags-fix.patch b/i2c-message-flags-fix.patch new file mode 100644 index 00000000000..e0b85077308 --- /dev/null +++ b/i2c-message-flags-fix.patch @@ -0,0 +1,97 @@ +Date: Tue, 8 Mar 2005 20:25:30 +0100 +From: Jean Delvare +To: "Randy.Dunlap" +Cc: Daniel Staaf , LKML , + Andrei Mikhailovsky , + Ian Campbell , + Ronald Bultje , + Gerd Knorr +Subject: [PATCH 2.6] Fix i2c messsage flags in video drivers +Lines: 86 + +While working on the saa7110 driver I found a problem with the way +various video drivers (found on Zoran-based boards) prepare i2c messages +to be used by i2c_transfer. The drivers improperly copy the i2c client +flags as the message flags, while both sets are mostly unrelated. The +net effect in this case is to trigger an I2C block read instead of the +expected I2C block write. The fix is simply not to pass any flag, +because none are needed. + +I think this patch qualifies hands down as a "critical bug fix" to be +included in whatever bug-fix-only trees exist these days. As far as I +can see, all Zoran-based boards are broken in 2.6.11 without this patch. + +Signed-off-by: Jean Delvare +Signed-off-by: Chris Wright + +diff -u -rN linux-2.6.11-bk3/drivers/media/video.orig/adv7170.c linux-2.6.11-bk3/drivers/media/video/adv7170.c +--- linux-2.6.11-bk3/drivers/media/video.orig/adv7170.c Tue Mar 8 10:27:14 2005 ++++ linux-2.6.11-bk3/drivers/media/video/adv7170.c Tue Mar 8 12:19:04 2005 +@@ -130,7 +130,7 @@ + u8 block_data[32]; + + msg.addr = client->addr; +- msg.flags = client->flags; ++ msg.flags = 0; + while (len >= 2) { + msg.buf = (char *) block_data; + msg.len = 0; +diff -u -rN linux-2.6.11-bk3/drivers/media/video.orig/adv7175.c linux-2.6.11-bk3/drivers/media/video/adv7175.c +--- linux-2.6.11-bk3/drivers/media/video.orig/adv7175.c Tue Mar 8 10:27:14 2005 ++++ linux-2.6.11-bk3/drivers/media/video/adv7175.c Tue Mar 8 12:18:57 2005 +@@ -126,7 +126,7 @@ + u8 block_data[32]; + + msg.addr = client->addr; +- msg.flags = client->flags; ++ msg.flags = 0; + while (len >= 2) { + msg.buf = (char *) block_data; + msg.len = 0; +diff -u -rN linux-2.6.11-bk3/drivers/media/video.orig/bt819.c linux-2.6.11-bk3/drivers/media/video/bt819.c +--- linux-2.6.11-bk3/drivers/media/video.orig/bt819.c Tue Mar 8 10:27:15 2005 ++++ linux-2.6.11-bk3/drivers/media/video/bt819.c Tue Mar 8 12:18:51 2005 +@@ -146,7 +146,7 @@ + u8 block_data[32]; + + msg.addr = client->addr; +- msg.flags = client->flags; ++ msg.flags = 0; + while (len >= 2) { + msg.buf = (char *) block_data; + msg.len = 0; +diff -u -rN linux-2.6.11-bk3/drivers/media/video.orig/saa7114.c linux-2.6.11-bk3/drivers/media/video/saa7114.c +--- linux-2.6.11-bk3/drivers/media/video.orig/saa7114.c Tue Mar 8 10:27:15 2005 ++++ linux-2.6.11-bk3/drivers/media/video/saa7114.c Tue Mar 8 12:18:20 2005 +@@ -163,7 +163,7 @@ + u8 block_data[32]; + + msg.addr = client->addr; +- msg.flags = client->flags; ++ msg.flags = 0; + while (len >= 2) { + msg.buf = (char *) block_data; + msg.len = 0; +diff -u -rN linux-2.6.11-bk3/drivers/media/video.orig/saa7185.c linux-2.6.11-bk3/drivers/media/video/saa7185.c +--- linux-2.6.11-bk3/drivers/media/video.orig/saa7185.c Tue Mar 8 10:27:15 2005 ++++ linux-2.6.11-bk3/drivers/media/video/saa7185.c Tue Mar 8 12:18:12 2005 +@@ -118,7 +118,7 @@ + u8 block_data[32]; + + msg.addr = client->addr; +- msg.flags = client->flags; ++ msg.flags = 0; + while (len >= 2) { + msg.buf = (char *) block_data; + msg.len = 0; + + + +-- +Jean Delvare +- +To unsubscribe from this list: send the line "unsubscribe linux-kernel" in +the body of a message to majordomo@vger.kernel.org +More majordomo info at http://vger.kernel.org/majordomo-info.html +Please read the FAQ at http://www.tux.org/lkml/ + -- 2.47.3