[OOPS] w83781d during rmmod (2.5.69-bk17)

[OOPS] w83781d during rmmod (2.5.69-bk17)

[Mark M. Hoffman]

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

[OOPS] w83781d during rmmod (2.5.69-bk17)

[Mark M. Hoffman]

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

[OOPS] w83781d during rmmod (2.5.69-bk17)

[Mark D. Studebaker]

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.

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?
> 
> Regards,
> 

[RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Mark M. Hoffman]

[-- 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;

[RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Mark M. Hoffman]

* 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

-------------- next part --------------
--- 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;

Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Greg KH]

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

[RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Greg KH]

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

Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Martin Schlemmer]

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

[RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Martin Schlemmer]

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

Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Philip Pokorny]

[-- 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) {

[RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Philip Pokorny]

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
-------------- next part --------------
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) {

Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Martin Schlemmer]

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

[RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Martin Schlemmer]

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

Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Mark M. Hoffman]

[-- 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;
 	}
 

[RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Mark M. Hoffman]

* 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

-------------- next part --------------
--- 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;
 	}
 

Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Greg KH]

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

[RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Greg KH]

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

Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Martin Schlemmer]

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

[RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Martin Schlemmer]

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

Re: [RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Martin Schlemmer]

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

[RFC PATCH] Re: [OOPS] w83781d during rmmod (2.5.69-bk17)

[Martin Schlemmer]

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

Re: OOPS w83781d during rmmod (2.5.70-bk1[1234])

[Mark M. Hoffman]

* 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

OOPS w83781d during rmmod (2.5.70-bk1[1234])

[Mark M. Hoffman]

* 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

Re: OOPS w83781d during rmmod (2.5.70-bk1[1234])

[Martin Schlemmer]

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

OOPS w83781d during rmmod (2.5.70-bk1[1234])

[Martin Schlemmer]

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

Re: OOPS w83781d during rmmod (2.5.70-bk1[1234])

[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 <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

OOPS w83781d during rmmod (2.5.70-bk1[1234])

[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 <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

[RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234]))

[Martin Schlemmer]

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

[RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d

[Martin Schlemmer]

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

Re: [RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234]))

[Mark M. Hoffman]

* 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

[RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d

[Mark M. Hoffman]

* 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

Re: [RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234]))

[Martin Schlemmer]

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

[RFC][2.5] list_for_each_safe not so safe (was Re: OOPS

[Martin Schlemmer]

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

Re: [RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234]))

[Martin Schlemmer]

[-- 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 --]

[RFC][2.5] list_for_each_safe not so safe (was Re: OOPS

[Martin Schlemmer]

Skipped content of type multipart/mixed-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20030614/b7b17afd/attachment.bin

Re: [RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d during rmmod (2.5.70-bk1[1234]))

[Greg KH]

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

[RFC][2.5] list_for_each_safe not so safe (was Re: OOPS w83781d

[Greg KH]

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

[PATCH 2.6] Fix conversion from milli volts in store_in_reg() for w83781d.c

[Martin Schlemmer]

[-- 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 --]

[PATCH 2.6] Fix conversion from milli volts in store_in_reg() for

[Martin Schlemmer]

Skipped content of type multipart/mixed-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20030903/8b8fcaa6/attachment.bin

Re: [PATCH 2.6] Fix conversion from milli volts in store_in_reg() for w83781d.c

[Greg KH]

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

[PATCH 2.6] Fix conversion from milli volts in store_in_reg() for

[Greg KH]

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

Re: [PATCH 2.6] Fix conversion from milli volts in store_in_reg() for w83781d.c

[Andrey Borzenkov]

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

[PATCH 2.6] Fix conversion from milli volts in store_in_reg() for

[Andrey Borzenkov]

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