Rework memory allocation in i2c chip drivers
RFC 1/1 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.
drivers/i2c/chips/adm1021.c renamed from drivers/i2c/chips.orig/adm1021.c +11 -6
@@ -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 ; }
drivers/i2c/chips/asb100.c renamed from drivers/i2c/chips.orig/asb100.c +11 -5
@@ -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 ;
drivers/i2c/chips/ds1621.c renamed from drivers/i2c/chips.orig/ds1621.c +11 -6
@@ -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 ;
drivers/i2c/chips/eeprom.c renamed from drivers/i2c/chips.orig/eeprom.c +11 -6
@@ -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 ;
drivers/i2c/chips/fscher.c renamed from drivers/i2c/chips.orig/fscher.c +11 -7
@@ -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 ; }
drivers/i2c/chips/gl518sm.c renamed from drivers/i2c/chips.orig/gl518sm.c +11 -6
@@ -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 ;
drivers/i2c/chips/it87.c renamed from drivers/i2c/chips.orig/it87.c +20 -13
@@ -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 ;
drivers/i2c/chips/lm75.c renamed from drivers/i2c/chips.orig/lm75.c +11 -6
@@ -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 ; }
drivers/i2c/chips/lm78.c renamed from drivers/i2c/chips.orig/lm78.c +11 -6
@@ -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 ;
drivers/i2c/chips/lm80.c renamed from drivers/i2c/chips.orig/lm80.c +11 -5
@@ -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 ; }
drivers/i2c/chips/lm83.c renamed from drivers/i2c/chips.orig/lm83.c +11 -7
@@ -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 ; }
drivers/i2c/chips/lm85.c renamed from drivers/i2c/chips.orig/lm85.c +11 -6
@@ -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 ; }
drivers/i2c/chips/lm90.c renamed from drivers/i2c/chips.orig/lm90.c +11 -7
@@ -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 ; }
drivers/i2c/chips/pcf8574.c renamed from drivers/i2c/chips.orig/pcf8574.c +10 -6
@@ -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 ; }
drivers/i2c/chips/via686a.c renamed from drivers/i2c/chips.orig/via686a.c +12 -7
@@ -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 ;
drivers/i2c/chips/w83627hf.c renamed from drivers/i2c/chips.orig/w83627hf.c +10 -6
@@ -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 ;
drivers/i2c/chips/w83781d.c renamed from drivers/i2c/chips.orig/w83781d.c +10 -5
@@ -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 ;
drivers/i2c/chips/w83l785ts.c renamed from drivers/i2c/chips.orig/w83l785ts.c +10 -7
@@ -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 ; }
Signature · 2 lines
Jean Delvare http://www.ensicaen.ismra.fr/~delvare/
Quoted 5 lines · Jean Delvare
Some times ago, Ralf Roesch reported that the memory allocation scheme used in the i2c eeprom driver was causing trouble on MIPS architecture:
Quoted 7 lines
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
Hello!
Quoted 10 lines
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):
drivers/i2c/chips/lm75.c renamed from drivers/i2c/chips/lm75.c.sensors-kmalloc +6 -8
@@ -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 ; }
Signature · 1 line
Sergey Vlasov
Quoted 13 lines · Sergey Vlasov
Hello!
Quoted 10 lines
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.
Quoted 7 lines
Instead of splitting one kmalloc in two, it would also be possible to add a "struct
i2c_clientclient" field to each of the*_datastructures - 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
PATCH 1/1 Rework memory allocation in i2c chip drivers (second try)
Quoted 11 lines
Quoted 5 lines
Instead of splitting one kmalloc in two, it would also be possible to add a "struct
i2c_clientclient" field to each of the*_datastructures - 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_clientstructures 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.
drivers/i2c/chips/adm1021.c renamed from drivers/i2c/chips.orig/adm1021.c +6 -8
@@ -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 ; }
drivers/i2c/chips/asb100.c renamed from drivers/i2c/chips.orig/asb100.c +6 -8
@@ -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 ; }
drivers/i2c/chips/ds1621.c renamed from drivers/i2c/chips.orig/ds1621.c +8 -10
@@ -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 ; }
drivers/i2c/chips/eeprom.c renamed from drivers/i2c/chips.orig/eeprom.c +6 -8
@@ -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 ; }
drivers/i2c/chips/fscher.c renamed from drivers/i2c/chips.orig/fscher.c +8 -9
@@ -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 ; }
drivers/i2c/chips/gl518sm.c renamed from drivers/i2c/chips.orig/gl518sm.c +6 -8
@@ -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 ; }
drivers/i2c/chips/it87.c renamed from drivers/i2c/chips.orig/it87.c +7 -9
@@ -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 ; }
drivers/i2c/chips/lm75.c renamed from drivers/i2c/chips.orig/lm75.c +6 -8
@@ -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 ; }
drivers/i2c/chips/lm78.c renamed from drivers/i2c/chips.orig/lm78.c +6 -8
@@ -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 ; }
drivers/i2c/chips/lm80.c renamed from drivers/i2c/chips.orig/lm80.c +6 -7
@@ -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 ; }
drivers/i2c/chips/lm83.c renamed from drivers/i2c/chips.orig/lm83.c +8 -9
@@ -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 ; }
drivers/i2c/chips/lm85.c renamed from drivers/i2c/chips.orig/lm85.c +6 -8
@@ -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 ; }
drivers/i2c/chips/lm90.c renamed from drivers/i2c/chips.orig/lm90.c +8 -9
@@ -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 ; }
drivers/i2c/chips/pcf8574.c renamed from drivers/i2c/chips.orig/pcf8574.c +6 -9
@@ -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 ; }
drivers/i2c/chips/pcf8591.c renamed from drivers/i2c/chips.orig/pcf8591.c +6 -9
@@ -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 ; }
drivers/i2c/chips/via686a.c renamed from drivers/i2c/chips.orig/via686a.c +6 -10
@@ -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 ; }
drivers/i2c/chips/w83627hf.c renamed from drivers/i2c/chips.orig/w83627hf.c +6 -9
@@ -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 ; }
drivers/i2c/chips/w83781d.c renamed from drivers/i2c/chips.orig/w83781d.c +6 -8
@@ -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 ; }
drivers/i2c/chips/w83l785ts.c renamed from drivers/i2c/chips.orig/w83l785ts.c +8 -10
@@ -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 ; }
Signature · 2 lines
Jean Delvare http://www.ensicaen.ismra.fr/~delvare/
Re: [PATCH 2.6] Rework memory allocation in i2c chip drivers (second try)
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Instead of splitting one kmalloc in two, it would also be possible to add a "struct
i2c_clientclient" field to each of the*_datastructures - 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).
Quoted 7 lines
Quoted 5 lines
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_clientstructures 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.
Signature · 2 lines
Jean Delvare http://www.ensicaen.ismra.fr/~delvare/
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.
Signature · 2 lines
Jean Delvare http://www.ensicaen.ismra.fr/~delvare/
PATCH 1/1 Fix memory leaks in w83781d and asb100
In reply to Jean DelvareQuoting myself
Quoted 9 lines
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 could test and actually saw memory leaking when cycling the w83781d driver at a sustained rate (5/s).
Quoted 10 lines
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*.
I ended up implementing 1*. That's cleaner and there's actually almost no extra code.
Mark, can you confirm that I'm doing the correct thing? I'll do something similar in our CVS repository (for now, the asb100 and w83781d drivers had not their memory allocation scheme reworked there).
Greg, please apply if it looks good to you. Sorry for introducing the leak in the first place...
Thanks.
drivers/i2c/chips/asb100.c renamed from drivers/i2c/chips/asb100.c.orig +7 -1
@@ -855,7 +855,13 @@return err; } -kfree (i2c_get_clientdata (client)); +if (i2c_get_clientdata (client)==NULL ) { +/* subclients */ +kfree (client); + }else { +/* main client */ +kfree (i2c_get_clientdata (client)); + }return 0 ; }
drivers/i2c/chips/w83781d.c renamed from drivers/i2c/chips/w83781d.c.orig +7 -1
@@ -1336,7 +1336,13 @@return err; } -kfree (i2c_get_clientdata (client)); +if (i2c_get_clientdata (client)==NULL ) { +/* subclients */ +kfree (client); + }else { +/* main client */ +kfree (i2c_get_clientdata (client)); + }return 0 ; }
Signature · 2 lines
Jean Delvare http://khali.linux-fr.org/
Greg KH
AppliedRe: [PATCH 2.6] Fix memory leaks in w83781d and asb100
Quoted 3 lines · Jean Delvare
Greg, please apply if it looks good to you. Sorry for introducing the leak in the first place...
Looks a bit odd, but ok.
Applied, thanks.
greg k-h
Re: [PATCH 2.6] Fix memory leaks in w83781d and asb100
Quoted 5 lines
Quoted 3 lines
Greg, please apply if it looks good to you. Sorry for introducing the leak in the first place...
Looks a bit odd, but ok.
Odd? Why that? If you can think of something better, just tell me. The only other way I could think of was faking subclient's private data so that it points to the i2c_client structure itself. That way, freeing i2c_get_clientdata(client) would always work. However, I was fearing some side effect. Maybe the code makes use of the NULL data for remembering which clients are subclients at some point? I think I remember that the 2.4 drivers did, but maybe the 2.6 ones don't. I can take another look and propose that different solution if you want.
Thanks.
Re: [PATCH 2.6] Fix memory leaks in w83781d and asb100
Quoted 7 lines · Jean Delvare
Quoted 5 lines
Quoted 3 lines
Greg, please apply if it looks good to you. Sorry for introducing the leak in the first place...
Looks a bit odd, but ok.
Odd? Why that? If you can think of something better, just tell me.
No, I couldn't, it's just strange looking code :)
thanks,
greg k-h