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

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

[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

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