Restore compatibility with 2.4 kernels

Hi all,

For the 2.9.0 release of i2c and lm_sensors, a big work has been done to address compatibility problems between 2.4 kernels and i2c/lm-sensors. The compatibility was announced as restored, but unfortunately, there is still a small difference in one structure defined in i2c.h, namely i2c_adapter.

I have tried to do a patch to fix the latest issues, but I am not sure it is correctly done.

In the kernel version of i2c.h, the client_count variable is present, whereas it is not in the i2c one. I have simply added it, as well as the code to increment/decrement it.

Then the complex part is about the semaphores:

In the kernel a semaphore called lock is used to only allow one i2c transfer at once. In i2c, a semaphore is also used for the same job, but called bus. The fix was easy, just renaming the variable.

But there is one more semaphore in i2c, called list, used to lock the list of i2c clients. This one is not present in the kernel, as this list was not locked in i2c 2.7.0 and previous versions. I supposed it has been added due to some race conditions that have been observed, so I choosed to use the same semaphore as for i2c transfer to lock the clients list. It has the drawback to not allow an i2c transfer while adding/removing a client or to not allow adding a new client during an i2c transfer. I don't consider that as a problem, as adding/removing a client does not happen so often. I have also checked that not i2c transfers are asked between too lock of the list.

I haven't tested my patch (only tested it build correctly), because I am not at home, so I can't change easily my 2.6 kernel to a 2.4 one. Nevertheless, I would like for people with good knowledge of i2c-core.c to tell me if the patch seems ok or not. Thanks.

Bye,
Aurelien

  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian GNU/Linux developer | Electrical Engineer
 `. `'   [email protected]         | [email protected]
   `-    people.debian.org/~aurel32 | www.aurel32.net
-------------- next part --------------
Index: kernel/i2c-core.c
=================================RCS file: /home/cvs/i2c/kernel/i2c-core.c,v
retrieving revision 1.106
diff -u -d -p -r1.106 i2c-core.c
--- kernel/i2c-core.c	22 Jan 2005 09:01:56 -0000	1.106
+++ kernel/i2c-core.c	8 Mar 2005 14:20:04 -0000
@@ -104,8 +104,7 @@ int i2c_add_adapter(struct i2c_adapter *
 	adapters[i] = adap;
 	
 	/* init data types */
-	init_MUTEX(&adap->bus);
-	init_MUTEX(&adap->list);
+	init_MUTEX(&adap->lock);
 
 	/* inform drivers of new adapters */
 	for (j=0;j<I2C_DRIVER_MAX;j++)
@@ -311,9 +310,9 @@ int i2c_check_addr (struct i2c_adapter *
 {
 	int rval;
 
-	down(&adapter->list);
+	down(&adapter->lock);
 	rval = __i2c_check_addr(adapter, addr);
-	up(&adapter->list);
+	up(&adapter->lock);
 
 	return rval;
 }
@@ -326,7 +325,7 @@ int i2c_attach_client(struct i2c_client 
 	if (i2c_check_addr(client->adapter,client->addr))
 		return -EBUSY;
 
-	down(&adapter->list);
+	down(&adapter->lock);
 	for (i = 0; i < I2C_CLIENT_MAX; i++)
 		if (NULL = adapter->clients[i])
 			break;
@@ -334,11 +333,12 @@ int i2c_attach_client(struct i2c_client 
 		printk(KERN_WARNING 
 		       " i2c-core.o: attach_client(%s) - enlarge I2C_CLIENT_MAX.\n",
 			client->name);
-		up(&adapter->list);
+		up(&adapter->lock);
 		return -ENOMEM;
 	}
 	adapter->clients[i] = client;
-	up(&adapter->list);
+	adapter->client_count++;
+	up(&adapter->lock);
 	
 	if (adapter->client_register) 
 		if (adapter->client_register(client)) 
@@ -371,7 +371,7 @@ int i2c_detach_client(struct i2c_client 
 			return res;
 		}
 
-	down(&adapter->list);
+	down(&adapter->lock);
 	for (i = 0; i < I2C_CLIENT_MAX; i++)
 		if (client = adapter->clients[i])
 			break;
@@ -379,11 +379,12 @@ int i2c_detach_client(struct i2c_client 
 		printk(KERN_WARNING " i2c-core.o: unregister_client "
 				    "[%s] not found\n",
 			client->name);
-		up(&adapter->list);
+		up(&adapter->lock);
 		return -ENODEV;
 	}
 	adapter->clients[i] = NULL;
-	up(&adapter->list);
+	adapter->client_count--;
+	up(&adapter->lock);
 
 	DEB(printk(KERN_DEBUG "i2c-core.o: client [%s] unregistered.\n",client->name));
 	return 0;
@@ -580,7 +581,7 @@ ssize_t i2cproc_bus_read(struct file * f
 	/* Order will hold the indexes of the clients
 	   sorted by address */
 	order_nr=0;
-	down(&adap->list);
+	down(&adap->lock);
 	for (j = 0; j < I2C_CLIENT_MAX; j++) {
 	    if ((client = adap->clients[j]) && 
 		(client->driver->id != I2C_DRIVERID_I2CDEV))  {
@@ -603,7 +604,7 @@ ssize_t i2cproc_bus_read(struct file * f
 			   client->name,
 			   client->driver->name);
 	}
-	up(&adap->list);
+	up(&adap->lock);
 	up(&core_lists);
 	
 	len = len - file->f_pos;
@@ -681,9 +682,9 @@ int i2c_transfer(struct i2c_adapter * ad
  	 	DEB2(printk(KERN_DEBUG "i2c-core.o: master_xfer: %s with %d msgs.\n",
 		            adap->name,num));
 
-		down(&adap->bus);
+		down(&adap->lock);
 		ret = adap->algo->master_xfer(adap,msgs,num);
-		up(&adap->bus);
+		up(&adap->lock);
 
 		return ret;
 	} else {
@@ -708,9 +709,9 @@ int i2c_master_send(struct i2c_client *c
 		DEB2(printk(KERN_DEBUG "i2c-core.o: master_send: writing %d bytes on %s.\n",
 			count,client->adapter->name));
 	
-		down(&adap->bus);
+		down(&adap->lock);
 		ret = adap->algo->master_xfer(adap,&msg,1);
-		up(&adap->bus);
+		up(&adap->lock);
 
 		/* if everything went ok (i.e. 1 msg transmitted), return #bytes
 		 * transmitted, else error code.
@@ -738,9 +739,9 @@ int i2c_master_recv(struct i2c_client *c
 		DEB2(printk(KERN_DEBUG "i2c-core.o: master_recv: reading %d bytes on %s.\n",
 			count,client->adapter->name));
 	
-		down(&adap->bus);
+		down(&adap->lock);
 		ret = adap->algo->master_xfer(adap,&msg,1);
-		up(&adap->bus);
+		up(&adap->lock);
 	
 		DEB2(printk(KERN_DEBUG "i2c-core.o: master_recv: return:%d (count:%d, addr:0x%02x)\n",
 			ret, count, client->addr));
@@ -1403,10 +1404,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter * 
 	}
 
 	if (adap->algo->smbus_xfer) {
-		down(&adap->bus);
+		down(&adap->lock);
 		res = adap->algo->smbus_xfer(adap,addr,flags,read_write,
 		                                command,size,data);
-		up(&adap->bus);
+		up(&adap->lock);
 	} else
 		res = i2c_smbus_xfer_emulated(adap,addr,flags,read_write,
 	                                      command,size,data);
Index: kernel/i2c.h
=================================RCS file: /home/cvs/i2c/kernel/i2c.h,v
retrieving revision 1.87
diff -u -d -p -r1.87 i2c.h
--- kernel/i2c.h	22 Jan 2005 09:04:13 -0000	1.87
+++ kernel/i2c.h	8 Mar 2005 14:20:05 -0000
@@ -237,11 +237,11 @@ struct i2c_adapter {
 			/* and can be set via the i2c_ioctl call	*/
 
 			/* data fields that are valid for all devices	*/
-	struct semaphore bus;
-	struct semaphore list;  
+	struct semaphore lock;
 	unsigned int flags;/* flags specifying div. data		*/
 
 	struct i2c_client *clients[I2C_CLIENT_MAX];
+	int client_count;
 
 	int timeout;
 	int retries;

Hi James,

I thought 2.9.0 had fixed the problem?

I'm a bit surprised that Aurelien's patch would help with sensors. Supposedly the problem is the lack of binary compatibility between i2c 2.9.0 and in-kernel i2c. If you use lm_sensors 2.9.0 then you must use i2c 2.9.0 as well, so what Aurelien's patch would address are problems with in-kernel, non-sensors i2c stuff such as media/video drivers.

Jean Delvare

Hi James,

Interesting, I had missed it myself. The original problem could indeed have a positive side effect. Some more notes however:

1* If the two actions actually lock each other out, there is something wrong at an upper level. You should not possibly unload a driver that has running code. That the code is an i2c bus transfer or something else doesn't make any difference.

2* i2c 2.9.0 solved a number of problems but did not fix all race conditions. In particular, bus drivers maintain their usage count themselves, which should not be done. Unfortunately this will never be fixed, because it would break compatibility with the 2.4 kernel.

3* There is also a known problem that sysctl/procfs access will increase the usage count of chip drivers but not bus drivers. Again this cannot be fixed without a significant architecture change, which we would prefer not to do. The "solution" is to always unload chip drivers before bus drivers.

On your recent oopses, were you only playing with chip drivers or also with bus driver? Frankly I am not surprised if you have oopses when cycling bus drivers with concurent procfs/sysctl accesses. I *am* surprised if you have oopses while only cycling chip drivers.

Jean Delvare

Hi Aurelien,

I have applied your patch to the 2.9.0 sources, built it, installed it and ran some tests.
In particular I was still seeing panics when unloading sensors
drivers. It seems to fix this.
I had sent out a script that loaded and unloaded modules as stress test, and I ran this, with another script reading a particular
modules proc entry. I do not seem to get any panics (though I need
to run this a wee bit longer to be sure). It actually has an interesting behavior. Sometimes it seems to seem to slow down the loading and unloading of modules (which is fine), and at other times it will cause the unload to fail with modprobe printing that device or
resource is busy. This to IMHO seems to be an OK behavior.

I am hoping to see a very long run, so I can call this panic gone. I will let you know.

Cheers...james

Nope, not always. It made it far more infrequent, but over the weeks
that passed we discovered it did not completely make the problem go away. I have output from a panic you would like to see it.

Well, I missed it too, but Dave Knierim (my partner in crime), noticed this paragraph
in Aurelian's email:

But there is one more semaphore in i2c, called list, used to lock the list of i2c clients. This one is not present in the kernel, as this list was not locked in i2c 2.7.0 and previous versions. I supposed it has been added due to some race conditions that have been observed, so I choosed to use the same semaphore as for i2c transfer to lock the clients
--->list. It has the drawback to not allow an i2c transfer while --->adding/removing a client or to not allow adding a new client during an --->i2c transfer. I don't consider that as a problem, as adding/removing a
client does not happen so often. I have also checked that not i2c transfers are asked between too lock of the list.

That is what Dave noticed, which sounded like it would completely remove the opportunity for someone to read the proc entries while the driver was going away, and it seems to work.
But then I might have missed something .

And I am pretty sure we don't use the in-kernel i2c stuff as I specifically turn off the building of all of those driverers and all things i2c and lm_sensors in our kernel that we build. Mainly because we build i2c and lm_sensors as seperate rpms so we can easily build it right from your CVS, but still have all the modules go to all the same places.

Like I said, though, it seems to have fixed the panics, but I won't _really_ know for days, as I have yet to figure out a sequence that would guarantee it once we switched to 2.9.0.

Here is the output from a bonified 2.9.0 panic while unloading drivers as someone is trying to read its proc entries:

localhost.localdomain login: Unable to handle kernel paging request
at virtual a
ddress f8a88293
*pde = 4057989c

   Oops: 0000
   eeprom smsc47m1 i2c-i801 i2c-isa i2c-proc i2c-dev i2c-core segled

power pciclock
s leds autofs4 mpc iptable_filter ip_tables e1000 i810-tco microcode ide-scsi
CPU: 1
EIP: 0060:[<f8a88293>] Tainted: P
EFLAGS: 00010296

EIP is at __insmod_i2c-i801_S.bss_L16 [i2c-i801] 0x3c4b
(2.4.21-27.0.2.ELprerel2
.0.0_50.2.0smp/i686)

   eax: 000000b6   ebx: 00000000   ecx: f8a84440   edx: 000000b6
   esi: 00000001   edi: 0000002c   ebp: f687ab80   esp: f5f9fd94
   ds: 0068   es: 0068   ss: 0068
   Process sensors (pid: 10359, stackpageõf9f000)
   Stack: f687ab80 0000002c c0158dcd c4f25c80 00000202 00000000
000001d2 00000000
          00000000 f687ab80 f687ab80 f8a883e6 f687ab80 0000002c
00000282 fff96000
          00000000 fff96280 f687abc4 f687ab80 f5f9fe60 00000000
c3526eec f8a88d19
   Call Trace:   [<c0158dcd>] __alloc_pages_limit [kernel] 0x7d (0xf5f9fd9c)
   [<c0143348>] do_no_page [kernel] 0x368 (0xf5f9fdf8)
   [<f8a7a790>] i2c_sysctl_real_Rsmp_e7981530 [i2c-proc] 0xe0 (0xf5f9fe10)
   [<f8a7a702>] i2c_sysctl_real_Rsmp_e7981530 [i2c-proc] 0x52 (0xf5f9fe30)
   [<c0143971>] handle_mm_fault [kernel] 0xd1 (0xf5f9fec0)
   [<c0130c05>] do_sysctl_strategy [kernel] 0x175 (0xf5f9ff00)
   [<c0130803>] do_sysctl [kernel] 0x93 (0xf5f9ff38)
   [<c01308bf>] sys_sysctl [kernel] 0x7f (0xf5f9ff70)

   Code: Bad EIP value.

   Kernel panic: Fatal exception
   lm87.o version 2.9.0 (20041228)
   lm87.o version 2.9.0 (20041228)
   lm87.o version 2.9.0 (20041228)

Not really sure my the module printed is name and version 3 times, but it really did.

Cheers...james

So is the correct behavior that it should always return that the device is busy if you try to unload while its still being used? Sounds reasonable, I was just wondering.

Warm fuzzies I'm not feeling. Course, most of the time your not loading and unloading drivers. Its mainly when we unload drivers on shutdown and we get a panic that causes us more concern. We can, though, not unload the drivers on shutdown...or we could try to make sure the watchdog does not get disabled before we unload the drivers.

We do do that. It just seems like the sane thing to do.

It was only chip drivers. The three drivers, and usually only one at
a time were: lm93, lm87 and eeprom. Now, when we first noticed that
the panics would still occur, it could have been with either. Basically, we would see the panics when we would either be rebooting the box, and the drivers would be unloaded (which we can stop doing), or when we are switching out versions of i2c/lm_sensors, and thus
unloading all the drivers. But in these cases which were not
expected, we did not capture what exactly had been loaded or unloaded.
For my testing today, though, the only things I have been loading and
unloading were those three chip drivers.

Thanks...james