* Mark D. Studebaker <[email protected]> [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
[email protected]
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
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
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) {
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
* Greg KH <[email protected]> [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
[email protected]
On Wed, Jun 04, 2003 at 10:39:22PM -0400, Mark M. Hoffman wrote:
> * Greg KH <[email protected]> [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
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 <[email protected]> [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
* Martin Schlemmer <[email protected]> [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
[email protected]
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 <[email protected]> [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
On Tue, 2003-06-10 at 07:41, Mark M. Hoffman wrote:
> * Martin Schlemmer <[email protected]> [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
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 <[email protected]> [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
On Tue, 2003-06-10 at 07:41, Mark M. Hoffman wrote:
> * Martin Schlemmer <[email protected]> [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
* Martin Schlemmer <[email protected]> [2003-06-12 08:57:02 +0200]:
> On Tue, 2003-06-10 at 07:41, Mark M. Hoffman wrote:
> > * Martin Schlemmer <[email protected]> [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
[email protected]
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
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
> [email protected]
Greg, this patch from Mark fixes it, please include in your stuff
to send to Linus.
Thanks,
--
Martin Schlemmer
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
> > [email protected]
>
> Greg, this patch from Mark fixes it, please include in your stuff
> to send to Linus.
Applied, thanks.
greg k-h
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
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
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