* [PATCH 10/24] media/radio: fix dangling pointers [not found] <1269094385-16114-1-git-send-email-w.sang@pengutronix.de> @ 2010-03-20 14:12 ` Wolfram Sang 2010-03-20 14:12 ` [PATCH 11/24] media/radio/si470x: " Wolfram Sang 2010-03-20 14:12 ` [PATCH 12/24] media/video: " Wolfram Sang 2 siblings, 0 replies; 14+ messages in thread From: Wolfram Sang @ 2010-03-20 14:12 UTC (permalink / raw) To: kernel-janitors Cc: linux-i2c, linux-kernel, Wolfram Sang, Mauro Carvalho Chehab, linux-media 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 11/24] media/radio/si470x: fix dangling pointers [not found] <1269094385-16114-1-git-send-email-w.sang@pengutronix.de> 2010-03-20 14:12 ` [PATCH 10/24] media/radio: fix dangling pointers Wolfram Sang @ 2010-03-20 14:12 ` Wolfram Sang 2010-03-20 14:12 ` [PATCH 12/24] media/video: " Wolfram Sang 2 siblings, 0 replies; 14+ messages in thread From: Wolfram Sang @ 2010-03-20 14:12 UTC (permalink / raw) To: kernel-janitors Cc: linux-i2c, linux-kernel, Wolfram Sang, Mauro Carvalho Chehab, linux-media 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 12/24] media/video: fix dangling pointers [not found] <1269094385-16114-1-git-send-email-w.sang@pengutronix.de> 2010-03-20 14:12 ` [PATCH 10/24] media/radio: fix dangling pointers Wolfram Sang 2010-03-20 14:12 ` [PATCH 11/24] media/radio/si470x: " Wolfram Sang @ 2010-03-20 14:12 ` Wolfram Sang 2010-03-20 22:02 ` Hans Verkuil 2 siblings, 1 reply; 14+ messages in thread From: Wolfram Sang @ 2010-03-20 14:12 UTC (permalink / raw) To: kernel-janitors Cc: linux-i2c, linux-kernel, Wolfram Sang, Mauro Carvalho Chehab, linux-media 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 12/24] media/video: fix dangling pointers 2010-03-20 14:12 ` [PATCH 12/24] media/video: " Wolfram Sang @ 2010-03-20 22:02 ` Hans Verkuil 2010-03-21 11:31 ` Wolfram Sang 2010-03-21 13:46 ` Jean Delvare 0 siblings, 2 replies; 14+ messages in thread From: Hans Verkuil @ 2010-03-20 22:02 UTC (permalink / raw) To: Wolfram Sang Cc: kernel-janitors, linux-i2c, linux-kernel, Mauro Carvalho Chehab, linux-media 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 12/24] media/video: fix dangling pointers 2010-03-20 22:02 ` Hans Verkuil @ 2010-03-21 11:31 ` Wolfram Sang 2010-03-21 13:46 ` Jean Delvare 1 sibling, 0 replies; 14+ messages in thread From: Wolfram Sang @ 2010-03-21 11:31 UTC (permalink / raw) To: Hans Verkuil Cc: kernel-janitors, linux-i2c, linux-kernel, Mauro Carvalho Chehab, linux-media [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 12/24] media/video: fix dangling pointers 2010-03-20 22:02 ` Hans Verkuil 2010-03-21 11:31 ` Wolfram Sang @ 2010-03-21 13:46 ` Jean Delvare 2010-03-21 14:14 ` Mark Brown 2010-03-22 20:36 ` Jean Delvare 1 sibling, 2 replies; 14+ messages in thread From: Jean Delvare @ 2010-03-21 13:46 UTC (permalink / raw) To: Hans Verkuil Cc: Wolfram Sang, kernel-janitors, linux-i2c, linux-kernel, Mauro Carvalho Chehab, linux-media, Mark Brown 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 12/24] media/video: fix dangling pointers 2010-03-21 13:46 ` Jean Delvare @ 2010-03-21 14:14 ` Mark Brown 2010-03-21 16:09 ` Hans Verkuil 2010-03-22 20:36 ` Jean Delvare 1 sibling, 1 reply; 14+ messages in thread From: Mark Brown @ 2010-03-21 14:14 UTC (permalink / raw) To: Jean Delvare Cc: Hans Verkuil, Wolfram Sang, kernel-janitors, linux-i2c, linux-kernel, Mauro Carvalho Chehab, linux-media 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 12/24] media/video: fix dangling pointers 2010-03-21 14:14 ` Mark Brown @ 2010-03-21 16:09 ` Hans Verkuil 2010-03-22 20:33 ` Jean Delvare 2010-03-30 12:39 ` Wolfram Sang 0 siblings, 2 replies; 14+ messages in thread From: Hans Verkuil @ 2010-03-21 16:09 UTC (permalink / raw) To: Mark Brown Cc: Jean Delvare, Wolfram Sang, kernel-janitors, linux-i2c, linux-kernel, Mauro Carvalho Chehab, linux-media 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 12/24] media/video: fix dangling pointers 2010-03-21 16:09 ` Hans Verkuil @ 2010-03-22 20:33 ` Jean Delvare 2010-03-22 21:51 ` Mark Brown 2010-03-23 0:35 ` Wolfram Sang 2010-03-30 12:39 ` Wolfram Sang 1 sibling, 2 replies; 14+ messages in thread From: Jean Delvare @ 2010-03-22 20:33 UTC (permalink / raw) To: Hans Verkuil Cc: Mark Brown, Wolfram Sang, kernel-janitors, linux-i2c, linux-kernel, Mauro Carvalho Chehab, linux-media 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 12/24] media/video: fix dangling pointers 2010-03-22 20:33 ` Jean Delvare @ 2010-03-22 21:51 ` Mark Brown 2010-03-23 0:35 ` Wolfram Sang 1 sibling, 0 replies; 14+ messages in thread From: Mark Brown @ 2010-03-22 21:51 UTC (permalink / raw) To: Jean Delvare Cc: Hans Verkuil, Wolfram Sang, kernel-janitors, linux-i2c, linux-kernel, Mauro Carvalho Chehab, linux-media 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 12/24] media/video: fix dangling pointers 2010-03-22 20:33 ` Jean Delvare 2010-03-22 21:51 ` Mark Brown @ 2010-03-23 0:35 ` Wolfram Sang 1 sibling, 0 replies; 14+ messages in thread From: Wolfram Sang @ 2010-03-23 0:35 UTC (permalink / raw) To: Jean Delvare Cc: Hans Verkuil, Mark Brown, kernel-janitors, linux-i2c, linux-kernel, Mauro Carvalho Chehab, linux-media [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 12/24] media/video: fix dangling pointers 2010-03-21 16:09 ` Hans Verkuil 2010-03-22 20:33 ` Jean Delvare @ 2010-03-30 12:39 ` Wolfram Sang 2010-04-01 8:32 ` Hans Verkuil 1 sibling, 1 reply; 14+ messages in thread From: Wolfram Sang @ 2010-03-30 12:39 UTC (permalink / raw) To: Hans Verkuil Cc: Mark Brown, Jean Delvare, kernel-janitors, linux-i2c, linux-kernel, Mauro Carvalho Chehab, linux-media [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 12/24] media/video: fix dangling pointers 2010-03-30 12:39 ` Wolfram Sang @ 2010-04-01 8:32 ` Hans Verkuil 0 siblings, 0 replies; 14+ messages in thread From: Hans Verkuil @ 2010-04-01 8:32 UTC (permalink / raw) To: Wolfram Sang Cc: Mark Brown, Jean Delvare, kernel-janitors, linux-i2c, linux-kernel, Mauro Carvalho Chehab, linux-media 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 12/24] media/video: fix dangling pointers 2010-03-21 13:46 ` Jean Delvare 2010-03-21 14:14 ` Mark Brown @ 2010-03-22 20:36 ` Jean Delvare 1 sibling, 0 replies; 14+ messages in thread From: Jean Delvare @ 2010-03-22 20:36 UTC (permalink / raw) To: Wolfram Sang Cc: Hans Verkuil, kernel-janitors, linux-i2c, linux-kernel, Mauro Carvalho Chehab, linux-media, Mark Brown 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2010-04-01 8:32 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1269094385-16114-1-git-send-email-w.sang@pengutronix.de>
2010-03-20 14:12 ` [PATCH 10/24] media/radio: fix dangling pointers Wolfram Sang
2010-03-20 14:12 ` [PATCH 11/24] media/radio/si470x: " Wolfram Sang
2010-03-20 14:12 ` [PATCH 12/24] media/video: " Wolfram Sang
2010-03-20 22:02 ` Hans Verkuil
2010-03-21 11:31 ` Wolfram Sang
2010-03-21 13:46 ` Jean Delvare
2010-03-21 14:14 ` Mark Brown
2010-03-21 16:09 ` Hans Verkuil
2010-03-22 20:33 ` Jean Delvare
2010-03-22 21:51 ` Mark Brown
2010-03-23 0:35 ` Wolfram Sang
2010-03-30 12:39 ` Wolfram Sang
2010-04-01 8:32 ` Hans Verkuil
2010-03-22 20:36 ` Jean Delvare
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox