bug in init_i82365 wrt sysfs

Unable to handle kernel NULL pointer dereference at virtual address 00000010
 printing eip:
c0192fc9
*pde = 00000000
Oops: 0000 [#1]
CPU:    1
EIP:    0060:[<c0192fc9>]    Not tainted
EFLAGS: 00010282

EIP is at sysfs_add_file+0x9/0xa0
eax: 00000000 ebx: 0000023c ecx: 00000001 edx: c2817ff0
esi: 00000001 edi: 00000000 ebp: c0d89f70 esp: c0d89f64
ds: 007b es: 007b ss: 0068
Process modprobe (pid: 481, threadinfo=c0d88000 task=c0e759b0)

Stack: 0000023c 00000001 c2817fe8 c0d89f80 c01dbf4c 00000000 c2817154 c0d89fa0
       c2805188 c2817fe8 c2817154 00000000 c029e8b8 c0d88000 c28172e0 c0d89fbc
       c0139cff 0804e738 0804e738 0804e738 0804e0e0 00000000 c0d88000 c0109897
Call Trace:
 [<c01dbf4c>] class_device_create_file+0x1c/0x30
 [<c2805188>] init_i82365+0xe8/0x1e8 [i82365]
 [<c0139cff>] sys_init_module+0x15f/0x2f0
 [<c0109897>] syscall_call+0x7/0xb

Code: 8b 77 10 6a 77 68 ab 88 27 c0 e8 e8 a2 f8 ff 8d 5e 70 58 89

Em Fri, Oct 10, 2003 at 12:59:40AM -0300, Arnaldo C. Melo escreveu:

Sorry, hit the send key too fast, here is what I researched:

We should use the same approach net/core/net-sysfs.c is using, i.e:

static struct class net_class = {
.name = "net",
.release = netdev_release,
#ifdef CONFIG_HOTPLUG
.hotplug = netdev_hotplug,
#endif
};

.
.
.

        class_dev->class = &net_class;
        class_dev->class_data = net;

Adjusted to pcmcia, of course, its late here, going to bed, tomorrow I'll try to fix this one, if nobody beats me to it, I'll be happy to test patches done while I'm asleep 8)

- Arnaldo

This oops has been caused by the need to register the class before registering any objects against it. Unfortunately, the class needs to be registered asynchronously in a separate thread to avoid driver model deadlock with yenta with cardbus cards inserted or standard PCMCIA cards not being detected correctly due to a race.

I think the only real solution is to remove the class_device_create_file calls from all socket drivers. This is just a simple commenting out of the calls, and should be suitable for the remainder of the -test kernels.

Due to the number of cases that we're encountering with PCMCIA, I'm beginning to wonder if the driver model could be fixed to be more kind to PCMCIA by avoiding some of these ordering dependencies. None of this would be a problem if the driver model would allow PCI device drivers to register PCI devices while their probe or remove functions were executing.

Below is a completely untested patch. Arnaldo - please test whether this fixes your problem.

===== drivers/pcmcia/i82365.c 1.46 vs edited =====

@@ -1211,6 +1211,7 @@
     return 0;
 } /* i365_set_mem_map */
 
+#if 0 /* driver model ordering issue */
 /*======================================================================
 
     Routines for accessing socket information and register dumps via

@@ -1250,6 +1251,7 @@
 
 static CLASS_DEVICE_ATTR(exca, S_IRUGO, show_exca, NULL);
 static CLASS_DEVICE_ATTR(info, S_IRUGO, show_info, NULL);
+#endif
 
 /*====================================================================*/
 

@@ -1414,10 +1416,12 @@
 			    pcmcia_unregister_socket(&socket[i].socket);
 		    break;
 	    }
+#if 0 /* driver model ordering issue */
 	   class_device_create_file(&socket[i].socket.dev,
 			   	    &class_device_attr_info);
 	   class_device_create_file(&socket[i].socket.dev,
 			   	    &class_device_attr_exca);
+#endif
     }
 
     /* Finally, schedule a polling interrupt */

Russell King ([email protected])	http://www.arm.linux.org.uk/personal/
      Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
      maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                      2.6 Serial core

Em Fri, Oct 10, 2003 at 09:01:04AM +0100, Russell King escreveu:

Yes, it fixes, thanks.

- Arnaldo

I am open to that suggestion, but after doing some tinkering, I've found that it would be very painful.

The problem is that we hold the bus's rwsem across the driver binding sequence. This includes walking either the list of devices or drivers regsitered with the bus and calling ->probe().

We could make the locking more fine-grained, and only hold it when actually iterating over the list, and dropping it on each iteration. However, I'm strongly opposed to it because it gets ugly and fragile very quickly. I'd much rather protect the entire iteration, since it's easy to understand.

So, that means you simply cannot register devices that are on the same bus from inside a ->probe() function, or unregister them in ->remove(). At least not the way it is. Any suggestions?

Thanks,

Pat

No. It means that PCMCIA/Cardbus will remain to be fairly fragile with an over-complex initialisation to work around this problem.

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core