[PATCH v2] af9015: avoid magically sized temporary buffer in eeprom_dump
Replace printing to magically sized temporary buffer with use of KERN_CONT for continual printing of eeprom registers dump. Since deb_info is defined as dprintk, which is conditionally defined to printk without additional parameters, meaning that deb_info is equivalent to direct printk (without adding KERN_ facility), we can use KERN_DEBUG and KERN_CONT in there, eliminating the need for sprintf into temporary buffer with not easily readable/magical size. Though it's strange, that deb_info definition uses printk without KERN_ facility and callers don't use it either. v2: removed comma after KERN_CONT Signed-off-by: Jan Nikitenko jan.nikite...@gmail.com --- I do not see better solution for the magical sized buffer, since print_hex_dump like functions need dump of registers in memory (so the magical sized temporary buffer would be needed for a copy anyway). If deb_info was defined with inside KERN_ facility, then this patch would not be valid. In that case the magically sized temporary buffer might be acceptable. This patch depends on 'af9015: fix stack corruption bug' patch. On 14:00 Thu 18 Jun , Trent Piepho wrote: On Thu, 18 Jun 2009, Jan Nikitenko wrote: + deb_info(KERN_CONT, --); No comma after KERN_CONT Just use printk() instead of deb_info() for the ones that use KERN_CONT. It's not possible to use printk instead of deb_info just for the ones that use KERN_CONT, because deb_info not always goes to printk, it depends on compile time dprintk macro expansion and on runtime configuration of debugging messages from dvb subsystem. If printk is used instead of all deb_info calls, this logging would not be anymore influenced by above mentioned settings for deb_info. I am not sure if duplicating all conditions of deb_info for direct printk in there would be any better. linux/drivers/media/dvb/dvb-usb/af9015.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff -r 722c6faf3ab5 linux/drivers/media/dvb/dvb-usb/af9015.c --- a/linux/drivers/media/dvb/dvb-usb/af9015.c Wed Jun 17 22:39:23 2009 -0300 +++ b/linux/drivers/media/dvb/dvb-usb/af9015.c Fri Jun 19 09:22:53 2009 +0200 @@ -541,24 +541,22 @@ /* dump eeprom */ static int af9015_eeprom_dump(struct dvb_usb_device *d) { - char buf[4+3*16+1], buf2[4]; u8 reg, val; for (reg = 0; ; reg++) { if (reg % 16 == 0) { if (reg) - deb_info(%s\n, buf); - sprintf(buf, %02x: , reg); + deb_info(KERN_CONT \n); + deb_info(KERN_DEBUG %02x:, reg); } if (af9015_read_reg_i2c(d, AF9015_I2C_EEPROM, reg, val) == 0) - sprintf(buf2, %02x , val); + deb_info(KERN_CONT %02x, val); else - strcpy(buf2, -- ); - strcat(buf, buf2); + deb_info(KERN_CONT --); if (reg == 0xff) break; } - deb_info(%s\n, buf); + deb_info(KERN_CONT \n); return 0; } -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] zl10353 and qt1010: fix stack corruption bug
This patch fixes stack corruption bug present in dump_regs function of zl10353 and qt1010 drivers: the buffer buf was one byte smaller than required - there are 4 chars for address prefix, 16 * 3 chars for dump of 16 eeprom bytes per line and 1 byte for zero ending the string required, i.e. 53 bytes, but only 52 were provided. The one byte missing in stack based buffer buf can cause stack corruption possibly leading to kernel oops, as discovered originally with af9015 driver (af9015: fix stack corruption bug). This is second version of the patch for zl10353 and qt1010 that uses continual printk instead of stack based buffer with proper magic number size. Signed-off-by: Jan Nikitenko jan.nikite...@gmail.com --- linux/drivers/media/common/tuners/qt1010.c | 12 +--- linux/drivers/media/dvb/frontends/zl10353.c | 12 +--- 2 files changed, 10 insertions(+), 14 deletions(-) diff -r 722c6faf3ab5 linux/drivers/media/common/tuners/qt1010.c --- a/linux/drivers/media/common/tuners/qt1010.cWed Jun 17 22:39:23 2009 -0300 +++ b/linux/drivers/media/common/tuners/qt1010.cThu Jun 18 08:49:58 2009 +0200 @@ -65,24 +65,22 @@ /* dump all registers */ static void qt1010_dump_regs(struct qt1010_priv *priv) { - char buf[52], buf2[4]; u8 reg, val; for (reg = 0; ; reg++) { if (reg % 16 == 0) { if (reg) - printk(%s\n, buf); - sprintf(buf, %02x: , reg); + printk(KERN_CONT \n); + printk(KERN_DEBUG %02x:, reg); } if (qt1010_readreg(priv, reg, val) == 0) - sprintf(buf2, %02x , val); + printk(KERN_CONT %02x, val); else - strcpy(buf2, -- ); - strcat(buf, buf2); + printk(KERN_CONT --); if (reg == 0x2f) break; } - printk(%s\n, buf); + printk(KERN_CONT \n); } static int qt1010_set_params(struct dvb_frontend *fe, diff -r 722c6faf3ab5 linux/drivers/media/dvb/frontends/zl10353.c --- a/linux/drivers/media/dvb/frontends/zl10353.c Wed Jun 17 22:39:23 2009 -0300 +++ b/linux/drivers/media/dvb/frontends/zl10353.c Thu Jun 18 08:49:58 2009 +0200 @@ -102,7 +102,6 @@ static void zl10353_dump_regs(struct dvb_frontend *fe) { struct zl10353_state *state = fe-demodulator_priv; - char buf[52], buf2[4]; int ret; u8 reg; @@ -110,19 +109,18 @@ for (reg = 0; ; reg++) { if (reg % 16 == 0) { if (reg) - printk(KERN_DEBUG %s\n, buf); - sprintf(buf, %02x: , reg); + printk(KERN_CONT \n); + printk(KERN_DEBUG %02x:, reg); } ret = zl10353_read_register(state, reg); if (ret = 0) - sprintf(buf2, %02x , (u8)ret); + printk(KERN_CONT %02x, (u8)ret); else - strcpy(buf2, -- ); - strcat(buf, buf2); + printk(KERN_CONT --); if (reg == 0xff) break; } - printk(KERN_DEBUG %s\n, buf); + printk(KERN_CONT \n); } #endif -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] af9015: avoid magically sized temporal buffer in eeprom_dump
Replace printing to magically sized temporal buffer with use of KERN_CONT for continual printing of eeprom registers dump. Since deb_info is defined as dprintk, which is defined as printk without additional parameters, meaning that deb_info is equivalent to direct printk (without KERN_ facility), we can use KERN_DEBUG and KERN_CONT in there, eliminating the need for sprintf into temporal buffer with not easily readable/magical size. Though it's strange, that deb_info definition uses printk without KERN_ facility and callers don't use it either. --- I do not see better solution for the magical sized buffer, since print_hex_dump like functions need dump of registers in memory (so the magical sized temporal buffer would be needed for a copy anyway). If deb_info was defined with inside KERN_ facility, then this patch would not be valid and so the magically sized temporal buffer might be acceptable to keep there. This patch depends on 'af9015: fix stack corruption bug' patch. linux/drivers/media/dvb/dvb-usb/af9015.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff -r 722c6faf3ab5 linux/drivers/media/dvb/dvb-usb/af9015.c --- a/linux/drivers/media/dvb/dvb-usb/af9015.c Wed Jun 17 22:39:23 2009 -0300 +++ b/linux/drivers/media/dvb/dvb-usb/af9015.c Thu Jun 18 08:49:58 2009 +0200 @@ -541,24 +541,22 @@ /* dump eeprom */ static int af9015_eeprom_dump(struct dvb_usb_device *d) { - char buf[4+3*16+1], buf2[4]; u8 reg, val; for (reg = 0; ; reg++) { if (reg % 16 == 0) { if (reg) - deb_info(%s\n, buf); - sprintf(buf, %02x: , reg); + deb_info(KERN_CONT \n); + deb_info(KERN_DEBUG %02x:, reg); } if (af9015_read_reg_i2c(d, AF9015_I2C_EEPROM, reg, val) == 0) - sprintf(buf2, %02x , val); + deb_info(KERN_CONT, %02x, val); else - strcpy(buf2, -- ); - strcat(buf, buf2); + deb_info(KERN_CONT, --); if (reg == 0xff) break; } - deb_info(%s\n, buf); + deb_info(KERN_CONT \n); return 0; } -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] af9015: avoid magically sized temporal buffer in eeprom_dump
Replace printing to magically sized temporal buffer with use of KERN_CONT for continual printing of eeprom registers dump. Since deb_info is defined as dprintk, which is defined as printk without additional parameters, meaning that deb_info is equivalent to direct printk (without KERN_ facility), we can use KERN_DEBUG and KERN_CONT in there, eliminating the need for sprintf into temporal buffer with not easily readable/magical size. Though it's strange, that deb_info definition uses printk without KERN_ facility and callers don't use it either. Signed-off-by: Jan Nikitenko jan.nikite...@gmail.com --- (added missing Singned-off) I do not see better solution for the magical sized buffer, since print_hex_dump like functions need dump of registers in memory (so the magical sized temporal buffer would be needed for a copy anyway). If deb_info was defined with inside KERN_ facility, then this patch would not be valid and so the magically sized temporal buffer might be acceptable to keep there. This patch depends on 'af9015: fix stack corruption bug' patch. linux/drivers/media/dvb/dvb-usb/af9015.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff -r 722c6faf3ab5 linux/drivers/media/dvb/dvb-usb/af9015.c --- a/linux/drivers/media/dvb/dvb-usb/af9015.c Wed Jun 17 22:39:23 2009 -0300 +++ b/linux/drivers/media/dvb/dvb-usb/af9015.c Thu Jun 18 08:49:58 2009 +0200 @@ -541,24 +541,22 @@ /* dump eeprom */ static int af9015_eeprom_dump(struct dvb_usb_device *d) { - char buf[4+3*16+1], buf2[4]; u8 reg, val; for (reg = 0; ; reg++) { if (reg % 16 == 0) { if (reg) - deb_info(%s\n, buf); - sprintf(buf, %02x: , reg); + deb_info(KERN_CONT \n); + deb_info(KERN_DEBUG %02x:, reg); } if (af9015_read_reg_i2c(d, AF9015_I2C_EEPROM, reg, val) == 0) - sprintf(buf2, %02x , val); + deb_info(KERN_CONT, %02x, val); else - strcpy(buf2, -- ); - strcat(buf, buf2); + deb_info(KERN_CONT, --); if (reg == 0xff) break; } - deb_info(%s\n, buf); + deb_info(KERN_CONT \n); return 0; } -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] zl10353 and qt1010: fix stack corruption bug
Mauro Carvalho Chehab wrote: Em Wed, 10 Jun 2009 08:21:20 +0200 Jan Nikitenko jan.nikite...@gmail.com escreveu: This patch fixes stack corruption bug present in dump_regs function of zl10353 and qt1010 drivers: the buffer buf is one byte smaller than required - there is 4 chars for address prefix, 16*3 chars for dump of 16 eeprom bytes per line and 1 byte for zero ending the string required, i.e. 53 bytes, but only 52 were provided. The one byte missing in stack based buffer buf can cause stack corruption possibly leading to kernel oops, as discovered originally with af9015 driver. Signed-off-by: Jan Nikitenko jan.nikite...@gmail.com --- Antti Palosaari wrote: On 06/10/2009 01:39 AM, Jan Nikitenko wrote: Solved with [PATCH] af9015: fix stack corruption bug. This error leads to the zl10353.c and there it was copied to qt1010.c and af9015.c. Antti, thanks for pointing out that the same problem was also in zl10353.c and qt1010.c. Include your Sign-off-by, please. Best regards, Jan linux/drivers/media/common/tuners/qt1010.c |2 +- linux/drivers/media/dvb/frontends/zl10353.c |2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff -r cff06234b725 linux/drivers/media/common/tuners/qt1010.c --- a/linux/drivers/media/common/tuners/qt1010.cSun May 31 23:07:01 2009 +0300 +++ b/linux/drivers/media/common/tuners/qt1010.cWed Jun 10 07:37:51 2009 +0200 @@ -65,7 +65,7 @@ /* dump all registers */ static void qt1010_dump_regs(struct qt1010_priv *priv) { - char buf[52], buf2[4]; + char buf[4+3*16+1], buf2[4]; CodingStyle is incorrect. It should be buf[4 + 3 * 16 + 1]. right. u8 reg, val; for (reg = 0; ; reg++) { diff -r cff06234b725 linux/drivers/media/dvb/frontends/zl10353.c --- a/linux/drivers/media/dvb/frontends/zl10353.c Sun May 31 23:07:01 2009 +0300 +++ b/linux/drivers/media/dvb/frontends/zl10353.c Wed Jun 10 07:37:51 2009 +0200 @@ -102,7 +102,7 @@ static void zl10353_dump_regs(struct dvb_frontend *fe) { struct zl10353_state *state = fe-demodulator_priv; - char buf[52], buf2[4]; + char buf[4+3*16+1], buf2[4]; Same CodingStyle issue here. agreed. int ret; u8 reg; Without having actually looking at the source code, would it be possible to change the logic in order to use something else instead of a magic number for buf size - e. g. using sizeof(something) ? I am not sure if that's possible - the buffer is used for sprintf in a loop to store text representation of registers dump, before printk-ing it. We could put there a comment, suggesting that 4 bytes are required for address prefix of form 00: , then 16 strings of form 00 (3 bytes each) and one byte for zero to terminate the string. Or we could use sizeof, like this: char buf[sizeof(00: ) - 1 + 16 * (sizeof(00 ) - 1) + 1] or char buf[sizeof(00: 00 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f )] but it is not very readable in my opinion either. Maybe the best way would be to avoid the need for temporal buffer completely by directly using printk in a loop, that is only the first printk with KERN_DEBUG, followed by sequence of printk with registers dump and final printk with end of line (but isn't a printk without KERN_ facility coding style problem as well?). I am not sure, what approach is the best - I just wanted to fix a bug, which did not allow to use my af9015 based tuner on mips platform. After that, Antti pointed out, that the same code with the same bug is also in other two sources, so I send the same fix for them as well... Best regards, Jan -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] zl10353 and qt1010: fix stack corruption bug
This patch fixes stack corruption bug present in dump_regs function of zl10353 and qt1010 drivers: the buffer buf is one byte smaller than required - there is 4 chars for address prefix, 16*3 chars for dump of 16 eeprom bytes per line and 1 byte for zero ending the string required, i.e. 53 bytes, but only 52 were provided. The one byte missing in stack based buffer buf can cause stack corruption possibly leading to kernel oops, as discovered originally with af9015 driver. Signed-off-by: Jan Nikitenko jan.nikite...@gmail.com --- Antti Palosaari wrote: On 06/10/2009 01:39 AM, Jan Nikitenko wrote: Solved with [PATCH] af9015: fix stack corruption bug. This error leads to the zl10353.c and there it was copied to qt1010.c and af9015.c. Antti, thanks for pointing out that the same problem was also in zl10353.c and qt1010.c. Include your Sign-off-by, please. Best regards, Jan linux/drivers/media/common/tuners/qt1010.c |2 +- linux/drivers/media/dvb/frontends/zl10353.c |2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff -r cff06234b725 linux/drivers/media/common/tuners/qt1010.c --- a/linux/drivers/media/common/tuners/qt1010.cSun May 31 23:07:01 2009 +0300 +++ b/linux/drivers/media/common/tuners/qt1010.cWed Jun 10 07:37:51 2009 +0200 @@ -65,7 +65,7 @@ /* dump all registers */ static void qt1010_dump_regs(struct qt1010_priv *priv) { - char buf[52], buf2[4]; + char buf[4+3*16+1], buf2[4]; u8 reg, val; for (reg = 0; ; reg++) { diff -r cff06234b725 linux/drivers/media/dvb/frontends/zl10353.c --- a/linux/drivers/media/dvb/frontends/zl10353.c Sun May 31 23:07:01 2009 +0300 +++ b/linux/drivers/media/dvb/frontends/zl10353.c Wed Jun 10 07:37:51 2009 +0200 @@ -102,7 +102,7 @@ static void zl10353_dump_regs(struct dvb_frontend *fe) { struct zl10353_state *state = fe-demodulator_priv; - char buf[52], buf2[4]; + char buf[4+3*16+1], buf2[4]; int ret; u8 reg; -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: AVerTV Volar Black HD: i2c oops in warm state on mips
Solved with [PATCH] af9015: fix stack corruption bug. Best regards, Jan -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
AVerTV Volar Black HD: i2c oops in warm state on mips
Hi, I am trying to get AverMedia AVerTV Volar Black HD (A850) usb dvb-t tuner running on mips32 little endian platform (to stream dvb-t from home router on LAN). I've cross compiled v4l-dvb mercurial sources (revision 11448:d4274bbb8605) and when plugging the tuner stick, I get following kernel oops log on serial console: dvb-usb: found a 'AverMedia AVerTV Volar Black HD (A850)' in cold state, will try to load a firmware usb 1-1: firmware: requesting dvb-usb-af9015.fw dvb-usb: downloading firmware from file 'dvb-usb-af9015.fw' dvb-usb: found a 'AverMedia AVerTV Volar Black HD (A850)' in warm state. dvb-usb: will use the device's hardware PID filter (table count: 32). DVB: registering new adapter (AverMedia AVerTV Volar Black HD (A850)) CPU 0 Unable to handle kernel paging request at virtual address , epc == 803a4488, ra == c049a1c8 Oops[#1]: Cpu 0 $ 0 : 10003c00 803a4468 $ 4 : 8f17c600 8f067b30 0002 0038 $ 8 : 0001 8faf3e98 11da000d 09010002 $12 : 000a $16 : 8f17c600 8f067b68 8faf3c00 8f067c04 $20 : 8f067b9c 0100 8f067bf0 80104100 $24 : 2aba9fb0 $28 : 8f066000 8f067af0 802cbc48 c049a1c8 Hi: Lo: epc : 803a4488 i2c_transfer+0x20/0x104 Not tainted ra: c049a1c8 af9013_read_reg+0x78/0xc4 [af9013] Status: 10003c03KERNEL EXL IE Cause : 00808008 BadVA : PrId : 03030200 (Au1550) Modules linked in: af9013 dvb_usb_af9015(+) dvb_usb dvb_core firmware_class i2c_au1550 au1550_spi Process modprobe (pid: 2757, threadinfo=8f066000, task=8fade098, tls=2aad6470) Stack : c049f5e0 80163090 805ba880 0100 8f067bf0 d733 8f067b68 8faf3c00 8f067c04 c049a1c8 80163bc0 8056a630 8f067b40 80163224 80569fc8 8f0033d7 0038 80140003 8f067b2c 00010038 c0420001 8f067b28 c049f5e0 0004 0004 c049a524 c049d5a8 c049d5a8 803a6700 8f17c600 c042a7a4 8f17c600 c042a7a4 c049c924 0002 613a6c00 ... Call Trace: [803a4488] i2c_transfer+0x20/0x104 [c049a1c8] af9013_read_reg+0x78/0xc4 [af9013] [c049a524] af9013_read_reg_bits+0x2c/0x70 [af9013] [c049c924] af9013_attach+0x98/0x65c [af9013] [c04257bc] af9015_af9013_frontend_attach+0x214/0x67c [dvb_usb_af9015] [c03e2428] dvb_usb_adapter_frontend_init+0x20/0x12c [dvb_usb] [c03e1ad8] dvb_usb_device_init+0x374/0x6b0 [dvb_usb] [c0426120] af9015_usb_probe+0x4fc/0xfcc [dvb_usb_af9015] [80381024] usb_probe_interface+0xbc/0x218 [803227fc] driver_probe_device+0x12c/0x30c [80322a80] __driver_attach+0xa4/0xac [80321ed0] bus_for_each_dev+0x60/0xd0 [8032162c] bus_add_driver+0x1e8/0x2a8 [80322cdc] driver_register+0x7c/0x17c [80380d30] usb_register_driver+0xa0/0x12c [c042e030] af9015_usb_module_init+0x30/0x6c [dvb_usb_af9015] [8010d2a4] __kprobes_text_end+0x3c/0x1f4 [80167150] sys_init_module+0xb8/0x1cc [80102370] stack_done+0x20/0x3c Code: afb10018 703f 00808021 8c43 703f 1060002d 00c09021 8f830014 3c02efff The used dvb-t sources work ok on intel x86 platform (used kernel 2.6.24), but on mips, there seems to be some problem with i2c after switch of the tuner into the warm state via firmware load (tested with 2.6.25.17 and 2.6.29-rc7 mips kernels, with the same result) - it seems to me, that adap pointer (or the structure) is not valid (adap-algo is NULL): (gdb) c Continuing. Breakpoint 1, i2c_transfer (adap=0x8f17c600, msgs=0x8f067b30, num=2) at /usr/src/linux/drivers/i2c/i2c-core.c:1036 1036if (adap-algo-master_xfer) { (gdb) p *msgs $4 = {addr = 56, flags = 0, len = 3, buf = 0x8f0c9b2c ×3} (gdb) p *adap $2 = {owner = 0x0, id = 0, class = 0, algo = 0x0, algo_data = 0x0, client_register = 0, client_unregister = 0, level = 0 '\0', bus_lock = {count = {counter = 0}, wait_lock = {raw_lock = {No data fields}}, wait_list = {next = 0xc042a00c, prev = 0x8fb55000}}, clist_lock = {count = {counter = 1}, wait_lock = {raw_lock = {No data fields}}, wait_list = {next = 0x1, prev = 0x1}}, timeout = -1894267336, retries = -1894267336, dev = {klist_children = {k_lock = {raw_lock = {No data fields}}, k_list = {next = 0x1, prev = 0x8f17c644}, get = 0x8f17c644, put = 0}, knode_parent = {n_klist = 0x0, n_node = {next = 0x0, prev = 0x4}, n_ref = { refcount = {counter = -1069368724}}}, knode_driver = {n_klist = 0x0, n_node = {next = 0x0, prev = 0x0}, n_ref = {refcount = { counter = 0}}}, knode_bus = {n_klist = 0x1, n_node = {next = 0x8f17c674, prev = 0x8f17c674}, n_ref = {refcount = { counter = 1}}}, parent = 0x8f17c680, kobj = {name = 0x8f17c680 \200Ć\027\217\200Ć\027\217, entry = {next = 0x0, prev = 0x0}, parent = 0x8fb3f494, kset = 0x8fb3f494, ktype = 0x8031f5b0, sd = 0x8031e8e8, kref = {refcount = {counter = -1883942816}}, state_initialized = 0, state_in_sysfs = 0, state_add_uevent_sent = 1, state_remove_uevent_sent = 0}, bus_id = \034Đ\004\217\001, '\0' repeats 14 times, uevent_suppress = 0,