| 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
| ||
|
Date: Sun, 25 Apr 2010 17:56:09 +0200
From: Florian Tobias Schandinat <FlorianSchandinat@....de>
To: Jonathan Corbet <corbet@....net>
CC: Harald Welte <laforge@...monks.org>, linux-kernel@...r.kernel.org,
Deepak Saxena <dsaxena@...top.org>,
linux-fbdev@...r.kernel.org, JosephChan@....com.tw,
ScottFang@...tech.com.cn
Subject: Re: [PATCH 10/11] viafb: rework the I2C support in the VIA framebuffer
driver
Hi Jon,
Jonathan Corbet schrieb:
> On Sat, 24 Apr 2010 15:53:16 +0200
> Harald Welte <laforge@...monks.org> wrote:
>
>> Simply converting the original two busses to the new code is definitely
>> the way to go. Maybe we'll keep the code for the other two around, but
>> somehow keep them inactive and don't advertise them unless somebody explicitly
>> does so?
>
> OK, my proposal would be to add the following patch into the early part
> of the series; that will help to avoid the creation of confusion in the
> middle until the full i2c/gpio configuration code is in.
>
> Look good?
Yes, it does. But it highlighted a bug in the original patch:
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<c11df2db>] i2c_transfer+0x19/0xb0
*pdpt = 000000000af77001 *pde = 0000000000000000
Oops: 0000 [#1] PREEMPT
last sysfs file: /sys/devices/virtual/vtconsole/vtcon1/bind
Modules linked in: viafb(+) fbcon font bitblit softcursor fb
i2c_algo_bit cfbcopyarea cfbimgblt cfbfillrect snd_hda_codec_realtek
snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_timer mmc_block
rtl8187 snd eeprom_93cx6 soundcore via_sdmmc snd_page_alloc i2c_viapro
ehci_hcd uhci_hcd mmc_core video output [last unloaded: viafb]
Pid: 3561, comm: insmod Tainted: G W 2.6.34-rc5 #1 IL1/
EIP: 0060:[<c11df2db>] EFLAGS: 00010296 CPU: 0
EIP is at i2c_transfer+0x19/0xb0
EAX: 00000000 EBX: ffffffa1 ECX: 00000002 EDX: c74f2d68
ESI: db034294 EDI: c74f2d96 EBP: c74f2d60 ESP: c74f2d48
DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
Process insmod (pid: 3561, ti=c74f2000 task=db022000 task.ti=c74f2000)
Stack:
00000002 c74f2d68 c74f2d97 c74f2d96 c74f2d97 c74f2d96 c74f2d88 dc7308f0
<0> 00000040 c74f0001 c74f2d83 00010040 c74f0001 c74f2d96 00000001 00000000
<0> c74f2da4 dc733a7f c74f2d96 00002db0 dc738a28 db0349a8 c74f2e84 c74f2db0
Call Trace:
[<dc7308f0>] ? viafb_i2c_readbyte+0x60/0x68 [viafb]
[<dc733a7f>] ? viafb_lvds_identify_vt1636+0x38/0xbc [viafb]
[<dc732a7f>] ? viafb_lvds_trasmitter_identify+0x2c/0x10d [viafb]
[<dc7304cc>] ? viafb_init_chip_info+0x15b/0x23f [viafb]
[<dc7340c3>] ? via_pci_probe+0x155/0x81c [viafb]
[<c111f1c0>] ? idr_get_empty_slot+0x152/0x226
[<c112e1ee>] ? pci_match_device+0xbf/0xca
[<c112e09c>] ? local_pci_probe+0xe/0x10
[<c112e71a>] ? pci_device_probe+0x43/0x66
[<c1186374>] ? driver_probe_device+0x78/0x103
[<c1186442>] ? __driver_attach+0x43/0x5f
[<c1185d79>] ? bus_for_each_dev+0x3d/0x67
[<c118624e>] ? driver_attach+0x14/0x16
[<c11863ff>] ? __driver_attach+0x0/0x5f
[<c11857f3>] ? bus_add_driver+0xa2/0x1d4
[<c1186687>] ? driver_register+0x8b/0xeb
[<c112e8fe>] ? __pci_register_driver+0x31/0x8a
[<dc6f7000>] ? viafb_init+0x0/0x297 [viafb]
[<dc6f7288>] ? viafb_init+0x288/0x297 [viafb]
[<c1001133>] ? do_one_initcall+0x4b/0x130
[<c1041ae9>] ? sys_init_module+0xa7/0x1de
[<c1002750>] ? sysenter_do_call+0x12/0x26
Code: 8d 55 f8 89 4d fc b9 af f0 1d c1 e8 47 4c fa ff c9 c3 55 89 e5 57
56 89 c6 53 bb a1 ff ff ff 83 ec 0c 89 55 ec 89 4d e8 8b 40 0c <83> 38
00 0f 84 84 00 00 00 89 e0 25 00 f0 ff ff 8b 0d 68 5c 3a
EIP: [<c11df2db>] i2c_transfer+0x19/0xb0 SS:ESP 0068:c74f2d48
CR2: 0000000000000000
as you can see it is caused in viafb_lvds_trasmitter_identify by
- viaparinfo->shared->i2c_stuff.i2c_port = GPIOPORTINDEX;
- if (viafb_lvds_identify_vt1636()) {
+ if (viafb_lvds_identify_vt1636(VIA_I2C_ADAP_26)) {
which should rather be
- viaparinfo->shared->i2c_stuff.i2c_port = GPIOPORTINDEX;
- if (viafb_lvds_identify_vt1636()) {
+ if (viafb_lvds_identify_vt1636(VIA_I2C_ADAP_2C)) {
after which your patches including this work fine (on VX800 and CLE266).
Thanks,
Florian Tobias Schandinat
> viafb: Only establish i2c busses on ports that always had them
>
> ...otherwise it seems we run into conflicts with shadowy other users which
> don't expect to see i2c taking control of ports it never used to do
> anything with.
>
> Reported-by: Florian Tobias Schandinat <FlorianSchandinat@....de>
> Signed-off-by: Jonathan Corbet <corbet@....net>
> ---
> drivers/video/via/via_i2c.c | 19 +++++++++++++------
> drivers/video/via/via_i2c.h | 1 +
> 2 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/via/via_i2c.c b/drivers/video/via/via_i2c.c
> index fe5535c..b5253e3 100644
> --- a/drivers/video/via/via_i2c.c
> +++ b/drivers/video/via/via_i2c.c
> @@ -170,13 +170,18 @@ static int create_i2c_bus(struct i2c_adapter *adapter,
> return i2c_bit_add_bus(adapter);
> }
>
> +/*
> + * By default, we only activate busses on ports 2c and 31 to avoid
> + * conflicts with other possible users; that default can be changed
> + * below.
> + */
> static struct via_i2c_adap_cfg adap_configs[] = {
> - [VIA_I2C_ADAP_26] = { VIA_I2C_I2C, VIASR, 0x26 },
> - [VIA_I2C_ADAP_31] = { VIA_I2C_I2C, VIASR, 0x31 },
> - [VIA_I2C_ADAP_25] = { VIA_I2C_GPIO, VIASR, 0x25 },
> - [VIA_I2C_ADAP_2C] = { VIA_I2C_GPIO, VIASR, 0x2c },
> - [VIA_I2C_ADAP_3D] = { VIA_I2C_GPIO, VIASR, 0x3d },
> - { 0, 0, 0 }
> + [VIA_I2C_ADAP_26] = { VIA_I2C_I2C, VIASR, 0x26, 0 },
> + [VIA_I2C_ADAP_31] = { VIA_I2C_I2C, VIASR, 0x31, 1 },
> + [VIA_I2C_ADAP_25] = { VIA_I2C_GPIO, VIASR, 0x25, 0 },
> + [VIA_I2C_ADAP_2C] = { VIA_I2C_GPIO, VIASR, 0x2c, 1 },
> + [VIA_I2C_ADAP_3D] = { VIA_I2C_GPIO, VIASR, 0x3d, 0 },
> + { 0, 0, 0, 0 }
> };
>
> int viafb_create_i2c_busses(struct viafb_par *viapar)
> @@ -189,6 +194,8 @@ int viafb_create_i2c_busses(struct viafb_par *viapar)
>
> if (adap_cfg->type == 0)
> break;
> + if (!adap_cfg->is_active)
> + continue;
>
> ret = create_i2c_bus(&i2c_stuff->adapter,
> &i2c_stuff->algo, adap_cfg,
> diff --git a/drivers/video/via/via_i2c.h b/drivers/video/via/via_i2c.h
> index 00ed978..73d682f 100644
> --- a/drivers/video/via/via_i2c.h
> +++ b/drivers/video/via/via_i2c.h
> @@ -35,6 +35,7 @@ struct via_i2c_adap_cfg {
> enum via_i2c_type type;
> u_int16_t io_port;
> u_int8_t ioport_index;
> + u8 is_active;
> };
>
> struct via_i2c_stuff {
--
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/
Powered by blists - more mailing lists