* [OOPS] w83781d during rmmod (2.5.69-bk17)
@ 2003-05-24 18:37 Mark M. Hoffman
[not found] ` <3ED8067E.1050503@paradyne.com>
0 siblings, 1 reply; 21+ messages in thread
From: Mark M. Hoffman @ 2003-05-24 18:37 UTC (permalink / raw)
To: LKML, Sensors
This applies to all kernel versions since w83781d was brought in from
the lm_sensors project.
The subclients of w83781d are never registered with i2c_attach_client().
But, w83781d_detach_client() tries to i2c_detach_client() them anyway.
This was harmless, until i2c-core was "listified"... because the old
array method silently ignored the attempt to detach a non-existent client.
The latest lm_sensors CVS of w83781d has the necessary i2c_attach_client()
calls - not sure why they were removed during conversion to 2.5.x. Do we
intend to attach these subclients or not?
Regards,
--
Mark M. Hoffman
mhoffman@lightlink.com
^ permalink raw reply [flat|nested] 21+ messages in thread[parent not found: <3ED8067E.1050503@paradyne.com>]
* [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17) [not found] ` <3ED8067E.1050503@paradyne.com> @ 2003-06-01 14:38 ` Mark M. Hoffman 2003-06-02 17:20 ` Greg KH 0 siblings, 1 reply; 21+ messages in thread From: Mark M. Hoffman @ 2003-06-01 14:38 UTC (permalink / raw) To: LKML, Sensors; +Cc: Mark D. Studebaker, Greg KH [-- Attachment #1: Type: text/plain, Size: 2089 bytes --] * Mark D. Studebaker <mds@paradyne.com> [2003-05-30 21:33:50 -0400]: > Mark M. Hoffman wrote: > >This applies to all kernel versions since w83781d was brought in from > >the lm_sensors project. > > > >The subclients of w83781d are never registered with i2c_attach_client(). > >But, w83781d_detach_client() tries to i2c_detach_client() them anyway. > >This was harmless, until i2c-core was "listified"... because the old > >array method silently ignored the attempt to detach a non-existent client. > > > >The latest lm_sensors CVS of w83781d has the necessary i2c_attach_client() > >calls - not sure why they were removed during conversion to 2.5.x. Do we > >intend to attach these subclients or not? > > Good catch. > Greg, this needs to be fixed. > Winbond chips take up 3 addresses on the i2c bus. > In the original code we registered two "subclients" with the i2c layer. > Somehow the 3-address chips need to be handled. This fixes the oops during w83781d module removal by putting the subclient registration back in. While I was in there, I split w83781d_detect in half in an attempt to reduce the goto madness. So, the /sys tree looks like this, where 48 & 49 are the subclients. There are no entries (besides name & power) for the subclients. /sys/bus/i2c/ |-- devices | |-- 0-002d -> ../../../devices/pci0/00:02.1/i2c-0/0-002d | |-- 0-0048 -> ../../../devices/pci0/00:02.1/i2c-0/0-0048 | `-- 0-0049 -> ../../../devices/pci0/00:02.1/i2c-0/0-0049 `-- drivers |-- i2c_adapter `-- w83781d |-- 0-002d -> ../../../../devices/pci0/00:02.1/i2c-0/0-002d |-- 0-0048 -> ../../../../devices/pci0/00:02.1/i2c-0/0-0048 `-- 0-0049 -> ../../../../devices/pci0/00:02.1/i2c-0/0-0049 Also, I fixed a bug where this driver would request and release an ISA region, then poke around in that region, then finally request it again. This patch against 2.5.70 works for me vs. an SMBus adapter. It needs re-testing against an ISA adapter since my particular chip is SMBus only. Please CC me w/ comments. Regards, -- Mark M. Hoffman mhoffman@lightlink.com [-- Attachment #2: patch-w83781d.txt --] [-- Type: text/plain, Size: 10367 bytes --] --- linux-2.5.70/drivers/i2c/chips/w83781d.c.orig 2003-06-01 10:08:56.000000000 -0400 +++ linux-2.5.70/drivers/i2c/chips/w83781d.c 2003-06-01 10:13:19.000000000 -0400 @@ -1031,14 +1031,112 @@ return i2c_detect(adapter, &addr_data, w83781d_detect); } +/* Assumes that adapter is of I2C, not ISA variety. + * OTHERWISE DON'T CALL THIS + */ +static int +w83781d_detect_subclients(struct i2c_adapter *adapter, int address, int kind, + struct i2c_client *new_client) +{ + int i, val1 = 0, id; + int err = 0; + const char *client_name; + struct w83781d_data *data = i2c_get_clientdata(new_client); + + if (!(data->lm75 = kmalloc(2 * sizeof (struct i2c_client), + GFP_KERNEL))) { + err = -ENOMEM; + goto ERROR_SC_0; + } + memset(data->lm75, 0x00, 2 * sizeof (struct i2c_client)); + + id = i2c_adapter_id(adapter); + + if (force_subclients[0] == id && force_subclients[1] == address) { + for (i = 2; i <= 3; i++) { + if (force_subclients[i] < 0x48 || + force_subclients[i] > 0x4f) { + dev_err(&new_client->dev, "Invalid subclient " + "address %d; must be 0x48-0x4f\n", + force_subclients[i]); + goto ERROR_SC_1; + } + } + w83781d_write_value(new_client, W83781D_REG_I2C_SUBADDR, + (force_subclients[2] & 0x07) | + ((force_subclients[3] & 0x07) << 4)); + data->lm75[0].addr = force_subclients[2]; + } else { + val1 = w83781d_read_value(new_client, W83781D_REG_I2C_SUBADDR); + data->lm75[0].addr = 0x48 + (val1 & 0x07); + } + + if (kind != w83783s) { + if (force_subclients[0] == id && + force_subclients[1] == address) { + data->lm75[1].addr = force_subclients[3]; + } else { + data->lm75[1].addr = 0x48 + ((val1 >> 4) & 0x07); + } + if (data->lm75[0].addr == data->lm75[1].addr) { + dev_err(&new_client->dev, + "Duplicate addresses 0x%x for subclients.\n", + data->lm75[0].addr); + goto ERROR_SC_1; + } + } + + if (kind == w83781d) + client_name = "W83781D subclient"; + else if (kind == w83782d) + client_name = "W83782D subclient"; + else if (kind == w83783s) + client_name = "W83783S subclient"; + else if (kind == w83627hf) + client_name = "W83627HF subclient"; + else if (kind == as99127f) + client_name = "AS99127F subclient"; + else + client_name = "unknown subclient?"; + + for (i = 0; i <= 1; i++) { + /* store all data in w83781d */ + i2c_set_clientdata(&data->lm75[i], NULL); + data->lm75[i].adapter = adapter; + data->lm75[i].driver = &w83781d_driver; + data->lm75[i].flags = 0; + strlcpy(data->lm75[i].dev.name, client_name, + DEVICE_NAME_SIZE); + if ((err = i2c_attach_client(&(data->lm75[i])))) { + dev_err(&new_client->dev, "Subclient %d " + "registration at address 0x%x " + "failed.\n", i, data->lm75[i].addr); + if (i == 1) + goto ERROR_SC_2; + goto ERROR_SC_1; + } + if (kind == w83783s) + break; + } + + return err; + +/* Undo inits in case of errors */ +ERROR_SC_2: + i2c_detach_client(&(data->lm75[0])); +ERROR_SC_1: + kfree(data->lm75); +ERROR_SC_0: + return err; +} + static int w83781d_detect(struct i2c_adapter *adapter, int address, int kind) { - int i = 0, val1 = 0, val2, id; + int i = 0, val1 = 0, val2; struct i2c_client *new_client; struct w83781d_data *data; int err = 0; - const char *type_name = ""; const char *client_name = ""; int is_isa = i2c_is_isa_adapter(adapter); enum vendor { winbond, asus } vendid; @@ -1047,11 +1145,9 @@ && !i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) goto ERROR0; - if (is_isa) { + if (is_isa) if (!request_region(address, W83781D_EXTENT, "w83781d")) goto ERROR0; - release_region(address, W83781D_EXTENT); - } /* Probe whether there is anything available on this address. Already done for SMBus clients */ @@ -1063,11 +1159,11 @@ if we read 'undefined' registers. */ i = inb_p(address + 1); if (inb_p(address + 2) != i) - goto ERROR0; + goto ERROR1; if (inb_p(address + 3) != i) - goto ERROR0; + goto ERROR1; if (inb_p(address + 7) != i) - goto ERROR0; + goto ERROR1; #undef REALLY_SLOW_IO /* Let's just hope nothing breaks here */ @@ -1087,7 +1183,7 @@ if (!(new_client = kmalloc(sizeof (struct i2c_client) + sizeof (struct w83781d_data), GFP_KERNEL))) { err = -ENOMEM; - goto ERROR0; + goto ERROR1; } memset(new_client, 0x00, sizeof (struct i2c_client) + @@ -1109,7 +1205,7 @@ bank. */ if (kind < 0) { if (w83781d_read_value(new_client, W83781D_REG_CONFIG) & 0x80) - goto ERROR1; + goto ERROR2; val1 = w83781d_read_value(new_client, W83781D_REG_BANK); val2 = w83781d_read_value(new_client, W83781D_REG_CHIPMAN); /* Check for Winbond or Asus ID if in bank 0 */ @@ -1118,13 +1214,13 @@ && (val2 != 0x94)) || ((val1 & 0x80) && (val2 != 0x5c) && (val2 != 0x12) && (val2 != 0x06)))) - goto ERROR1; + goto ERROR2; /* If Winbond SMBus, check address at 0x48. Asus doesn't support */ if ((!is_isa) && (((!(val1 & 0x80)) && (val2 == 0xa3)) || ((val1 & 0x80) && (val2 == 0x5c)))) { if (w83781d_read_value (new_client, W83781D_REG_I2C_ADDR) != address) - goto ERROR1; + goto ERROR2; } } @@ -1144,7 +1240,7 @@ else if ((val2 == 0x12) || (val2 == 0x06)) vendid = asus; else - goto ERROR1; + goto ERROR2; /* mask off lower bit, not reliable */ val1 = w83781d_read_value(new_client, W83781D_REG_WCHIPID) & 0xfe; @@ -1166,38 +1262,28 @@ "Ignoring 'force' parameter for unknown chip at" "adapter %d, address 0x%02x\n", i2c_adapter_id(adapter), address); - goto ERROR1; + goto ERROR2; } } if (kind == w83781d) { - type_name = "w83781d"; client_name = "W83781D chip"; } else if (kind == w83782d) { - type_name = "w83782d"; client_name = "W83782D chip"; } else if (kind == w83783s) { - type_name = "w83783s"; client_name = "W83783S chip"; } else if (kind == w83627hf) { - type_name = "w83627hf"; client_name = "W83627HF chip"; } else if (kind == as99127f) { - type_name = "as99127f"; client_name = "AS99127F chip"; } else if (kind == w83697hf) { - type_name = "w83697hf"; client_name = "W83697HF chip"; } else { dev_err(&new_client->dev, "Internal error: unknown kind (%d)?!?", kind); - goto ERROR1; + goto ERROR2; } - /* Reserve the ISA region */ - if (is_isa) - request_region(address, W83781D_EXTENT, type_name); - - /* Fill in the remaining client fields and put it into the global list */ + /* Fill in the remaining client fields and put into the global list */ strlcpy(new_client->dev.name, client_name, DEVICE_NAME_SIZE); data->type = kind; @@ -1206,76 +1292,13 @@ /* Tell the I2C layer a new client has arrived */ if ((err = i2c_attach_client(new_client))) - goto ERROR3; + goto ERROR2; /* attach secondary i2c lm75-like clients */ if (!is_isa) { - if (!(data->lm75 = kmalloc(2 * sizeof (struct i2c_client), - GFP_KERNEL))) { - err = -ENOMEM; - goto ERROR4; - } - - memset(data->lm75, 0x00, 2 * sizeof (struct i2c_client)); - - id = i2c_adapter_id(adapter); - if (force_subclients[0] == id && force_subclients[1] == address) { - for (i = 2; i <= 3; i++) { - if (force_subclients[i] < 0x48 || - force_subclients[i] > 0x4f) { - dev_err(&new_client->dev, - "Invalid subclient address %d; must be 0x48-0x4f\n", - force_subclients[i]); - goto ERROR5; - } - } - w83781d_write_value(new_client, - W83781D_REG_I2C_SUBADDR, - (force_subclients[2] & 0x07) | - ((force_subclients[3] & 0x07) << - 4)); - data->lm75[0].addr = force_subclients[2]; - } else { - val1 = w83781d_read_value(new_client, - W83781D_REG_I2C_SUBADDR); - data->lm75[0].addr = 0x48 + (val1 & 0x07); - } - if (kind != w83783s) { - if (force_subclients[0] == id && - force_subclients[1] == address) { - data->lm75[1].addr = force_subclients[3]; - } else { - data->lm75[1].addr = - 0x48 + ((val1 >> 4) & 0x07); - } - if (data->lm75[0].addr == data->lm75[1].addr) { - dev_err(&new_client->dev, - "Duplicate addresses 0x%x for subclients.\n", - data->lm75[0].addr); - goto ERROR5; - } - } - if (kind == w83781d) - client_name = "W83781D subclient"; - else if (kind == w83782d) - client_name = "W83782D subclient"; - else if (kind == w83783s) - client_name = "W83783S subclient"; - else if (kind == w83627hf) - client_name = "W83627HF subclient"; - else if (kind == as99127f) - client_name = "AS99127F subclient"; - - for (i = 0; i <= 1; i++) { - i2c_set_clientdata(&data->lm75[i], NULL); /* store all data in w83781d */ - data->lm75[i].adapter = adapter; - data->lm75[i].driver = &w83781d_driver; - data->lm75[i].flags = 0; - strlcpy(data->lm75[i].dev.name, client_name, - DEVICE_NAME_SIZE); - if (kind == w83783s) - break; - } + if ((err = w83781d_detect_subclients(adapter, address, + kind, new_client))) + goto ERROR3; } else { data->lm75 = NULL; } @@ -1346,24 +1369,14 @@ w83781d_init_client(new_client); return 0; -/* OK, this is not exactly good programming practice, usually. But it is - very code-efficient in this case. */ - - ERROR5: - if (!is_isa) { - i2c_detach_client(&data->lm75[0]); - if (data->type != w83783s) - i2c_detach_client(&data->lm75[1]); - kfree(data->lm75); - } - ERROR4: +ERROR3: i2c_detach_client(new_client); - ERROR3: +ERROR2: + kfree(new_client); +ERROR1: if (is_isa) release_region(address, W83781D_EXTENT); - ERROR1: - kfree(new_client); - ERROR0: +ERROR0: return err; } @@ -1373,12 +1386,7 @@ struct w83781d_data *data = i2c_get_clientdata(client); int err; - if ((err = i2c_detach_client(client))) { - dev_err(&client->dev, - "Client deregistration failed, client not detached.\n"); - return err; - } - + /* release ISA region or I2C subclients first */ if (i2c_is_isa_client(client)) { release_region(client->addr, W83781D_EXTENT); } else { @@ -1387,6 +1395,14 @@ i2c_detach_client(&data->lm75[1]); kfree(data->lm75); } + + /* now it's safe to scrap the rest */ + if ((err = i2c_detach_client(client))) { + dev_err(&client->dev, + "Client deregistration failed, client not detached.\n"); + return err; + } + kfree(client); return 0; ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17) 2003-06-01 14:38 ` [RFC PATCH] " Mark M. Hoffman @ 2003-06-02 17:20 ` Greg KH 2003-06-03 5:22 ` Martin Schlemmer 2003-06-05 2:39 ` Mark M. Hoffman 0 siblings, 2 replies; 21+ messages in thread From: Greg KH @ 2003-06-02 17:20 UTC (permalink / raw) To: LKML, Sensors, Mark D. Studebaker On Sun, Jun 01, 2003 at 10:38:08AM -0400, Mark M. Hoffman wrote: > > This fixes the oops during w83781d module removal by putting the > subclient registration back in. While I was in there, I split > w83781d_detect in half in an attempt to reduce the goto madness. > > So, the /sys tree looks like this, where 48 & 49 are the subclients. > There are no entries (besides name & power) for the subclients. > > /sys/bus/i2c/ > |-- devices > | |-- 0-002d -> ../../../devices/pci0/00:02.1/i2c-0/0-002d > | |-- 0-0048 -> ../../../devices/pci0/00:02.1/i2c-0/0-0048 > | `-- 0-0049 -> ../../../devices/pci0/00:02.1/i2c-0/0-0049 > `-- drivers > |-- i2c_adapter > `-- w83781d > |-- 0-002d -> ../../../../devices/pci0/00:02.1/i2c-0/0-002d > |-- 0-0048 -> ../../../../devices/pci0/00:02.1/i2c-0/0-0048 > `-- 0-0049 -> ../../../../devices/pci0/00:02.1/i2c-0/0-0049 > > Also, I fixed a bug where this driver would request and release an > ISA region, then poke around in that region, then finally request > it again. > > This patch against 2.5.70 works for me vs. an SMBus adapter. It needs > re-testing against an ISA adapter since my particular chip is SMBus only. I've applied this and will send it off to Linus in a bit. thanks, greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17) 2003-06-02 17:20 ` Greg KH @ 2003-06-03 5:22 ` Martin Schlemmer 2003-06-03 19:43 ` Philip Pokorny 2003-06-05 2:39 ` Mark M. Hoffman 1 sibling, 1 reply; 21+ messages in thread From: Martin Schlemmer @ 2003-06-03 5:22 UTC (permalink / raw) To: Greg KH; +Cc: LKML, Sensors On Mon, 2003-06-02 at 19:20, Greg KH wrote: Hiya Greg While sorda on the topic ... since I did the w83781d driver some time ago, I changed boards for a P4C800 (Intel 875 chipset), that have a ICH5 southbridge, and not a ICH4 one .... I tried to add the ID's to the i810 driver, and although it does load (even without the ID's added), the I2C bus/sensor does not show in /sys. The w83781d driver also load fine btw. Any ideas ? Anybody working on 875 support that I can help test ? Thanks, -- Martin Schlemmer ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17) 2003-06-03 5:22 ` Martin Schlemmer @ 2003-06-03 19:43 ` Philip Pokorny 2003-06-04 5:57 ` Martin Schlemmer 0 siblings, 1 reply; 21+ messages in thread From: Philip Pokorny @ 2003-06-03 19:43 UTC (permalink / raw) To: Martin Schlemmer; +Cc: Greg KH, LKML, Sensors [-- Attachment #1: Type: text/plain, Size: 1250 bytes --] Martin Schlemmer wrote: > On Mon, 2003-06-02 at 19:20, Greg KH wrote: > > Hiya Greg > > While sorda on the topic ... since I did the w83781d driver some time > ago, I changed boards for a P4C800 (Intel 875 chipset), that have a > ICH5 southbridge, and not a ICH4 one .... I tried to add the ID's > to the i810 driver, and although it does load (even without the > ID's added), the I2C bus/sensor does not show in /sys. The w83781d > driver also load fine btw. My system (SuperMicro) with an '875 and ICH5 reports the ICH5 as an '801EB' which means you should be using the i2c-i801 driver not i2c-i810... I'm also betting that you need to set 'isich4' to true in the case of the ich5 as well... Try the attached patch... NOTE: I have *not* consulted the Intel DOC's on the ICH4 and ICH5 to see if the register interface has changed in other interesting ways... > Any ideas ? Anybody working on 875 support that I can help test ? Unfortuntately, the SuperMicro has a Winbond '627HF chip on the ISA bus and no other SMBus sensors, so the i2c-i801 support didn't help me much. :v) -- Philip Pokorny, Director of Engineering Tel: 415-358-2635 Fax: 415-358-2646 Toll Free: 888-PENGUIN PENGUIN COMPUTING, INC. www.penguincomputing.com [-- Attachment #2: lm_sensors-2.7.0-ich5-1.patch --] [-- Type: text/plain, Size: 1527 bytes --] diff -ru lm_sensors-2.7.0/kernel/busses/i2c-i801.c lm_sensors-2.7.0.ich5/kernel/busses/i2c-i801.c --- lm_sensors-2.7.0/kernel/busses/i2c-i801.c 2002-08-10 11:29:40.000000000 -0700 +++ lm_sensors-2.7.0.ich5/kernel/busses/i2c-i801.c 2003-06-02 21:11:32.000000000 -0700 @@ -27,6 +27,7 @@ 82801BA 2443 82801CA/CAM 2483 82801DB 24C3 (HW PEC supported, 32 byte buffer not supported) + 82801EB 24D3 (HW PEC supported, 32 byte buffer not supported) This driver supports several versions of Intel's I/O Controller Hubs (ICH). For SMBus support, they are similar to the PIIX4 and are part @@ -71,11 +72,16 @@ #define PCI_DEVICE_ID_INTEL_82801CA_SMBUS 0x2483 #define PCI_DEVICE_ID_INTEL_82801DB_SMBUS 0x24C3 +#ifndef PCI_DEVICE_ID_INTEL_82801EB_SMBUS +#define PCI_DEVICE_ID_INTEL_82801EB_SMBUS 0x24D3 +#endif + static int supported[] = {PCI_DEVICE_ID_INTEL_82801AA_3, PCI_DEVICE_ID_INTEL_82801AB_3, PCI_DEVICE_ID_INTEL_82801BA_2, PCI_DEVICE_ID_INTEL_82801CA_SMBUS, PCI_DEVICE_ID_INTEL_82801DB_SMBUS, + PCI_DEVICE_ID_INTEL_82801EB_SMBUS, 0 }; /* I801 SMBus address offsets */ @@ -214,7 +220,9 @@ error_return = -ENODEV; goto END; } - isich4 = *num == PCI_DEVICE_ID_INTEL_82801DB_SMBUS; + isich4 = (*num == PCI_DEVICE_ID_INTEL_82801DB_SMBUS) + || (*num == PCI_DEVICE_ID_INTEL_82801EB_SMBUS) + ; /* Determine the address of the SMBus areas */ if (force_addr) { ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17) 2003-06-03 19:43 ` Philip Pokorny @ 2003-06-04 5:57 ` Martin Schlemmer 0 siblings, 0 replies; 21+ messages in thread From: Martin Schlemmer @ 2003-06-04 5:57 UTC (permalink / raw) To: Philip Pokorny; +Cc: Greg KH, LKML, Sensors On Tue, 2003-06-03 at 21:43, Philip Pokorny wrote: > Martin Schlemmer wrote: > > On Mon, 2003-06-02 at 19:20, Greg KH wrote: > > > > Hiya Greg > > > > While sorda on the topic ... since I did the w83781d driver some time > > ago, I changed boards for a P4C800 (Intel 875 chipset), that have a > > ICH5 southbridge, and not a ICH4 one .... I tried to add the ID's > > to the i810 driver, and although it does load (even without the > > ID's added), the I2C bus/sensor does not show in /sys. The w83781d > > driver also load fine btw. > > My system (SuperMicro) with an '875 and ICH5 reports the ICH5 as an > '801EB' which means you should be using the i2c-i801 driver not i2c-i810... > Right ... at work, so could not verify. > I'm also betting that you need to set 'isich4' to true in the case of > the ich5 as well... > > Try the attached patch... NOTE: I have *not* consulted the Intel DOC's > on the ICH4 and ICH5 to see if the register interface has changed in > other interesting ways... > Will let you know, thanks. Regards, -- Martin Schlemmer ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17) 2003-06-02 17:20 ` Greg KH 2003-06-03 5:22 ` Martin Schlemmer @ 2003-06-05 2:39 ` Mark M. Hoffman 2003-06-05 19:47 ` Greg KH 1 sibling, 1 reply; 21+ messages in thread From: Mark M. Hoffman @ 2003-06-05 2:39 UTC (permalink / raw) To: LKML, Sensors; +Cc: Greg KH, Martin Schlemmer [-- Attachment #1: Type: text/plain, Size: 640 bytes --] * Greg KH <greg@kroah.com> [2003-06-02 10:20:40 -0700]: > On Sun, Jun 01, 2003 at 10:38:08AM -0400, Mark M. Hoffman wrote: > > > > This patch against 2.5.70 works for me vs. an SMBus adapter. It needs > > re-testing against an ISA adapter since my particular chip is SMBus only. > > I've applied this and will send it off to Linus in a bit. Thanks! This patch fixes the various return values in the w83781d_detect() error paths. It also cleans up some formatting here and there. It should be applied on top of the previous one. It works for me; same caveat as above w.r.t. ISA. Regards, -- Mark M. Hoffman mhoffman@lightlink.com [-- Attachment #2: patch-w83781d-2.txt --] [-- Type: text/plain, Size: 4622 bytes --] --- linux-2.5.70/drivers/i2c/chips/w83781d.c.orig1 2003-06-04 21:05:35.000000000 -0400 +++ linux-2.5.70/drivers/i2c/chips/w83781d.c 2003-06-04 21:37:11.000000000 -0400 @@ -1039,7 +1039,7 @@ struct i2c_client *new_client) { int i, val1 = 0, id; - int err = 0; + int err; const char *client_name; struct w83781d_data *data = i2c_get_clientdata(new_client); @@ -1058,7 +1058,8 @@ force_subclients[i] > 0x4f) { dev_err(&new_client->dev, "Invalid subclient " "address %d; must be 0x48-0x4f\n", - force_subclients[i]); + force_subclients[i]); + err = -EINVAL; goto ERROR_SC_1; } } @@ -1082,6 +1083,7 @@ dev_err(&new_client->dev, "Duplicate addresses 0x%x for subclients.\n", data->lm75[0].addr); + err = -EBUSY; goto ERROR_SC_1; } } @@ -1119,7 +1121,7 @@ break; } - return err; + return 0; /* Undo inits in case of errors */ ERROR_SC_2: @@ -1136,18 +1138,22 @@ int i = 0, val1 = 0, val2; struct i2c_client *new_client; struct w83781d_data *data; - int err = 0; + int err; const char *client_name = ""; int is_isa = i2c_is_isa_adapter(adapter); enum vendor { winbond, asus } vendid; if (!is_isa - && !i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) + && !i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) { + err = -EINVAL; goto ERROR0; + } if (is_isa) - if (!request_region(address, W83781D_EXTENT, "w83781d")) + if (!request_region(address, W83781D_EXTENT, "w83781d")) { + err = -EBUSY; goto ERROR0; + } /* Probe whether there is anything available on this address. Already done for SMBus clients */ @@ -1155,15 +1161,21 @@ if (is_isa) { #define REALLY_SLOW_IO - /* We need the timeouts for at least some LM78-like chips. But only - if we read 'undefined' registers. */ + /* We need the timeouts for at least some LM78-like + chips. But only if we read 'undefined' registers. */ i = inb_p(address + 1); - if (inb_p(address + 2) != i) + if (inb_p(address + 2) != i) { + err = -ENODEV; goto ERROR1; - if (inb_p(address + 3) != i) + } + if (inb_p(address + 3) != i) { + err = -ENODEV; goto ERROR1; - if (inb_p(address + 7) != i) + } + if (inb_p(address + 7) != i) { + err = -ENODEV; goto ERROR1; + } #undef REALLY_SLOW_IO /* Let's just hope nothing breaks here */ @@ -1171,7 +1183,8 @@ outb_p(~i & 0x7f, address + 5); if ((inb_p(address + 5) & 0x7f) != (~i & 0x7f)) { outb_p(i, address + 5); - return 0; + err = -ENODEV; + goto ERROR1; } } } @@ -1204,8 +1217,10 @@ force_*=... parameter, and the Winbond will be reset to the right bank. */ if (kind < 0) { - if (w83781d_read_value(new_client, W83781D_REG_CONFIG) & 0x80) + if (w83781d_read_value(new_client, W83781D_REG_CONFIG) & 0x80){ + err = -ENODEV; goto ERROR2; + } val1 = w83781d_read_value(new_client, W83781D_REG_BANK); val2 = w83781d_read_value(new_client, W83781D_REG_CHIPMAN); /* Check for Winbond or Asus ID if in bank 0 */ @@ -1213,14 +1228,19 @@ (((!(val1 & 0x80)) && (val2 != 0xa3) && (val2 != 0xc3) && (val2 != 0x94)) || ((val1 & 0x80) && (val2 != 0x5c) && (val2 != 0x12) - && (val2 != 0x06)))) + && (val2 != 0x06)))) { + err = -ENODEV; goto ERROR2; - /* If Winbond SMBus, check address at 0x48. Asus doesn't support */ + } + /* If Winbond SMBus, check address at 0x48. + Asus doesn't support */ if ((!is_isa) && (((!(val1 & 0x80)) && (val2 == 0xa3)) || ((val1 & 0x80) && (val2 == 0x5c)))) { if (w83781d_read_value - (new_client, W83781D_REG_I2C_ADDR) != address) + (new_client, W83781D_REG_I2C_ADDR) != address) { + err = -ENODEV; goto ERROR2; + } } } @@ -1239,8 +1259,11 @@ vendid = winbond; else if ((val2 == 0x12) || (val2 == 0x06)) vendid = asus; - else + else { + err = -ENODEV; goto ERROR2; + } + /* mask off lower bit, not reliable */ val1 = w83781d_read_value(new_client, W83781D_REG_WCHIPID) & 0xfe; @@ -1262,6 +1285,7 @@ "Ignoring 'force' parameter for unknown chip at" "adapter %d, address 0x%02x\n", i2c_adapter_id(adapter), address); + err = -EINVAL; goto ERROR2; } } @@ -1279,7 +1303,9 @@ } else if (kind == w83697hf) { client_name = "W83697HF chip"; } else { - dev_err(&new_client->dev, "Internal error: unknown kind (%d)?!?", kind); + dev_err(&new_client->dev, "Internal error: unknown " + "kind (%d)?!?", kind); + err = -ENODEV; goto ERROR2; } ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17) 2003-06-05 2:39 ` Mark M. Hoffman @ 2003-06-05 19:47 ` Greg KH 2003-06-09 5:34 ` Martin Schlemmer 0 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2003-06-05 19:47 UTC (permalink / raw) To: LKML, Sensors, Martin Schlemmer On Wed, Jun 04, 2003 at 10:39:22PM -0400, Mark M. Hoffman wrote: > * Greg KH <greg@kroah.com> [2003-06-02 10:20:40 -0700]: > > On Sun, Jun 01, 2003 at 10:38:08AM -0400, Mark M. Hoffman wrote: > > > > > > This patch against 2.5.70 works for me vs. an SMBus adapter. It needs > > > re-testing against an ISA adapter since my particular chip is SMBus only. > > > > I've applied this and will send it off to Linus in a bit. > > Thanks! > > This patch fixes the various return values in the w83781d_detect() > error paths. It also cleans up some formatting here and there. > It should be applied on top of the previous one. > > It works for me; same caveat as above w.r.t. ISA. Applied, thanks. greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17) 2003-06-05 19:47 ` Greg KH @ 2003-06-09 5:34 ` Martin Schlemmer 2003-06-10 5:38 ` Martin Schlemmer 2003-06-10 5:41 ` OOPS w83781d during rmmod (2.5.70-bk1[1234]) Mark M. Hoffman 0 siblings, 2 replies; 21+ messages in thread From: Martin Schlemmer @ 2003-06-09 5:34 UTC (permalink / raw) To: Greg KH; +Cc: LKML, Sensors On Thu, 2003-06-05 at 21:47, Greg KH wrote: > On Wed, Jun 04, 2003 at 10:39:22PM -0400, Mark M. Hoffman wrote: > > * Greg KH <greg@kroah.com> [2003-06-02 10:20:40 -0700]: > > > On Sun, Jun 01, 2003 at 10:38:08AM -0400, Mark M. Hoffman wrote: > > > > > > > > This patch against 2.5.70 works for me vs. an SMBus adapter. It needs > > > > re-testing against an ISA adapter since my particular chip is SMBus only. > > > > > > I've applied this and will send it off to Linus in a bit. > > > > Thanks! > > > > This patch fixes the various return values in the w83781d_detect() > > error paths. It also cleans up some formatting here and there. > > It should be applied on top of the previous one. > > > > It works for me; same caveat as above w.r.t. ISA. > > Applied, thanks. > Things have changed since I converted this driver to 2.5. I no longer have the 850E chipset mobo with w83781d sensor, but a 875p chipset mobo, with W83627THF (basically the same as the W83627HF, just with advance fan control .. prob the Q-Fan option in the Asus board?). I sorda got the ICH5 talking, and can get the driver loaded for the W83627THF (as W83627HF), but all the values seems borked. Unfortunately I cannot get a spec sheet on the W83627THF. Is anybody working on ICH5/W83627THF support ? Anyhow, Only change I have made to the w83781d driver, is one line (just tell it to that if the chip id is 0x72, its also of type w83726HF), but now (2.5.70-bk1[123]) it segfaults for me on rmmod, where it did not with 2.5.68 kernels when I still had the other board. I will attach a oops tomorrow or such when I get home. Regards, -- Martin Schlemmer ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17) 2003-06-09 5:34 ` Martin Schlemmer @ 2003-06-10 5:38 ` Martin Schlemmer 2003-06-10 5:41 ` OOPS w83781d during rmmod (2.5.70-bk1[1234]) Mark M. Hoffman 1 sibling, 0 replies; 21+ messages in thread From: Martin Schlemmer @ 2003-06-10 5:38 UTC (permalink / raw) To: Greg KH; +Cc: LKML, Sensors On Mon, 2003-06-09 at 07:34, Martin Schlemmer wrote: > On Thu, 2003-06-05 at 21:47, Greg KH wrote: > > On Wed, Jun 04, 2003 at 10:39:22PM -0400, Mark M. Hoffman wrote: > > > * Greg KH <greg@kroah.com> [2003-06-02 10:20:40 -0700]: > > > > On Sun, Jun 01, 2003 at 10:38:08AM -0400, Mark M. Hoffman wrote: > > > > > > > > > > This patch against 2.5.70 works for me vs. an SMBus adapter. It needs > > > > > re-testing against an ISA adapter since my particular chip is SMBus only. > > > > > > > > I've applied this and will send it off to Linus in a bit. > > > > > > Thanks! > > > > > > This patch fixes the various return values in the w83781d_detect() > > > error paths. It also cleans up some formatting here and there. > > > It should be applied on top of the previous one. > > > > > > It works for me; same caveat as above w.r.t. ISA. > > > > Applied, thanks. > > > > Things have changed since I converted this driver to 2.5. I no longer > have the 850E chipset mobo with w83781d sensor, but a 875p chipset > mobo, with W83627THF (basically the same as the W83627HF, just with > advance fan control .. prob the Q-Fan option in the Asus board?). > > I sorda got the ICH5 talking, and can get the driver loaded for the > W83627THF (as W83627HF), but all the values seems borked. Unfortunately > I cannot get a spec sheet on the W83627THF. Is anybody working on > ICH5/W83627THF support ? > > Anyhow, Only change I have made to the w83781d driver, is one line > (just tell it to that if the chip id is 0x72, its also of type > w83726HF), but now (2.5.70-bk1[123]) it segfaults for me on rmmod, where > it did not with 2.5.68 kernels when I still had the other board. I will > attach a oops tomorrow or such when I get home. > Ok, here is the oops. Shout if more info is needed. --------- ksymoops 2.4.8 on i686 2.5.70-bk14. Options used -V (default) -k /proc/ksyms (default) -l /proc/modules (default) -o /lib/modules/2.5.70-bk14/ (default) -m /usr/src/linux/System.map (default) Warning: You did not tell me where to find symbol information. I will assume that the log matches the kernel and modules that are running right now and I'll use the default options above for symbol resolution. If the current kernel and/or modules do not match the log, you can get more accurate output by telling me the kernel version and where to find map, modules, ksyms etc. ksymoops -h explains the options. Error (regular_file): read_ksyms stat /proc/ksyms failed No modules in ksyms, skipping objects No ksyms, skipping lsmod Oops: 0000 [#1] CPU: 0 EIP: 0060:[<fc868421>] Not tainted Using defaults from ksymoops -t elf32-i386 -a i386 EFLAGS: 00010203 eax: fc848744 ebx: f6e3851c ecx: 00000000 edx: 6b6b6b6b esi: 6b6b6b6b edi: fc84874c ebp: fc8485e0 esp: f2195f2c ds: 007b es: 007b ss: 0068 Stack: f6e38310 0000004c f7232b18 00000296 f7ffd760 fc903164 fc903600 00000880 c0309298 00000000 fc900107 fc903160 c012f049 fc903600 bffff728 0000003b 00000000 37333877 00643138 c0141620 f757ddf4 f7232b1c f7232870 40016000 Call Trace: [<fc903164>] w83781d_driver+0x4/0xa8 [w83781d] [<fc903600>] +0x0/0x200 [w83781d] [<fc900107>] +0xf/0x13 [w83781d] [<fc903160>] w83781d_driver+0x0/0xa8 [w83781d] [<c012f049>] sys_delete_module+0x12b/0x195 [<fc903600>] +0x0/0x200 [w83781d] [<c0141620>] do_munmap+0x146/0x183 [<c010a7d5>] sysenter_past_esp+0x52/0x71 Code: 8b 36 39 c2 75 df 8b 0f 8b 01 89 cf 89 c1 0f 18 00 90 81 ff >>EIP; fc868421 <_end+3c3981dd/3fb2ddbc> <===== >>eax; fc848744 <_end+3c378500/3fb2ddbc> >>ebx; f6e3851c <_end+369682d8/3fb2ddbc> >>edi; fc84874c <_end+3c378508/3fb2ddbc> >>ebp; fc8485e0 <_end+3c37839c/3fb2ddbc> >>esp; f2195f2c <_end+31cc5ce8/3fb2ddbc> Trace; fc903164 <_end+3c432f20/3fb2ddbc> Trace; fc903600 <_end+3c4333bc/3fb2ddbc> Trace; fc900107 <_end+3c42fec3/3fb2ddbc> Trace; fc903160 <_end+3c432f1c/3fb2ddbc> Trace; c012f049 <sys_delete_module+12b/195> Trace; fc903600 <_end+3c4333bc/3fb2ddbc> Trace; c0141620 <do_munmap+146/183> Trace; c010a7d5 <sysenter_past_esp+52/71> Code; fc868421 <_end+3c3981dd/3fb2ddbc> 00000000 <_EIP>: Code; fc868421 <_end+3c3981dd/3fb2ddbc> <===== 0: 8b 36 mov (%esi),%esi <===== Code; fc868423 <_end+3c3981df/3fb2ddbc> 2: 39 c2 cmp %eax,%edx Code; fc868425 <_end+3c3981e1/3fb2ddbc> 4: 75 df jne ffffffe5 <_EIP+0xffffffe5> Code; fc868427 <_end+3c3981e3/3fb2ddbc> 6: 8b 0f mov (%edi),%ecx Code; fc868429 <_end+3c3981e5/3fb2ddbc> 8: 8b 01 mov (%ecx),%eax Code; fc86842b <_end+3c3981e7/3fb2ddbc> a: 89 cf mov %ecx,%edi Code; fc86842d <_end+3c3981e9/3fb2ddbc> c: 89 c1 mov %eax,%ecx Code; fc86842f <_end+3c3981eb/3fb2ddbc> e: 0f 18 00 prefetchnta (%eax) Code; fc868432 <_end+3c3981ee/3fb2ddbc> 11: 90 nop Code; fc868433 <_end+3c3981ef/3fb2ddbc> 12: 81 ff 00 00 00 00 cmp $0x0,%edi 1 warning and 1 error issued. Results may not be reliable. -- Martin Schlemmer ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: OOPS w83781d during rmmod (2.5.70-bk1[1234]) 2003-06-09 5:34 ` Martin Schlemmer 2003-06-10 5:38 ` Martin Schlemmer @ 2003-06-10 5:41 ` Mark M. Hoffman 2003-06-10 5:51 ` Martin Schlemmer 2003-06-12 6:57 ` [RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234])) Martin Schlemmer 1 sibling, 2 replies; 21+ messages in thread From: Mark M. Hoffman @ 2003-06-10 5:41 UTC (permalink / raw) To: Martin Schlemmer; +Cc: Greg KH, LKML, Sensors * Martin Schlemmer <azarah@gentoo.org> [2003-06-09 07:34:30 +0200]: > > Anyhow, Only change I have made to the w83781d driver, is one line > (just tell it to that if the chip id is 0x72, its also of type > w83726HF), but now (2.5.70-bk1[123]) it segfaults for me on rmmod, where > it did not with 2.5.68 kernels when I still had the other board. I will > attach a oops tomorrow or such when I get home. I reproduced the segfault here. It looks like i2c_del_driver() tries to call w83781d_detach_client() more than once now, partly because of the safe list fix in 2.5.70-bk11. But that function should only be called for the "primary" client, not the subclients. The quick/ugly patch below fixes the symptom, but maybe not the disease. There might be more fundamental brokenness in the whole subclient scheme. I'll keep looking when I get the chance. --- linux-2.5.70-bk14/drivers/i2c/chips/w83781d.c 2003-06-10 00:49:19.831210956 -0400 +++ linux-2.5.70/drivers/i2c/chips/w83781d.c 2003-06-10 00:53:35.041027614 -0400 @@ -1412,6 +1412,10 @@ struct w83781d_data *data = i2c_get_clientdata(client); int err; + /* if this is a subclient, do nothing */ + if (!data) + return 0; + /* release ISA region or I2C subclients first */ if (i2c_is_isa_client(client)) { release_region(client->addr, W83781D_EXTENT); Regards, -- Mark M. Hoffman mhoffman@lightlink.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: OOPS w83781d during rmmod (2.5.70-bk1[1234]) 2003-06-10 5:41 ` OOPS w83781d during rmmod (2.5.70-bk1[1234]) Mark M. Hoffman @ 2003-06-10 5:51 ` Martin Schlemmer 2003-06-11 5:44 ` Martin Schlemmer 2003-06-12 6:57 ` [RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234])) Martin Schlemmer 1 sibling, 1 reply; 21+ messages in thread From: Martin Schlemmer @ 2003-06-10 5:51 UTC (permalink / raw) To: Mark M. Hoffman; +Cc: Greg KH, LKML, Sensors On Tue, 2003-06-10 at 07:41, Mark M. Hoffman wrote: > * Martin Schlemmer <azarah@gentoo.org> [2003-06-09 07:34:30 +0200]: > > > > Anyhow, Only change I have made to the w83781d driver, is one line > > (just tell it to that if the chip id is 0x72, its also of type > > w83726HF), but now (2.5.70-bk1[123]) it segfaults for me on rmmod, where > > it did not with 2.5.68 kernels when I still had the other board. I will > > attach a oops tomorrow or such when I get home. > > I reproduced the segfault here. It looks like i2c_del_driver() tries > to call w83781d_detach_client() more than once now, partly because of > the safe list fix in 2.5.70-bk11. But that function should only be > called for the "primary" client, not the subclients. > > The quick/ugly patch below fixes the symptom, but maybe not the disease. > There might be more fundamental brokenness in the whole subclient scheme. > I'll keep looking when I get the chance. > Ok, I will give it a go tonight. I already sent the oops to the list before reading this if need be ... Regards, -- Martin Schlemmer ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: OOPS w83781d during rmmod (2.5.70-bk1[1234]) 2003-06-10 5:51 ` Martin Schlemmer @ 2003-06-11 5:44 ` Martin Schlemmer 0 siblings, 0 replies; 21+ messages in thread From: Martin Schlemmer @ 2003-06-11 5:44 UTC (permalink / raw) To: Mark M. Hoffman; +Cc: Greg KH, LKML, Sensors On Tue, 2003-06-10 at 07:51, Martin Schlemmer wrote: > On Tue, 2003-06-10 at 07:41, Mark M. Hoffman wrote: > > * Martin Schlemmer <azarah@gentoo.org> [2003-06-09 07:34:30 +0200]: > > > > > > Anyhow, Only change I have made to the w83781d driver, is one line > > > (just tell it to that if the chip id is 0x72, its also of type > > > w83726HF), but now (2.5.70-bk1[123]) it segfaults for me on rmmod, where > > > it did not with 2.5.68 kernels when I still had the other board. I will > > > attach a oops tomorrow or such when I get home. > > > > I reproduced the segfault here. It looks like i2c_del_driver() tries > > to call w83781d_detach_client() more than once now, partly because of > > the safe list fix in 2.5.70-bk11. But that function should only be > > called for the "primary" client, not the subclients. > > > > The quick/ugly patch below fixes the symptom, but maybe not the disease. > > There might be more fundamental brokenness in the whole subclient scheme. > > I'll keep looking when I get the chance. > > > Nope, It does not fix it. It basically happens when i2c_del_driver is called for w83781d_driver. I did add a few printk's, but it do not seem to be anything that w83781d_detach_client does. Rather, it looks to be while i2c_del_driver is walking the clients for the adapter, after it ran w83781d_detach_client for the chip (and not the two clients). I am assuming it grabs the client for the main chip, calls w83781d_detach_client which then in turn free all the memory for the main client and the two lm75 clients, return, and then check the next client for the adapter, which happens to be one of the lm75 clients for who we already freed the structures .... I hope that was sort of comprehensible. Anyhow, I will poke at it more tonight, if thoughts, let me know. Regards, -- Martin Schlemmer ^ permalink raw reply [flat|nested] 21+ messages in thread
* [RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234])) 2003-06-10 5:41 ` OOPS w83781d during rmmod (2.5.70-bk1[1234]) Mark M. Hoffman 2003-06-10 5:51 ` Martin Schlemmer @ 2003-06-12 6:57 ` Martin Schlemmer 2003-06-13 2:36 ` Mark M. Hoffman 1 sibling, 1 reply; 21+ messages in thread From: Martin Schlemmer @ 2003-06-12 6:57 UTC (permalink / raw) To: Greg KH; +Cc: Mark M. Hoffman, LKML, Sensors On Tue, 2003-06-10 at 07:41, Mark M. Hoffman wrote: > * Martin Schlemmer <azarah@gentoo.org> [2003-06-09 07:34:30 +0200]: > > > > Anyhow, Only change I have made to the w83781d driver, is one line > > (just tell it to that if the chip id is 0x72, its also of type > > w83726HF), but now (2.5.70-bk1[123]) it segfaults for me on rmmod, where > > it did not with 2.5.68 kernels when I still had the other board. I will > > attach a oops tomorrow or such when I get home. > > I reproduced the segfault here. It looks like i2c_del_driver() tries > to call w83781d_detach_client() more than once now, partly because of > the safe list fix in 2.5.70-bk11. But that function should only be > called for the "primary" client, not the subclients. > > The quick/ugly patch below fixes the symptom, but maybe not the disease. > There might be more fundamental brokenness in the whole subclient scheme. > I'll keep looking when I get the chance. > > --- linux-2.5.70-bk14/drivers/i2c/chips/w83781d.c 2003-06-10 00:49:19.831210956 -0400 > +++ linux-2.5.70/drivers/i2c/chips/w83781d.c 2003-06-10 00:53:35.041027614 -0400 > @@ -1412,6 +1412,10 @@ > struct w83781d_data *data = i2c_get_clientdata(client); > int err; > > + /* if this is a subclient, do nothing */ > + if (!data) > + return 0; > + > /* release ISA region or I2C subclients first */ > if (i2c_is_isa_client(client)) { > release_region(client->addr, W83781D_EXTENT); > Nope, this do not fix it. The problem is actually at the i2c driver side. From drivers/i2c/i2c-core.c in i2c_del_driver(), we have: ----------------------------------- } else { /* not if (driver->detach_adapter) */ list_for_each_safe(item2, _n, &adap->clients) { client = list_entry(item2, struct i2c_client, list); if (client->driver != driver) continue; DEB2(printk(KERN_DEBUG "i2c-core.o: " "detaching client %s:\n", client->dev.name)); if ((res = driver->detach_client(client))) { dev_err(&adap->dev, "while " "unregistering driver " "`%s', the client at " "address %02x of " "adapter could not " "be detached; driver " "not unloaded!", driver->name, client->addr); goto out_unlock; } } } ----------------------------------- As list_for_each_safe walk the list, _n gets pointed to a list that will be removed when detach_client() is called, and on the next loop, item2 will be set to _n, which is by now invalid, causing the list_entry() call to put a bogus pointer in 'client', and ultimately ending up in a oops when 'if (client->driver != driver)' gets executed. A working fix is as follows: ----------------------------------- --- 1/drivers/i2c/i2c-core.c 2003-06-12 00:03:36.000000000 +0200 +++ 2/drivers/i2c/i2c-core.c 2003-06-12 00:06:39.000000000 +0200 @@ -263,6 +263,12 @@ int i2c_del_driver(struct i2c_driver *dr client->addr); goto out_unlock; } + /* detach_client() is going to delete the list (&adap->clients) + * from under us, so make sure it is still there before calling + * list_entry again ... + */ + if (list_empty(&adap->clients)) + break; } } } ----------------------------------- This however brings me to a maybe bigger issue ... 1) Does i2c_del_driver use list_for_each_safe properly 2) And if it does, is list_for_each_safe as 'safe' as it should be ? The following piece of code demonstrates what happens here (yes, its not a work of art): ----------------------------------- #include <stdlib.h> #include <stdio.h> struct list; struct list { struct list *prev; struct list *next; }; int main(void) { struct list a, b, c, d; struct list *head = &a, *pos, *n; a.prev = &d; a.next = &b; b.prev = &a; b.next = &c; c.prev = &b; c.next = &d; d.prev = &c; d.next = &a; printf("a = 0x%X, b = 0x%X, c = 0x%X, d = 0x%X\n", &a, &b, &c, &d); printf("\nRun once, no member removing\n\n"); /* this for is basically what list_for_each_safe() does ... */ for (pos = (head)->next, n = pos->next; pos != (head); \ pos = n, n = pos->next) { printf(" pos = 0x%X\n", pos); } printf("\n\nRun again, but remove 'c' when pos points to 'b',\n"); printf("and n points to 'c'\n\n"); for (pos = (head)->next, n = pos->next; pos != (head); \ pos = n, n = pos->next) { printf(" new pos = 0x%X\n", pos); if (c.prev != (struct list *)0x02020202) { printf(" * Removing 'c'\n"); c.prev = (struct list *)0x02020202; c.next = (struct list *)0x01010101; b.next = &d; d.prev = &b; } } return 0; } ----------------------------------- This give the following result: ----------------------------------- # ./foo a = 0xBFFFF664, b = 0xBFFFF65C, c = 0xBFFFF654, d = 0xBFFFF64C Run once, no member removing pos = 0xBFFFF65C pos = 0xBFFFF654 pos = 0xBFFFF64C Run again, but remove 'c' when pos points to 'b', and n points to 'c' new pos = 0xBFFFF65C * Removing 'c' new pos = 0xBFFFF654 Segmentation fault # ----------------------------------- Basically, as list_for_each_safe go over the list, it sets 'n' to the next item, and 'pos' to the previous value of 'n'. If list item contained in 'n' is now however deleted in this instance of the loop, with the next loop 'pos' will be set to an invalid list item ... in this case it causes an oops. If list_for_each_safe *is* used correctly in this case, then I am afraid that we could see more of this type of problems. In only the i2c core drivers, list_for_each_safe is used once more in the same fasion. In the whole tree, it is used more than 200 times .... Anyhow, comments, etc appreciated. Regards, -- Martin Schlemmer ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234])) 2003-06-12 6:57 ` [RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234])) Martin Schlemmer @ 2003-06-13 2:36 ` Mark M. Hoffman 2003-06-13 6:08 ` Martin Schlemmer 2003-06-14 6:26 ` Martin Schlemmer 0 siblings, 2 replies; 21+ messages in thread From: Mark M. Hoffman @ 2003-06-13 2:36 UTC (permalink / raw) To: Martin Schlemmer; +Cc: LKML, Sensors, Greg KH * Martin Schlemmer <azarah@gentoo.org> [2003-06-12 08:57:02 +0200]: > On Tue, 2003-06-10 at 07:41, Mark M. Hoffman wrote: > > * Martin Schlemmer <azarah@gentoo.org> [2003-06-09 07:34:30 +0200]: > > > > > > Anyhow, Only change I have made to the w83781d driver, is one line > > > (just tell it to that if the chip id is 0x72, its also of type > > > w83726HF), but now (2.5.70-bk1[123]) it segfaults for me on rmmod, where > > > it did not with 2.5.68 kernels when I still had the other board. I will > > > attach a oops tomorrow or such when I get home. > > > > I reproduced the segfault here. It looks like i2c_del_driver() tries > > to call w83781d_detach_client() more than once now, partly because of > > the safe list fix in 2.5.70-bk11. But that function should only be > > called for the "primary" client, not the subclients. > > > > The quick/ugly patch below fixes the symptom, but maybe not the disease. > > There might be more fundamental brokenness in the whole subclient scheme. > > I'll keep looking when I get the chance. > > > > <broken patch ommitted> > > Nope, this do not fix it. > > The problem is actually at the i2c driver side. From > drivers/i2c/i2c-core.c in i2c_del_driver(), we have: <cut> To recap: list_for_each_safe() is not safe for deleting other than the current "item". But I disagree with you Martin because I think the usage in i2c_del_driver is ok as is; and that w83781d.c should change... My first patch was naive; the patch below solves the problem by letting w83781d_detach_client remove the three clients (1 * primary + 2 * subclients) independently. It's a noisy patch because I had to change the way the subclients were kmalloc'ed - sorry. The meat is around line 1422. This patch works for me... comments? Regards, --- linux-2.5.70-bk14/drivers/i2c/chips/w83781d.c 2003-06-10 00:49:19.000000000 -0400 +++ linux-2.5.70/drivers/i2c/chips/w83781d.c 2003-06-12 22:24:47.000000000 -0400 @@ -299,8 +299,8 @@ char valid; /* !=0 if following fields are valid */ unsigned long last_updated; /* In jiffies */ - struct i2c_client *lm75; /* for secondary I2C addresses */ - /* pointer to array of 2 subclients */ + struct i2c_client *lm75[2]; /* for secondary I2C addresses */ + /* array of 2 pointers to subclients */ u8 in[9]; /* Register value - 8 & 9 for 782D only */ u8 in_max[9]; /* Register value - 8 & 9 for 782D only */ @@ -1043,12 +1043,12 @@ const char *client_name; struct w83781d_data *data = i2c_get_clientdata(new_client); - if (!(data->lm75 = kmalloc(2 * sizeof (struct i2c_client), - GFP_KERNEL))) { + data->lm75[0] = kmalloc(sizeof(struct i2c_client), GFP_KERNEL); + if (!(data->lm75[0])) { err = -ENOMEM; goto ERROR_SC_0; } - memset(data->lm75, 0x00, 2 * sizeof (struct i2c_client)); + memset(data->lm75[0], 0x00, sizeof (struct i2c_client)); id = i2c_adapter_id(adapter); @@ -1066,25 +1066,33 @@ w83781d_write_value(new_client, W83781D_REG_I2C_SUBADDR, (force_subclients[2] & 0x07) | ((force_subclients[3] & 0x07) << 4)); - data->lm75[0].addr = force_subclients[2]; + data->lm75[0]->addr = force_subclients[2]; } else { val1 = w83781d_read_value(new_client, W83781D_REG_I2C_SUBADDR); - data->lm75[0].addr = 0x48 + (val1 & 0x07); + data->lm75[0]->addr = 0x48 + (val1 & 0x07); } if (kind != w83783s) { + + data->lm75[1] = kmalloc(sizeof(struct i2c_client), GFP_KERNEL); + if (!(data->lm75[1])) { + err = -ENOMEM; + goto ERROR_SC_1; + } + memset(data->lm75[1], 0x0, sizeof(struct i2c_client)); + if (force_subclients[0] == id && force_subclients[1] == address) { - data->lm75[1].addr = force_subclients[3]; + data->lm75[1]->addr = force_subclients[3]; } else { - data->lm75[1].addr = 0x48 + ((val1 >> 4) & 0x07); + data->lm75[1]->addr = 0x48 + ((val1 >> 4) & 0x07); } - if (data->lm75[0].addr == data->lm75[1].addr) { + if (data->lm75[0]->addr == data->lm75[1]->addr) { dev_err(&new_client->dev, "Duplicate addresses 0x%x for subclients.\n", - data->lm75[0].addr); + data->lm75[0]->addr); err = -EBUSY; - goto ERROR_SC_1; + goto ERROR_SC_2; } } @@ -1103,19 +1111,19 @@ for (i = 0; i <= 1; i++) { /* store all data in w83781d */ - i2c_set_clientdata(&data->lm75[i], NULL); - data->lm75[i].adapter = adapter; - data->lm75[i].driver = &w83781d_driver; - data->lm75[i].flags = 0; - strlcpy(data->lm75[i].dev.name, client_name, + i2c_set_clientdata(data->lm75[i], NULL); + data->lm75[i]->adapter = adapter; + data->lm75[i]->driver = &w83781d_driver; + data->lm75[i]->flags = 0; + strlcpy(data->lm75[i]->dev.name, client_name, DEVICE_NAME_SIZE); - if ((err = i2c_attach_client(&(data->lm75[i])))) { + if ((err = i2c_attach_client(data->lm75[i]))) { dev_err(&new_client->dev, "Subclient %d " "registration at address 0x%x " - "failed.\n", i, data->lm75[i].addr); + "failed.\n", i, data->lm75[i]->addr); if (i == 1) - goto ERROR_SC_2; - goto ERROR_SC_1; + goto ERROR_SC_3; + goto ERROR_SC_2; } if (kind == w83783s) break; @@ -1124,10 +1132,14 @@ return 0; /* Undo inits in case of errors */ +ERROR_SC_3: + i2c_detach_client(data->lm75[0]); ERROR_SC_2: - i2c_detach_client(&(data->lm75[0])); + if (NULL != data->lm75[1]) + kfree(data->lm75[1]); ERROR_SC_1: - kfree(data->lm75); + if (NULL != data->lm75[0]) + kfree(data->lm75[0]); ERROR_SC_0: return err; } @@ -1326,7 +1338,8 @@ kind, new_client))) goto ERROR3; } else { - data->lm75 = NULL; + data->lm75[0] = NULL; + data->lm75[1] = NULL; } device_create_file_in(new_client, 0); @@ -1409,20 +1422,11 @@ static int w83781d_detach_client(struct i2c_client *client) { - struct w83781d_data *data = i2c_get_clientdata(client); int err; - /* release ISA region or I2C subclients first */ - if (i2c_is_isa_client(client)) { + if (i2c_is_isa_client(client)) release_region(client->addr, W83781D_EXTENT); - } else { - i2c_detach_client(&data->lm75[0]); - if (data->type != w83783s) - i2c_detach_client(&data->lm75[1]); - kfree(data->lm75); - } - /* now it's safe to scrap the rest */ if ((err = i2c_detach_client(client))) { dev_err(&client->dev, "Client deregistration failed, client not detached.\n"); @@ -1484,7 +1488,7 @@ res = i2c_smbus_read_byte_data(client, reg & 0xff); } else { /* switch to subclient */ - cl = &data->lm75[bank - 1]; + cl = data->lm75[bank - 1]; /* convert from ISA to LM75 I2C addresses */ switch (reg & 0xff) { case 0x50: /* TEMP */ @@ -1555,7 +1559,7 @@ value & 0xff); } else { /* switch to subclient */ - cl = &data->lm75[bank - 1]; + cl = data->lm75[bank - 1]; /* convert from ISA to LM75 I2C addresses */ switch (reg & 0xff) { case 0x52: /* CONFIG */ -- Mark M. Hoffman mhoffman@lightlink.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234])) 2003-06-13 2:36 ` Mark M. Hoffman @ 2003-06-13 6:08 ` Martin Schlemmer 2003-06-14 6:26 ` Martin Schlemmer 1 sibling, 0 replies; 21+ messages in thread From: Martin Schlemmer @ 2003-06-13 6:08 UTC (permalink / raw) To: Mark M. Hoffman; +Cc: LKML, Sensors, Greg KH On Fri, 2003-06-13 at 04:36, Mark M. Hoffman wrote: > > Nope, this do not fix it. > > > > The problem is actually at the i2c driver side. From > > drivers/i2c/i2c-core.c in i2c_del_driver(), we have: > <cut> > > To recap: list_for_each_safe() is not safe for deleting other than > the current "item". But I disagree with you Martin because I think > the usage in i2c_del_driver is ok as is; and that w83781d.c should > change... > Right, I will not argue, as I was not sure on usage (and thus the mail). Might be right thing (tm) to send a patch to make it more clear that its only 'safe' for current item deletion (against list.h) ? > My first patch was naive; the patch below solves the problem by > letting w83781d_detach_client remove the three clients (1 * primary > + 2 * subclients) independently. It's a noisy patch because I had > to change the way the subclients were kmalloc'ed - sorry. The meat > is around line 1422. This patch works for me... comments? > I did try this way, but did it rather brutish =) It thus still did not fix it. Your patch looks good ... I will try it tonight at home. Regards, -- Martin Schlemmer ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234])) 2003-06-13 2:36 ` Mark M. Hoffman 2003-06-13 6:08 ` Martin Schlemmer @ 2003-06-14 6:26 ` Martin Schlemmer 2003-06-16 18:41 ` Greg KH 1 sibling, 1 reply; 21+ messages in thread From: Martin Schlemmer @ 2003-06-14 6:26 UTC (permalink / raw) To: Greg KH; +Cc: LKML, Sensors, Mark M. Hoffman [-- Attachment #1.1: Type: text/plain, Size: 980 bytes --] On Fri, 2003-06-13 at 04:36, Mark M. Hoffman wrote: > > > > Anyhow, Only change I have made to the w83781d driver, is one line > > > > (just tell it to that if the chip id is 0x72, its also of type > > > > w83726HF), but now (2.5.70-bk1[123]) it segfaults for me on rmmod, where > > > > it did not with 2.5.68 kernels when I still had the other board. I will > > > > attach a oops tomorrow or such when I get home. > > > > My first patch was naive; the patch below solves the problem by > letting w83781d_detach_client remove the three clients (1 * primary > + 2 * subclients) independently. It's a noisy patch because I had > to change the way the subclients were kmalloc'ed - sorry. The meat > is around line 1422. This patch works for me... comments? > > Regards, > > --- > Mark M. Hoffman > mhoffman@lightlink.com Greg, this patch from Mark fixes it, please include in your stuff to send to Linus. Thanks, -- Martin Schlemmer [-- Attachment #1.2: w83781d-fix-rmmod-segfault.patch --] [-- Type: text/plain, Size: 5210 bytes --] --- linux-2.5.70-bk14/drivers/i2c/chips/w83781d.c 2003-06-10 00:49:19.000000000 -0400 +++ linux-2.5.70/drivers/i2c/chips/w83781d.c 2003-06-12 22:24:47.000000000 -0400 @@ -299,8 +299,8 @@ char valid; /* !=0 if following fields are valid */ unsigned long last_updated; /* In jiffies */ - struct i2c_client *lm75; /* for secondary I2C addresses */ - /* pointer to array of 2 subclients */ + struct i2c_client *lm75[2]; /* for secondary I2C addresses */ + /* array of 2 pointers to subclients */ u8 in[9]; /* Register value - 8 & 9 for 782D only */ u8 in_max[9]; /* Register value - 8 & 9 for 782D only */ @@ -1043,12 +1043,12 @@ const char *client_name; struct w83781d_data *data = i2c_get_clientdata(new_client); - if (!(data->lm75 = kmalloc(2 * sizeof (struct i2c_client), - GFP_KERNEL))) { + data->lm75[0] = kmalloc(sizeof(struct i2c_client), GFP_KERNEL); + if (!(data->lm75[0])) { err = -ENOMEM; goto ERROR_SC_0; } - memset(data->lm75, 0x00, 2 * sizeof (struct i2c_client)); + memset(data->lm75[0], 0x00, sizeof (struct i2c_client)); id = i2c_adapter_id(adapter); @@ -1066,25 +1066,33 @@ w83781d_write_value(new_client, W83781D_REG_I2C_SUBADDR, (force_subclients[2] & 0x07) | ((force_subclients[3] & 0x07) << 4)); - data->lm75[0].addr = force_subclients[2]; + data->lm75[0]->addr = force_subclients[2]; } else { val1 = w83781d_read_value(new_client, W83781D_REG_I2C_SUBADDR); - data->lm75[0].addr = 0x48 + (val1 & 0x07); + data->lm75[0]->addr = 0x48 + (val1 & 0x07); } if (kind != w83783s) { + + data->lm75[1] = kmalloc(sizeof(struct i2c_client), GFP_KERNEL); + if (!(data->lm75[1])) { + err = -ENOMEM; + goto ERROR_SC_1; + } + memset(data->lm75[1], 0x0, sizeof(struct i2c_client)); + if (force_subclients[0] == id && force_subclients[1] == address) { - data->lm75[1].addr = force_subclients[3]; + data->lm75[1]->addr = force_subclients[3]; } else { - data->lm75[1].addr = 0x48 + ((val1 >> 4) & 0x07); + data->lm75[1]->addr = 0x48 + ((val1 >> 4) & 0x07); } - if (data->lm75[0].addr == data->lm75[1].addr) { + if (data->lm75[0]->addr == data->lm75[1]->addr) { dev_err(&new_client->dev, "Duplicate addresses 0x%x for subclients.\n", - data->lm75[0].addr); + data->lm75[0]->addr); err = -EBUSY; - goto ERROR_SC_1; + goto ERROR_SC_2; } } @@ -1103,19 +1111,19 @@ for (i = 0; i <= 1; i++) { /* store all data in w83781d */ - i2c_set_clientdata(&data->lm75[i], NULL); - data->lm75[i].adapter = adapter; - data->lm75[i].driver = &w83781d_driver; - data->lm75[i].flags = 0; - strlcpy(data->lm75[i].dev.name, client_name, + i2c_set_clientdata(data->lm75[i], NULL); + data->lm75[i]->adapter = adapter; + data->lm75[i]->driver = &w83781d_driver; + data->lm75[i]->flags = 0; + strlcpy(data->lm75[i]->dev.name, client_name, DEVICE_NAME_SIZE); - if ((err = i2c_attach_client(&(data->lm75[i])))) { + if ((err = i2c_attach_client(data->lm75[i]))) { dev_err(&new_client->dev, "Subclient %d " "registration at address 0x%x " - "failed.\n", i, data->lm75[i].addr); + "failed.\n", i, data->lm75[i]->addr); if (i == 1) - goto ERROR_SC_2; - goto ERROR_SC_1; + goto ERROR_SC_3; + goto ERROR_SC_2; } if (kind == w83783s) break; @@ -1124,10 +1132,14 @@ return 0; /* Undo inits in case of errors */ +ERROR_SC_3: + i2c_detach_client(data->lm75[0]); ERROR_SC_2: - i2c_detach_client(&(data->lm75[0])); + if (NULL != data->lm75[1]) + kfree(data->lm75[1]); ERROR_SC_1: - kfree(data->lm75); + if (NULL != data->lm75[0]) + kfree(data->lm75[0]); ERROR_SC_0: return err; } @@ -1326,7 +1338,8 @@ kind, new_client))) goto ERROR3; } else { - data->lm75 = NULL; + data->lm75[0] = NULL; + data->lm75[1] = NULL; } device_create_file_in(new_client, 0); @@ -1409,20 +1422,11 @@ static int w83781d_detach_client(struct i2c_client *client) { - struct w83781d_data *data = i2c_get_clientdata(client); int err; - /* release ISA region or I2C subclients first */ - if (i2c_is_isa_client(client)) { + if (i2c_is_isa_client(client)) release_region(client->addr, W83781D_EXTENT); - } else { - i2c_detach_client(&data->lm75[0]); - if (data->type != w83783s) - i2c_detach_client(&data->lm75[1]); - kfree(data->lm75); - } - /* now it's safe to scrap the rest */ if ((err = i2c_detach_client(client))) { dev_err(&client->dev, "Client deregistration failed, client not detached.\n"); @@ -1484,7 +1488,7 @@ res = i2c_smbus_read_byte_data(client, reg & 0xff); } else { /* switch to subclient */ - cl = &data->lm75[bank - 1]; + cl = data->lm75[bank - 1]; /* convert from ISA to LM75 I2C addresses */ switch (reg & 0xff) { case 0x50: /* TEMP */ @@ -1555,7 +1559,7 @@ value & 0xff); } else { /* switch to subclient */ - cl = &data->lm75[bank - 1]; + cl = data->lm75[bank - 1]; /* convert from ISA to LM75 I2C addresses */ switch (reg & 0xff) { case 0x52: /* CONFIG */ [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234])) 2003-06-14 6:26 ` Martin Schlemmer @ 2003-06-16 18:41 ` Greg KH 2003-09-03 20:54 ` [PATCH 2.6] Fix conversion from milli volts in store_in_reg() for w83781d.c Martin Schlemmer 0 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2003-06-16 18:41 UTC (permalink / raw) To: Martin Schlemmer; +Cc: LKML, Sensors, Mark M. Hoffman On Sat, Jun 14, 2003 at 08:26:35AM +0200, Martin Schlemmer wrote: > On Fri, 2003-06-13 at 04:36, Mark M. Hoffman wrote: > > > > > Anyhow, Only change I have made to the w83781d driver, is one line > > > > > (just tell it to that if the chip id is 0x72, its also of type > > > > > w83726HF), but now (2.5.70-bk1[123]) it segfaults for me on rmmod, where > > > > > it did not with 2.5.68 kernels when I still had the other board. I will > > > > > attach a oops tomorrow or such when I get home. > > > > > > > My first patch was naive; the patch below solves the problem by > > letting w83781d_detach_client remove the three clients (1 * primary > > + 2 * subclients) independently. It's a noisy patch because I had > > to change the way the subclients were kmalloc'ed - sorry. The meat > > is around line 1422. This patch works for me... comments? > > > > Regards, > > > > --- > > Mark M. Hoffman > > mhoffman@lightlink.com > > Greg, this patch from Mark fixes it, please include in your stuff > to send to Linus. Applied, thanks. greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2.6] Fix conversion from milli volts in store_in_reg() for w83781d.c 2003-06-16 18:41 ` Greg KH @ 2003-09-03 20:54 ` Martin Schlemmer 2003-09-04 18:40 ` Greg KH 2003-09-07 15:41 ` Andrey Borzenkov 0 siblings, 2 replies; 21+ messages in thread From: Martin Schlemmer @ 2003-09-03 20:54 UTC (permalink / raw) To: Greg KH; +Cc: Andrey Borzenkov, LKML, Sensors [-- Attachment #1.1: Type: text/plain, Size: 1048 bytes --] Hi I am not sure if it was a later patch from me that fixed in_* to display milli volts in sysfs, or if it was a patch from Jan Dittmer, but the conversion in the store_in_*() functions is wrong, and cause something like: ---------------------------- nosferatu patch # cat /sys/bus/i2c/devices/1-0290/in_max2 3632 nosferatu patch # echo '3700' > /sys/bus/i2c/devices/1-0290/in_max2 nosferatu patch # cat /sys/bus/i2c/devices/1-0290/in_max2 400 nosferatu patch # echo '3640' > /sys/bus/i2c/devices/1-0290/in_max2 nosferatu patch # cat /sys/bus/i2c/devices/1-0290/in_max2 400 nosferatu patch # echo '3632' > /sys/bus/i2c/devices/1-0290/in_max2 nosferatu patch # cat /sys/bus/i2c/devices/1-0290/in_max2 400 nosferatu patch # ----------------------------- I think Andrey also noticed this (if I did not smoke too much Weedbix this morning ;) - if so, please verify that this fixes it ... it does here at least. Anyhow, patch is attached and should apply to 2.6.0-test4-bk5. Regards, -- Martin Schlemmer [-- Attachment #1.2: w83781d-fixup-in_store.patch --] [-- Type: text/plain, Size: 570 bytes --] --- 1/drivers/i2c/chips/w83781d.c 2003-09-03 22:42:02.199165296 +0200 +++ 2/drivers/i2c/chips/w83781d.c 2003-09-03 22:35:02.378987664 +0200 @@ -378,8 +378,8 @@ static ssize_t store_in_##reg (struct de struct w83781d_data *data = i2c_get_clientdata(client); \ u32 val; \ \ - val = simple_strtoul(buf, NULL, 10); \ - data->in_##reg[nr] = (IN_TO_REG(val) / 10); \ + val = simple_strtoul(buf, NULL, 10) / 10; \ + data->in_##reg[nr] = IN_TO_REG(val); \ w83781d_write_value(client, W83781D_REG_IN_##REG(nr), data->in_##reg[nr]); \ \ return count; \ [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2.6] Fix conversion from milli volts in store_in_reg() for w83781d.c 2003-09-03 20:54 ` [PATCH 2.6] Fix conversion from milli volts in store_in_reg() for w83781d.c Martin Schlemmer @ 2003-09-04 18:40 ` Greg KH 2003-09-07 15:41 ` Andrey Borzenkov 1 sibling, 0 replies; 21+ messages in thread From: Greg KH @ 2003-09-04 18:40 UTC (permalink / raw) To: Martin Schlemmer; +Cc: Andrey Borzenkov, LKML, Sensors On Wed, Sep 03, 2003 at 10:54:12PM +0200, Martin Schlemmer wrote: > Hi > > I am not sure if it was a later patch from me that fixed in_* to display > milli volts in sysfs, or if it was a patch from Jan Dittmer, but the > conversion in the store_in_*() functions is wrong, and cause something > like: > > ---------------------------- > nosferatu patch # cat /sys/bus/i2c/devices/1-0290/in_max2 > 3632 > nosferatu patch # echo '3700' > /sys/bus/i2c/devices/1-0290/in_max2 > nosferatu patch # cat /sys/bus/i2c/devices/1-0290/in_max2 > 400 > nosferatu patch # echo '3640' > /sys/bus/i2c/devices/1-0290/in_max2 > nosferatu patch # cat /sys/bus/i2c/devices/1-0290/in_max2 > 400 > nosferatu patch # echo '3632' > /sys/bus/i2c/devices/1-0290/in_max2 > nosferatu patch # cat /sys/bus/i2c/devices/1-0290/in_max2 > 400 > nosferatu patch # > ----------------------------- > > I think Andrey also noticed this (if I did not smoke too much Weedbix > this morning ;) - if so, please verify that this fixes it ... it does > here at least. > > Anyhow, patch is attached and should apply to 2.6.0-test4-bk5. Applied, thanks. greg k-h ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2.6] Fix conversion from milli volts in store_in_reg() for w83781d.c 2003-09-03 20:54 ` [PATCH 2.6] Fix conversion from milli volts in store_in_reg() for w83781d.c Martin Schlemmer 2003-09-04 18:40 ` Greg KH @ 2003-09-07 15:41 ` Andrey Borzenkov 1 sibling, 0 replies; 21+ messages in thread From: Andrey Borzenkov @ 2003-09-07 15:41 UTC (permalink / raw) To: azarah, Greg KH; +Cc: LKML, Sensors On Thursday 04 September 2003 00:54, Martin Schlemmer wrote: [...] > nosferatu patch # echo '3700' > /sys/bus/i2c/devices/1-0290/in_max2 > nosferatu patch # cat /sys/bus/i2c/devices/1-0290/in_max2 > 400 [...] > I think Andrey also noticed this (if I did not smoke too much Weedbix > this morning ;) - if so, please verify that this fixes it ... it does > here at least. > yes, it has fixed it. thank you -andrey ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2003-09-07 15:44 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2003-05-24 18:37 [OOPS] w83781d during rmmod (2.5.69-bk17) Mark M. Hoffman
[not found] ` <3ED8067E.1050503@paradyne.com>
2003-06-01 14:38 ` [RFC PATCH] " Mark M. Hoffman
2003-06-02 17:20 ` Greg KH
2003-06-03 5:22 ` Martin Schlemmer
2003-06-03 19:43 ` Philip Pokorny
2003-06-04 5:57 ` Martin Schlemmer
2003-06-05 2:39 ` Mark M. Hoffman
2003-06-05 19:47 ` Greg KH
2003-06-09 5:34 ` Martin Schlemmer
2003-06-10 5:38 ` Martin Schlemmer
2003-06-10 5:41 ` OOPS w83781d during rmmod (2.5.70-bk1[1234]) Mark M. Hoffman
2003-06-10 5:51 ` Martin Schlemmer
2003-06-11 5:44 ` Martin Schlemmer
2003-06-12 6:57 ` [RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234])) Martin Schlemmer
2003-06-13 2:36 ` Mark M. Hoffman
2003-06-13 6:08 ` Martin Schlemmer
2003-06-14 6:26 ` Martin Schlemmer
2003-06-16 18:41 ` Greg KH
2003-09-03 20:54 ` [PATCH 2.6] Fix conversion from milli volts in store_in_reg() for w83781d.c Martin Schlemmer
2003-09-04 18:40 ` Greg KH
2003-09-07 15:41 ` Andrey Borzenkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).