i2c-drivers: Remove dangling pointers

i2c-drivers: Remove dangling pointers

[Wolfram Sang]

Hi,

here is a patch series fixing I2C drivers which free their private data
structure and leave the clientdata pointer dangling or do the cleanup in the
wrong order. All occurences were found using coccinelle [1] and the semantic
patch below. The matches have been reviewed, and sometimes adapted.

The complete series goes to linux-i2c and kernel-janitors. The rest of the CCs
is handled per patch using get_maintainers.pl (which always adds LKML, so you
have the complete series there, too).

Please review, comment, apply!

Kind regards,

   Wolfram

[1] http://coccinelle.lip6.fr/

===

@@
type T;
identifier client, data;
@@

// Check if function uses clientdata
(
	i2c_set_clientdata(client, data);
|
	data = i2c_get_clientdata(client);
|
	T data = i2c_get_clientdata(client);
)
// Anything inbetween
	...
// Check if clientdata gets NULLed before data is freed
(
	i2c_set_clientdata(client, NULL);
	...
 	kfree(data);
|
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
	...
-	i2c_set_clientdata(client, NULL);
|
+	i2c_set_clientdata(client, NULL);
? 	kfree(data);
)

===

 drivers/gpio/adp5588-gpio.c                   |    1 +
 drivers/gpio/max732x.c                        |    1 +
 drivers/gpio/pca953x.c                        |    1 +
 drivers/gpio/pcf857x.c                        |    7 ++++---
 drivers/hwmon/ad7414.c                        |    2 ++
 drivers/hwmon/ad7418.c                        |    2 ++
 drivers/hwmon/adm1021.c                       |    2 ++
 drivers/hwmon/adm1025.c                       |    2 ++
 drivers/hwmon/adm1026.c                       |    2 ++
 drivers/hwmon/adm1029.c                       |    2 ++
 drivers/hwmon/adm1031.c                       |    2 ++
 drivers/hwmon/adm9240.c                       |    2 ++
 drivers/hwmon/ads7828.c                       |    5 ++++-
 drivers/hwmon/adt7462.c                       |    2 ++
 drivers/hwmon/adt7470.c                       |    2 ++
 drivers/hwmon/adt7475.c                       |    2 ++
 drivers/hwmon/amc6821.c                       |    2 ++
 drivers/hwmon/asb100.c                        |    2 ++
 drivers/hwmon/atxp1.c                         |    2 ++
 drivers/hwmon/dme1737.c                       |    2 ++
 drivers/hwmon/ds1621.c                        |    2 ++
 drivers/hwmon/f75375s.c                       |    4 ++--
 drivers/hwmon/g760a.c                         |    4 ++--
 drivers/hwmon/gl518sm.c                       |    2 ++
 drivers/hwmon/gl520sm.c                       |    2 ++
 drivers/hwmon/lm63.c                          |    2 ++
 drivers/hwmon/lm77.c                          |    2 ++
 drivers/hwmon/lm78.c                          |    2 ++
 drivers/hwmon/lm80.c                          |    2 ++
 drivers/hwmon/lm83.c                          |    2 ++
 drivers/hwmon/lm85.c                          |    2 ++
 drivers/hwmon/lm87.c                          |    2 ++
 drivers/hwmon/lm90.c                          |    2 ++
 drivers/hwmon/lm92.c                          |    2 ++
 drivers/hwmon/lm93.c                          |    2 ++
 drivers/hwmon/lm95241.c                       |    1 +
 drivers/hwmon/ltc4215.c                       |    2 ++
 drivers/hwmon/ltc4245.c                       |    2 ++
 drivers/hwmon/max1619.c                       |    2 ++
 drivers/hwmon/max6650.c                       |    2 ++
 drivers/hwmon/pcf8591.c                       |    6 +++++-
 drivers/hwmon/smsc47m192.c                    |    2 ++
 drivers/hwmon/thmc50.c                        |    2 ++
 drivers/hwmon/tmp401.c                        |    1 +
 drivers/hwmon/w83791d.c                       |    2 ++
 drivers/hwmon/w83792d.c                       |    2 ++
 drivers/hwmon/w83793.c                        |    1 +
 drivers/hwmon/w83l785ts.c                     |    2 ++
 drivers/hwmon/w83l786ng.c                     |    2 ++
 drivers/input/keyboard/lm8323.c               |    2 ++
 drivers/input/keyboard/qt2160.c               |    2 +-
 drivers/input/touchscreen/mcs5000_ts.c        |    2 +-
 drivers/input/touchscreen/tsc2007.c           |    1 +
 drivers/leds/leds-lp3944.c                    |    2 +-
 drivers/leds/leds-pca9532.c                   |    4 ++--
 drivers/leds/leds-pca955x.c                   |    4 ++--
 drivers/macintosh/therm_adt746x.c             |    2 ++
 drivers/media/radio/radio-tea5764.c           |    2 ++
 drivers/media/radio/si470x/radio-si470x-i2c.c |    2 +-
 drivers/media/video/cs5345.c                  |    1 +
 drivers/media/video/cs53l32a.c                |    1 +
 drivers/media/video/ir-kbd-i2c.c              |    2 ++
 drivers/media/video/tda9840.c                 |    1 +
 drivers/media/video/tea6415c.c                |    1 +
 drivers/media/video/tea6420.c                 |    1 +
 drivers/media/video/ths7303.c                 |    1 +
 drivers/mfd/88pm860x-i2c.c                    |    1 +
 drivers/mfd/ab3100-core.c                     |    2 ++
 drivers/mfd/da903x.c                          |    1 +
 drivers/mfd/menelaus.c                        |    3 ++-
 drivers/mfd/pcf50633-core.c                   |    1 +
 drivers/mfd/tps65010.c                        |    2 +-
 drivers/mfd/wm8350-i2c.c                      |    2 ++
 drivers/misc/ad525x_dpot.c                    |    2 +-
 drivers/misc/eeprom/at24.c                    |    2 +-
 drivers/misc/eeprom/eeprom.c                  |    6 +++++-
 drivers/misc/eeprom/max6875.c                 |    2 ++
 drivers/misc/ics932s401.c                     |    2 ++
 drivers/misc/isl29003.c                       |    6 +++++-
 drivers/misc/tsl2550.c                        |    6 +++++-
 drivers/mtd/maps/pismo.c                      |    8 +++++++-
 drivers/power/bq27x00_battery.c               |    2 ++
 drivers/power/ds2782_battery.c                |    4 ++--
 drivers/regulator/max1586.c                   |    2 +-
 drivers/regulator/max8649.c                   |    3 ++-
 drivers/regulator/max8660.c                   |    2 +-
 drivers/rtc/rtc-ds1307.c                      |    2 ++
 drivers/rtc/rtc-fm3130.c                      |    2 ++
 drivers/rtc/rtc-m41t80.c                      |    2 ++
 drivers/rtc/rtc-pcf8563.c                     |    2 ++
 drivers/rtc/rtc-pcf8583.c                     |    2 ++
 drivers/rtc/rtc-rs5c372.c                     |    2 ++
 drivers/rtc/rtc-s35390a.c                     |    4 ++--
 drivers/staging/dream/synaptics_i2c_rmi.c     |    2 ++
 drivers/staging/go7007/wis-saa7113.c          |    1 +
 drivers/staging/go7007/wis-saa7115.c          |    1 +
 drivers/staging/go7007/wis-tw9903.c           |    1 +
 drivers/staging/iio/adc/max1363_core.c        |    2 ++
 drivers/staging/iio/light/tsl2563.c           |    2 ++
 drivers/usb/otg/isp1301_omap.c                |    5 ++++-
 drivers/video/matrox/matroxfb_maven.c         |    1 +
 drivers/w1/masters/ds2482.c                   |    2 ++
 102 files changed, 198 insertions(+), 33 deletions(-)

[PATCH 04/24] gpio: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/gpio/adp5588-gpio.c |    1 +
 drivers/gpio/max732x.c      |    1 +
 drivers/gpio/pca953x.c      |    1 +
 drivers/gpio/pcf857x.c      |    7 ++++---
 4 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/adp5588-gpio.c b/drivers/gpio/adp5588-gpio.c
index afc097a..c0cb5c1 100644
--- a/drivers/gpio/adp5588-gpio.c
+++ b/drivers/gpio/adp5588-gpio.c
@@ -227,6 +227,7 @@ static int __devexit adp5588_gpio_remove(struct i2c_client *client)
 		return ret;
 	}
 
+	i2c_set_clientdata(client, NULL);
 	kfree(dev);
 	return 0;
 }
diff --git a/drivers/gpio/max732x.c b/drivers/gpio/max732x.c
index f786824..85fe675 100644
--- a/drivers/gpio/max732x.c
+++ b/drivers/gpio/max732x.c
@@ -356,6 +356,7 @@ static int __devexit max732x_remove(struct i2c_client *client)
 	if (chip->client_dummy)
 		i2c_unregister_device(chip->client_dummy);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(chip);
 	return 0;
 }
diff --git a/drivers/gpio/pca953x.c b/drivers/gpio/pca953x.c
index ab5daab..05242dc 100644
--- a/drivers/gpio/pca953x.c
+++ b/drivers/gpio/pca953x.c
@@ -571,6 +571,7 @@ static int pca953x_remove(struct i2c_client *client)
 
 	pca953x_irq_teardown(chip);
 	kfree(chip->dyn_pdata);
+	i2c_set_clientdata(client, NULL);
 	kfree(chip);
 	return 0;
 }
diff --git a/drivers/gpio/pcf857x.c b/drivers/gpio/pcf857x.c
index 29f19ce..c2aed8a 100644
--- a/drivers/gpio/pcf857x.c
+++ b/drivers/gpio/pcf857x.c
@@ -259,9 +259,7 @@ static int pcf857x_probe(struct i2c_client *client,
 		goto fail;
 
 	gpio->chip.label = client->name;
-
 	gpio->client = client;
-	i2c_set_clientdata(client, gpio);
 
 	/* NOTE:  these chips have strange "quasi-bidirectional" I/O pins.
 	 * We can't actually know whether a pin is configured (a) as output
@@ -307,6 +305,7 @@ static int pcf857x_probe(struct i2c_client *client,
 			dev_warn(&client->dev, "setup --> %d\n", status);
 	}
 
+	i2c_set_clientdata(client, gpio);
 	return 0;
 
 fail:
@@ -334,8 +333,10 @@ static int pcf857x_remove(struct i2c_client *client)
 	}
 
 	status = gpiochip_remove(&gpio->chip);
-	if (status == 0)
+	if (status == 0) {
+		i2c_set_clientdata(client, NULL);
 		kfree(gpio);
+	}
 	else
 		dev_err(&client->dev, "%s --> %d\n", "remove", status);
 	return status;
-- 
1.7.0

Re: [PATCH 04/24] gpio: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]

On Sat, Mar 20, 2010 at 03:12:45PM +0100, Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

please drop this this patch. We reached an agreement that a) setting the
clientdata-pointer to NULL should be done in the i2c-core [1] and b) how to do
it. Based on that, I will do the modification of the i2c-core for 2.6.34 (as it
fixes the dangling pointers) and then create one single patch removing the then
superflous calls to i2c_set_clientdata for 2.6.35 (as it is a cleanup). If you
already applied the above patch to your branch, you don't have to revert; we
will fix it for you in the next round.

Sorry for the detour!

Kind regards,

   Wolfram

[1] http://thread.gmane.org/gmane.linux.drivers.i2c/5674/focus=5729

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 06/24] input/keyboard: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/input/keyboard/lm8323.c |    2 ++
 drivers/input/keyboard/qt2160.c |    2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/input/keyboard/lm8323.c b/drivers/input/keyboard/lm8323.c
index 574eda2..2b8bf05 100644
--- a/drivers/input/keyboard/lm8323.c
+++ b/drivers/input/keyboard/lm8323.c
@@ -768,6 +768,7 @@ fail2:
 			led_classdev_unregister(&lm->pwm[i].cdev);
 fail1:
 	input_free_device(idev);
+	i2c_set_clientdata(client, NULL);
 	kfree(lm);
 	return err;
 }
@@ -789,6 +790,7 @@ static int __devexit lm8323_remove(struct i2c_client *client)
 		if (lm->pwm[i].enabled)
 			led_classdev_unregister(&lm->pwm[i].cdev);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(lm);
 
 	return 0;
diff --git a/drivers/input/keyboard/qt2160.c b/drivers/input/keyboard/qt2160.c
index 31f3008..43db5e9 100644
--- a/drivers/input/keyboard/qt2160.c
+++ b/drivers/input/keyboard/qt2160.c
@@ -356,9 +356,9 @@ static int __devexit qt2160_remove(struct i2c_client *client)
 	cancel_delayed_work_sync(&qt2160->dwork);
 
 	input_unregister_device(qt2160->input);
+	i2c_set_clientdata(client, NULL);
 	kfree(qt2160);
 
-	i2c_set_clientdata(client, NULL);
 	return 0;
 }
 
-- 
1.7.0

Re: [PATCH 06/24] input/keyboard: fix dangling pointers

[Dmitry Torokhov]

Hi Wolfram,

On Sat, Mar 20, 2010 at 03:12:47PM +0100, Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.
> 

I am not sure if setting clientdata to NULL before freeing the data is
that important; we really want to be sure that we don't leave clientdata
dangling when we finish unbinding the driver. If there are another
thread the change will not really help the problem of accessing
non-existing client data.

I will apply lm8323 portion of the patch but leave qt2160 as is.

Thanks.

-- 
Dmitry

Re: [PATCH 06/24] input/keyboard: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

Hello Dmitry,

On Sat, Mar 20, 2010 at 12:20:07PM -0700, Dmitry Torokhov wrote:
> Hi Wolfram,
> 
> On Sat, Mar 20, 2010 at 03:12:47PM +0100, Wolfram Sang wrote:
> > Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> > structure it points to. Also fix drivers which do this _after_ the structure
> > was freed already.
> > 
> 
> I am not sure if setting clientdata to NULL before freeing the data is
> that important; we really want to be sure that we don't leave clientdata
> dangling when we finish unbinding the driver. If there are another
> thread the change will not really help the problem of accessing
> non-existing client data.

I agree it won't matter in practice. Jean and I concluded it is better
style, though (http://comments.gmane.org/gmane.linux.drivers.i2c/5392).
Still, I will not insist and leave it up the subsystem maintainers, of
course. I also won't check for that case again in the future.

> I will apply lm8323 portion of the patch but leave qt2160 as is.

Thanks!

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 06/24] input/keyboard: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]

Hi Dmitry,

> On Sat, Mar 20, 2010 at 03:12:47PM +0100, Wolfram Sang wrote:
> > Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> > structure it points to. Also fix drivers which do this _after_ the structure
> > was freed already.
> > 
> 
> I am not sure if setting clientdata to NULL before freeing the data is
> that important; we really want to be sure that we don't leave clientdata
> dangling when we finish unbinding the driver. If there are another
> thread the change will not really help the problem of accessing
> non-existing client data.
> 
> I will apply lm8323 portion of the patch but leave qt2160 as is.

We reached an agreement that a) setting the clientdata-pointer to NULL should
be done in the i2c-core [1] and b) how to do it. Based on that, I will do the
modification of the i2c-core for 2.6.34 (as it fixes the dangling pointers) and
then create one single patch removing the then superflous calls to
i2c_set_clientdata for 2.6.35 (as it is a cleanup). As you already applied the
above patch to your branch: you don't have to revert, we will fix it for you in
the next round.

Sorry for the detour!

Kind regards,

   Wolfram

[1] http://thread.gmane.org/gmane.linux.drivers.i2c/5674/focus=5729

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 08/24] leds: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: Riku Voipio <riku.voipio@iki.fi>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/leds/leds-lp3944.c  |    2 +-
 drivers/leds/leds-pca9532.c |    4 ++--
 drivers/leds/leds-pca955x.c |    4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/leds-lp3944.c b/drivers/leds/leds-lp3944.c
index 5946208..7dca8f2 100644
--- a/drivers/leds/leds-lp3944.c
+++ b/drivers/leds/leds-lp3944.c
@@ -425,8 +425,8 @@ static int __devexit lp3944_remove(struct i2c_client *client)
 			break;
 		}
 
-	kfree(data);
 	i2c_set_clientdata(client, NULL);
+	kfree(data);
 
 	return 0;
 }
diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index adc561e..0f1f8f9 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -320,8 +320,8 @@ static int pca9532_probe(struct i2c_client *client,
 
 	err = pca9532_configure(client, data, pca9532_pdata);
 	if (err) {
-		kfree(data);
 		i2c_set_clientdata(client, NULL);
+		kfree(data);
 	}
 
 	return err;
@@ -349,8 +349,8 @@ static int pca9532_remove(struct i2c_client *client)
 			break;
 		}
 
-	kfree(data);
 	i2c_set_clientdata(client, NULL);
+	kfree(data);
 	return 0;
 }
 
diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 4e2d1a4..279fc31 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -340,8 +340,8 @@ exit:
 		cancel_work_sync(&pca955x[i].work);
 	}
 
-	kfree(pca955x);
 	i2c_set_clientdata(client, NULL);
+	kfree(pca955x);
 
 	return err;
 }
@@ -356,8 +356,8 @@ static int __devexit pca955x_remove(struct i2c_client *client)
 		cancel_work_sync(&pca955x[i].work);
 	}
 
-	kfree(pca955x);
 	i2c_set_clientdata(client, NULL);
+	kfree(pca955x);
 
 	return 0;
 }
-- 
1.7.0

Re: [PATCH 08/24] leds: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]

On Sat, Mar 20, 2010 at 03:12:49PM +0100, Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org>
> Cc: Riku Voipio <riku.voipio-X3B1VOXEql0@public.gmane.org>

please drop this this patch. We reached an agreement that a) setting the
clientdata-pointer to NULL should be done in the i2c-core [1] and b) how to do
it. Based on that, I will do the modification of the i2c-core for 2.6.34 (as it
fixes the dangling pointers) and then create one single patch removing the then
superflous calls to i2c_set_clientdata for 2.6.35 (as it is a cleanup). If you
already applied the above patch to your branch, you don't have to revert; we
will fix it for you in the next round.

Sorry for the detour!

Kind regards,

   Wolfram

[1] http://thread.gmane.org/gmane.linux.drivers.i2c/5674/focus=5729

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 09/24] macintosh: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Colin Leroy <colin@colino.net>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/macintosh/therm_adt746x.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/macintosh/therm_adt746x.c b/drivers/macintosh/therm_adt746x.c
index c42eeb4..16d82f1 100644
--- a/drivers/macintosh/therm_adt746x.c
+++ b/drivers/macintosh/therm_adt746x.c
@@ -182,6 +182,7 @@ remove_thermostat(struct i2c_client *client)
 
 	thermostat = NULL;
 
+	i2c_set_clientdata(client, NULL);
 	kfree(th);
 
 	return 0;
@@ -399,6 +400,7 @@ static int probe_thermostat(struct i2c_client *client,
 	rc = read_reg(th, CONFIG_REG);
 	if (rc < 0) {
 		dev_err(&client->dev, "Thermostat failed to read config!\n");
+		i2c_set_clientdata(client, NULL);
 		kfree(th);
 		return -ENODEV;
 	}
-- 
1.7.0

[PATCH 10/24] media/radio: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/media/radio/radio-tea5764.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/media/radio/radio-tea5764.c b/drivers/media/radio/radio-tea5764.c
index 8e718bf..8a6be0a 100644
--- a/drivers/media/radio/radio-tea5764.c
+++ b/drivers/media/radio/radio-tea5764.c
@@ -571,6 +571,7 @@ static int __devinit tea5764_i2c_probe(struct i2c_client *client,
 	return 0;
 errrel:
 	video_device_release(radio->videodev);
+	i2c_set_clientdata(client, NULL);
 errfr:
 	kfree(radio);
 	return ret;
@@ -584,6 +585,7 @@ static int __devexit tea5764_i2c_remove(struct i2c_client *client)
 	if (radio) {
 		tea5764_power_down(radio);
 		video_unregister_device(radio->videodev);
+		i2c_set_clientdata(client, NULL);
 		kfree(radio);
 	}
 	return 0;
-- 
1.7.0

[PATCH 12/24] media/video: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/media/video/cs5345.c     |    1 +
 drivers/media/video/cs53l32a.c   |    1 +
 drivers/media/video/ir-kbd-i2c.c |    2 ++
 drivers/media/video/tda9840.c    |    1 +
 drivers/media/video/tea6415c.c   |    1 +
 drivers/media/video/tea6420.c    |    1 +
 drivers/media/video/ths7303.c    |    1 +
 7 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/cs5345.c b/drivers/media/video/cs5345.c
index 57dc170..cd21aa8 100644
--- a/drivers/media/video/cs5345.c
+++ b/drivers/media/video/cs5345.c
@@ -196,6 +196,7 @@ static int cs5345_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 
 	v4l2_device_unregister_subdev(sd);
+	i2c_set_clientdata(client, NULL);
 	kfree(sd);
 	return 0;
 }
diff --git a/drivers/media/video/cs53l32a.c b/drivers/media/video/cs53l32a.c
index 80bca8d..527f731 100644
--- a/drivers/media/video/cs53l32a.c
+++ b/drivers/media/video/cs53l32a.c
@@ -199,6 +199,7 @@ static int cs53l32a_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 
 	v4l2_device_unregister_subdev(sd);
+	i2c_set_clientdata(client, NULL);
 	kfree(sd);
 	return 0;
 }
diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
index da18d69..f29c5cd 100644
--- a/drivers/media/video/ir-kbd-i2c.c
+++ b/drivers/media/video/ir-kbd-i2c.c
@@ -461,6 +461,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	return 0;
 
  err_out_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(ir);
 	return err;
 }
@@ -476,6 +477,7 @@ static int ir_remove(struct i2c_client *client)
 	ir_input_unregister(ir->input);
 
 	/* free memory */
+	i2c_set_clientdata(client, NULL);
 	kfree(ir);
 	return 0;
 }
diff --git a/drivers/media/video/tda9840.c b/drivers/media/video/tda9840.c
index d381fce..b608aaf 100644
--- a/drivers/media/video/tda9840.c
+++ b/drivers/media/video/tda9840.c
@@ -188,6 +188,7 @@ static int tda9840_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 
 	v4l2_device_unregister_subdev(sd);
+	i2c_set_clientdata(client, NULL);
 	kfree(sd);
 	return 0;
 }
diff --git a/drivers/media/video/tea6415c.c b/drivers/media/video/tea6415c.c
index 1585839..49a6606 100644
--- a/drivers/media/video/tea6415c.c
+++ b/drivers/media/video/tea6415c.c
@@ -164,6 +164,7 @@ static int tea6415c_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 
 	v4l2_device_unregister_subdev(sd);
+	i2c_set_clientdata(client, NULL);
 	kfree(sd);
 	return 0;
 }
diff --git a/drivers/media/video/tea6420.c b/drivers/media/video/tea6420.c
index 6bf6bc7..821085d 100644
--- a/drivers/media/video/tea6420.c
+++ b/drivers/media/video/tea6420.c
@@ -146,6 +146,7 @@ static int tea6420_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 
 	v4l2_device_unregister_subdev(sd);
+	i2c_set_clientdata(client, NULL);
 	kfree(sd);
 	return 0;
 }
diff --git a/drivers/media/video/ths7303.c b/drivers/media/video/ths7303.c
index 21781f8..d411a73 100644
--- a/drivers/media/video/ths7303.c
+++ b/drivers/media/video/ths7303.c
@@ -114,6 +114,7 @@ static int ths7303_remove(struct i2c_client *client)
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
 
 	v4l2_device_unregister_subdev(sd);
+	i2c_set_clientdata(client, NULL);
 	kfree(sd);
 
 	return 0;
-- 
1.7.0

Re: [PATCH 12/24] media/video: fix dangling pointers

[Hans Verkuil]

On Saturday 20 March 2010 15:12:53 Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.

I feel I am missing something here. Why does clientdata have to be set to
NULL when we are tearing down the device anyway?

And if there is a good reason for doing this, then it should be done in
v4l2_device_unregister_subdev or even in the i2c core, not in each drivers.

And why does coccinelle apparently find this in e.g. cs5345.c but not in
saa7115.c, which has exactly the same construct? For that matter, I think
almost all v4l i2c drivers do this.

Regards,

	Hans

> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
> ---
> 
> Found using coccinelle, then reviewed. Full patchset is available via
> kernel-janitors, linux-i2c, and LKML.
> ---
>  drivers/media/video/cs5345.c     |    1 +
>  drivers/media/video/cs53l32a.c   |    1 +
>  drivers/media/video/ir-kbd-i2c.c |    2 ++
>  drivers/media/video/tda9840.c    |    1 +
>  drivers/media/video/tea6415c.c   |    1 +
>  drivers/media/video/tea6420.c    |    1 +
>  drivers/media/video/ths7303.c    |    1 +
>  7 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/cs5345.c b/drivers/media/video/cs5345.c
> index 57dc170..cd21aa8 100644
> --- a/drivers/media/video/cs5345.c
> +++ b/drivers/media/video/cs5345.c
> @@ -196,6 +196,7 @@ static int cs5345_remove(struct i2c_client *client)
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  
>  	v4l2_device_unregister_subdev(sd);
> +	i2c_set_clientdata(client, NULL);
>  	kfree(sd);
>  	return 0;
>  }
> diff --git a/drivers/media/video/cs53l32a.c b/drivers/media/video/cs53l32a.c
> index 80bca8d..527f731 100644
> --- a/drivers/media/video/cs53l32a.c
> +++ b/drivers/media/video/cs53l32a.c
> @@ -199,6 +199,7 @@ static int cs53l32a_remove(struct i2c_client *client)
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  
>  	v4l2_device_unregister_subdev(sd);
> +	i2c_set_clientdata(client, NULL);
>  	kfree(sd);
>  	return 0;
>  }
> diff --git a/drivers/media/video/ir-kbd-i2c.c b/drivers/media/video/ir-kbd-i2c.c
> index da18d69..f29c5cd 100644
> --- a/drivers/media/video/ir-kbd-i2c.c
> +++ b/drivers/media/video/ir-kbd-i2c.c
> @@ -461,6 +461,7 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	return 0;
>  
>   err_out_free:
> +	i2c_set_clientdata(client, NULL);
>  	kfree(ir);
>  	return err;
>  }
> @@ -476,6 +477,7 @@ static int ir_remove(struct i2c_client *client)
>  	ir_input_unregister(ir->input);
>  
>  	/* free memory */
> +	i2c_set_clientdata(client, NULL);
>  	kfree(ir);
>  	return 0;
>  }
> diff --git a/drivers/media/video/tda9840.c b/drivers/media/video/tda9840.c
> index d381fce..b608aaf 100644
> --- a/drivers/media/video/tda9840.c
> +++ b/drivers/media/video/tda9840.c
> @@ -188,6 +188,7 @@ static int tda9840_remove(struct i2c_client *client)
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  
>  	v4l2_device_unregister_subdev(sd);
> +	i2c_set_clientdata(client, NULL);
>  	kfree(sd);
>  	return 0;
>  }
> diff --git a/drivers/media/video/tea6415c.c b/drivers/media/video/tea6415c.c
> index 1585839..49a6606 100644
> --- a/drivers/media/video/tea6415c.c
> +++ b/drivers/media/video/tea6415c.c
> @@ -164,6 +164,7 @@ static int tea6415c_remove(struct i2c_client *client)
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  
>  	v4l2_device_unregister_subdev(sd);
> +	i2c_set_clientdata(client, NULL);
>  	kfree(sd);
>  	return 0;
>  }
> diff --git a/drivers/media/video/tea6420.c b/drivers/media/video/tea6420.c
> index 6bf6bc7..821085d 100644
> --- a/drivers/media/video/tea6420.c
> +++ b/drivers/media/video/tea6420.c
> @@ -146,6 +146,7 @@ static int tea6420_remove(struct i2c_client *client)
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  
>  	v4l2_device_unregister_subdev(sd);
> +	i2c_set_clientdata(client, NULL);
>  	kfree(sd);
>  	return 0;
>  }
> diff --git a/drivers/media/video/ths7303.c b/drivers/media/video/ths7303.c
> index 21781f8..d411a73 100644
> --- a/drivers/media/video/ths7303.c
> +++ b/drivers/media/video/ths7303.c
> @@ -114,6 +114,7 @@ static int ths7303_remove(struct i2c_client *client)
>  	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>  
>  	v4l2_device_unregister_subdev(sd);
> +	i2c_set_clientdata(client, NULL);
>  	kfree(sd);
>  
>  	return 0;
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH 12/24] media/video: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

Hello Hans,

> > Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> > structure it points to. Also fix drivers which do this _after_ the structure
> > was freed already.
> 
> I feel I am missing something here. Why does clientdata have to be set to
> NULL when we are tearing down the device anyway?
> 
> And if there is a good reason for doing this, then it should be done in
> v4l2_device_unregister_subdev or even in the i2c core, not in each drivers.

Discussion to take this into the i2c-layer has already started. Regarding V4L,
I noticed there is a v4l2_i2c_subdev_init() but no v4l2_i2c_subdev_exit(), so I
grepped what drivers are doing. There are some which set clientdata to NULL, so
I thought this was accepted in general.

> And why does coccinelle apparently find this in e.g. cs5345.c but not in
> saa7115.c, which has exactly the same construct? For that matter, I think

It was the to_state()-call inside kfree() which prevented the match. I would
need to extend my patch for V4L, it seems.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 12/24] media/video: fix dangling pointers

[Jean Delvare]

Hi Hans,

On Sat, 20 Mar 2010 23:02:49 +0100, Hans Verkuil wrote:
> On Saturday 20 March 2010 15:12:53 Wolfram Sang wrote:
> > Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> > structure it points to. Also fix drivers which do this _after_ the structure
> > was freed already.
> 
> I feel I am missing something here. Why does clientdata have to be set to
> NULL when we are tearing down the device anyway?

We're not tearing down the device, that's the point. We are only
unbinding it from its driver. The device itself survives the operation.

(This is different from the legacy i2c binding model where the device
itself would indeed be removed at that point, and that would be the
reason why so many i2c drivers have it wrong now.)

> And if there is a good reason for doing this, then it should be done in
> v4l2_device_unregister_subdev or even in the i2c core, not in each drivers.

Mark Brown (Cc'd) suggested this already, but I have mixed feelings
about this. My reasoning is:

1* It is good practice to have memory freed not too far from where it
was allocated, otherwise there is always a risk of unmatched pairs. In
this regard, it seems preferable to let each i2c driver kfree the
device memory it kalloc'd, be it in probe() or remove().

2* References to allocated memory should be dropped before that memory
is freed. This means that we want to call i2c_set_clientdata(c, NULL)
before kfree(d). As a corollary, we can't do the former in i2c-core and
the later in device drivers.

So we are in the difficult situation where we can't do both in i2c-core
because that violates point 1 above, we can't do half in i2c-core and
half in device drivers because this violates point 2 above, so we fall
back to doing both in device drivers, which doesn't violate any point
but duplicates the code all around.

Now, if we decide to handle this at a deeper driver model layer
(i2c-core instead of device drivers) then I'm not sure why we would
stop there... Wouldn't it be the driver core's job to clear the
reference and free the allocated memory?

I get the feeling that this would be a job for managed resources as
some drivers already do for I/O ports and IRQs. Managed resources don't
care about symmetry of allocation and freeing, by design (so it can
violate point 1 above.) Aha! Isn't it exactly what devm_kzalloc() is
all about?

At this point, I guess that each subsystem maintainer can decide to
either apply Wolfram's patch, or switch the drivers to managed
resources. Very nice that we don't have to make this a subsystem-wide
decision, so every actor can do the changes if/when they see fit.

> And why does coccinelle apparently find this in e.g. cs5345.c but not in
> saa7115.c, which has exactly the same construct? For that matter, I think
> almost all v4l i2c drivers do this.

That I can't say, sorry.

-- 
Jean Delvare

Re: [PATCH 12/24] media/video: fix dangling pointers

[Mark Brown]

On Sun, Mar 21, 2010 at 02:46:55PM +0100, Jean Delvare wrote:
> On Sat, 20 Mar 2010 23:02:49 +0100, Hans Verkuil wrote:

> > I feel I am missing something here. Why does clientdata have to be set to
> > NULL when we are tearing down the device anyway?

> We're not tearing down the device, that's the point. We are only
> unbinding it from its driver. The device itself survives the operation.

That's the subsystem point of view, not the driver point of view.  As
far as the driver is concerned the device appears when probe() is called
and vanishes after remove() has completed, any management the subsystem
does in between is up to it.

> 1* It is good practice to have memory freed not too far from where it
> was allocated, otherwise there is always a risk of unmatched pairs. In
> this regard, it seems preferable to let each i2c driver kfree the
> device memory it kalloc'd, be it in probe() or remove().

I agree with this.  There are also some use cases where the device data
is actually static (eg, a generic description of the device or a
reference to some other shared resource rather than per device allocated
data).

> 2* References to allocated memory should be dropped before that memory
> is freed. This means that we want to call i2c_set_clientdata(c, NULL)
> before kfree(d). As a corollary, we can't do the former in i2c-core and
> the later in device drivers.

This is where the mismatch between the subsystem view of the device
lifetime and the driver view of the device lifetime comes into play.
For the driver once the device is unregistered the device no longer
exists - if the driver tries to work with the device it's buggy.  This
means that to the driver returning from the remove() function is
dropping the reference to the data and there's no reason for the driver
to take any other action.

The device may hang around after the remove() has happened, but if the
device driver knows or cares about it then it's doing something wrong.
Similarly on probe() we can't assme anything about the pointer since
even if we saw the device before we can't guarantee that some other
driver didn't do so as well.  The situation is similar to that with
kfree() - we don't memset() data we're freeing with that, even though it
might contain pointers to other things.

> So we are in the difficult situation where we can't do both in i2c-core
> because that violates point 1 above, we can't do half in i2c-core and
> half in device drivers because this violates point 2 above, so we fall
> back to doing both in device drivers, which doesn't violate any point
> but duplicates the code all around.

Personally I'd much rather just not bother setting the driver data in
the removal path, it seems unneeded.  I had assumed that the subsystem
code cared for some reason when I saw the patch series.

Re: [PATCH 12/24] media/video: fix dangling pointers

[Hans Verkuil]

On Sunday 21 March 2010 15:14:17 Mark Brown wrote:
> On Sun, Mar 21, 2010 at 02:46:55PM +0100, Jean Delvare wrote:
> > On Sat, 20 Mar 2010 23:02:49 +0100, Hans Verkuil wrote:
> 
> > > I feel I am missing something here. Why does clientdata have to be set to
> > > NULL when we are tearing down the device anyway?
> 
> > We're not tearing down the device, that's the point. We are only
> > unbinding it from its driver. The device itself survives the operation.
> 
> That's the subsystem point of view, not the driver point of view.  As
> far as the driver is concerned the device appears when probe() is called
> and vanishes after remove() has completed, any management the subsystem
> does in between is up to it.

Right. And from the point of view of the driver I see no reason why I would
have to zero the client data pointer when I know nobody will ever access it
again. If there is a good reason, then that is (again, from the PoV of the
driver) completely unexpected and it is guaranteed that driver writers will
continue to make that mistake in the future.

> > 1* It is good practice to have memory freed not too far from where it
> > was allocated, otherwise there is always a risk of unmatched pairs. In
> > this regard, it seems preferable to let each i2c driver kfree the
> > device memory it kalloc'd, be it in probe() or remove().
> 
> I agree with this.  There are also some use cases where the device data
> is actually static (eg, a generic description of the device or a
> reference to some other shared resource rather than per device allocated
> data).

Definitely. Freeing should be done in the i2c drivers.
 
> > 2* References to allocated memory should be dropped before that memory
> > is freed. This means that we want to call i2c_set_clientdata(c, NULL)
> > before kfree(d). As a corollary, we can't do the former in i2c-core and
> > the later in device drivers.
> 
> This is where the mismatch between the subsystem view of the device
> lifetime and the driver view of the device lifetime comes into play.
> For the driver once the device is unregistered the device no longer
> exists - if the driver tries to work with the device it's buggy.  This
> means that to the driver returning from the remove() function is
> dropping the reference to the data and there's no reason for the driver
> to take any other action.
> 
> The device may hang around after the remove() has happened, but if the
> device driver knows or cares about it then it's doing something wrong.
> Similarly on probe() we can't assme anything about the pointer since
> even if we saw the device before we can't guarantee that some other
> driver didn't do so as well.  The situation is similar to that with
> kfree() - we don't memset() data we're freeing with that, even though it
> might contain pointers to other things.

Indeed. The client data is for the client. Once the client is gone (unbound)
the client data can safely be set back to NULL.
 
> > So we are in the difficult situation where we can't do both in i2c-core
> > because that violates point 1 above, we can't do half in i2c-core and
> > half in device drivers because this violates point 2 above, so we fall
> > back to doing both in device drivers, which doesn't violate any point
> > but duplicates the code all around.
> 
> Personally I'd much rather just not bother setting the driver data in
> the removal path, it seems unneeded.  I had assumed that the subsystem
> code cared for some reason when I saw the patch series.

Anyway, should this really be necessary, then for the media drivers this
should be done in v4l2_device_unregister_subdev() and not in every driver.

But this just feels like an i2c core thing to me. After remove() is called
the core should just set the client data to NULL. If there are drivers that
rely on the current behavior, then those drivers should be reviewed first as
to the reason why they need it.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH 12/24] media/video: fix dangling pointers

[Jean Delvare]

Hi Hans, Mark, Wolfram,

On Sun, 21 Mar 2010 17:09:56 +0100, Hans Verkuil wrote:
> On Sunday 21 March 2010 15:14:17 Mark Brown wrote:
> > On Sun, Mar 21, 2010 at 02:46:55PM +0100, Jean Delvare wrote:
> > > On Sat, 20 Mar 2010 23:02:49 +0100, Hans Verkuil wrote:
> > 
> > > > I feel I am missing something here. Why does clientdata have to be set to
> > > > NULL when we are tearing down the device anyway?
> > 
> > > We're not tearing down the device, that's the point. We are only
> > > unbinding it from its driver. The device itself survives the operation.
> > 
> > That's the subsystem point of view, not the driver point of view.  As
> > far as the driver is concerned the device appears when probe() is called
> > and vanishes after remove() has completed, any management the subsystem
> > does in between is up to it.
> 
> Right. And from the point of view of the driver I see no reason why I would
> have to zero the client data pointer when I know nobody will ever access it
> again. If there is a good reason, then that is (again, from the PoV of the
> driver) completely unexpected and it is guaranteed that driver writers will
> continue to make that mistake in the future.

I can't disagree. Given that there is no way to enforce the rule, it is
likely not everybody will follow it.

> > > 1* It is good practice to have memory freed not too far from where it
> > > was allocated, otherwise there is always a risk of unmatched pairs. In
> > > this regard, it seems preferable to let each i2c driver kfree the
> > > device memory it kalloc'd, be it in probe() or remove().
> > 
> > I agree with this.  There are also some use cases where the device data
> > is actually static (eg, a generic description of the device or a
> > reference to some other shared resource rather than per device allocated
> > data).

>From a technical perspective, there is little rationale to have the
client data pointed to static data. If you could reach it from probe(),
it has to be a global, and if it is a global, you can reach it again
directly from the rest of your code.

That being said, that doesn't mean drivers aren't actually doing it.

> Definitely. Freeing should be done in the i2c drivers.
>  
> > > 2* References to allocated memory should be dropped before that memory
> > > is freed. This means that we want to call i2c_set_clientdata(c, NULL)
> > > before kfree(d). As a corollary, we can't do the former in i2c-core and
> > > the later in device drivers.
> > 
> > This is where the mismatch between the subsystem view of the device
> > lifetime and the driver view of the device lifetime comes into play.
> > For the driver once the device is unregistered the device no longer
> > exists - if the driver tries to work with the device it's buggy.  This
> > means that to the driver returning from the remove() function is
> > dropping the reference to the data and there's no reason for the driver
> > to take any other action.
> > 
> > The device may hang around after the remove() has happened, but if the
> > device driver knows or cares about it then it's doing something wrong.
> > Similarly on probe() we can't assme anything about the pointer since
> > even if we saw the device before we can't guarantee that some other
> > driver didn't do so as well.  The situation is similar to that with
> > kfree() - we don't memset() data we're freeing with that, even though it
> > might contain pointers to other things.

I'm not sure if this comparison really holds. When you free memory,
nobody is supposed to use that memory afterwards, so whether it contains
pointers to other memory locations is irrelevant. Unbinding a device
doesn't make it disappear, it might still get access, now or later, so
the value of struct device members is more relevant. But of course we
can discuss the guarantees made on these struct members over the
device's lifetime. I don't this is documented anywhere.

> Indeed. The client data is for the client. Once the client is gone (unbound)
> the client data can safely be set back to NULL.
>  
> > > So we are in the difficult situation where we can't do both in i2c-core
> > > because that violates point 1 above, we can't do half in i2c-core and
> > > half in device drivers because this violates point 2 above, so we fall
> > > back to doing both in device drivers, which doesn't violate any point
> > > but duplicates the code all around.
> > 
> > Personally I'd much rather just not bother setting the driver data in
> > the removal path, it seems unneeded.  I had assumed that the subsystem
> > code cared for some reason when I saw the patch series.
> 
> Anyway, should this really be necessary, then for the media drivers this
> should be done in v4l2_device_unregister_subdev() and not in every driver.
> 
> But this just feels like an i2c core thing to me. After remove() is called
> the core should just set the client data to NULL. If there are drivers that
> rely on the current behavior, then those drivers should be reviewed first as
> to the reason why they need it.

I tend to agree, now.

Wolfram, how do you feel about this? I feel a little sorry that I more
or less encouraged you to submit this patch series, and now I get to
agree with the objections which were raised against it.

My key motivation was that I wanted i2c_set_clientdata() to be called
before kfree(). Now that everybody seems to agree that the latter
belongs to the drivers while the former belongs to lower layers
(i2c-core or even driver core), this is not going to happen. So I guess
we want to remove calls to i2c_set_clientdata(NULL) from all drivers
and have only one in i2c-core for now?

-- 
Jean Delvare

Re: [PATCH 12/24] media/video: fix dangling pointers

[Mark Brown]

On Mon, Mar 22, 2010 at 09:33:58PM +0100, Jean Delvare wrote:
> On Sun, 21 Mar 2010 17:09:56 +0100, Hans Verkuil wrote:
> > On Sunday 21 March 2010 15:14:17 Mark Brown wrote:

> > > I agree with this.  There are also some use cases where the device data
> > > is actually static (eg, a generic description of the device or a
> > > reference to some other shared resource rather than per device allocated
> > > data).

> From a technical perspective, there is little rationale to have the
> client data pointed to static data. If you could reach it from probe(),
> it has to be a global, and if it is a global, you can reach it again
> directly from the rest of your code.

The use case I can think of there is bus type specific stuff for devices
that support multiple buses.

Re: [PATCH 12/24] media/video: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1981 bytes --]


> > > Personally I'd much rather just not bother setting the driver data in
> > > the removal path, it seems unneeded.  I had assumed that the subsystem
> > > code cared for some reason when I saw the patch series.
> > 
> > Anyway, should this really be necessary, then for the media drivers this
> > should be done in v4l2_device_unregister_subdev() and not in every driver.
> > 
> > But this just feels like an i2c core thing to me. After remove() is called
> > the core should just set the client data to NULL. If there are drivers that
> > rely on the current behavior, then those drivers should be reviewed first as
> > to the reason why they need it.
> 
> I tend to agree, now.
> 
> Wolfram, how do you feel about this? I feel a little sorry that I more
> or less encouraged you to submit this patch series, and now I get to
> agree with the objections which were raised against it.

Well, this is a valuable outcome in my book. Maybe seeing the actual amount of
modifications necessary for cleaning up clientdata helped the process. While
working on it, I also got the impression that it should be handled differently
in the future, of course. Although I was thinking of something different
('i2c_(allocate|free)_clientdata' as mentioned before), I prefer the above
proposal as it is most simple.

> My key motivation was that I wanted i2c_set_clientdata() to be called
> before kfree(). Now that everybody seems to agree that the latter
> belongs to the drivers while the former belongs to lower layers
> (i2c-core or even driver core), this is not going to happen. So I guess
> we want to remove calls to i2c_set_clientdata(NULL) from all drivers
> and have only one in i2c-core for now?

Fine with me. Let me know if I can assist you with the series.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 12/24] media/video: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 622 bytes --]

Hans,

> But this just feels like an i2c core thing to me. After remove() is called
> the core should just set the client data to NULL. If there are drivers that
> rely on the current behavior, then those drivers should be reviewed first as
> to the reason why they need it.

It will be done this way now. As you have taken part in the discussion, I guess
the media-subsystem never really considered picking those patches up ;)

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 12/24] media/video: fix dangling pointers

[Hans Verkuil]

On Tuesday 30 March 2010 14:39:12 Wolfram Sang wrote:
> Hans,
> 
> > But this just feels like an i2c core thing to me. After remove() is called
> > the core should just set the client data to NULL. If there are drivers that
> > rely on the current behavior, then those drivers should be reviewed first as
> > to the reason why they need it.
> 
> It will be done this way now. As you have taken part in the discussion, I guess
> the media-subsystem never really considered picking those patches up ;)

I remember that there was one patch in your patch series where the client data
was set after it was freed.

That should still be fixed (by just removing the i2c_set_clientdata).

Can you post that one again?

Regards,

	Hans


> 
> Regards,
> 
>    Wolfram
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH 12/24] media/video: fix dangling pointers

[Jean Delvare]

Replying to myself...

On Sun, 21 Mar 2010 14:46:55 +0100, Jean Delvare wrote:
> I get the feeling that this would be a job for managed resources as
> some drivers already do for I/O ports and IRQs. Managed resources don't
> care about symmetry of allocation and freeing, by design (so it can
> violate point 1 above.) Aha! Isn't it exactly what devm_kzalloc() is
> all about?

Thinking about it again, this really only addresses the calls to
kfree(), not the calls to i2c_set_clientdata(), so apparently I'm quite
off-topic for this discussion. I still think that moving drivers to
managed resources is the way to go, but that's a different issue.

-- 
Jean Delvare

[PATCH 13/24] mfd: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/mfd/88pm860x-i2c.c  |    1 +
 drivers/mfd/ab3100-core.c   |    2 ++
 drivers/mfd/da903x.c        |    1 +
 drivers/mfd/menelaus.c      |    3 ++-
 drivers/mfd/pcf50633-core.c |    1 +
 drivers/mfd/tps65010.c      |    2 +-
 drivers/mfd/wm8350-i2c.c    |    2 ++
 7 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/88pm860x-i2c.c b/drivers/mfd/88pm860x-i2c.c
index c37e12b..aa81448 100644
--- a/drivers/mfd/88pm860x-i2c.c
+++ b/drivers/mfd/88pm860x-i2c.c
@@ -201,6 +201,7 @@ static int __devexit pm860x_remove(struct i2c_client *client)
 	i2c_unregister_device(chip->companion);
 	i2c_set_clientdata(chip->companion, NULL);
 	i2c_set_clientdata(chip->client, NULL);
+	i2c_set_clientdata(client, NULL);
 	kfree(chip);
 	return 0;
 }
diff --git a/drivers/mfd/ab3100-core.c b/drivers/mfd/ab3100-core.c
index a2ce3b6..e6fc43f 100644
--- a/drivers/mfd/ab3100-core.c
+++ b/drivers/mfd/ab3100-core.c
@@ -919,6 +919,7 @@ static int __init ab3100_probe(struct i2c_client *client,
 	i2c_unregister_device(ab3100->testreg_client);
  exit_no_testreg_client:
  exit_no_detect:
+	i2c_set_clientdata(client, NULL);
 	kfree(ab3100);
 	return err;
 }
@@ -940,6 +941,7 @@ static int __exit ab3100_remove(struct i2c_client *client)
 	 * their notifiers so deactivate IRQ
 	 */
 	free_irq(client->irq, ab3100);
+	i2c_set_clientdata(client, NULL);
 	kfree(ab3100);
 	return 0;
 }
diff --git a/drivers/mfd/da903x.c b/drivers/mfd/da903x.c
index e5ffe56..ec8178c 100644
--- a/drivers/mfd/da903x.c
+++ b/drivers/mfd/da903x.c
@@ -543,6 +543,7 @@ static int __devexit da903x_remove(struct i2c_client *client)
 	struct da903x_chip *chip = i2c_get_clientdata(client);
 
 	da903x_remove_subdevs(chip);
+	i2c_set_clientdata(client, NULL);
 	kfree(chip);
 	return 0;
 }
diff --git a/drivers/mfd/menelaus.c b/drivers/mfd/menelaus.c
index 970afa1..d9e60ba 100644
--- a/drivers/mfd/menelaus.c
+++ b/drivers/mfd/menelaus.c
@@ -1227,6 +1227,7 @@ fail2:
 	free_irq(client->irq, menelaus);
 	flush_scheduled_work();
 fail1:
+	i2c_set_clientdata(client, NULL);
 	kfree(menelaus);
 	return err;
 }
@@ -1236,8 +1237,8 @@ static int __exit menelaus_remove(struct i2c_client *client)
 	struct menelaus_chip	*menelaus = i2c_get_clientdata(client);
 
 	free_irq(client->irq, menelaus);
-	kfree(menelaus);
 	i2c_set_clientdata(client, NULL);
+	kfree(menelaus);
 	the_menelaus = NULL;
 	return 0;
 }
diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
index 03dcc92..ab7b4dd 100644
--- a/drivers/mfd/pcf50633-core.c
+++ b/drivers/mfd/pcf50633-core.c
@@ -675,6 +675,7 @@ static int __devexit pcf50633_remove(struct i2c_client *client)
 	for (i = 0; i < PCF50633_NUM_REGULATORS; i++)
 		platform_device_unregister(pcf->regulator_pdev[i]);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(pcf);
 
 	return 0;
diff --git a/drivers/mfd/tps65010.c b/drivers/mfd/tps65010.c
index e595530..9b22a77 100644
--- a/drivers/mfd/tps65010.c
+++ b/drivers/mfd/tps65010.c
@@ -530,8 +530,8 @@ static int __exit tps65010_remove(struct i2c_client *client)
 	cancel_delayed_work(&tps->work);
 	flush_scheduled_work();
 	debugfs_remove(tps->file);
-	kfree(tps);
 	i2c_set_clientdata(client, NULL);
+	kfree(tps);
 	the_tps = NULL;
 	return 0;
 }
diff --git a/drivers/mfd/wm8350-i2c.c b/drivers/mfd/wm8350-i2c.c
index 8d8c932..2dd3e8a 100644
--- a/drivers/mfd/wm8350-i2c.c
+++ b/drivers/mfd/wm8350-i2c.c
@@ -81,6 +81,7 @@ static int wm8350_i2c_probe(struct i2c_client *i2c,
 	return ret;
 
 err:
+	i2c_set_clientdata(i2c, NULL);
 	kfree(wm8350);
 	return ret;
 }
@@ -90,6 +91,7 @@ static int wm8350_i2c_remove(struct i2c_client *i2c)
 	struct wm8350 *wm8350 = i2c_get_clientdata(i2c);
 
 	wm8350_device_exit(wm8350);
+	i2c_set_clientdata(i2c, NULL);
 	kfree(wm8350);
 
 	return 0;
-- 
1.7.0

Re: [PATCH 13/24] mfd: fix dangling pointers

[Mark Brown]

On Sat, Mar 20, 2010 at 03:12:54PM +0100, Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

but it really does seem like this is something that the I2C core ought
to handle - the assignment to null is boiler plate code that's getting
added to the overwhelming majority of I2C devices in their teardown
path, it'd seem a lot more sensible for the core to just trash driver
data after the driver is unbound if it's important that this happens.

Re: [PATCH 13/24] mfd: fix dangling pointers

[Jean Delvare]

On Sat, 20 Mar 2010 17:22:41 +0000, Mark Brown wrote:
> On Sat, Mar 20, 2010 at 03:12:54PM +0100, Wolfram Sang wrote:
> > Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> > structure it points to. Also fix drivers which do this _after_ the structure
> > was freed already.
> 
> Acked-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
> 
> but it really does seem like this is something that the I2C core ought
> to handle - the assignment to null is boiler plate code that's getting
> added to the overwhelming majority of I2C devices in their teardown
> path, it'd seem a lot more sensible for the core to just trash driver
> data after the driver is unbound if it's important that this happens.

This is an interesting point. Do you know of any other subsystem doing
this? The core touching private data seems wrong to me, but I have to
agree that it would avoid a lot of duplication across drivers.

Can someone find a reasonable use case for client data surviving the
driver's .remove() call?

-- 
Jean Delvare

Re: [PATCH 13/24] mfd: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1059 bytes --]

Hello Mark,

> but it really does seem like this is something that the I2C core ought
> to handle - the assignment to null is boiler plate code that's getting
> added to the overwhelming majority of I2C devices in their teardown
> path, it'd seem a lot more sensible for the core to just trash driver
> data after the driver is unbound if it's important that this happens.

While working on this, I got the same impression that it should be handled at
core-level. I wondered if functions ala i2c_alloc_clientdata() and
i2c_free_clientdata() could help. But seeing the myriad ways of setting up
private data in those drivers, I thought this to be the next step after
cleaning up the status quo, because it surely needs discussion. I wanted to
write about all this in the introductory message but sadly forgot. Thanks for
starting the discussion, very welcome.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 13/24] mfd: fix dangling pointers

[Samuel Ortiz]

Hi Wolfram,

On Sat, Mar 20, 2010 at 03:12:54PM +0100, Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.
Patch applied, many thanks.

Cheers,
Samuel.


> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Samuel Ortiz <sameo-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Cc: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
> ---
> 
> Found using coccinelle, then reviewed. Full patchset is available via
> kernel-janitors, linux-i2c, and LKML.
> ---
>  drivers/mfd/88pm860x-i2c.c  |    1 +
>  drivers/mfd/ab3100-core.c   |    2 ++
>  drivers/mfd/da903x.c        |    1 +
>  drivers/mfd/menelaus.c      |    3 ++-
>  drivers/mfd/pcf50633-core.c |    1 +
>  drivers/mfd/tps65010.c      |    2 +-
>  drivers/mfd/wm8350-i2c.c    |    2 ++
>  7 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mfd/88pm860x-i2c.c b/drivers/mfd/88pm860x-i2c.c
> index c37e12b..aa81448 100644
> --- a/drivers/mfd/88pm860x-i2c.c
> +++ b/drivers/mfd/88pm860x-i2c.c
> @@ -201,6 +201,7 @@ static int __devexit pm860x_remove(struct i2c_client *client)
>  	i2c_unregister_device(chip->companion);
>  	i2c_set_clientdata(chip->companion, NULL);
>  	i2c_set_clientdata(chip->client, NULL);
> +	i2c_set_clientdata(client, NULL);
>  	kfree(chip);
>  	return 0;
>  }
> diff --git a/drivers/mfd/ab3100-core.c b/drivers/mfd/ab3100-core.c
> index a2ce3b6..e6fc43f 100644
> --- a/drivers/mfd/ab3100-core.c
> +++ b/drivers/mfd/ab3100-core.c
> @@ -919,6 +919,7 @@ static int __init ab3100_probe(struct i2c_client *client,
>  	i2c_unregister_device(ab3100->testreg_client);
>   exit_no_testreg_client:
>   exit_no_detect:
> +	i2c_set_clientdata(client, NULL);
>  	kfree(ab3100);
>  	return err;
>  }
> @@ -940,6 +941,7 @@ static int __exit ab3100_remove(struct i2c_client *client)
>  	 * their notifiers so deactivate IRQ
>  	 */
>  	free_irq(client->irq, ab3100);
> +	i2c_set_clientdata(client, NULL);
>  	kfree(ab3100);
>  	return 0;
>  }
> diff --git a/drivers/mfd/da903x.c b/drivers/mfd/da903x.c
> index e5ffe56..ec8178c 100644
> --- a/drivers/mfd/da903x.c
> +++ b/drivers/mfd/da903x.c
> @@ -543,6 +543,7 @@ static int __devexit da903x_remove(struct i2c_client *client)
>  	struct da903x_chip *chip = i2c_get_clientdata(client);
>  
>  	da903x_remove_subdevs(chip);
> +	i2c_set_clientdata(client, NULL);
>  	kfree(chip);
>  	return 0;
>  }
> diff --git a/drivers/mfd/menelaus.c b/drivers/mfd/menelaus.c
> index 970afa1..d9e60ba 100644
> --- a/drivers/mfd/menelaus.c
> +++ b/drivers/mfd/menelaus.c
> @@ -1227,6 +1227,7 @@ fail2:
>  	free_irq(client->irq, menelaus);
>  	flush_scheduled_work();
>  fail1:
> +	i2c_set_clientdata(client, NULL);
>  	kfree(menelaus);
>  	return err;
>  }
> @@ -1236,8 +1237,8 @@ static int __exit menelaus_remove(struct i2c_client *client)
>  	struct menelaus_chip	*menelaus = i2c_get_clientdata(client);
>  
>  	free_irq(client->irq, menelaus);
> -	kfree(menelaus);
>  	i2c_set_clientdata(client, NULL);
> +	kfree(menelaus);
>  	the_menelaus = NULL;
>  	return 0;
>  }
> diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
> index 03dcc92..ab7b4dd 100644
> --- a/drivers/mfd/pcf50633-core.c
> +++ b/drivers/mfd/pcf50633-core.c
> @@ -675,6 +675,7 @@ static int __devexit pcf50633_remove(struct i2c_client *client)
>  	for (i = 0; i < PCF50633_NUM_REGULATORS; i++)
>  		platform_device_unregister(pcf->regulator_pdev[i]);
>  
> +	i2c_set_clientdata(client, NULL);
>  	kfree(pcf);
>  
>  	return 0;
> diff --git a/drivers/mfd/tps65010.c b/drivers/mfd/tps65010.c
> index e595530..9b22a77 100644
> --- a/drivers/mfd/tps65010.c
> +++ b/drivers/mfd/tps65010.c
> @@ -530,8 +530,8 @@ static int __exit tps65010_remove(struct i2c_client *client)
>  	cancel_delayed_work(&tps->work);
>  	flush_scheduled_work();
>  	debugfs_remove(tps->file);
> -	kfree(tps);
>  	i2c_set_clientdata(client, NULL);
> +	kfree(tps);
>  	the_tps = NULL;
>  	return 0;
>  }
> diff --git a/drivers/mfd/wm8350-i2c.c b/drivers/mfd/wm8350-i2c.c
> index 8d8c932..2dd3e8a 100644
> --- a/drivers/mfd/wm8350-i2c.c
> +++ b/drivers/mfd/wm8350-i2c.c
> @@ -81,6 +81,7 @@ static int wm8350_i2c_probe(struct i2c_client *i2c,
>  	return ret;
>  
>  err:
> +	i2c_set_clientdata(i2c, NULL);
>  	kfree(wm8350);
>  	return ret;
>  }
> @@ -90,6 +91,7 @@ static int wm8350_i2c_remove(struct i2c_client *i2c)
>  	struct wm8350 *wm8350 = i2c_get_clientdata(i2c);
>  
>  	wm8350_device_exit(wm8350);
> +	i2c_set_clientdata(i2c, NULL);
>  	kfree(wm8350);
>  
>  	return 0;
> -- 
> 1.7.0
> 

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 13/24] mfd: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

On Thu, Mar 25, 2010 at 11:41:38AM +0100, Samuel Ortiz wrote:
> Hi Wolfram,
> 
> On Sat, Mar 20, 2010 at 03:12:54PM +0100, Wolfram Sang wrote:
> > Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> > structure it points to. Also fix drivers which do this _after_ the structure
> > was freed already.
> Patch applied, many thanks.

Thanks. Sadly some fixup will be needed:

We reached an agreement that a) setting the clientdata-pointer to NULL should
be done in the i2c-core [1] and b) how to do it. Based on that, I will do the
modification of the i2c-core for 2.6.34 (as it fixes the dangling pointers) and
then create one single patch removing the then superflous calls to
i2c_set_clientdata for 2.6.35 (as it is a cleanup). As you already applied the
above patch to your branch: you don't have to revert, we will fix it for you in
the next round.

Sorry for the detour!

Kind regards,

   Wolfram

[1] http://thread.gmane.org/gmane.linux.drivers.i2c/5674/focus=5729

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 14/24] misc: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/misc/ad525x_dpot.c |    2 +-
 drivers/misc/ics932s401.c  |    2 ++
 drivers/misc/isl29003.c    |    6 +++++-
 drivers/misc/tsl2550.c     |    6 +++++-
 4 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/ad525x_dpot.c b/drivers/misc/ad525x_dpot.c
index 30a59f2..3b6f437 100644
--- a/drivers/misc/ad525x_dpot.c
+++ b/drivers/misc/ad525x_dpot.c
@@ -567,8 +567,8 @@ static int ad525x_probe(struct i2c_client *client,
 	return 0;
 
 exit_free:
-	kfree(data);
 	i2c_set_clientdata(client, NULL);
+	kfree(data);
 exit:
 	dev_err(dev, "failed to create client\n");
 	return err;
diff --git a/drivers/misc/ics932s401.c b/drivers/misc/ics932s401.c
index 395a4ea..f60b7e7 100644
--- a/drivers/misc/ics932s401.c
+++ b/drivers/misc/ics932s401.c
@@ -465,6 +465,7 @@ static int ics932s401_probe(struct i2c_client *client,
 	return 0;
 
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -475,6 +476,7 @@ static int ics932s401_remove(struct i2c_client *client)
 	struct ics932s401_data *data = i2c_get_clientdata(client);
 
 	sysfs_remove_group(&client->dev.kobj, &data->attrs);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/misc/isl29003.c b/drivers/misc/isl29003.c
index a71e245..5bd4d8b 100644
--- a/drivers/misc/isl29003.c
+++ b/drivers/misc/isl29003.c
@@ -397,15 +397,19 @@ static int __devinit isl29003_probe(struct i2c_client *client,
 	return 0;
 
 exit_kfree:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return err;
 }
 
 static int __devexit isl29003_remove(struct i2c_client *client)
 {
+	struct isl29003_data *data = i2c_get_clientdata(client);
+
 	sysfs_remove_group(&client->dev.kobj, &isl29003_attr_group);
 	isl29003_set_power_state(client, 0);
-	kfree(i2c_get_clientdata(client));
+	i2c_set_clientdata(client, NULL);
+	kfree(data);
 	return 0;
 }
 
diff --git a/drivers/misc/tsl2550.c b/drivers/misc/tsl2550.c
index 483ae5f..230c326 100644
--- a/drivers/misc/tsl2550.c
+++ b/drivers/misc/tsl2550.c
@@ -400,6 +400,7 @@ static int __devinit tsl2550_probe(struct i2c_client *client,
 	return 0;
 
 exit_kfree:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -407,12 +408,15 @@ exit:
 
 static int __devexit tsl2550_remove(struct i2c_client *client)
 {
+	struct tsl2550_data *data = i2c_get_clientdata(client);
+
 	sysfs_remove_group(&client->dev.kobj, &tsl2550_attr_group);
 
 	/* Power down the device */
 	tsl2550_set_power_state(client, 0);
 
-	kfree(i2c_get_clientdata(client));
+	i2c_set_clientdata(client, NULL);
+	kfree(data);
 
 	return 0;
 }
-- 
1.7.0

Re: [PATCH 14/24] misc: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

On Sat, Mar 20, 2010 at 03:12:55PM +0100, Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Uhm, Greg should know by now to drop these patches? ;)

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 15/24] misc/eeprom: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/misc/eeprom/at24.c    |    2 +-
 drivers/misc/eeprom/eeprom.c  |    6 +++++-
 drivers/misc/eeprom/max6875.c |    2 ++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index db7d0f2..cff9b93 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -603,8 +603,8 @@ static int __devexit at24_remove(struct i2c_client *client)
 		i2c_unregister_device(at24->client[i]);
 
 	kfree(at24->writebuf);
-	kfree(at24);
 	i2c_set_clientdata(client, NULL);
+	kfree(at24);
 	return 0;
 }
 
diff --git a/drivers/misc/eeprom/eeprom.c b/drivers/misc/eeprom/eeprom.c
index f939ebc..2643582 100644
--- a/drivers/misc/eeprom/eeprom.c
+++ b/drivers/misc/eeprom/eeprom.c
@@ -201,6 +201,7 @@ static int eeprom_probe(struct i2c_client *client,
 	return 0;
 
 exit_kfree:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -208,8 +209,11 @@ exit:
 
 static int eeprom_remove(struct i2c_client *client)
 {
+	struct eeprom_data *data = i2c_get_clientdata(client);
+
 	sysfs_remove_bin_file(&client->dev.kobj, &eeprom_attr);
-	kfree(i2c_get_clientdata(client));
+	i2c_set_clientdata(client, NULL);
+	kfree(data);
 
 	return 0;
 }
diff --git a/drivers/misc/eeprom/max6875.c b/drivers/misc/eeprom/max6875.c
index 5a6b2bc..54fc568 100644
--- a/drivers/misc/eeprom/max6875.c
+++ b/drivers/misc/eeprom/max6875.c
@@ -178,6 +178,7 @@ static int max6875_probe(struct i2c_client *client,
 exit_remove_fake:
 	i2c_unregister_device(data->fake_client);
 exit_kfree:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return err;
 }
@@ -189,6 +190,7 @@ static int max6875_remove(struct i2c_client *client)
 	i2c_unregister_device(data->fake_client);
 
 	sysfs_remove_bin_file(&client->dev.kobj, &user_eeprom_attr);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 
 	return 0;
-- 
1.7.0

[PATCH 01/24] mtd/maps/pismo: remove dangling pointer and a leak

[Wolfram Sang]

While looking for drivers which forgot to clear i2c_clientdata before freeing
the data structure it points to, I found that the pismo driver even has a leak
on the probe error path.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: David Woodhouse <David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/mtd/maps/pismo.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/maps/pismo.c b/drivers/mtd/maps/pismo.c
index 30e12c8..86081bc 100644
--- a/drivers/mtd/maps/pismo.c
+++ b/drivers/mtd/maps/pismo.c
@@ -233,6 +233,7 @@ static int __devexit pismo_remove(struct i2c_client *client)
 	/* FIXME: set_vpp needs saner arguments */
 	pismo_setvpp_remove_fix(pismo);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(pismo);
 
 	return 0;
@@ -271,7 +272,7 @@ static int __devinit pismo_probe(struct i2c_client *client,
 	ret = pismo_eeprom_read(client, &eeprom, 0, sizeof(eeprom));
 	if (ret < 0) {
 		dev_err(&client->dev, "error reading EEPROM: %d\n", ret);
-		return ret;
+		goto exit_free;
 	}
 
 	dev_info(&client->dev, "%.15s board found\n", eeprom.board);
@@ -282,6 +283,11 @@ static int __devinit pismo_probe(struct i2c_client *client,
 				      pdata->cs_addrs[i]);
 
 	return 0;
+
+ exit_free:
+	i2c_set_clientdata(client, NULL);
+	kfree(prismo);
+	return ret;
 }
 
 static const struct i2c_device_id pismo_id[] = {
-- 
1.7.0

Re: [PATCH 01/24] mtd/maps/pismo: remove dangling pointer and a leak

[Russell King]

On Sat, Mar 20, 2010 at 03:12:42PM +0100, Wolfram Sang wrote:
> + exit_free:
> +	i2c_set_clientdata(client, NULL);
> +	kfree(prismo);

And this is a compile error...

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

Re: [PATCH 01/24] mtd/maps/pismo: remove dangling pointer and a leak

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2127 bytes --]

On Sat, Mar 20, 2010 at 02:57:30PM +0000, Russell King wrote:
> On Sat, Mar 20, 2010 at 03:12:42PM +0100, Wolfram Sang wrote:
> > + exit_free:
> > +	i2c_set_clientdata(client, NULL);
> > +	kfree(prismo);
> 
> And this is a compile error...

[grabbing some brown paper bags]

I'm sorry. That was one of the manually adapted ones :(
Here is the typo corrected:

Subject: [PATCH V2] mtd/maps/pismo: remove dangling pointer and a leak

While looking for drivers which forgot to clear i2c_clientdata before freeing
the data structure it points to, I found that the pismo driver even has a leak
on the probe error path.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
Cc: David Woodhouse <David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/mtd/maps/pismo.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/maps/pismo.c b/drivers/mtd/maps/pismo.c
index 30e12c8..f021018 100644
--- a/drivers/mtd/maps/pismo.c
+++ b/drivers/mtd/maps/pismo.c
@@ -233,6 +233,7 @@ static int __devexit pismo_remove(struct i2c_client *client)
 	/* FIXME: set_vpp needs saner arguments */
 	pismo_setvpp_remove_fix(pismo);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(pismo);
 
 	return 0;
@@ -271,7 +272,7 @@ static int __devinit pismo_probe(struct i2c_client *client,
 	ret = pismo_eeprom_read(client, &eeprom, 0, sizeof(eeprom));
 	if (ret < 0) {
 		dev_err(&client->dev, "error reading EEPROM: %d\n", ret);
-		return ret;
+		goto exit_free;
 	}
 
 	dev_info(&client->dev, "%.15s board found\n", eeprom.board);
@@ -282,6 +283,11 @@ static int __devinit pismo_probe(struct i2c_client *client,
 				      pdata->cs_addrs[i]);
 
 	return 0;
+
+ exit_free:
+	i2c_set_clientdata(client, NULL);
+	kfree(pismo);
+	return ret;
 }
 
 static const struct i2c_device_id pismo_id[] = {

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 01/24] mtd/maps/pismo: remove dangling pointer and a leak

[Russell King]

On Sat, Mar 20, 2010 at 04:31:46PM +0100, Wolfram Sang wrote:
> Subject: [PATCH V2] mtd/maps/pismo: remove dangling pointer and a leak
> 
> While looking for drivers which forgot to clear i2c_clientdata before freeing
> the data structure it points to, I found that the pismo driver even has a leak
> on the probe error path.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>

Thanks.

Acked-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>

David, are you going to pick this up?

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

Re: [PATCH 01/24] mtd/maps/pismo: remove dangling pointer and a leak

[David Woodhouse]

On Sat, 2010-03-20 at 15:41 +0000, Russell King wrote:
> 
> Thanks.
> 
> Acked-by: Russell King <rmk+kernel-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> 
> David, are you going to pick this up? 

http://git.infradead.org/mtd-2.6.git/commitdiff/395b2288

Thanks.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org                              Intel Corporation

Re: [PATCH 01/24] mtd/maps/pismo: remove dangling pointer and a leak

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 580 bytes --]

On Sun, Mar 21, 2010 at 08:30:41AM +0000, David Woodhouse wrote:

> > Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
> > 
> > David, are you going to pick this up? 
> 
> http://git.infradead.org/mtd-2.6.git/commitdiff/395b2288

Thanks. The handling of the dangling pointer will be handled differently from
2.6.35 on, but I will prepare the necessary fixup then. The fixed leak was
good to have, I think.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 02/24] power/ds2782: really clear i2c_clientdata on exit

[Wolfram Sang]

Probably due to a copy & paste bug, clientdata was set again to the data
structure (which is freed immediately afterwards) when it should be NULLed.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Ryan Mallon <ryan-7Wk5F4Od5/oYd5yxfr4S2w@public.gmane.org>
Cc: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/power/ds2782_battery.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/power/ds2782_battery.c b/drivers/power/ds2782_battery.c
index da14f37..6971b85 100644
--- a/drivers/power/ds2782_battery.c
+++ b/drivers/power/ds2782_battery.c
@@ -236,7 +236,7 @@ static int ds2782_battery_remove(struct i2c_client *client)
 	idr_remove(&battery_id, info->id);
 	mutex_unlock(&battery_lock);
 
-	i2c_set_clientdata(client, info);
+	i2c_set_clientdata(client, NULL);
 
 	kfree(info);
 	return 0;
@@ -289,7 +289,7 @@ static int ds2782_battery_probe(struct i2c_client *client,
 fail_register:
 	kfree(info->battery.name);
 fail_name:
-	i2c_set_clientdata(client, info);
+	i2c_set_clientdata(client, NULL);
 	kfree(info);
 fail_info:
 	mutex_lock(&battery_lock);
-- 
1.7.0

Re: [PATCH 02/24] power/ds2782: really clear i2c_clientdata on exit

[Ryan Mallon]

Wolfram Sang wrote:
> Probably due to a copy & paste bug, clientdata was set again to the data
> structure (which is freed immediately afterwards) when it should be NULLed.

Good catch. Thanks.

Acked-by: Ryan Mallon <ryan-7Wk5F4Od5/oYd5yxfr4S2w@public.gmane.org>

> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Ryan Mallon <ryan-7Wk5F4Od5/oYd5yxfr4S2w@public.gmane.org>
> Cc: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/power/ds2782_battery.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/ds2782_battery.c b/drivers/power/ds2782_battery.c
> index da14f37..6971b85 100644
> --- a/drivers/power/ds2782_battery.c
> +++ b/drivers/power/ds2782_battery.c
> @@ -236,7 +236,7 @@ static int ds2782_battery_remove(struct i2c_client *client)
>  	idr_remove(&battery_id, info->id);
>  	mutex_unlock(&battery_lock);
>  
> -	i2c_set_clientdata(client, info);
> +	i2c_set_clientdata(client, NULL);
>  
>  	kfree(info);
>  	return 0;
> @@ -289,7 +289,7 @@ static int ds2782_battery_probe(struct i2c_client *client,
>  fail_register:
>  	kfree(info->battery.name);
>  fail_name:
> -	i2c_set_clientdata(client, info);
> +	i2c_set_clientdata(client, NULL);
>  	kfree(info);
>  fail_info:
>  	mutex_lock(&battery_lock);

Re: [PATCH 02/24] power/ds2782: really clear i2c_clientdata on exit

[Anton Vorontsov]

On Sat, Mar 20, 2010 at 03:12:43PM +0100, Wolfram Sang wrote:
> Probably due to a copy & paste bug, clientdata was set again to the data
> structure (which is freed immediately afterwards) when it should be NULLed.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Ryan Mallon <ryan-7Wk5F4Od5/oYd5yxfr4S2w@public.gmane.org>
> Cc: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/power/ds2782_battery.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/ds2782_battery.c b/drivers/power/ds2782_battery.c
> index da14f37..6971b85 100644
> --- a/drivers/power/ds2782_battery.c
> +++ b/drivers/power/ds2782_battery.c
> @@ -236,7 +236,7 @@ static int ds2782_battery_remove(struct i2c_client *client)
>  	idr_remove(&battery_id, info->id);
>  	mutex_unlock(&battery_lock);
>  
> -	i2c_set_clientdata(client, info);
> +	i2c_set_clientdata(client, NULL);
[...]
> -	i2c_set_clientdata(client, info);
> +	i2c_set_clientdata(client, NULL);

Why is this needed? I'd vote for just removing set_clientdata in
fail/remove paths.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
irc://irc.freenode.net/bd2

Re: [PATCH 02/24] power/ds2782: really clear i2c_clientdata on exit

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 358 bytes --]

> Why is this needed? I'd vote for just removing set_clientdata in
> fail/remove paths.

Yes, it will be done this way. I will prepare the necessary cleanups for
2.6.35. Please drop this patch.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 03/24] usb/otg/isp1301_omap: Fix dangling pointer

[Wolfram Sang]

Fix this i2c-driver which missed setting clientdata to NULL before freeing the
structure it points to. Found by a semantic patch, but fixed by hand.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: Greg Kroah-Hartman <gregkh-l3A5Bk7waGM@public.gmane.org>
---
 drivers/usb/otg/isp1301_omap.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/otg/isp1301_omap.c b/drivers/usb/otg/isp1301_omap.c
index 78a2097..0ad16e3 100644
--- a/drivers/usb/otg/isp1301_omap.c
+++ b/drivers/usb/otg/isp1301_omap.c
@@ -1224,7 +1224,9 @@ static void isp1301_release(struct device *dev)
 	/* ugly -- i2c hijacks our memory hook to wait_for_completion() */
 	if (isp->i2c_release)
 		isp->i2c_release(dev);
-	kfree (isp);
+
+	i2c_set_clientdata(isp->client, NULL);
+	kfree(isp);
 }
 
 static struct isp1301 *the_transceiver;
@@ -1629,6 +1631,7 @@ isp1301_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	return 0;
 
 fail:
+	i2c_set_clientdata(i2c, NULL);
 	kfree(isp);
 	return -ENODEV;
 }
-- 
1.7.0

Re: [PATCH 03/24] usb/otg/isp1301_omap: Fix dangling pointer

[Felipe Balbi]

On Sat, Mar 20, 2010 at 03:12:44PM +0100, Wolfram Sang wrote:
> Fix this i2c-driver which missed setting clientdata to NULL before freeing the
> structure it points to. Found by a semantic patch, but fixed by hand.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Cc: Greg Kroah-Hartman <gregkh-l3A5Bk7waGM@public.gmane.org>

Acked-by: Felipe Balbi <felipe.balbi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>

Greg,

if you wish, you can take queue it already unless Dave has any comments.

-- 
balbi

Re: [PATCH 03/24] usb/otg/isp1301_omap: Fix dangling pointer

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

On Sat, Mar 20, 2010 at 10:09:07PM +0200, Felipe Balbi wrote:
> On Sat, Mar 20, 2010 at 03:12:44PM +0100, Wolfram Sang wrote:
> > Fix this i2c-driver which missed setting clientdata to NULL before freeing the
> > structure it points to. Found by a semantic patch, but fixed by hand.
> > 
> > Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> > Cc: Greg Kroah-Hartman <gregkh-l3A5Bk7waGM@public.gmane.org>
> 
> Acked-by: Felipe Balbi <felipe.balbi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> 
> Greg,
> 
> if you wish, you can take queue it already unless Dave has any comments.

Thanks but:

please drop this this patch. We reached an agreement that a) setting the
clientdata-pointer to NULL should be done in the i2c-core [1] and b) how to do
it. Based on that, I will do the modification of the i2c-core for 2.6.34 (as it
fixes the dangling pointers) and then create one single patch removing the then
superflous calls to i2c_set_clientdata for 2.6.35 (as it is a cleanup). If you
already applied the above patch to your branch, you don't have to revert; we
will fix it for you in the next round.

Sorry for the detour!

Kind regards,

   Wolfram

[1] http://thread.gmane.org/gmane.linux.drivers.i2c/5674/focus=5729

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 05/24] hwmon: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Corentin Labbe <corentin.labbe-Um+J1D3rkBVWj0EZb7rXcA@public.gmane.org>
Cc: "Mark M. Hoffman" <mhoffman-xQSgfq/1h4JiLUuM0BA3LQ@public.gmane.org>
Cc: Juerg Haefliger <juergh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Riku Voipio <riku.voipio-X3B1VOXEql0@public.gmane.org>
Cc: "Hans J. Koch" <hjk-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
Cc: Marc Hulsman <m.hulsman-hGVxb2UgFK3z+pZb47iToQ@public.gmane.org>
Cc: Rudolf Marek <r.marek-/xGekIyIa4Ap1Coe8Ar9gA@public.gmane.org>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/hwmon/ad7414.c     |    2 ++
 drivers/hwmon/ad7418.c     |    2 ++
 drivers/hwmon/adm1021.c    |    2 ++
 drivers/hwmon/adm1025.c    |    2 ++
 drivers/hwmon/adm1026.c    |    2 ++
 drivers/hwmon/adm1029.c    |    2 ++
 drivers/hwmon/adm1031.c    |    2 ++
 drivers/hwmon/adm9240.c    |    2 ++
 drivers/hwmon/ads7828.c    |    5 ++++-
 drivers/hwmon/adt7462.c    |    2 ++
 drivers/hwmon/adt7470.c    |    2 ++
 drivers/hwmon/adt7475.c    |    2 ++
 drivers/hwmon/amc6821.c    |    2 ++
 drivers/hwmon/asb100.c     |    2 ++
 drivers/hwmon/atxp1.c      |    2 ++
 drivers/hwmon/dme1737.c    |    2 ++
 drivers/hwmon/ds1621.c     |    2 ++
 drivers/hwmon/f75375s.c    |    4 ++--
 drivers/hwmon/g760a.c      |    4 ++--
 drivers/hwmon/gl518sm.c    |    2 ++
 drivers/hwmon/gl520sm.c    |    2 ++
 drivers/hwmon/lm63.c       |    2 ++
 drivers/hwmon/lm77.c       |    2 ++
 drivers/hwmon/lm78.c       |    2 ++
 drivers/hwmon/lm80.c       |    2 ++
 drivers/hwmon/lm83.c       |    2 ++
 drivers/hwmon/lm85.c       |    2 ++
 drivers/hwmon/lm87.c       |    2 ++
 drivers/hwmon/lm90.c       |    2 ++
 drivers/hwmon/lm92.c       |    2 ++
 drivers/hwmon/lm93.c       |    2 ++
 drivers/hwmon/lm95241.c    |    1 +
 drivers/hwmon/ltc4215.c    |    2 ++
 drivers/hwmon/ltc4245.c    |    2 ++
 drivers/hwmon/max1619.c    |    2 ++
 drivers/hwmon/max6650.c    |    2 ++
 drivers/hwmon/pcf8591.c    |    6 +++++-
 drivers/hwmon/smsc47m192.c |    2 ++
 drivers/hwmon/thmc50.c     |    2 ++
 drivers/hwmon/tmp401.c     |    1 +
 drivers/hwmon/w83791d.c    |    2 ++
 drivers/hwmon/w83792d.c    |    2 ++
 drivers/hwmon/w83793.c     |    1 +
 drivers/hwmon/w83l785ts.c  |    2 ++
 drivers/hwmon/w83l786ng.c  |    2 ++
 45 files changed, 92 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/ad7414.c b/drivers/hwmon/ad7414.c
index bfda8c8..0cfae01 100644
--- a/drivers/hwmon/ad7414.c
+++ b/drivers/hwmon/ad7414.c
@@ -220,6 +220,7 @@ static int ad7414_probe(struct i2c_client *client,
 exit_remove:
 	sysfs_remove_group(&client->dev.kobj, &ad7414_group);
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -231,6 +232,7 @@ static int __devexit ad7414_remove(struct i2c_client *client)
 
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &ad7414_group);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/ad7418.c b/drivers/hwmon/ad7418.c
index f97b5b3..bc2b16f 100644
--- a/drivers/hwmon/ad7418.c
+++ b/drivers/hwmon/ad7418.c
@@ -283,6 +283,7 @@ static int ad7418_probe(struct i2c_client *client,
 exit_remove:
 	sysfs_remove_group(&client->dev.kobj, &data->attrs);
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -293,6 +294,7 @@ static int ad7418_remove(struct i2c_client *client)
 	struct ad7418_data *data = i2c_get_clientdata(client);
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &data->attrs);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/adm1021.c b/drivers/hwmon/adm1021.c
index 1ad0a88..af04bef 100644
--- a/drivers/hwmon/adm1021.c
+++ b/drivers/hwmon/adm1021.c
@@ -375,6 +375,7 @@ static int adm1021_probe(struct i2c_client *client,
 error3:
 	sysfs_remove_group(&client->dev.kobj, &adm1021_group);
 error1:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 error0:
 	return err;
@@ -396,6 +397,7 @@ static int adm1021_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &adm1021_group);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/adm1025.c b/drivers/hwmon/adm1025.c
index 251b631..778067a 100644
--- a/drivers/hwmon/adm1025.c
+++ b/drivers/hwmon/adm1025.c
@@ -485,6 +485,7 @@ exit_remove:
 	sysfs_remove_group(&client->dev.kobj, &adm1025_group);
 	sysfs_remove_group(&client->dev.kobj, &adm1025_group_in4);
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -540,6 +541,7 @@ static int adm1025_remove(struct i2c_client *client)
 	sysfs_remove_group(&client->dev.kobj, &adm1025_group);
 	sysfs_remove_group(&client->dev.kobj, &adm1025_group_in4);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/adm1026.c b/drivers/hwmon/adm1026.c
index 65335b2..6a45c72 100644
--- a/drivers/hwmon/adm1026.c
+++ b/drivers/hwmon/adm1026.c
@@ -1743,6 +1743,7 @@ exitremove:
 	else
 		sysfs_remove_group(&client->dev.kobj, &adm1026_group_temp3);
 exitfree:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -1757,6 +1758,7 @@ static int adm1026_remove(struct i2c_client *client)
 		sysfs_remove_group(&client->dev.kobj, &adm1026_group_in8_9);
 	else
 		sysfs_remove_group(&client->dev.kobj, &adm1026_group_temp3);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/adm1029.c b/drivers/hwmon/adm1029.c
index 0b8a3b1..54dec88 100644
--- a/drivers/hwmon/adm1029.c
+++ b/drivers/hwmon/adm1029.c
@@ -369,6 +369,7 @@ static int adm1029_probe(struct i2c_client *client,
  exit_remove_files:
 	sysfs_remove_group(&client->dev.kobj, &adm1029_group);
  exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
  exit:
 	return err;
@@ -398,6 +399,7 @@ static int adm1029_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &adm1029_group);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/adm1031.c b/drivers/hwmon/adm1031.c
index 1644b92..dad192f 100644
--- a/drivers/hwmon/adm1031.c
+++ b/drivers/hwmon/adm1031.c
@@ -880,6 +880,7 @@ exit_remove:
 	sysfs_remove_group(&client->dev.kobj, &adm1031_group);
 	sysfs_remove_group(&client->dev.kobj, &adm1031_group_opt);
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -892,6 +893,7 @@ static int adm1031_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &adm1031_group);
 	sysfs_remove_group(&client->dev.kobj, &adm1031_group_opt);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/adm9240.c b/drivers/hwmon/adm9240.c
index 0727ad2..bf409ee 100644
--- a/drivers/hwmon/adm9240.c
+++ b/drivers/hwmon/adm9240.c
@@ -620,6 +620,7 @@ static int adm9240_probe(struct i2c_client *new_client,
 exit_remove:
 	sysfs_remove_group(&new_client->dev.kobj, &adm9240_group);
 exit_free:
+	i2c_set_clientdata(new_client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -632,6 +633,7 @@ static int adm9240_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &adm9240_group);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
index aac85f3..5b84256 100644
--- a/drivers/hwmon/ads7828.c
+++ b/drivers/hwmon/ads7828.c
@@ -158,9 +158,11 @@ static const struct attribute_group ads7828_group = {
 static int ads7828_remove(struct i2c_client *client)
 {
 	struct ads7828_data *data = i2c_get_clientdata(client);
+
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &ads7828_group);
-	kfree(i2c_get_clientdata(client));
+	i2c_set_clientdata(client, NULL);
+	kfree(data);
 	return 0;
 }
 
@@ -247,6 +249,7 @@ static int ads7828_probe(struct i2c_client *client,
 exit_remove:
 	sysfs_remove_group(&client->dev.kobj, &ads7828_group);
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
diff --git a/drivers/hwmon/adt7462.c b/drivers/hwmon/adt7462.c
index b8156b4..6b44d7e 100644
--- a/drivers/hwmon/adt7462.c
+++ b/drivers/hwmon/adt7462.c
@@ -1959,6 +1959,7 @@ static int adt7462_probe(struct i2c_client *client,
 exit_remove:
 	sysfs_remove_group(&client->dev.kobj, &data->attrs);
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -1970,6 +1971,7 @@ static int adt7462_remove(struct i2c_client *client)
 
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &data->attrs);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index 3445ce1..d106d0d 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -1295,6 +1295,7 @@ exit_unregister:
 exit_remove:
 	sysfs_remove_group(&client->dev.kobj, &data->attrs);
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -1308,6 +1309,7 @@ static int adt7470_remove(struct i2c_client *client)
 	wait_for_completion(&data->auto_update_stop);
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &data->attrs);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index a0c3851..e2bfbf9 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -1388,6 +1388,7 @@ static int adt7475_probe(struct i2c_client *client,
 eremove:
 	adt7475_remove_files(client, data);
 efree:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return ret;
 }
@@ -1398,6 +1399,7 @@ static int adt7475_remove(struct i2c_client *client)
 
 	hwmon_device_unregister(data->hwmon_dev);
 	adt7475_remove_files(client, data);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 
 	return 0;
diff --git a/drivers/hwmon/amc6821.c b/drivers/hwmon/amc6821.c
index fa9708c..3403613 100644
--- a/drivers/hwmon/amc6821.c
+++ b/drivers/hwmon/amc6821.c
@@ -889,6 +889,7 @@ static int amc6821_probe(
 	dev_err(&client->dev, "error registering hwmon device.\n");
 	sysfs_remove_group(&client->dev.kobj, &amc6821_attr_grp);
 err_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return err;
 }
@@ -900,6 +901,7 @@ static int amc6821_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &amc6821_attr_grp);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 
 	return 0;
diff --git a/drivers/hwmon/asb100.c b/drivers/hwmon/asb100.c
index 7dada55..18d879a 100644
--- a/drivers/hwmon/asb100.c
+++ b/drivers/hwmon/asb100.c
@@ -784,6 +784,7 @@ ERROR3:
 	i2c_unregister_device(data->lm75[1]);
 	i2c_unregister_device(data->lm75[0]);
 ERROR1:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 ERROR0:
 	return err;
@@ -799,6 +800,7 @@ static int asb100_remove(struct i2c_client *client)
 	i2c_unregister_device(data->lm75[1]);
 	i2c_unregister_device(data->lm75[0]);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 
 	return 0;
diff --git a/drivers/hwmon/atxp1.c b/drivers/hwmon/atxp1.c
index 94cadc1..9672280 100644
--- a/drivers/hwmon/atxp1.c
+++ b/drivers/hwmon/atxp1.c
@@ -349,6 +349,7 @@ static int atxp1_probe(struct i2c_client *new_client,
 exit_remove_files:
 	sysfs_remove_group(&new_client->dev.kobj, &atxp1_group);
 exit_free:
+	i2c_set_clientdata(new_client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -361,6 +362,7 @@ static int atxp1_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &atxp1_group);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 
 	return 0;
diff --git a/drivers/hwmon/dme1737.c b/drivers/hwmon/dme1737.c
index 823dd28..070ccf8 100644
--- a/drivers/hwmon/dme1737.c
+++ b/drivers/hwmon/dme1737.c
@@ -2282,6 +2282,7 @@ static int dme1737_i2c_probe(struct i2c_client *client,
 exit_remove:
 	dme1737_remove_files(dev);
 exit_kfree:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -2294,6 +2295,7 @@ static int dme1737_i2c_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	dme1737_remove_files(&client->dev);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/ds1621.c b/drivers/hwmon/ds1621.c
index e113634..e88e50f 100644
--- a/drivers/hwmon/ds1621.c
+++ b/drivers/hwmon/ds1621.c
@@ -286,6 +286,7 @@ static int ds1621_probe(struct i2c_client *client,
       exit_remove_files:
 	sysfs_remove_group(&client->dev.kobj, &ds1621_group);
       exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
       exit:
 	return err;
@@ -298,6 +299,7 @@ static int ds1621_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &ds1621_group);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 
 	return 0;
diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c
index 277398f..9a720a3 100644
--- a/drivers/hwmon/f75375s.c
+++ b/drivers/hwmon/f75375s.c
@@ -660,8 +660,8 @@ static int f75375_probe(struct i2c_client *client,
 exit_remove:
 	sysfs_remove_group(&client->dev.kobj, &f75375_group);
 exit_free:
-	kfree(data);
 	i2c_set_clientdata(client, NULL);
+	kfree(data);
 	return err;
 }
 
@@ -670,8 +670,8 @@ static int f75375_remove(struct i2c_client *client)
 	struct f75375_data *data = i2c_get_clientdata(client);
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &f75375_group);
-	kfree(data);
 	i2c_set_clientdata(client, NULL);
+	kfree(data);
 	return 0;
 }
 
diff --git a/drivers/hwmon/g760a.c b/drivers/hwmon/g760a.c
index 09ea12e..5adfdf8 100644
--- a/drivers/hwmon/g760a.c
+++ b/drivers/hwmon/g760a.c
@@ -235,8 +235,8 @@ static int g760a_probe(struct i2c_client *client,
 error_hwmon_device_register:
 	sysfs_remove_group(&client->dev.kobj, &g760a_group);
 error_sysfs_create_group:
-	kfree(data);
 	i2c_set_clientdata(client, NULL);
+	kfree(data);
 
 	return err;
 }
@@ -246,8 +246,8 @@ static int g760a_remove(struct i2c_client *client)
 	struct g760a_data *data = i2c_get_clientdata(client);
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &g760a_group);
-	kfree(data);
 	i2c_set_clientdata(client, NULL);
+	kfree(data);
 
 	return 0;
 }
diff --git a/drivers/hwmon/gl518sm.c b/drivers/hwmon/gl518sm.c
index e7ae574..e76aac2 100644
--- a/drivers/hwmon/gl518sm.c
+++ b/drivers/hwmon/gl518sm.c
@@ -548,6 +548,7 @@ exit_remove_files:
 	if (data->type == gl518sm_r80)
 		sysfs_remove_group(&client->dev.kobj, &gl518_group_r80);
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -581,6 +582,7 @@ static int gl518_remove(struct i2c_client *client)
 	if (data->type == gl518sm_r80)
 		sysfs_remove_group(&client->dev.kobj, &gl518_group_r80);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/gl520sm.c b/drivers/hwmon/gl520sm.c
index ec58802..efaa0d5 100644
--- a/drivers/hwmon/gl520sm.c
+++ b/drivers/hwmon/gl520sm.c
@@ -759,6 +759,7 @@ exit_remove_files:
 	sysfs_remove_group(&client->dev.kobj, &gl520_group);
 	sysfs_remove_group(&client->dev.kobj, &gl520_group_opt);
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -811,6 +812,7 @@ static int gl520_remove(struct i2c_client *client)
 	sysfs_remove_group(&client->dev.kobj, &gl520_group);
 	sysfs_remove_group(&client->dev.kobj, &gl520_group_opt);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/lm63.c b/drivers/hwmon/lm63.c
index bf81aff..ea6ffe4 100644
--- a/drivers/hwmon/lm63.c
+++ b/drivers/hwmon/lm63.c
@@ -496,6 +496,7 @@ exit_remove_files:
 	sysfs_remove_group(&new_client->dev.kobj, &lm63_group);
 	sysfs_remove_group(&new_client->dev.kobj, &lm63_group_fan1);
 exit_free:
+	i2c_set_clientdata(new_client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -544,6 +545,7 @@ static int lm63_remove(struct i2c_client *client)
 	sysfs_remove_group(&client->dev.kobj, &lm63_group);
 	sysfs_remove_group(&client->dev.kobj, &lm63_group_fan1);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/lm77.c b/drivers/hwmon/lm77.c
index b28a297..3eae81f 100644
--- a/drivers/hwmon/lm77.c
+++ b/drivers/hwmon/lm77.c
@@ -344,6 +344,7 @@ static int lm77_probe(struct i2c_client *new_client,
 exit_remove:
 	sysfs_remove_group(&new_client->dev.kobj, &lm77_group);
 exit_free:
+	i2c_set_clientdata(new_client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -354,6 +355,7 @@ static int lm77_remove(struct i2c_client *client)
 	struct lm77_data *data = i2c_get_clientdata(client);
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &lm77_group);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/lm78.c b/drivers/hwmon/lm78.c
index 72ff2c4..9e37d03 100644
--- a/drivers/hwmon/lm78.c
+++ b/drivers/hwmon/lm78.c
@@ -646,6 +646,7 @@ static int lm78_i2c_probe(struct i2c_client *client,
 ERROR4:
 	sysfs_remove_group(&client->dev.kobj, &lm78_group);
 ERROR3:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return err;
 }
@@ -656,6 +657,7 @@ static int lm78_i2c_remove(struct i2c_client *client)
 
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &lm78_group);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 
 	return 0;
diff --git a/drivers/hwmon/lm80.c b/drivers/hwmon/lm80.c
index 18a0e6c..5e2fa4f 100644
--- a/drivers/hwmon/lm80.c
+++ b/drivers/hwmon/lm80.c
@@ -504,6 +504,7 @@ static int lm80_probe(struct i2c_client *client,
 error_remove:
 	sysfs_remove_group(&client->dev.kobj, &lm80_group);
 error_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -516,6 +517,7 @@ static int lm80_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &lm80_group);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/lm83.c b/drivers/hwmon/lm83.c
index 8290476..918e558 100644
--- a/drivers/hwmon/lm83.c
+++ b/drivers/hwmon/lm83.c
@@ -376,6 +376,7 @@ exit_remove_files:
 	sysfs_remove_group(&new_client->dev.kobj, &lm83_group);
 	sysfs_remove_group(&new_client->dev.kobj, &lm83_group_opt);
 exit_free:
+	i2c_set_clientdata(new_client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -389,6 +390,7 @@ static int lm83_remove(struct i2c_client *client)
 	sysfs_remove_group(&client->dev.kobj, &lm83_group);
 	sysfs_remove_group(&client->dev.kobj, &lm83_group_opt);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/lm85.c b/drivers/hwmon/lm85.c
index b3841a6..e5b90c4 100644
--- a/drivers/hwmon/lm85.c
+++ b/drivers/hwmon/lm85.c
@@ -1308,6 +1308,7 @@ static int lm85_probe(struct i2c_client *client,
 	if (data->type == emc6d100)
 		sysfs_remove_group(&client->dev.kobj, &lm85_group_in567);
  err_kfree:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return err;
 }
@@ -1320,6 +1321,7 @@ static int lm85_remove(struct i2c_client *client)
 	sysfs_remove_group(&client->dev.kobj, &lm85_group_in4);
 	if (data->type == emc6d100)
 		sysfs_remove_group(&client->dev.kobj, &lm85_group_in567);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/lm87.c b/drivers/hwmon/lm87.c
index f1e6e75..e699b28 100644
--- a/drivers/hwmon/lm87.c
+++ b/drivers/hwmon/lm87.c
@@ -824,6 +824,7 @@ exit_remove:
 	sysfs_remove_group(&new_client->dev.kobj, &lm87_group_opt);
 exit_free:
 	lm87_write_value(new_client, LM87_REG_CONFIG, data->config);
+	i2c_set_clientdata(new_client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -880,6 +881,7 @@ static int lm87_remove(struct i2c_client *client)
 	sysfs_remove_group(&client->dev.kobj, &lm87_group_opt);
 
 	lm87_write_value(client, LM87_REG_CONFIG, data->config);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 7cc2708..d5db33b 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -851,6 +851,7 @@ exit_remove_files:
 	sysfs_remove_group(&new_client->dev.kobj, &lm90_group);
 	device_remove_file(&new_client->dev, &dev_attr_pec);
 exit_free:
+	i2c_set_clientdata(new_client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -907,6 +908,7 @@ static int lm90_remove(struct i2c_client *client)
 	i2c_smbus_write_byte_data(client, LM90_REG_W_CONFIG1,
 				  data->config_orig);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/lm92.c b/drivers/hwmon/lm92.c
index 7c31e62..b5cbc15 100644
--- a/drivers/hwmon/lm92.c
+++ b/drivers/hwmon/lm92.c
@@ -376,6 +376,7 @@ static int lm92_probe(struct i2c_client *new_client,
 exit_remove:
 	sysfs_remove_group(&new_client->dev.kobj, &lm92_group);
 exit_free:
+	i2c_set_clientdata(new_client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -388,6 +389,7 @@ static int lm92_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &lm92_group);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/lm93.c b/drivers/hwmon/lm93.c
index 6669255..e3b06b6 100644
--- a/drivers/hwmon/lm93.c
+++ b/drivers/hwmon/lm93.c
@@ -2584,6 +2584,7 @@ static int lm93_probe(struct i2c_client *client,
 	dev_err(&client->dev, "error registering hwmon device.\n");
 	sysfs_remove_group(&client->dev.kobj, &lm93_attr_grp);
 err_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 err_out:
 	return err;
@@ -2596,6 +2597,7 @@ static int lm93_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &lm93_attr_grp);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/lm95241.c b/drivers/hwmon/lm95241.c
index 8fc8eb8..f83fa36 100644
--- a/drivers/hwmon/lm95241.c
+++ b/drivers/hwmon/lm95241.c
@@ -367,6 +367,7 @@ static int lm95241_probe(struct i2c_client *new_client,
 exit_remove_files:
 	sysfs_remove_group(&new_client->dev.kobj, &lm95241_group);
 exit_free:
+	i2c_set_clientdata(new_client, NULL);
 	kfree(data);
 exit:
 	return err;
diff --git a/drivers/hwmon/ltc4215.c b/drivers/hwmon/ltc4215.c
index 00d975e..c9bcf0c 100644
--- a/drivers/hwmon/ltc4215.c
+++ b/drivers/hwmon/ltc4215.c
@@ -276,6 +276,7 @@ static int ltc4215_probe(struct i2c_client *client,
 out_hwmon_device_register:
 	sysfs_remove_group(&client->dev.kobj, &ltc4215_group);
 out_sysfs_create_group:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 out_kzalloc:
 	return ret;
@@ -288,6 +289,7 @@ static int ltc4215_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &ltc4215_group);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 
 	return 0;
diff --git a/drivers/hwmon/ltc4245.c b/drivers/hwmon/ltc4245.c
index 65c232a..24cb172 100644
--- a/drivers/hwmon/ltc4245.c
+++ b/drivers/hwmon/ltc4245.c
@@ -396,6 +396,7 @@ static int ltc4245_probe(struct i2c_client *client,
 out_hwmon_device_register:
 	sysfs_remove_group(&client->dev.kobj, &ltc4245_group);
 out_sysfs_create_group:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 out_kzalloc:
 	return ret;
@@ -408,6 +409,7 @@ static int ltc4245_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &ltc4245_group);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 
 	return 0;
diff --git a/drivers/hwmon/max1619.c b/drivers/hwmon/max1619.c
index 022ded0..08cc0e8 100644
--- a/drivers/hwmon/max1619.c
+++ b/drivers/hwmon/max1619.c
@@ -289,6 +289,7 @@ static int max1619_probe(struct i2c_client *new_client,
 exit_remove_files:
 	sysfs_remove_group(&new_client->dev.kobj, &max1619_group);
 exit_free:
+	i2c_set_clientdata(new_client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -316,6 +317,7 @@ static int max1619_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &max1619_group);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c
index a0160ee..e341ad4 100644
--- a/drivers/hwmon/max6650.c
+++ b/drivers/hwmon/max6650.c
@@ -590,6 +590,7 @@ static int max6650_probe(struct i2c_client *client,
 	dev_err(&client->dev, "error registering hwmon device.\n");
 	sysfs_remove_group(&client->dev.kobj, &max6650_attr_grp);
 err_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return err;
 }
@@ -600,6 +601,7 @@ static int max6650_remove(struct i2c_client *client)
 
 	sysfs_remove_group(&client->dev.kobj, &max6650_attr_grp);
 	hwmon_device_unregister(data->hwmon_dev);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/pcf8591.c b/drivers/hwmon/pcf8591.c
index d447879..ceec56b 100644
--- a/drivers/hwmon/pcf8591.c
+++ b/drivers/hwmon/pcf8591.c
@@ -227,6 +227,7 @@ exit_sysfs_remove:
 	sysfs_remove_group(&client->dev.kobj, &pcf8591_attr_group_opt);
 	sysfs_remove_group(&client->dev.kobj, &pcf8591_attr_group);
 exit_kfree:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -234,9 +235,12 @@ exit:
 
 static int pcf8591_remove(struct i2c_client *client)
 {
+	struct pcf8591_data *data = i2c_get_clientdata(client);
+
 	sysfs_remove_group(&client->dev.kobj, &pcf8591_attr_group_opt);
 	sysfs_remove_group(&client->dev.kobj, &pcf8591_attr_group);
-	kfree(i2c_get_clientdata(client));
+	i2c_set_clientdata(client, NULL);
+	kfree(data);
 	return 0;
 }
 
diff --git a/drivers/hwmon/smsc47m192.c b/drivers/hwmon/smsc47m192.c
index 40b2667..565b68d 100644
--- a/drivers/hwmon/smsc47m192.c
+++ b/drivers/hwmon/smsc47m192.c
@@ -555,6 +555,7 @@ exit_remove_files:
 	sysfs_remove_group(&client->dev.kobj, &smsc47m192_group);
 	sysfs_remove_group(&client->dev.kobj, &smsc47m192_group_in4);
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -568,6 +569,7 @@ static int smsc47m192_remove(struct i2c_client *client)
 	sysfs_remove_group(&client->dev.kobj, &smsc47m192_group);
 	sysfs_remove_group(&client->dev.kobj, &smsc47m192_group_in4);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 
 	return 0;
diff --git a/drivers/hwmon/thmc50.c b/drivers/hwmon/thmc50.c
index 7dfb4de..240a393 100644
--- a/drivers/hwmon/thmc50.c
+++ b/drivers/hwmon/thmc50.c
@@ -384,6 +384,7 @@ exit_remove_sysfs:
 exit_remove_sysfs_thmc50:
 	sysfs_remove_group(&client->dev.kobj, &thmc50_group);
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -398,6 +399,7 @@ static int thmc50_remove(struct i2c_client *client)
 	if (data->has_temp3)
 		sysfs_remove_group(&client->dev.kobj, &temp3_group);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 
 	return 0;
diff --git a/drivers/hwmon/tmp401.c b/drivers/hwmon/tmp401.c
index d14a1af..8df3a1c 100644
--- a/drivers/hwmon/tmp401.c
+++ b/drivers/hwmon/tmp401.c
@@ -598,6 +598,7 @@ static int tmp401_remove(struct i2c_client *client)
 					   &tmp411_attr[i].dev_attr);
 	}
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/w83791d.c b/drivers/hwmon/w83791d.c
index 400a88b..3cf5980 100644
--- a/drivers/hwmon/w83791d.c
+++ b/drivers/hwmon/w83791d.c
@@ -1371,6 +1371,7 @@ error3:
 	if (data->lm75[1] != NULL)
 		i2c_unregister_device(data->lm75[1]);
 error1:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 error0:
 	return err;
@@ -1388,6 +1389,7 @@ static int w83791d_remove(struct i2c_client *client)
 	if (data->lm75[1] != NULL)
 		i2c_unregister_device(data->lm75[1]);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/w83792d.c b/drivers/hwmon/w83792d.c
index 679718e..bbf0a00 100644
--- a/drivers/hwmon/w83792d.c
+++ b/drivers/hwmon/w83792d.c
@@ -1382,6 +1382,7 @@ ERROR3:
 	if (data->lm75[1] != NULL)
 		i2c_unregister_device(data->lm75[1]);
 ERROR1:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 ERROR0:
 	return err;
@@ -1404,6 +1405,7 @@ w83792d_remove(struct i2c_client *client)
 	if (data->lm75[1] != NULL)
 		i2c_unregister_device(data->lm75[1]);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/w83793.c b/drivers/hwmon/w83793.c
index 9de81a4..94ef5ab 100644
--- a/drivers/hwmon/w83793.c
+++ b/drivers/hwmon/w83793.c
@@ -1884,6 +1884,7 @@ exit_remove:
 	if (data->lm75[1] != NULL)
 		i2c_unregister_device(data->lm75[1]);
 free_mem:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
diff --git a/drivers/hwmon/w83l785ts.c b/drivers/hwmon/w83l785ts.c
index 20781de..fa73868 100644
--- a/drivers/hwmon/w83l785ts.c
+++ b/drivers/hwmon/w83l785ts.c
@@ -225,6 +225,7 @@ exit_remove:
 			   &sensor_dev_attr_temp1_input.dev_attr);
 	device_remove_file(&new_client->dev,
 			   &sensor_dev_attr_temp1_max.dev_attr);
+	i2c_set_clientdata(new_client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -240,6 +241,7 @@ static int w83l785ts_remove(struct i2c_client *client)
 	device_remove_file(&client->dev,
 			   &sensor_dev_attr_temp1_max.dev_attr);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
diff --git a/drivers/hwmon/w83l786ng.c b/drivers/hwmon/w83l786ng.c
index 0254e18..217ed0c 100644
--- a/drivers/hwmon/w83l786ng.c
+++ b/drivers/hwmon/w83l786ng.c
@@ -667,6 +667,7 @@ w83l786ng_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 exit_remove:
 	sysfs_remove_group(&client->dev.kobj, &w83l786ng_group);
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -680,6 +681,7 @@ w83l786ng_remove(struct i2c_client *client)
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &w83l786ng_group);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 
 	return 0;
-- 
1.7.0

Re: [PATCH 05/24] hwmon: fix dangling pointers

[Jean Delvare]

On Sat, 20 Mar 2010 15:12:46 +0100, Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Corentin Labbe <corentin.labbe-Um+J1D3rkBVWj0EZb7rXcA@public.gmane.org>
> Cc: "Mark M. Hoffman" <mhoffman-xQSgfq/1h4JiLUuM0BA3LQ@public.gmane.org>
> Cc: Juerg Haefliger <juergh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Riku Voipio <riku.voipio-X3B1VOXEql0@public.gmane.org>
> Cc: "Hans J. Koch" <hjk-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> Cc: Marc Hulsman <m.hulsman-hGVxb2UgFK3z+pZb47iToQ@public.gmane.org>
> Cc: Rudolf Marek <r.marek-/xGekIyIa4Ap1Coe8Ar9gA@public.gmane.org>
> ---
> 
> Found using coccinelle, then reviewed. Full patchset is available via
> kernel-janitors, linux-i2c, and LKML.
> ---
>  drivers/hwmon/ad7414.c     |    2 ++
>  drivers/hwmon/ad7418.c     |    2 ++
>  drivers/hwmon/adm1021.c    |    2 ++
>  drivers/hwmon/adm1025.c    |    2 ++
>  drivers/hwmon/adm1026.c    |    2 ++
>  drivers/hwmon/adm1029.c    |    2 ++
>  drivers/hwmon/adm1031.c    |    2 ++
>  drivers/hwmon/adm9240.c    |    2 ++
>  drivers/hwmon/ads7828.c    |    5 ++++-
>  drivers/hwmon/adt7462.c    |    2 ++
>  drivers/hwmon/adt7470.c    |    2 ++
>  drivers/hwmon/adt7475.c    |    2 ++
>  drivers/hwmon/amc6821.c    |    2 ++
>  drivers/hwmon/asb100.c     |    2 ++
>  drivers/hwmon/atxp1.c      |    2 ++
>  drivers/hwmon/dme1737.c    |    2 ++
>  drivers/hwmon/ds1621.c     |    2 ++
>  drivers/hwmon/f75375s.c    |    4 ++--
>  drivers/hwmon/g760a.c      |    4 ++--
>  drivers/hwmon/gl518sm.c    |    2 ++
>  drivers/hwmon/gl520sm.c    |    2 ++
>  drivers/hwmon/lm63.c       |    2 ++
>  drivers/hwmon/lm77.c       |    2 ++
>  drivers/hwmon/lm78.c       |    2 ++
>  drivers/hwmon/lm80.c       |    2 ++
>  drivers/hwmon/lm83.c       |    2 ++
>  drivers/hwmon/lm85.c       |    2 ++
>  drivers/hwmon/lm87.c       |    2 ++
>  drivers/hwmon/lm90.c       |    2 ++
>  drivers/hwmon/lm92.c       |    2 ++
>  drivers/hwmon/lm93.c       |    2 ++
>  drivers/hwmon/lm95241.c    |    1 +
>  drivers/hwmon/ltc4215.c    |    2 ++
>  drivers/hwmon/ltc4245.c    |    2 ++
>  drivers/hwmon/max1619.c    |    2 ++
>  drivers/hwmon/max6650.c    |    2 ++
>  drivers/hwmon/pcf8591.c    |    6 +++++-
>  drivers/hwmon/smsc47m192.c |    2 ++
>  drivers/hwmon/thmc50.c     |    2 ++
>  drivers/hwmon/tmp401.c     |    1 +
>  drivers/hwmon/w83791d.c    |    2 ++
>  drivers/hwmon/w83792d.c    |    2 ++
>  drivers/hwmon/w83793.c     |    1 +
>  drivers/hwmon/w83l785ts.c  |    2 ++
>  drivers/hwmon/w83l786ng.c  |    2 ++
>  45 files changed, 92 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/ad7414.c b/drivers/hwmon/ad7414.c
> index bfda8c8..0cfae01 100644
> --- a/drivers/hwmon/ad7414.c
> +++ b/drivers/hwmon/ad7414.c
> @@ -220,6 +220,7 @@ static int ad7414_probe(struct i2c_client *client,
>  exit_remove:
>  	sysfs_remove_group(&client->dev.kobj, &ad7414_group);
>  exit_free:
> +	i2c_set_clientdata(client, NULL);
>  	kfree(data);
>  exit:
>  	return err;
> @@ -231,6 +232,7 @@ static int __devexit ad7414_remove(struct i2c_client *client)
>  
>  	hwmon_device_unregister(data->hwmon_dev);
>  	sysfs_remove_group(&client->dev.kobj, &ad7414_group);
> +	i2c_set_clientdata(client, NULL);
>  	kfree(data);
>  	return 0;
>  }
> (...)

As discussed on the i2c list, I won't apply this patch. Instead, we
want i2c-core to call i2c_set_clientdata() after the remove() function
returns. This saves a lot of code duplication.

-- 
Jean Delvare

[PATCH 07/24] input/touchscreen: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/input/touchscreen/mcs5000_ts.c |    2 +-
 drivers/input/touchscreen/tsc2007.c    |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/input/touchscreen/mcs5000_ts.c b/drivers/input/touchscreen/mcs5000_ts.c
index 4c28b89..98689a7 100644
--- a/drivers/input/touchscreen/mcs5000_ts.c
+++ b/drivers/input/touchscreen/mcs5000_ts.c
@@ -254,8 +254,8 @@ static int __devexit mcs5000_ts_remove(struct i2c_client *client)
 
 	free_irq(client->irq, data);
 	input_unregister_device(data->input_dev);
-	kfree(data);
 	i2c_set_clientdata(client, NULL);
+	kfree(data);
 
 	return 0;
 }
diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index be23780..4c63fda 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -353,6 +353,7 @@ static int __devexit tsc2007_remove(struct i2c_client *client)
 		pdata->exit_platform_hw();
 
 	input_unregister_device(ts->input);
+	i2c_set_clientdata(client, NULL);
 	kfree(ts);
 
 	return 0;
-- 
1.7.0

[PATCH 11/24] media/radio/si470x: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Mauro Carvalho Chehab <mchehab-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/media/radio/si470x/radio-si470x-i2c.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/radio/si470x/radio-si470x-i2c.c b/drivers/media/radio/si470x/radio-si470x-i2c.c
index 5466015..2dabfac 100644
--- a/drivers/media/radio/si470x/radio-si470x-i2c.c
+++ b/drivers/media/radio/si470x/radio-si470x-i2c.c
@@ -480,8 +480,8 @@ static __devexit int si470x_i2c_remove(struct i2c_client *client)
 	free_irq(client->irq, radio);
 	cancel_work_sync(&radio->radio_work);
 	video_unregister_device(radio->videodev);
-	kfree(radio);
 	i2c_set_clientdata(client, NULL);
+	kfree(radio);
 
 	return 0;
 }
-- 
1.7.0

[PATCH 16/24] power: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/power/bq27x00_battery.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/power/bq27x00_battery.c b/drivers/power/bq27x00_battery.c
index bece33e..b909103 100644
--- a/drivers/power/bq27x00_battery.c
+++ b/drivers/power/bq27x00_battery.c
@@ -399,6 +399,7 @@ static int bq27x00_battery_probe(struct i2c_client *client,
 
 batt_failed_4:
 	kfree(bus);
+	i2c_set_clientdata(client, NULL);
 batt_failed_3:
 	kfree(di);
 batt_failed_2:
@@ -423,6 +424,7 @@ static int bq27x00_battery_remove(struct i2c_client *client)
 	idr_remove(&battery_id, di->id);
 	mutex_unlock(&battery_mutex);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(di);
 
 	return 0;
-- 
1.7.0

[PATCH 17/24] regulator: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Liam Girdwood <lrg-kDsPt+C1G03kYMGBc/C6ZA@public.gmane.org>
Cc: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/regulator/max1586.c |    2 +-
 drivers/regulator/max8649.c |    3 ++-
 drivers/regulator/max8660.c |    2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/max1586.c b/drivers/regulator/max1586.c
index a49fc95..c0b09e1 100644
--- a/drivers/regulator/max1586.c
+++ b/drivers/regulator/max1586.c
@@ -243,8 +243,8 @@ static int __devexit max1586_pmic_remove(struct i2c_client *client)
 	for (i = 0; i <= MAX1586_V6; i++)
 		if (rdev[i])
 			regulator_unregister(rdev[i]);
-	kfree(rdev);
 	i2c_set_clientdata(client, NULL);
+	kfree(rdev);
 
 	return 0;
 }
diff --git a/drivers/regulator/max8649.c b/drivers/regulator/max8649.c
index 3ebdf69..833aaed 100644
--- a/drivers/regulator/max8649.c
+++ b/drivers/regulator/max8649.c
@@ -356,6 +356,7 @@ static int __devinit max8649_regulator_probe(struct i2c_client *client,
 	dev_info(info->dev, "Max8649 regulator device is detected.\n");
 	return 0;
 out:
+	i2c_set_clientdata(client, NULL);
 	kfree(info);
 	return ret;
 }
@@ -367,9 +368,9 @@ static int __devexit max8649_regulator_remove(struct i2c_client *client)
 	if (info) {
 		if (info->regulator)
 			regulator_unregister(info->regulator);
+		i2c_set_clientdata(client, NULL);
 		kfree(info);
 	}
-	i2c_set_clientdata(client, NULL);
 
 	return 0;
 }
diff --git a/drivers/regulator/max8660.c b/drivers/regulator/max8660.c
index f12f1bb..47f90b2 100644
--- a/drivers/regulator/max8660.c
+++ b/drivers/regulator/max8660.c
@@ -470,8 +470,8 @@ static int __devexit max8660_remove(struct i2c_client *client)
 	for (i = 0; i < MAX8660_V_END; i++)
 		if (rdev[i])
 			regulator_unregister(rdev[i]);
-	kfree(rdev);
 	i2c_set_clientdata(client, NULL);
+	kfree(rdev);
 
 	return 0;
 }
-- 
1.7.0

Re: [PATCH 17/24] regulator: fix dangling pointers

[Mark Brown]

On Sat, Mar 20, 2010 at 03:12:58PM +0100, Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.

Acked-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>

but like I say this doesn't seem like something that belongs in the
drivers.

Re: [PATCH 17/24] regulator: fix dangling pointers

[Liam Girdwood]

On Sat, 2010-03-20 at 15:12 +0100, Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Liam Girdwood <lrg@slimlogic.co.uk>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
> 

Applied.

Thanks

Liam

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH 17/24] regulator: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

On Mon, Mar 22, 2010 at 07:49:10PM +0000, Liam Girdwood wrote:
> On Sat, 2010-03-20 at 15:12 +0100, Wolfram Sang wrote:
> > Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> > structure it points to. Also fix drivers which do this _after_ the structure
> > was freed already.
> > 
> > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> > Cc: Liam Girdwood <lrg@slimlogic.co.uk>
> > Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> > ---
> > 
> 
> Applied.

Thanks. An update will be needed though. But we will take care of it!

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 19/24] staging/dream: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Greg Kroah-Hartman <gregkh-l3A5Bk7waGM@public.gmane.org>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/staging/dream/synaptics_i2c_rmi.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/dream/synaptics_i2c_rmi.c b/drivers/staging/dream/synaptics_i2c_rmi.c
index 4de6bc9..a318c18 100644
--- a/drivers/staging/dream/synaptics_i2c_rmi.c
+++ b/drivers/staging/dream/synaptics_i2c_rmi.c
@@ -534,6 +534,7 @@ err_input_register_device_failed:
 err_input_dev_alloc_failed:
 err_detect_failed:
 err_power_failed:
+	i2c_set_clientdata(client, NULL);
 	kfree(ts);
 err_alloc_data_failed:
 err_check_functionality_failed:
@@ -551,6 +552,7 @@ static int synaptics_ts_remove(struct i2c_client *client)
 	else
 		hrtimer_cancel(&ts->timer);
 	input_unregister_device(ts->input_dev);
+	i2c_set_clientdata(client, NULL);
 	kfree(ts);
 	return 0;
 }
-- 
1.7.0

[PATCH 22/24] staging/iio/light: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Greg Kroah-Hartman <gregkh-l3A5Bk7waGM@public.gmane.org>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/staging/iio/light/tsl2563.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/iio/light/tsl2563.c b/drivers/staging/iio/light/tsl2563.c
index 78b9432..286559d 100644
--- a/drivers/staging/iio/light/tsl2563.c
+++ b/drivers/staging/iio/light/tsl2563.c
@@ -681,6 +681,7 @@ static int __devinit tsl2563_probe(struct i2c_client *client,
 fail2:
 	iio_device_unregister(chip->indio_dev);
 fail1:
+	i2c_set_clientdata(client, NULL);
 	kfree(chip);
 	return err;
 }
@@ -691,6 +692,7 @@ static int tsl2563_remove(struct i2c_client *client)
 
 	iio_device_unregister(chip->indio_dev);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(chip);
 	return 0;
 }
-- 
1.7.0

Re: [PATCH 22/24] staging/iio/light: fix dangling pointers

[Jonathan Cameron]

On 03/20/10 14:13, Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>

Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

Thanks for this one as well!
> ---
> 
> Found using coccinelle, then reviewed. Full patchset is available via
> kernel-janitors, linux-i2c, and LKML.
> ---
>  drivers/staging/iio/light/tsl2563.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2563.c b/drivers/staging/iio/light/tsl2563.c
> index 78b9432..286559d 100644
> --- a/drivers/staging/iio/light/tsl2563.c
> +++ b/drivers/staging/iio/light/tsl2563.c
> @@ -681,6 +681,7 @@ static int __devinit tsl2563_probe(struct i2c_client *client,
>  fail2:
>  	iio_device_unregister(chip->indio_dev);
>  fail1:
> +	i2c_set_clientdata(client, NULL);
>  	kfree(chip);
>  	return err;
>  }
> @@ -691,6 +692,7 @@ static int tsl2563_remove(struct i2c_client *client)
>  
>  	iio_device_unregister(chip->indio_dev);
>  
> +	i2c_set_clientdata(client, NULL);
>  	kfree(chip);
>  	return 0;
>  }

Re: i2c-drivers: Remove dangling pointers

[Ben Dooks]

On Sat, Mar 20, 2010 at 03:12:41PM +0100, Wolfram Sang wrote:
> Hi,
> 
> here is a patch series fixing I2C drivers which free their private data
> structure and leave the clientdata pointer dangling or do the cleanup in the
> wrong order. All occurences were found using coccinelle [1] and the semantic
> patch below. The matches have been reviewed, and sometimes adapted.
> 
> The complete series goes to linux-i2c and kernel-janitors. The rest of the CCs
> is handled per patch using get_maintainers.pl (which always adds LKML, so you
> have the complete series there, too).
> 
> Please review, comment, apply!
> 
> Kind regards,
> 
>    Wolfram
> 
> [1] http://coccinelle.lip6.fr/

>From the list of things here, there's nothing in here that I should be
worried about (just checking) ?
 
> ===
> 
> @@
> type T;
> identifier client, data;
> @@
> 
> // Check if function uses clientdata
> (
> 	i2c_set_clientdata(client, data);
> |
> 	data = i2c_get_clientdata(client);
> |
> 	T data = i2c_get_clientdata(client);
> )
> // Anything inbetween
> 	...
> // Check if clientdata gets NULLed before data is freed
> (
> 	i2c_set_clientdata(client, NULL);
> 	...
>  	kfree(data);
> |
> +	i2c_set_clientdata(client, NULL);
>  	kfree(data);
> 	...
> -	i2c_set_clientdata(client, NULL);
> |
> +	i2c_set_clientdata(client, NULL);
> ? 	kfree(data);
> )
> 
> ===
> 
>  drivers/gpio/adp5588-gpio.c                   |    1 +
>  drivers/gpio/max732x.c                        |    1 +
>  drivers/gpio/pca953x.c                        |    1 +
>  drivers/gpio/pcf857x.c                        |    7 ++++---
>  drivers/hwmon/ad7414.c                        |    2 ++
>  drivers/hwmon/ad7418.c                        |    2 ++
>  drivers/hwmon/adm1021.c                       |    2 ++
>  drivers/hwmon/adm1025.c                       |    2 ++
>  drivers/hwmon/adm1026.c                       |    2 ++
>  drivers/hwmon/adm1029.c                       |    2 ++
>  drivers/hwmon/adm1031.c                       |    2 ++
>  drivers/hwmon/adm9240.c                       |    2 ++
>  drivers/hwmon/ads7828.c                       |    5 ++++-
>  drivers/hwmon/adt7462.c                       |    2 ++
>  drivers/hwmon/adt7470.c                       |    2 ++
>  drivers/hwmon/adt7475.c                       |    2 ++
>  drivers/hwmon/amc6821.c                       |    2 ++
>  drivers/hwmon/asb100.c                        |    2 ++
>  drivers/hwmon/atxp1.c                         |    2 ++
>  drivers/hwmon/dme1737.c                       |    2 ++
>  drivers/hwmon/ds1621.c                        |    2 ++
>  drivers/hwmon/f75375s.c                       |    4 ++--
>  drivers/hwmon/g760a.c                         |    4 ++--
>  drivers/hwmon/gl518sm.c                       |    2 ++
>  drivers/hwmon/gl520sm.c                       |    2 ++
>  drivers/hwmon/lm63.c                          |    2 ++
>  drivers/hwmon/lm77.c                          |    2 ++
>  drivers/hwmon/lm78.c                          |    2 ++
>  drivers/hwmon/lm80.c                          |    2 ++
>  drivers/hwmon/lm83.c                          |    2 ++
>  drivers/hwmon/lm85.c                          |    2 ++
>  drivers/hwmon/lm87.c                          |    2 ++
>  drivers/hwmon/lm90.c                          |    2 ++
>  drivers/hwmon/lm92.c                          |    2 ++
>  drivers/hwmon/lm93.c                          |    2 ++
>  drivers/hwmon/lm95241.c                       |    1 +
>  drivers/hwmon/ltc4215.c                       |    2 ++
>  drivers/hwmon/ltc4245.c                       |    2 ++
>  drivers/hwmon/max1619.c                       |    2 ++
>  drivers/hwmon/max6650.c                       |    2 ++
>  drivers/hwmon/pcf8591.c                       |    6 +++++-
>  drivers/hwmon/smsc47m192.c                    |    2 ++
>  drivers/hwmon/thmc50.c                        |    2 ++
>  drivers/hwmon/tmp401.c                        |    1 +
>  drivers/hwmon/w83791d.c                       |    2 ++
>  drivers/hwmon/w83792d.c                       |    2 ++
>  drivers/hwmon/w83793.c                        |    1 +
>  drivers/hwmon/w83l785ts.c                     |    2 ++
>  drivers/hwmon/w83l786ng.c                     |    2 ++
>  drivers/input/keyboard/lm8323.c               |    2 ++
>  drivers/input/keyboard/qt2160.c               |    2 +-
>  drivers/input/touchscreen/mcs5000_ts.c        |    2 +-
>  drivers/input/touchscreen/tsc2007.c           |    1 +
>  drivers/leds/leds-lp3944.c                    |    2 +-
>  drivers/leds/leds-pca9532.c                   |    4 ++--
>  drivers/leds/leds-pca955x.c                   |    4 ++--
>  drivers/macintosh/therm_adt746x.c             |    2 ++
>  drivers/media/radio/radio-tea5764.c           |    2 ++
>  drivers/media/radio/si470x/radio-si470x-i2c.c |    2 +-
>  drivers/media/video/cs5345.c                  |    1 +
>  drivers/media/video/cs53l32a.c                |    1 +
>  drivers/media/video/ir-kbd-i2c.c              |    2 ++
>  drivers/media/video/tda9840.c                 |    1 +
>  drivers/media/video/tea6415c.c                |    1 +
>  drivers/media/video/tea6420.c                 |    1 +
>  drivers/media/video/ths7303.c                 |    1 +
>  drivers/mfd/88pm860x-i2c.c                    |    1 +
>  drivers/mfd/ab3100-core.c                     |    2 ++
>  drivers/mfd/da903x.c                          |    1 +
>  drivers/mfd/menelaus.c                        |    3 ++-
>  drivers/mfd/pcf50633-core.c                   |    1 +
>  drivers/mfd/tps65010.c                        |    2 +-
>  drivers/mfd/wm8350-i2c.c                      |    2 ++
>  drivers/misc/ad525x_dpot.c                    |    2 +-
>  drivers/misc/eeprom/at24.c                    |    2 +-
>  drivers/misc/eeprom/eeprom.c                  |    6 +++++-
>  drivers/misc/eeprom/max6875.c                 |    2 ++
>  drivers/misc/ics932s401.c                     |    2 ++
>  drivers/misc/isl29003.c                       |    6 +++++-
>  drivers/misc/tsl2550.c                        |    6 +++++-
>  drivers/mtd/maps/pismo.c                      |    8 +++++++-
>  drivers/power/bq27x00_battery.c               |    2 ++
>  drivers/power/ds2782_battery.c                |    4 ++--
>  drivers/regulator/max1586.c                   |    2 +-
>  drivers/regulator/max8649.c                   |    3 ++-
>  drivers/regulator/max8660.c                   |    2 +-
>  drivers/rtc/rtc-ds1307.c                      |    2 ++
>  drivers/rtc/rtc-fm3130.c                      |    2 ++
>  drivers/rtc/rtc-m41t80.c                      |    2 ++
>  drivers/rtc/rtc-pcf8563.c                     |    2 ++
>  drivers/rtc/rtc-pcf8583.c                     |    2 ++
>  drivers/rtc/rtc-rs5c372.c                     |    2 ++
>  drivers/rtc/rtc-s35390a.c                     |    4 ++--
>  drivers/staging/dream/synaptics_i2c_rmi.c     |    2 ++
>  drivers/staging/go7007/wis-saa7113.c          |    1 +
>  drivers/staging/go7007/wis-saa7115.c          |    1 +
>  drivers/staging/go7007/wis-tw9903.c           |    1 +
>  drivers/staging/iio/adc/max1363_core.c        |    2 ++
>  drivers/staging/iio/light/tsl2563.c           |    2 ++
>  drivers/usb/otg/isp1301_omap.c                |    5 ++++-
>  drivers/video/matrox/matroxfb_maven.c         |    1 +
>  drivers/w1/masters/ds2482.c                   |    2 ++
>  102 files changed, 198 insertions(+), 33 deletions(-)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: i2c-drivers: Remove dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

Hi Ben,

> > here is a patch series fixing I2C drivers which free their private data
> > structure and leave the clientdata pointer dangling or do the cleanup in the
> > wrong order. All occurences were found using coccinelle [1] and the semantic
> > patch below. The matches have been reviewed, and sometimes adapted.

[...]

> From the list of things here, there's nothing in here that I should be
> worried about (just checking) ?

Correct. Just ignore this series.

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 18/24] rtc: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Paul Gortmaker <p_gortmaker@yahoo.com>
Cc: Alessandro Zummo <a.zummo@towertech.it>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/rtc/rtc-ds1307.c  |    2 ++
 drivers/rtc/rtc-fm3130.c  |    2 ++
 drivers/rtc/rtc-m41t80.c  |    2 ++
 drivers/rtc/rtc-pcf8563.c |    2 ++
 drivers/rtc/rtc-pcf8583.c |    2 ++
 drivers/rtc/rtc-rs5c372.c |    2 ++
 drivers/rtc/rtc-s35390a.c |    4 ++--
 7 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-ds1307.c b/drivers/rtc/rtc-ds1307.c
index c4ec5c1..eee887c 100644
--- a/drivers/rtc/rtc-ds1307.c
+++ b/drivers/rtc/rtc-ds1307.c
@@ -900,6 +900,7 @@ read_rtc:
 exit_irq:
 	rtc_device_unregister(ds1307->rtc);
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(ds1307);
 	return err;
 }
@@ -917,6 +918,7 @@ static int __devexit ds1307_remove(struct i2c_client *client)
 		sysfs_remove_bin_file(&client->dev.kobj, &nvram);
 
 	rtc_device_unregister(ds1307->rtc);
+	i2c_set_clientdata(client, NULL);
 	kfree(ds1307);
 	return 0;
 }
diff --git a/drivers/rtc/rtc-fm3130.c b/drivers/rtc/rtc-fm3130.c
index 812c667..6c4e90f 100644
--- a/drivers/rtc/rtc-fm3130.c
+++ b/drivers/rtc/rtc-fm3130.c
@@ -462,6 +462,7 @@ exit_bad:
 	}
 	return 0;
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(fm3130);
 	return err;
 }
@@ -471,6 +472,7 @@ static int __devexit fm3130_remove(struct i2c_client *client)
 	struct fm3130 *fm3130 = i2c_get_clientdata(client);
 
 	rtc_device_unregister(fm3130->rtc);
+	i2c_set_clientdata(client, NULL);
 	kfree(fm3130);
 	return 0;
 }
diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index 60fe266..2fd8d7c 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -863,6 +863,7 @@ ht_err:
 exit:
 	if (rtc)
 		rtc_device_unregister(rtc);
+	i2c_set_clientdata(client, NULL);
 	kfree(clientdata);
 	return rc;
 }
@@ -880,6 +881,7 @@ static int m41t80_remove(struct i2c_client *client)
 #endif
 	if (rtc)
 		rtc_device_unregister(rtc);
+	i2c_set_clientdata(client, NULL);
 	kfree(clientdata);
 
 	return 0;
diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index 65f346b..4be76a4 100644
--- a/drivers/rtc/rtc-pcf8563.c
+++ b/drivers/rtc/rtc-pcf8563.c
@@ -225,6 +225,7 @@ static int pcf8563_probe(struct i2c_client *client,
 	return 0;
 
 exit_kfree:
+	i2c_set_clientdata(client, NULL);
 	kfree(pcf8563);
 
 	return err;
@@ -237,6 +238,7 @@ static int pcf8563_remove(struct i2c_client *client)
 	if (pcf8563->rtc)
 		rtc_device_unregister(pcf8563->rtc);
 
+	i2c_set_clientdata(client, NULL);
 	kfree(pcf8563);
 
 	return 0;
diff --git a/drivers/rtc/rtc-pcf8583.c b/drivers/rtc/rtc-pcf8583.c
index 2d201af..e122d9f 100644
--- a/drivers/rtc/rtc-pcf8583.c
+++ b/drivers/rtc/rtc-pcf8583.c
@@ -290,6 +290,7 @@ static int pcf8583_probe(struct i2c_client *client,
 	return 0;
 
 exit_kfree:
+	i2c_set_clientdata(client, NULL);
 	kfree(pcf8583);
 	return err;
 }
@@ -300,6 +301,7 @@ static int __devexit pcf8583_remove(struct i2c_client *client)
 
 	if (pcf8583->rtc)
 		rtc_device_unregister(pcf8583->rtc);
+	i2c_set_clientdata(client, NULL);
 	kfree(pcf8583);
 	return 0;
 }
diff --git a/drivers/rtc/rtc-rs5c372.c b/drivers/rtc/rtc-rs5c372.c
index 2f2c68d..75e0423 100644
--- a/drivers/rtc/rtc-rs5c372.c
+++ b/drivers/rtc/rtc-rs5c372.c
@@ -694,6 +694,7 @@ exit_devreg:
 	rtc_device_unregister(rs5c372->rtc);
 
 exit_kfree:
+	i2c_set_clientdata(client, NULL);
 	kfree(rs5c372);
 
 exit:
@@ -706,6 +707,7 @@ static int rs5c372_remove(struct i2c_client *client)
 
 	rtc_device_unregister(rs5c372->rtc);
 	rs5c_sysfs_unregister(&client->dev);
+	i2c_set_clientdata(client, NULL);
 	kfree(rs5c372);
 	return 0;
 }
diff --git a/drivers/rtc/rtc-s35390a.c b/drivers/rtc/rtc-s35390a.c
index def4d39..da39041 100644
--- a/drivers/rtc/rtc-s35390a.c
+++ b/drivers/rtc/rtc-s35390a.c
@@ -274,8 +274,8 @@ exit_dummy:
 	for (i = 1; i < 8; ++i)
 		if (s35390a->client[i])
 			i2c_unregister_device(s35390a->client[i]);
-	kfree(s35390a);
 	i2c_set_clientdata(client, NULL);
+	kfree(s35390a);
 
 exit:
 	return err;
@@ -291,8 +291,8 @@ static int s35390a_remove(struct i2c_client *client)
 			i2c_unregister_device(s35390a->client[i]);
 
 	rtc_device_unregister(s35390a->rtc);
-	kfree(s35390a);
 	i2c_set_clientdata(client, NULL);
+	kfree(s35390a);
 
 	return 0;
 }
-- 
1.7.0

Re: [PATCH 18/24] rtc: fix dangling pointers

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

On Sat, Mar 20, 2010 at 03:12:59PM +0100, Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Paul Gortmaker <p_gortmaker-/E1597aS9LQAvxtiuMwx3w@public.gmane.org>
> Cc: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>

please drop this this patch. We reached an agreement that a) setting the
clientdata-pointer to NULL should be done in the i2c-core [1] and b) how to do
it. Based on that, I will do the modification of the i2c-core for 2.6.34 (as it
fixes the dangling pointers) and then create one single patch removing the then
superflous calls to i2c_set_clientdata for 2.6.35 (as it is a cleanup). If you
already applied the above patch to your branch, you don't have to revert; we
will fix it for you in the next round.

Sorry for the detour!

Kind regards,

   Wolfram

[1] http://thread.gmane.org/gmane.linux.drivers.i2c/5674/focus=5729

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH 20/24] staging/go7007: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/staging/go7007/wis-saa7113.c |    1 +
 drivers/staging/go7007/wis-saa7115.c |    1 +
 drivers/staging/go7007/wis-tw9903.c  |    1 +
 3 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/go7007/wis-saa7113.c b/drivers/staging/go7007/wis-saa7113.c
index d196e16..26365af 100644
--- a/drivers/staging/go7007/wis-saa7113.c
+++ b/drivers/staging/go7007/wis-saa7113.c
@@ -288,6 +288,7 @@ static int wis_saa7113_probe(struct i2c_client *client,
 	if (write_regs(client, initial_registers) < 0) {
 		printk(KERN_ERR
 			"wis-saa7113: error initializing SAA7113\n");
+		i2c_set_clientdata(client, NULL);
 		kfree(dec);
 		return -ENODEV;
 	}
diff --git a/drivers/staging/go7007/wis-saa7115.c b/drivers/staging/go7007/wis-saa7115.c
index 0f2b4a0..0d71f6a 100644
--- a/drivers/staging/go7007/wis-saa7115.c
+++ b/drivers/staging/go7007/wis-saa7115.c
@@ -421,6 +421,7 @@ static int wis_saa7115_probe(struct i2c_client *client,
 	if (write_regs(client, initial_registers) < 0) {
 		printk(KERN_ERR
 			"wis-saa7115: error initializing SAA7115\n");
+		i2c_set_clientdata(client, NULL);
 		kfree(dec);
 		return -ENODEV;
 	}
diff --git a/drivers/staging/go7007/wis-tw9903.c b/drivers/staging/go7007/wis-tw9903.c
index f97e2be..c21b88a 100644
--- a/drivers/staging/go7007/wis-tw9903.c
+++ b/drivers/staging/go7007/wis-tw9903.c
@@ -293,6 +293,7 @@ static int wis_tw9903_probe(struct i2c_client *client,
 
 	if (write_regs(client, initial_registers) < 0) {
 		printk(KERN_ERR "wis-tw9903: error initializing TW9903\n");
+		i2c_set_clientdata(client, NULL);
 		kfree(dec);
 		return -ENODEV;
 	}
-- 
1.7.0

[PATCH 21/24] staging/iio/adc: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/staging/iio/adc/max1363_core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/iio/adc/max1363_core.c b/drivers/staging/iio/adc/max1363_core.c
index 9703881..1682fd0 100644
--- a/drivers/staging/iio/adc/max1363_core.c
+++ b/drivers/staging/iio/adc/max1363_core.c
@@ -556,6 +556,7 @@ error_put_reg:
 	if (!IS_ERR(st->reg))
 		regulator_put(st->reg);
 error_free_st:
+	i2c_set_clientdata(client, NULL);
 	kfree(st);
 
 error_ret:
@@ -573,6 +574,7 @@ static int max1363_remove(struct i2c_client *client)
 		regulator_disable(st->reg);
 		regulator_put(st->reg);
 	}
+	i2c_set_clientdata(client, NULL);
 	kfree(st);
 
 	return 0;
-- 
1.7.0

Re: [PATCH 21/24] staging/iio/adc: fix dangling pointers

[Jonathan Cameron]

On 03/20/10 14:13, Wolfram Sang wrote:
> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Greg Kroah-Hartman <gregkh-l3A5Bk7waGM@public.gmane.org>
Acked-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>

Thanks,
> ---
> 
> Found using coccinelle, then reviewed. Full patchset is available via
> kernel-janitors, linux-i2c, and LKML.
> ---
>  drivers/staging/iio/adc/max1363_core.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/max1363_core.c b/drivers/staging/iio/adc/max1363_core.c
> index 9703881..1682fd0 100644
> --- a/drivers/staging/iio/adc/max1363_core.c
> +++ b/drivers/staging/iio/adc/max1363_core.c
> @@ -556,6 +556,7 @@ error_put_reg:
>  	if (!IS_ERR(st->reg))
>  		regulator_put(st->reg);
>  error_free_st:
> +	i2c_set_clientdata(client, NULL);
>  	kfree(st);
>  
>  error_ret:
> @@ -573,6 +574,7 @@ static int max1363_remove(struct i2c_client *client)
>  		regulator_disable(st->reg);
>  		regulator_put(st->reg);
>  	}
> +	i2c_set_clientdata(client, NULL);
>  	kfree(st);
>  
>  	return 0;

[PATCH 23/24] video/matrox: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Petr Vandrovec <vandrove@vc.cvut.cz>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/video/matrox/matroxfb_maven.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/video/matrox/matroxfb_maven.c b/drivers/video/matrox/matroxfb_maven.c
index 91af915..e7b0ec3 100644
--- a/drivers/video/matrox/matroxfb_maven.c
+++ b/drivers/video/matrox/matroxfb_maven.c
@@ -1254,6 +1254,7 @@ static int maven_probe(struct i2c_client *client,
 		goto ERROR4;
 	return 0;
 ERROR4:;
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 ERROR0:;
 	return err;
-- 
1.7.0

Re: [PATCH 23/24] video/matrox: fix dangling pointers

[James Simmons]

On Sat, 20 Mar 2010, Wolfram Sang wrote:

> Fix I2C-drivers which missed setting clientdata to NULL before freeing the
> structure it points to. Also fix drivers which do this _after_ the structure
> was freed already.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Petr Vandrovec <vandrove-hnqZr3NxcozrBKCeMvbIDA@public.gmane.org>
> ---

Acked-by: James Simmons <jsimmons-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
 
> Found using coccinelle, then reviewed. Full patchset is available via
> kernel-janitors, linux-i2c, and LKML.
> ---
>  drivers/video/matrox/matroxfb_maven.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/matrox/matroxfb_maven.c b/drivers/video/matrox/matroxfb_maven.c
> index 91af915..e7b0ec3 100644
> --- a/drivers/video/matrox/matroxfb_maven.c
> +++ b/drivers/video/matrox/matroxfb_maven.c
> @@ -1254,6 +1254,7 @@ static int maven_probe(struct i2c_client *client,
>  		goto ERROR4;
>  	return 0;
>  ERROR4:;
> +	i2c_set_clientdata(client, NULL);
>  	kfree(data);
>  ERROR0:;
>  	return err;
> -- 
> 1.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 24/24] w1/masters: fix dangling pointers

[Wolfram Sang]

Fix I2C-drivers which missed setting clientdata to NULL before freeing the
structure it points to. Also fix drivers which do this _after_ the structure
was freed already.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
Cc: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
---

Found using coccinelle, then reviewed. Full patchset is available via
kernel-janitors, linux-i2c, and LKML.
---
 drivers/w1/masters/ds2482.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/w1/masters/ds2482.c b/drivers/w1/masters/ds2482.c
index e5f7441..b92b1c6 100644
--- a/drivers/w1/masters/ds2482.c
+++ b/drivers/w1/masters/ds2482.c
@@ -484,6 +484,7 @@ exit_w1_remove:
 			w1_remove_master_device(&data->w1_ch[idx].w1_bm);
 	}
 exit_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 exit:
 	return err;
@@ -501,6 +502,7 @@ static int ds2482_remove(struct i2c_client *client)
 	}
 
 	/* Free the memory */
+	i2c_set_clientdata(client, NULL);
 	kfree(data);
 	return 0;
 }
-- 
1.7.0