| Message ID | 1349875236-5707-2-git-send-email-broonie@opensource.wolfsonmicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 10, 2012 at 10:20:35PM +0900, Mark Brown wrote: > Saves a little code and eliminates the possibility of introducing some > leaks. > *sigh* OK, I guess devm_* is here to stay and I have to get on with the program. I am still unhappy that half of the patches converting/using devm_* APIs are wrong (not these ones), but I will apply these 3. Thanks. > Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> > --- > drivers/input/touchscreen/wm831x-ts.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c > index bf0a4a6..c7eee56 100644 > --- a/drivers/input/touchscreen/wm831x-ts.c > +++ b/drivers/input/touchscreen/wm831x-ts.c > @@ -245,7 +245,8 @@ static __devinit int wm831x_ts_probe(struct platform_device *pdev) > if (core_pdata) > pdata = core_pdata->touch; > > - wm831x_ts = kzalloc(sizeof(struct wm831x_ts), GFP_KERNEL); > + wm831x_ts = devm_kzalloc(&pdev->dev, sizeof(struct wm831x_ts), > + GFP_KERNEL); > input_dev = input_allocate_device(); > if (!wm831x_ts || !input_dev) { > error = -ENOMEM; > @@ -376,7 +377,6 @@ err_data_irq: > free_irq(wm831x_ts->data_irq, wm831x_ts); > err_alloc: > input_free_device(input_dev); > - kfree(wm831x_ts); > > return error; > } > @@ -388,7 +388,6 @@ static __devexit int wm831x_ts_remove(struct platform_device *pdev) > free_irq(wm831x_ts->pd_irq, wm831x_ts); > free_irq(wm831x_ts->data_irq, wm831x_ts); > input_unregister_device(wm831x_ts->input_dev); > - kfree(wm831x_ts); > > return 0; > } > -- > 1.7.10.4 >
On Thu, Oct 11, 2012 at 12:39:55AM -0700, Dmitry Torokhov wrote: > On Wed, Oct 10, 2012 at 10:20:35PM +0900, Mark Brown wrote: > > Saves a little code and eliminates the possibility of introducing some > > leaks. > *sigh* OK, I guess devm_* is here to stay and I have to get on with the > program. I am still unhappy that half of the patches converting/using > devm_* APIs are wrong (not these ones), but I will apply these 3. What's the error pattern you're seeing? I've not noticed much of an issue here, but if there is one perhaps we can do something to make the error more obvious or harder to introduce. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 11, 2012 at 05:07:24PM +0900, Mark Brown wrote: > On Thu, Oct 11, 2012 at 12:39:55AM -0700, Dmitry Torokhov wrote: > > On Wed, Oct 10, 2012 at 10:20:35PM +0900, Mark Brown wrote: > > > Saves a little code and eliminates the possibility of introducing some > > > leaks. > > > *sigh* OK, I guess devm_* is here to stay and I have to get on with the > > program. I am still unhappy that half of the patches converting/using > > devm_* APIs are wrong (not these ones), but I will apply these 3. > > What's the error pattern you're seeing? I've not noticed much of an > issue here, but if there is one perhaps we can do something to make the > error more obvious or harder to introduce. int driver_probe() { devm_kzalloc(); input_allocate_device(); ... devm_request_irq(); ... input_register_device(); .... } void driver_remove() { input_unregister_device(); /* rely on deves for cleanup */ } The problem is that input device is freed but interrupts are still fully functional.
On Thu, Oct 11, 2012 at 01:27:49AM -0700, Dmitry Torokhov wrote: > On Thu, Oct 11, 2012 at 05:07:24PM +0900, Mark Brown wrote: > > > > What's the error pattern you're seeing? I've not noticed much of an > > issue here, but if there is one perhaps we can do something to make the > > error more obvious or harder to introduce. > devm_request_irq(); > The problem is that input device is freed but interrupts are still fully > functional. Ah, yes - that one I do spot all the time. I agree that devm_request_irq() is a menace, that error is far too easy to introduce and it always seems more work to work out if it's safe than the benefit in the cases where it can be used. The other devm APIs are less problematic, though. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 11, 2012 at 05:33:24PM +0900, Mark Brown wrote: > On Thu, Oct 11, 2012 at 01:27:49AM -0700, Dmitry Torokhov wrote: > > On Thu, Oct 11, 2012 at 05:07:24PM +0900, Mark Brown wrote: > > > > > > > What's the error pattern you're seeing? I've not noticed much of an > > > issue here, but if there is one perhaps we can do something to make the > > > error more obvious or harder to introduce. > > > devm_request_irq(); > > > The problem is that input device is freed but interrupts are still fully > > functional. > > Ah, yes - that one I do spot all the time. I agree that devm_request_irq() > is a menace, that error is far too easy to introduce and it always seems > more work to work out if it's safe than the benefit in the cases where > it can be used. Right. Another one (the IRQ again): have IRQ schedule [delayed] work and then use cancel_delayed_work() in ->remove() but rely on devres to free IRQ which is the wrong order. > > The other devm APIs are less problematic, though. I agree, they are indeed safer. I guess if I add devm_input* the some of the devm_request_*irq() will be safe as well, but we are not there yet.
diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c index bf0a4a6..c7eee56 100644 --- a/drivers/input/touchscreen/wm831x-ts.c +++ b/drivers/input/touchscreen/wm831x-ts.c @@ -245,7 +245,8 @@ static __devinit int wm831x_ts_probe(struct platform_device *pdev) if (core_pdata) pdata = core_pdata->touch; - wm831x_ts = kzalloc(sizeof(struct wm831x_ts), GFP_KERNEL); + wm831x_ts = devm_kzalloc(&pdev->dev, sizeof(struct wm831x_ts), + GFP_KERNEL); input_dev = input_allocate_device(); if (!wm831x_ts || !input_dev) { error = -ENOMEM; @@ -376,7 +377,6 @@ err_data_irq: free_irq(wm831x_ts->data_irq, wm831x_ts); err_alloc: input_free_device(input_dev); - kfree(wm831x_ts); return error; } @@ -388,7 +388,6 @@ static __devexit int wm831x_ts_remove(struct platform_device *pdev) free_irq(wm831x_ts->pd_irq, wm831x_ts); free_irq(wm831x_ts->data_irq, wm831x_ts); input_unregister_device(wm831x_ts->input_dev); - kfree(wm831x_ts); return 0; }
Saves a little code and eliminates the possibility of introducing some leaks. Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com> --- drivers/input/touchscreen/wm831x-ts.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)