2004-04-03 17:11:41

by Jean Delvare

[permalink] [raw]
Subject: [RFC 2.6] Rework memory allocation in i2c chip drivers

Hi Greg, hi all,

Some times ago, Ralf Roesch reported that the memory allocation scheme
used in the i2c eeprom driver was causing trouble on MIPS architecture:

http://archives.andrew.net.au/lm-sensors/msg07233.html

The cause of the problems is that we do allocate two structures with a
single kmalloc, which breaks alignment. This doesn't seem to be a
problem on x86, but is on mips and probably on other architectures as
well. It happens that all other chip drivers work the same way too, so
they all would need to be fixed.

Here comes my proposal to fix the problem. A few notes:

1* The patch is against 2.6.5-rc3-mm4. Greg, I'd like to know the status
of two patches that are pending: swap_bytes and pcf8591. Since they are
not part of 2.6.5-rc3-mm4, my proposal ignores them, but I'll update it
if needed.

2* The problem being the same for all the drivers, the fix is the same
too.

3* However, it87 and via686a had bugs in the error paths (as far as I
could say) so I fixed them too.

4* Contrary to the patch I proposed for the lm_sensors CVS repository,
this one is meant to minimize the changes. I avoided renaming labels
most of the time (new labels have a "B" appended when necessary), so the
patch is both less error-prone and more readable.

Tested on w83781d and lm90 drivers, work for me.

Thanks.

diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/adm1021.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/adm1021.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/adm1021.c Sat Apr 3 12:02:31 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/adm1021.c Sat Apr 3 14:12:44 2004
@@ -228,16 +228,18 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access adm1021_{read,write}_value. */

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct adm1021_data),
- GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto error0;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct adm1021_data));
+ memset(new_client, 0x00, sizeof(struct i2c_client));
+
+ if (!(data = kmalloc(sizeof(struct adm1021_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto error0b;
+ }
+ memset(data, 0x00, sizeof(struct adm1021_data));

- data = (struct adm1021_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -329,6 +331,8 @@
return 0;

error1:
+ kfree(data);
+error0b:
kfree(new_client);
error0:
return err;
@@ -352,6 +356,7 @@
return err;
}

+ kfree(i2c_get_clientdata(client));
kfree(client);
return 0;
}
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/asb100.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/asb100.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/asb100.c Sat Apr 3 12:02:31 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/asb100.c Sat Apr 3 14:04:46 2004
@@ -722,17 +722,20 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access asb100_{read,write}_value. */

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct asb100_data), GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
pr_debug("asb100.o: detect failed, kmalloc failed!\n");
err = -ENOMEM;
goto ERROR0;
}
+ memset(new_client, 0, sizeof(struct i2c_client));

- memset(new_client, 0,
- sizeof(struct i2c_client) + sizeof(struct asb100_data));
+ if (!(data = kmalloc(sizeof(struct asb100_data), GFP_KERNEL))) {
+ pr_debug("asb100.o: detect failed, kmalloc failed!\n");
+ err = -ENOMEM;
+ goto ERROR0B;
+ }
+ memset(data, 0, sizeof(struct asb100_data));

- data = (struct asb100_data *) (new_client + 1);
init_MUTEX(&data->lock);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
@@ -842,6 +845,8 @@
ERROR2:
i2c_detach_client(new_client);
ERROR1:
+ kfree(data);
+ERROR0B:
kfree(new_client);
ERROR0:
return err;
@@ -857,6 +862,7 @@
return err;
}

+ kfree(i2c_get_clientdata(client));
kfree(client);

return 0;
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/ds1621.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/ds1621.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/ds1621.c Sat Apr 3 12:01:38 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/ds1621.c Sat Apr 3 14:06:40 2004
@@ -202,16 +202,18 @@
/* OK. For now, we presume we have a valid client. We now create the
client structure, even though we cannot fill it completely yet.
But it allows us to access ds1621_{read,write}_value. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct ds1621_data),
- GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0, sizeof(struct i2c_client) +
- sizeof(struct ds1621_data));
+ memset(new_client, 0, sizeof(struct i2c_client));
+
+ if (!(data = kmalloc(sizeof(struct ds1621_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto exit_free1;
+ }
+ memset(data, 0, sizeof(struct ds1621_data));

- data = (struct ds1621_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -264,6 +266,8 @@
/* OK, this is not exactly good programming practice, usually. But it is
very code-efficient in this case. */
exit_free:
+ kfree(data);
+ exit_free1:
kfree(new_client);
exit:
return err;
@@ -279,6 +283,7 @@
return err;
}

+ kfree(i2c_get_clientdata(client));
kfree(client);

return 0;
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/eeprom.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/eeprom.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/eeprom.c Sat Apr 3 12:02:31 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/eeprom.c Sat Apr 3 14:06:39 2004
@@ -187,16 +187,18 @@
/* OK. For now, we presume we have a valid client. We now create the
client structure, even though we cannot fill it completely yet.
But it allows us to access eeprom_{read,write}_value. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct eeprom_data),
- GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct eeprom_data));
+ memset(new_client, 0x00, sizeof(struct i2c_client));
+
+ if (!(data = kmalloc(sizeof(struct eeprom_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto exit_kfree1;
+ }
+ memset(data, 0x00, sizeof(struct eeprom_data));

- data = (struct eeprom_data *) (new_client + 1);
memset(data->data, 0xff, EEPROM_SIZE);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
@@ -244,6 +246,8 @@
return 0;

exit_kfree:
+ kfree(data);
+exit_kfree1:
kfree(new_client);
exit:
return err;
@@ -259,6 +263,7 @@
return err;
}

+ kfree(i2c_get_clientdata(client));
kfree(client);

return 0;
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/fscher.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/fscher.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/fscher.c Sat Apr 3 12:01:38 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/fscher.c Sat Apr 3 14:06:39 2004
@@ -309,17 +309,18 @@
/* OK. For now, we presume we have a valid client. We now create the
* client structure, even though we cannot fill it completely yet.
* But it allows us to access i2c_smbus_read_byte_data. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct fscher_data), GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct fscher_data));
+ memset(new_client, 0x00, sizeof(struct i2c_client));
+
+ if (!(data = kmalloc(sizeof(struct fscher_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto exit_free1;
+ }
+ memset(data, 0x00, sizeof(struct fscher_data));

- /* The Hermes-specific data is placed right after the common I2C
- * client data. */
- data = (struct fscher_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -371,6 +372,8 @@
return 0;

exit_free:
+ kfree(data);
+exit_free1:
kfree(new_client);
exit:
return err;
@@ -386,6 +389,7 @@
return err;
}

+ kfree(i2c_get_clientdata(client));
kfree(client);
return 0;
}
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/gl518sm.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/gl518sm.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/gl518sm.c Sat Apr 3 12:01:38 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/gl518sm.c Sat Apr 3 14:06:38 2004
@@ -354,16 +354,18 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access gl518_{read,write}_value. */

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct gl518_data),
- GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct gl518_data));
+ memset(new_client, 0x00, sizeof(struct i2c_client));
+
+ if (!(data = kmalloc(sizeof(struct gl518_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto exit_free1;
+ }
+ memset(data, 0x00, sizeof(struct gl518_data));

- data = (struct gl518_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);

new_client->addr = address;
@@ -445,6 +447,8 @@
very code-efficient in this case. */

exit_free:
+ kfree(data);
+exit_free1:
kfree(new_client);
exit:
return err;
@@ -479,6 +483,7 @@
return err;
}

+ kfree(i2c_get_clientdata(client));
kfree(client);

return 0;
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/it87.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/it87.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/it87.c Sat Apr 3 12:01:39 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/it87.c Sat Apr 3 14:06:36 2004
@@ -545,7 +545,7 @@
outb_p(~i & 0x7f, address + 5);
if ((inb_p(address + 5) & 0x7f) != (~i & 0x7f)) {
outb_p(i, address + 5);
- return 0;
+ goto ERROR1;
}
}
}
@@ -554,16 +554,18 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access it87_{read,write}_value. */

- if (!(new_client = kmalloc((sizeof(struct i2c_client)) +
- sizeof(struct it87_data),
- GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto ERROR1;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct it87_data));
+ memset(new_client, 0x00, sizeof(struct i2c_client));
+
+ if (!(data = kmalloc(sizeof(struct it87_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto ERROR2;
+ }
+ memset(data, 0x00, sizeof(struct it87_data));

- data = (struct it87_data *) (new_client + 1);
if (is_isa)
init_MUTEX(&data->lock);
i2c_set_clientdata(new_client, data);
@@ -576,10 +578,10 @@

if (kind < 0) {
if (it87_read_value(new_client, IT87_REG_CONFIG) & 0x80)
- goto ERROR1;
+ goto ERROR3;
if (!is_isa
- && (it87_read_value(new_client, IT87_REG_I2C_ADDR) !=
- address)) goto ERROR1;
+ && it87_read_value(new_client, IT87_REG_I2C_ADDR) != address)
+ goto ERROR3;
}

/* Determine the chip type. */
@@ -594,7 +596,7 @@
"Ignoring 'force' parameter for unknown chip at "
"adapter %d, address 0x%02x\n",
i2c_adapter_id(adapter), address);
- goto ERROR1;
+ goto ERROR3;
}
}

@@ -613,7 +615,7 @@

/* Tell the I2C layer a new client has arrived */
if ((err = i2c_attach_client(new_client)))
- goto ERROR1;
+ goto ERROR3;

/* Initialize the IT87 chip */
it87_init_client(new_client, data);
@@ -669,9 +671,13 @@

return 0;

-ERROR1:
+ERROR3:
+ kfree(data);
+
+ERROR2:
kfree(new_client);

+ERROR1:
if (is_isa)
release_region(address, IT87_EXTENT);
ERROR0:
@@ -690,6 +696,7 @@

if(i2c_is_isa_client(client))
release_region(client->addr, IT87_EXTENT);
+ kfree(i2c_get_clientdata(client));
kfree(client);

return 0;
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/lm75.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/lm75.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/lm75.c Sat Apr 3 12:01:39 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/lm75.c Sat Apr 3 14:06:36 2004
@@ -135,16 +135,18 @@
/* OK. For now, we presume we have a valid client. We now create the
client structure, even though we cannot fill it completely yet.
But it allows us to access lm75_{read,write}_value. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct lm75_data),
- GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct lm75_data));
+ memset(new_client, 0x00, sizeof(struct i2c_client));
+
+ if (!(data = kmalloc(sizeof(struct lm75_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto exit_free1;
+ }
+ memset(data, 0x00, sizeof(struct lm75_data));

- data = (struct lm75_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -194,6 +196,8 @@
return 0;

exit_free:
+ kfree(data);
+exit_free1:
kfree(new_client);
exit:
return err;
@@ -202,6 +206,7 @@
static int lm75_detach_client(struct i2c_client *client)
{
i2c_detach_client(client);
+ kfree(i2c_get_clientdata(client));
kfree(client);
return 0;
}
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/lm78.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/lm78.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/lm78.c Sat Apr 3 12:02:31 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/lm78.c Sat Apr 3 14:06:34 2004
@@ -552,16 +552,18 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access lm78_{read,write}_value. */

- if (!(new_client = kmalloc((sizeof(struct i2c_client)) +
- sizeof(struct lm78_data),
- GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto ERROR1;
}
- memset(new_client, 0, sizeof(struct i2c_client) +
- sizeof(struct lm78_data));
+ memset(new_client, 0, sizeof(struct i2c_client));
+
+ if (!(data = kmalloc(sizeof(struct lm78_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto ERROR1B;
+ }
+ memset(data, 0, sizeof(struct lm78_data));

- data = (struct lm78_data *) (new_client + 1);
if (is_isa)
init_MUTEX(&data->lock);
i2c_set_clientdata(new_client, data);
@@ -671,6 +673,8 @@
return 0;

ERROR2:
+ kfree(data);
+ERROR1B:
kfree(new_client);
ERROR1:
if (is_isa)
@@ -694,6 +698,7 @@
return err;
}

+ kfree(i2c_get_clientdata(client));
kfree(client);

return 0;
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/lm80.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/lm80.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/lm80.c Sat Apr 3 12:01:39 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/lm80.c Sat Apr 3 14:06:34 2004
@@ -357,15 +357,18 @@
/* OK. For now, we presume we have a valid client. We now create the
client structure, even though we cannot fill it completely yet.
But it allows us to access lm80_{read,write}_value. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct lm80_data), GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct lm80_data));
+ memset(new_client, 0x00, sizeof(struct i2c_client));
+
+ if (!(data = kmalloc(sizeof(struct lm80_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto error_free1;
+ }
+ memset(data, 0x00, sizeof(struct lm80_data));

- data = (struct lm80_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -439,6 +442,8 @@
return 0;

error_free:
+ kfree(data);
+error_free1:
kfree(new_client);
exit:
return err;
@@ -454,6 +459,7 @@
return err;
}

+ kfree(i2c_get_clientdata(client));
kfree(client);
return 0;
}
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/lm83.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/lm83.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/lm83.c Sat Apr 3 12:01:39 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/lm83.c Sat Apr 3 14:06:33 2004
@@ -234,17 +234,18 @@
if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
goto exit;

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct lm83_data), GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct lm83_data));
+ memset(new_client, 0x00, sizeof(struct i2c_client));
+
+ if (!(data = kmalloc(sizeof(struct lm83_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto exit_free1;
+ }
+ memset(data, 0x00, sizeof(struct lm83_data));

- /* The LM83-specific data is placed right after the common I2C
- * client data. */
- data = (struct lm83_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -329,6 +330,8 @@
return 0;

exit_free:
+ kfree(data);
+exit_free1:
kfree(new_client);
exit:
return err;
@@ -344,6 +347,7 @@
return err;
}

+ kfree(i2c_get_clientdata(client));
kfree(client);
return 0;
}
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/lm85.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/lm85.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/lm85.c Sat Apr 3 12:01:39 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/lm85.c Sat Apr 3 14:06:32 2004
@@ -736,16 +736,18 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access lm85_{read,write}_value. */

- if (!(new_client = kmalloc((sizeof(struct i2c_client)) +
- sizeof(struct lm85_data),
- GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto ERROR0;
}
+ memset(new_client, 0, sizeof(struct i2c_client));
+
+ if (!(data = kmalloc(sizeof(struct lm85_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto ERROR0B;
+ }
+ memset(data, 0, sizeof(struct lm85_data));

- memset(new_client, 0, sizeof(struct i2c_client) +
- sizeof(struct lm85_data));
- data = (struct lm85_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -886,6 +888,8 @@

/* Error out and cleanup code */
ERROR1:
+ kfree(data);
+ ERROR0B:
kfree(new_client);
ERROR0:
return err;
@@ -894,6 +898,7 @@
int lm85_detach_client(struct i2c_client *client)
{
i2c_detach_client(client);
+ kfree(i2c_get_clientdata(client));
kfree(client);
return 0;
}
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/lm90.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/lm90.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/lm90.c Sat Apr 3 12:01:39 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/lm90.c Sat Apr 3 14:06:31 2004
@@ -280,17 +280,18 @@
if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
goto exit;

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct lm90_data), GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct lm90_data));
+ memset(new_client, 0x00, sizeof(struct i2c_client));
+
+ if (!(data = kmalloc(sizeof(struct lm90_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto exit_free1;
+ }
+ memset(data, 0x00, sizeof(struct lm90_data));

- /* The LM90-specific data is placed right after the common I2C
- * client data. */
- data = (struct lm90_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -390,6 +391,8 @@
return 0;

exit_free:
+ kfree(data);
+exit_free1:
kfree(new_client);
exit:
return err;
@@ -420,6 +423,7 @@
return err;
}

+ kfree(i2c_get_clientdata(client));
kfree(client);
return 0;
}
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/pcf8574.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/pcf8574.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/pcf8574.c Sat Apr 3 12:02:31 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/pcf8574.c Sat Apr 3 14:06:30 2004
@@ -127,17 +127,18 @@

/* OK. For now, we presume we have a valid client. We now create the
client structure, even though we cannot fill it completely yet. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct pcf8574_data),
- GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
+ memset(new_client, 0, sizeof(struct i2c_client));

- memset(new_client, 0, sizeof(struct i2c_client) +
- sizeof(struct pcf8574_data));
+ if (!(data = kmalloc(sizeof(struct pcf8574_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto exit_free1;
+ }
+ memset(data, 0, sizeof(struct pcf8574_data));

- data = (struct pcf8574_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -182,6 +183,8 @@
very code-efficient in this case. */

exit_free:
+ kfree(data);
+ exit_free1:
kfree(new_client);
exit:
return err;
@@ -197,6 +200,7 @@
return err;
}

+ kfree(i2c_get_clientdata(client));
kfree(client);
return 0;
}
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/via686a.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/via686a.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/via686a.c Sat Apr 3 12:02:31 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/via686a.c Sat Apr 3 14:06:29 2004
@@ -687,16 +687,18 @@
return -ENODEV;
}

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct via686a_data),
- GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto ERROR0;
}
+ memset(new_client, 0x00, sizeof(struct i2c_client));
+
+ if (!(data = kmalloc(sizeof(struct via686a_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto ERROR1;
+ }
+ memset(data, 0x00, sizeof(struct via686a_data));

- memset(new_client,0x00, sizeof(struct i2c_client) +
- sizeof(struct via686a_data));
- data = (struct via686a_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -752,9 +754,11 @@
return 0;

ERROR3:
- release_region(address, VIA686A_EXTENT);
+ kfree(data);
+ ERROR1:
kfree(new_client);
ERROR0:
+ release_region(address, VIA686A_EXTENT);
return err;
}

@@ -769,6 +773,7 @@
}

release_region(client->addr, VIA686A_EXTENT);
+ kfree(i2c_get_clientdata(client));
kfree(client);

return 0;
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/w83627hf.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/w83627hf.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/w83627hf.c Sat Apr 3 12:02:31 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/w83627hf.c Sat Apr 3 14:06:22 2004
@@ -941,17 +941,18 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access w83627hf_{read,write}_value. */

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct w83627hf_data),
- GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto ERROR1;
}
+ memset(new_client, 0x00, sizeof(struct i2c_client));

- memset(new_client, 0x00, sizeof (struct i2c_client) +
- sizeof (struct w83627hf_data));
+ if (!(data = kmalloc(sizeof(struct w83627hf_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto ERROR1B;
+ }
+ memset(data, 0x00, sizeof (struct w83627hf_data));

- data = (struct w83627hf_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
init_MUTEX(&data->lock);
@@ -1042,6 +1043,8 @@
return 0;

ERROR2:
+ kfree(data);
+ ERROR1B:
kfree(new_client);
ERROR1:
release_region(address, WINB_EXTENT);
@@ -1060,6 +1063,7 @@
}

release_region(client->addr, WINB_EXTENT);
+ kfree(i2c_get_clientdata(client));
kfree(client);

return 0;
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/w83781d.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/w83781d.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/w83781d.c Sat Apr 3 12:02:31 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/w83781d.c Sat Apr 3 14:06:21 2004
@@ -1117,16 +1117,18 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access w83781d_{read,write}_value. */

- if (!(new_client = kmalloc(sizeof (struct i2c_client) +
- sizeof (struct w83781d_data), GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto ERROR1;
}
+ memset(new_client, 0x00, sizeof(struct i2c_client));

- memset(new_client, 0x00, sizeof (struct i2c_client) +
- sizeof (struct w83781d_data));
+ if (!(data = kmalloc(sizeof(struct w83781d_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto ERROR1B;
+ }
+ memset(data, 0x00, sizeof (struct w83781d_data));

- data = (struct w83781d_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
init_MUTEX(&data->lock);
@@ -1326,6 +1328,8 @@
ERROR3:
i2c_detach_client(new_client);
ERROR2:
+ kfree(data);
+ERROR1B:
kfree(new_client);
ERROR1:
if (is_isa)
@@ -1348,6 +1352,7 @@
return err;
}

+ kfree(i2c_get_clientdata(client));
kfree(client);

return 0;
diff -ru linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/w83l785ts.c linux-2.6.5-rc3-mm4/drivers/i2c/chips/w83l785ts.c
--- linux-2.6.5-rc3-mm4/drivers/i2c/chips.orig/w83l785ts.c Sat Apr 3 12:01:39 2004
+++ linux-2.6.5-rc3-mm4/drivers/i2c/chips/w83l785ts.c Sat Apr 3 14:06:20 2004
@@ -164,18 +164,18 @@
if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
goto exit;

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct w83l785ts_data), GFP_KERNEL))) {
+ if (!(new_client = kmalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct w83l785ts_data));
+ memset(new_client, 0x00, sizeof(struct i2c_client));

+ if (!(data = kmalloc(sizeof(struct w83l785ts_data), GFP_KERNEL))) {
+ err = -ENOMEM;
+ goto exit_free1;
+ }
+ memset(data, 0x00, sizeof(struct w83l785ts_data));

- /* The W83L785TS-specific data is placed right after the common I2C
- * client data. */
- data = (struct w83l785ts_data *) (new_client + 1);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -255,6 +255,8 @@
return 0;

exit_free:
+ kfree(data);
+exit_free1:
kfree(new_client);
exit:
return err;
@@ -270,6 +272,7 @@
return err;
}

+ kfree(i2c_get_clientdata(client));
kfree(client);
return 0;
}


--
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/


2004-04-03 19:07:55

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [RFC 2.6] Rework memory allocation in i2c chip drivers

On Sat, 3 Apr 2004, Jean Delvare wrote:
> Some times ago, Ralf Roesch reported that the memory allocation scheme
> used in the i2c eeprom driver was causing trouble on MIPS architecture:
>
> http://archives.andrew.net.au/lm-sensors/msg07233.html
>
> The cause of the problems is that we do allocate two structures with a
> single kmalloc, which breaks alignment. This doesn't seem to be a
> problem on x86, but is on mips and probably on other architectures as
> well. It happens that all other chip drivers work the same way too, so
> they all would need to be fixed.
>
> Here comes my proposal to fix the problem. A few notes:

Alternatively, define a new struct containing the two structs

struct combined {
struct part1 part1;
struct part2 part2;
};

and allocate a struct combined, and the C compiler will take care of alignment.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2004-04-03 20:21:41

by Sergey Vlasov

[permalink] [raw]
Subject: Re: [RFC 2.6] Rework memory allocation in i2c chip drivers

Hello!

> Some times ago, Ralf Roesch reported that the memory allocation scheme
> used in the i2c eeprom driver was causing trouble on MIPS architecture:
>
> http://archives.andrew.net.au/lm-sensors/msg07233.html
>
> The cause of the problems is that we do allocate two structures with a
> single kmalloc, which breaks alignment. This doesn't seem to be a
> problem on x86, but is on mips and probably on other architectures as
> well. It happens that all other chip drivers work the same way too, so
> they all would need to be fixed.

Instead of splitting one kmalloc in two, it would also be possible to
add a "struct i2c_client client" field to each of the *_data
structures - the compiler should align all fields appropriately.
Probably this way will result in less changes to the code (and also
less labels and less error paths).

Example patch for lm75 (untested):

--- linux/drivers/i2c/chips/lm75.c.sensors-kmalloc 2004-02-18 06:57:11 +0300
+++ linux/drivers/i2c/chips/lm75.c 2004-04-04 00:11:41 +0400
@@ -50,6 +50,7 @@

/* Each client has this additional data */
struct lm75_data {
+ struct i2c_client client;
struct semaphore update_lock;
char valid; /* !=0 if following fields are valid */
unsigned long last_updated; /* In jiffies */
@@ -141,16 +142,13 @@
/* OK. For now, we presume we have a valid client. We now create the
client structure, even though we cannot fill it completely yet.
But it allows us to access lm75_{read,write}_value. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct lm75_data),
- GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct lm75_data), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct lm75_data));
+ memset(data, 0x00, sizeof(struct lm75_data));

- data = (struct lm75_data *) (new_client + 1);
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -204,7 +202,7 @@
return 0;

exit_free:
- kfree(new_client);
+ kfree(data);
exit:
return err;
}
@@ -212,7 +210,7 @@
static int lm75_detach_client(struct i2c_client *client)
{
i2c_detach_client(client);
- kfree(client);
+ kfree(i2c_get_clientdata(client));
return 0;
}


--
Sergey Vlasov


Attachments:
(No filename) (2.27 kB)
(No filename) (189.00 B)
Download all attachments

2004-04-09 17:33:43

by Greg KH

[permalink] [raw]
Subject: Re: [RFC 2.6] Rework memory allocation in i2c chip drivers

On Sun, Apr 04, 2004 at 12:20:42AM +0400, Sergey Vlasov wrote:
> Hello!
>
> > Some times ago, Ralf Roesch reported that the memory allocation scheme
> > used in the i2c eeprom driver was causing trouble on MIPS architecture:
> >
> > http://archives.andrew.net.au/lm-sensors/msg07233.html
> >
> > The cause of the problems is that we do allocate two structures with a
> > single kmalloc, which breaks alignment. This doesn't seem to be a
> > problem on x86, but is on mips and probably on other architectures as
> > well. It happens that all other chip drivers work the same way too, so
> > they all would need to be fixed.
>
> Instead of splitting one kmalloc in two, it would also be possible to
> add a "struct i2c_client client" field to each of the *_data
> structures - the compiler should align all fields appropriately.
> Probably this way will result in less changes to the code (and also
> less labels and less error paths).
>
> Example patch for lm75 (untested):

I like this version a lot better. It's simpler and if we do this, we
can easily switch to the proper refcount handling of the i2c_client
structures like we should do in 2.7.

Jean, care to redo your patch in this form?

thanks,

greg k-h

2004-04-10 14:58:58

by Jean Delvare

[permalink] [raw]
Subject: [PATCH 2.6] Rework memory allocation in i2c chip drivers (second try)

> > Instead of splitting one kmalloc in two, it would also be possible
> > to add a "struct i2c_client client" field to each of the *_data
> > structures - the compiler should align all fields appropriately.
> > Probably this way will result in less changes to the code (and also
> > less labels and less error paths).
>
> I like this version a lot better. It's simpler and if we do this, we
> can easily switch to the proper refcount handling of the i2c_client
> structures like we should do in 2.7.
>
> Jean, care to redo your patch in this form?

OK, here you go. Thanks Sergey for the insightful example!

Additional remarks:

1* This patch also removes an unused struct member in via686a and fix an
error message in ds1621.

2* I discovered error path problems in it87 and via686a detection
functions. For the it87, I think that this patch makes it even more
broken. I will fix both drivers in a later patch (really soon).

Thanks.

diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/adm1021.c linux-2.6.5-mm3/drivers/i2c/chips/adm1021.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/adm1021.c Fri Apr 9 22:36:33 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/adm1021.c Sat Apr 10 10:33:47 2004
@@ -101,6 +101,7 @@

/* Each client has this additional data */
struct adm1021_data {
+ struct i2c_client client;
enum chips type;

struct semaphore update_lock;
@@ -228,16 +229,13 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access adm1021_{read,write}_value. */

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct adm1021_data),
- GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct adm1021_data), GFP_KERNEL))) {
err = -ENOMEM;
goto error0;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct adm1021_data));
+ memset(data, 0, sizeof(struct adm1021_data));

- data = (struct adm1021_data *) (new_client + 1);
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -329,7 +327,7 @@
return 0;

error1:
- kfree(new_client);
+ kfree(data);
error0:
return err;
}
@@ -352,7 +350,7 @@
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));
return 0;
}

diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/asb100.c linux-2.6.5-mm3/drivers/i2c/chips/asb100.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/asb100.c Fri Apr 9 22:36:33 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/asb100.c Sat Apr 10 09:37:12 2004
@@ -193,6 +193,7 @@
data is pointed to by client->data. The structure itself is
dynamically allocated, at the same time the client itself is allocated. */
struct asb100_data {
+ struct i2c_client client;
struct semaphore lock;
enum chips type;

@@ -722,17 +723,14 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access asb100_{read,write}_value. */

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct asb100_data), GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct asb100_data), GFP_KERNEL))) {
pr_debug("asb100.o: detect failed, kmalloc failed!\n");
err = -ENOMEM;
goto ERROR0;
}
+ memset(data, 0, sizeof(struct asb100_data));

- memset(new_client, 0,
- sizeof(struct i2c_client) + sizeof(struct asb100_data));
-
- data = (struct asb100_data *) (new_client + 1);
+ new_client = &data->client;
init_MUTEX(&data->lock);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
@@ -842,7 +840,7 @@
ERROR2:
i2c_detach_client(new_client);
ERROR1:
- kfree(new_client);
+ kfree(data);
ERROR0:
return err;
}
@@ -857,7 +855,7 @@
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));

return 0;
}
diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/ds1621.c linux-2.6.5-mm3/drivers/i2c/chips/ds1621.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/ds1621.c Fri Apr 9 22:36:33 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/ds1621.c Sat Apr 10 09:34:04 2004
@@ -70,6 +70,7 @@

/* Each client has this additional data */
struct ds1621_data {
+ struct i2c_client client;
struct semaphore update_lock;
char valid; /* !=0 if following fields are valid */
unsigned long last_updated; /* In jiffies */
@@ -196,16 +197,13 @@
/* OK. For now, we presume we have a valid client. We now create the
client structure, even though we cannot fill it completely yet.
But it allows us to access ds1621_{read,write}_value. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct ds1621_data),
- GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct ds1621_data), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0, sizeof(struct i2c_client) +
- sizeof(struct ds1621_data));
+ memset(data, 0, sizeof(struct ds1621_data));

- data = (struct ds1621_data *) (new_client + 1);
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -258,7 +256,7 @@
/* OK, this is not exactly good programming practice, usually. But it is
very code-efficient in this case. */
exit_free:
- kfree(new_client);
+ kfree(data);
exit:
return err;
}
@@ -268,12 +266,12 @@
int err;

if ((err = i2c_detach_client(client))) {
- dev_err(&client->dev,
- "ds1621.o: Client deregistration failed, client not detached.\n");
+ dev_err(&client->dev, "Client deregistration failed, "
+ "client not detached.\n");
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));

return 0;
}
diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/eeprom.c linux-2.6.5-mm3/drivers/i2c/chips/eeprom.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/eeprom.c Fri Apr 9 22:36:33 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/eeprom.c Sat Apr 10 10:33:41 2004
@@ -63,6 +63,7 @@

/* Each client has this additional data */
struct eeprom_data {
+ struct i2c_client client;
struct semaphore update_lock;
u8 valid; /* bitfield, bit!=0 if slice is valid */
unsigned long last_updated[8]; /* In jiffies, 8 slices */
@@ -187,16 +188,13 @@
/* OK. For now, we presume we have a valid client. We now create the
client structure, even though we cannot fill it completely yet.
But it allows us to access eeprom_{read,write}_value. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct eeprom_data),
- GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct eeprom_data), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct eeprom_data));
+ memset(data, 0, sizeof(struct eeprom_data));

- data = (struct eeprom_data *) (new_client + 1);
+ new_client = &data->client;
memset(data->data, 0xff, EEPROM_SIZE);
i2c_set_clientdata(new_client, data);
new_client->addr = address;
@@ -244,7 +242,7 @@
return 0;

exit_kfree:
- kfree(new_client);
+ kfree(data);
exit:
return err;
}
@@ -259,7 +257,7 @@
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));

return 0;
}
diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/fscher.c linux-2.6.5-mm3/drivers/i2c/chips/fscher.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/fscher.c Sun Apr 4 09:52:29 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/fscher.c Sat Apr 10 10:33:18 2004
@@ -133,6 +133,7 @@
*/

struct fscher_data {
+ struct i2c_client client;
struct semaphore update_lock;
char valid; /* zero until following fields are valid */
unsigned long last_updated; /* in jiffies */
@@ -309,17 +310,15 @@
/* OK. For now, we presume we have a valid client. We now create the
* client structure, even though we cannot fill it completely yet.
* But it allows us to access i2c_smbus_read_byte_data. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct fscher_data), GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct fscher_data), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct fscher_data));
+ memset(data, 0, sizeof(struct fscher_data));

- /* The Hermes-specific data is placed right after the common I2C
- * client data. */
- data = (struct fscher_data *) (new_client + 1);
+ /* The common I2C client data is placed right before the
+ * Hermes-specific data. */
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -371,7 +370,7 @@
return 0;

exit_free:
- kfree(new_client);
+ kfree(data);
exit:
return err;
}
@@ -386,7 +385,7 @@
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));
return 0;
}

diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/gl518sm.c linux-2.6.5-mm3/drivers/i2c/chips/gl518sm.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/gl518sm.c Fri Apr 9 22:36:33 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/gl518sm.c Sat Apr 10 10:33:14 2004
@@ -118,6 +118,7 @@

/* Each client has this additional data */
struct gl518_data {
+ struct i2c_client client;
enum chips type;

struct semaphore update_lock;
@@ -354,16 +355,13 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access gl518_{read,write}_value. */

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct gl518_data),
- GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct gl518_data), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct gl518_data));
+ memset(data, 0, sizeof(struct gl518_data));

- data = (struct gl518_data *) (new_client + 1);
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);

new_client->addr = address;
@@ -445,7 +443,7 @@
very code-efficient in this case. */

exit_free:
- kfree(new_client);
+ kfree(data);
exit:
return err;
}
@@ -479,7 +477,7 @@
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));

return 0;
}
diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/it87.c linux-2.6.5-mm3/drivers/i2c/chips/it87.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/it87.c Sun Apr 4 09:52:29 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/it87.c Sat Apr 10 10:33:09 2004
@@ -134,6 +134,7 @@
dynamically allocated, at the same time when a new it87 client is
allocated. */
struct it87_data {
+ struct i2c_client client;
struct semaphore lock;
enum chips type;

@@ -508,7 +509,7 @@
int it87_detect(struct i2c_adapter *adapter, int address, int kind)
{
int i;
- struct i2c_client *new_client = NULL;
+ struct i2c_client *new_client;
struct it87_data *data;
int err = 0;
const char *name = "";
@@ -554,16 +555,13 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access it87_{read,write}_value. */

- if (!(new_client = kmalloc((sizeof(struct i2c_client)) +
- sizeof(struct it87_data),
- GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct it87_data), GFP_KERNEL))) {
err = -ENOMEM;
goto ERROR1;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct it87_data));
+ memset(data, 0, sizeof(struct it87_data));

- data = (struct it87_data *) (new_client + 1);
+ new_client = &data->client;
if (is_isa)
init_MUTEX(&data->lock);
i2c_set_clientdata(new_client, data);
@@ -670,7 +668,7 @@
return 0;

ERROR1:
- kfree(new_client);
+ kfree(data);

if (is_isa)
release_region(address, IT87_EXTENT);
@@ -690,7 +688,7 @@

if(i2c_is_isa_client(client))
release_region(client->addr, IT87_EXTENT);
- kfree(client);
+ kfree(i2c_get_clientdata(client));

return 0;
}
diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/lm75.c linux-2.6.5-mm3/drivers/i2c/chips/lm75.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/lm75.c Fri Apr 9 22:36:33 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/lm75.c Sat Apr 10 10:33:06 2004
@@ -46,6 +46,7 @@

/* Each client has this additional data */
struct lm75_data {
+ struct i2c_client client;
struct semaphore update_lock;
char valid; /* !=0 if following fields are valid */
unsigned long last_updated; /* In jiffies */
@@ -135,16 +136,13 @@
/* OK. For now, we presume we have a valid client. We now create the
client structure, even though we cannot fill it completely yet.
But it allows us to access lm75_{read,write}_value. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct lm75_data),
- GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct lm75_data), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct lm75_data));
+ memset(data, 0, sizeof(struct lm75_data));

- data = (struct lm75_data *) (new_client + 1);
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -194,7 +192,7 @@
return 0;

exit_free:
- kfree(new_client);
+ kfree(data);
exit:
return err;
}
@@ -202,7 +200,7 @@
static int lm75_detach_client(struct i2c_client *client)
{
i2c_detach_client(client);
- kfree(client);
+ kfree(i2c_get_clientdata(client));
return 0;
}

diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/lm78.c linux-2.6.5-mm3/drivers/i2c/chips/lm78.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/lm78.c Fri Apr 9 22:36:33 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/lm78.c Sat Apr 10 09:09:45 2004
@@ -192,6 +192,7 @@
dynamically allocated, at the same time when a new lm78 client is
allocated. */
struct lm78_data {
+ struct i2c_client client;
struct semaphore lock;
enum chips type;

@@ -552,16 +553,13 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access lm78_{read,write}_value. */

- if (!(new_client = kmalloc((sizeof(struct i2c_client)) +
- sizeof(struct lm78_data),
- GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct lm78_data), GFP_KERNEL))) {
err = -ENOMEM;
goto ERROR1;
}
- memset(new_client, 0, sizeof(struct i2c_client) +
- sizeof(struct lm78_data));
+ memset(data, 0, sizeof(struct lm78_data));

- data = (struct lm78_data *) (new_client + 1);
+ new_client = &data->client;
if (is_isa)
init_MUTEX(&data->lock);
i2c_set_clientdata(new_client, data);
@@ -671,7 +669,7 @@
return 0;

ERROR2:
- kfree(new_client);
+ kfree(data);
ERROR1:
if (is_isa)
release_region(address, LM78_EXTENT);
@@ -694,7 +692,7 @@
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));

return 0;
}
diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/lm80.c linux-2.6.5-mm3/drivers/i2c/chips/lm80.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/lm80.c Sat Apr 10 07:31:52 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/lm80.c Sat Apr 10 10:33:01 2004
@@ -110,6 +110,7 @@
*/

struct lm80_data {
+ struct i2c_client client;
struct semaphore update_lock;
char valid; /* !=0 if following fields are valid */
unsigned long last_updated; /* In jiffies */
@@ -394,15 +395,13 @@
/* OK. For now, we presume we have a valid client. We now create the
client structure, even though we cannot fill it completely yet.
But it allows us to access lm80_{read,write}_value. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct lm80_data), GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct lm80_data), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct lm80_data));
+ memset(data, 0, sizeof(struct lm80_data));

- data = (struct lm80_data *) (new_client + 1);
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -480,7 +479,7 @@
return 0;

error_free:
- kfree(new_client);
+ kfree(data);
exit:
return err;
}
@@ -495,7 +494,7 @@
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));
return 0;
}

diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/lm83.c linux-2.6.5-mm3/drivers/i2c/chips/lm83.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/lm83.c Sun Apr 4 09:52:29 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/lm83.c Sat Apr 10 10:32:57 2004
@@ -134,6 +134,7 @@
*/

struct lm83_data {
+ struct i2c_client client;
struct semaphore update_lock;
char valid; /* zero until following fields are valid */
unsigned long last_updated; /* in jiffies */
@@ -234,17 +235,15 @@
if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
goto exit;

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct lm83_data), GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct lm83_data), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct lm83_data));
+ memset(data, 0, sizeof(struct lm83_data));

- /* The LM83-specific data is placed right after the common I2C
- * client data. */
- data = (struct lm83_data *) (new_client + 1);
+ /* The common I2C client data is placed right after the
+ * LM83-specific data. */
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -329,7 +328,7 @@
return 0;

exit_free:
- kfree(new_client);
+ kfree(data);
exit:
return err;
}
@@ -344,7 +343,7 @@
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));
return 0;
}

diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/lm85.c linux-2.6.5-mm3/drivers/i2c/chips/lm85.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/lm85.c Sun Apr 4 09:52:29 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/lm85.c Sat Apr 10 09:01:14 2004
@@ -351,6 +351,7 @@
};

struct lm85_data {
+ struct i2c_client client;
struct semaphore lock;
enum chips type;

@@ -736,16 +737,13 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access lm85_{read,write}_value. */

- if (!(new_client = kmalloc((sizeof(struct i2c_client)) +
- sizeof(struct lm85_data),
- GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct lm85_data), GFP_KERNEL))) {
err = -ENOMEM;
goto ERROR0;
}
+ memset(data, 0, sizeof(struct lm85_data));

- memset(new_client, 0, sizeof(struct i2c_client) +
- sizeof(struct lm85_data));
- data = (struct lm85_data *) (new_client + 1);
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -886,7 +884,7 @@

/* Error out and cleanup code */
ERROR1:
- kfree(new_client);
+ kfree(data);
ERROR0:
return err;
}
@@ -894,7 +892,7 @@
int lm85_detach_client(struct i2c_client *client)
{
i2c_detach_client(client);
- kfree(client);
+ kfree(i2c_get_clientdata(client));
return 0;
}

diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/lm90.c linux-2.6.5-mm3/drivers/i2c/chips/lm90.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/lm90.c Sun Apr 4 09:52:29 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/lm90.c Sat Apr 10 10:32:53 2004
@@ -142,6 +142,7 @@
*/

struct lm90_data {
+ struct i2c_client client;
struct semaphore update_lock;
char valid; /* zero until following fields are valid */
unsigned long last_updated; /* in jiffies */
@@ -280,17 +281,15 @@
if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
goto exit;

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct lm90_data), GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct lm90_data), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct lm90_data));
+ memset(data, 0, sizeof(struct lm90_data));

- /* The LM90-specific data is placed right after the common I2C
- * client data. */
- data = (struct lm90_data *) (new_client + 1);
+ /* The common I2C client data is placed right before the
+ LM90-specific data. */
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -390,7 +389,7 @@
return 0;

exit_free:
- kfree(new_client);
+ kfree(data);
exit:
return err;
}
@@ -420,7 +419,7 @@
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));
return 0;
}

diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/pcf8574.c linux-2.6.5-mm3/drivers/i2c/chips/pcf8574.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/pcf8574.c Fri Apr 9 22:36:33 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/pcf8574.c Sat Apr 10 08:56:42 2004
@@ -55,6 +55,7 @@

/* Each client has this additional data */
struct pcf8574_data {
+ struct i2c_client client;
struct semaphore update_lock;

u8 read, write; /* Register values */
@@ -127,17 +128,13 @@

/* OK. For now, we presume we have a valid client. We now create the
client structure, even though we cannot fill it completely yet. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct pcf8574_data),
- GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct pcf8574_data), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
+ memset(data, 0, sizeof(struct pcf8574_data));

- memset(new_client, 0, sizeof(struct i2c_client) +
- sizeof(struct pcf8574_data));
-
- data = (struct pcf8574_data *) (new_client + 1);
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -182,7 +179,7 @@
very code-efficient in this case. */

exit_free:
- kfree(new_client);
+ kfree(data);
exit:
return err;
}
@@ -197,7 +194,7 @@
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));
return 0;
}

diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/pcf8591.c linux-2.6.5-mm3/drivers/i2c/chips/pcf8591.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/pcf8591.c Sat Apr 10 07:31:37 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/pcf8591.c Sat Apr 10 08:51:58 2004
@@ -76,6 +76,7 @@
#define REG_TO_SIGNED(reg) (((reg) & 0x80)?((reg) - 256):(reg))

struct pcf8591_data {
+ struct i2c_client client;
struct semaphore update_lock;

u8 control;
@@ -178,17 +179,13 @@

/* OK. For now, we presume we have a valid client. We now create the
client structure, even though we cannot fill it completely yet. */
- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct pcf8591_data),
- GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct pcf8591_data), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
-
- memset(new_client, 0, sizeof(struct i2c_client) +
- sizeof(struct pcf8591_data));
+ memset(data, 0, sizeof(struct pcf8591_data));

- data = (struct pcf8591_data *) (new_client + 1);
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -236,7 +233,7 @@
very code-efficient in this case. */

exit_kfree:
- kfree(new_client);
+ kfree(data);
exit:
return err;
}
@@ -251,7 +248,7 @@
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));
return 0;
}

diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/via686a.c linux-2.6.5-mm3/drivers/i2c/chips/via686a.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/via686a.c Fri Apr 9 22:36:33 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/via686a.c Sat Apr 10 10:32:48 2004
@@ -369,8 +369,7 @@
dynamically allocated, at the same time when a new via686a client is
allocated. */
struct via686a_data {
- int sysctl_id;
-
+ struct i2c_client client;
struct semaphore update_lock;
char valid; /* !=0 if following fields are valid */
unsigned long last_updated; /* In jiffies */
@@ -687,16 +686,13 @@
return -ENODEV;
}

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct via686a_data),
- GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct via686a_data), GFP_KERNEL))) {
err = -ENOMEM;
goto ERROR0;
}
+ memset(data, 0, sizeof(struct via686a_data));

- memset(new_client,0x00, sizeof(struct i2c_client) +
- sizeof(struct via686a_data));
- data = (struct via686a_data *) (new_client + 1);
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -753,7 +749,7 @@

ERROR3:
release_region(address, VIA686A_EXTENT);
- kfree(new_client);
+ kfree(data);
ERROR0:
return err;
}
@@ -769,7 +765,7 @@
}

release_region(client->addr, VIA686A_EXTENT);
- kfree(client);
+ kfree(i2c_get_clientdata(client));

return 0;
}
diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/w83627hf.c linux-2.6.5-mm3/drivers/i2c/chips/w83627hf.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/w83627hf.c Fri Apr 9 22:36:33 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/w83627hf.c Sat Apr 10 10:34:44 2004
@@ -277,6 +277,7 @@
data is pointed to by w83627hf_list[NR]->data. The structure itself is
dynamically allocated, at the same time when a new client is allocated. */
struct w83627hf_data {
+ struct i2c_client client;
struct semaphore lock;
enum chips type;

@@ -941,17 +942,13 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access w83627hf_{read,write}_value. */

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct w83627hf_data),
- GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct w83627hf_data), GFP_KERNEL))) {
err = -ENOMEM;
goto ERROR1;
}
+ memset(data, 0, sizeof(struct w83627hf_data));

- memset(new_client, 0x00, sizeof (struct i2c_client) +
- sizeof (struct w83627hf_data));
-
- data = (struct w83627hf_data *) (new_client + 1);
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
init_MUTEX(&data->lock);
@@ -1042,7 +1039,7 @@
return 0;

ERROR2:
- kfree(new_client);
+ kfree(data);
ERROR1:
release_region(address, WINB_EXTENT);
ERROR0:
@@ -1060,7 +1057,7 @@
}

release_region(client->addr, WINB_EXTENT);
- kfree(client);
+ kfree(i2c_get_clientdata(client));

return 0;
}
diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/w83781d.c linux-2.6.5-mm3/drivers/i2c/chips/w83781d.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/w83781d.c Fri Apr 9 22:36:33 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/w83781d.c Sat Apr 10 10:35:50 2004
@@ -226,6 +226,7 @@
dynamically allocated, at the same time when a new w83781d client is
allocated. */
struct w83781d_data {
+ struct i2c_client client;
struct semaphore lock;
enum chips type;

@@ -1112,16 +1113,13 @@
client structure, even though we cannot fill it completely yet.
But it allows us to access w83781d_{read,write}_value. */

- if (!(new_client = kmalloc(sizeof (struct i2c_client) +
- sizeof (struct w83781d_data), GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct w83781d_data), GFP_KERNEL))) {
err = -ENOMEM;
goto ERROR1;
}
+ memset(data, 0, sizeof(struct w83781d_data));

- memset(new_client, 0x00, sizeof (struct i2c_client) +
- sizeof (struct w83781d_data));
-
- data = (struct w83781d_data *) (new_client + 1);
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
init_MUTEX(&data->lock);
@@ -1321,7 +1319,7 @@
ERROR3:
i2c_detach_client(new_client);
ERROR2:
- kfree(new_client);
+ kfree(data);
ERROR1:
if (is_isa)
release_region(address, W83781D_EXTENT);
@@ -1343,7 +1341,7 @@
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));

return 0;
}
diff -ru linux-2.6.5-mm3/drivers/i2c/chips.orig/w83l785ts.c linux-2.6.5-mm3/drivers/i2c/chips/w83l785ts.c
--- linux-2.6.5-mm3/drivers/i2c/chips.orig/w83l785ts.c Sun Apr 4 09:52:29 2004
+++ linux-2.6.5-mm3/drivers/i2c/chips/w83l785ts.c Sat Apr 10 10:32:32 2004
@@ -105,7 +105,7 @@
*/

struct w83l785ts_data {
-
+ struct i2c_client client;
struct semaphore update_lock;
char valid; /* zero until following fields are valid */
unsigned long last_updated; /* in jiffies */
@@ -164,18 +164,16 @@
if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
goto exit;

- if (!(new_client = kmalloc(sizeof(struct i2c_client) +
- sizeof(struct w83l785ts_data), GFP_KERNEL))) {
+ if (!(data = kmalloc(sizeof(struct w83l785ts_data), GFP_KERNEL))) {
err = -ENOMEM;
goto exit;
}
- memset(new_client, 0x00, sizeof(struct i2c_client) +
- sizeof(struct w83l785ts_data));
+ memset(data, 0, sizeof(struct w83l785ts_data));


- /* The W83L785TS-specific data is placed right after the common I2C
- * client data. */
- data = (struct w83l785ts_data *) (new_client + 1);
+ /* The common I2C client data is placed right before the
+ * W83L785TS-specific data. */
+ new_client = &data->client;
i2c_set_clientdata(new_client, data);
new_client->addr = address;
new_client->adapter = adapter;
@@ -255,7 +253,7 @@
return 0;

exit_free:
- kfree(new_client);
+ kfree(data);
exit:
return err;
}
@@ -270,7 +268,7 @@
return err;
}

- kfree(client);
+ kfree(i2c_get_clientdata(client));
return 0;
}



--
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

2004-04-17 12:53:00

by Jean Delvare

[permalink] [raw]
Subject: Re: [PATCH 2.6] Rework memory allocation in i2c chip drivers (second try)

> > > Instead of splitting one kmalloc in two, it would also be possible
> > > to add a "struct i2c_client client" field to each of the *_data
> > > structures - the compiler should align all fields appropriately.
> > > Probably this way will result in less changes to the code (and
> > > also less labels and less error paths).
> >
> > I like this version a lot better. It's simpler and if we do this,
> > we can easily switch to the proper refcount handling of the
> > i2c_client structures like we should do in 2.7.
> >
> > Jean, care to redo your patch in this form?
>
> OK, here you go. Thanks Sergey for the insightful example!

U-ho. I think I've introduced a memory leak with this patch :(

For drivers that handle subclients (asb100 and w83781d on i2c), the
sublient memory is never released if I read the code correctly. This is
because we now free the private data on unload, assuming that it
contains the i2c client data as well. That's true for the main i2c
client, but not for the subclients (data == NULL so nothing is freed).

Could someone take a look and confirm?

I can see two different fixes:

1* When freeing the memory, free the data if it's not NULL (main
client), else free client (subclients). Cleaner (I suppose?).

2* When creating subclients, do data = &client instead of data = NULL.
Then freeing will work. Less code, faster. Are there side effects? (I
don't think so)

My preference would go to 2*.

Thanks.

--
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/

2004-04-18 06:00:59

by Jean Delvare

[permalink] [raw]
Subject: Re: [PATCH 2.6] Rework memory allocation in i2c chip drivers (second try)

Hu!? Any idea why this (duplicate) mail went through vger.kernel.org and
nivdia.com before reaching the lm_sensors mailing-list? Broken mail
server at nvidia, or what?

Just curious.

Thanks.

--
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/