| Message ID | 1379188343-18904-1-git-send-email-badarkhe.manish@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Manish, Thanks for the patch. I have few comments inline, though. On Sunday 15 of September 2013 01:22:23 Manish Badarkhe wrote: > Update the code to use devm_* API so that driver core will manage > resources. > > Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com> > --- > > :100644 100644 440ce32... b4db721... M drivers/input/keyboard/gpio_keys.c > > drivers/input/keyboard/gpio_keys.c | 18 ++++++------------ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/drivers/input/keyboard/gpio_keys.c > b/drivers/input/keyboard/gpio_keys.c index 440ce32..b4db721 100644 > --- a/drivers/input/keyboard/gpio_keys.c > +++ b/drivers/input/keyboard/gpio_keys.c > @@ -588,7 +588,7 @@ gpio_keys_get_devtree_pdata(struct device *dev) > goto err_out; > } > > - pdata = kzalloc(sizeof(*pdata) + nbuttons * (sizeof *button), > + pdata = devm_kzalloc(dev, sizeof(*pdata) + nbuttons * > (sizeof(*button)), GFP_KERNEL); > if (!pdata) { > error = -ENOMEM; > @@ -618,7 +618,7 @@ gpio_keys_get_devtree_pdata(struct device *dev) > dev_err(dev, > "Failed to get gpio flags, error: %d\n", > error); > - goto err_free_pdata; > + goto err_out; Since the only thing done after err_out label is returning the error, you can simply return the error here. > } > > button = &pdata->buttons[i++]; > @@ -630,7 +630,7 @@ gpio_keys_get_devtree_pdata(struct device *dev) > dev_err(dev, "Button without keycode: 0x%x\n", > button->gpio); > error = -EINVAL; > - goto err_free_pdata; > + goto err_out; Ditto. > } > > button->desc = of_get_property(pp, "label", NULL); > @@ -647,13 +647,11 @@ gpio_keys_get_devtree_pdata(struct device *dev) > > if (pdata->nbuttons == 0) { > error = -EINVAL; > - goto err_free_pdata; > + goto err_out; Ditto. > } > > return pdata; > > -err_free_pdata: > - kfree(pdata); > err_out: > return ERR_PTR(error); Then the whole error path here can be dropped. > } > @@ -699,10 +697,10 @@ static int gpio_keys_probe(struct platform_device > *pdev) return PTR_ERR(pdata); > } > > - ddata = kzalloc(sizeof(struct gpio_keys_drvdata) + > + ddata = devm_kzalloc(&pdev->dev, sizeof(struct gpio_keys_drvdata) + > pdata->nbuttons * sizeof(struct gpio_button_data), > GFP_KERNEL); > - input = input_allocate_device(); > + input = devm_input_allocate_device(&pdev->dev); > if (!ddata || !input) { > dev_err(dev, "failed to allocate state\n"); > error = -ENOMEM; > @@ -768,8 +766,6 @@ static int gpio_keys_probe(struct platform_device > *pdev) gpio_remove_key(&ddata->data[i]); > > fail1: > - input_free_device(input); > - kfree(ddata); > /* If we have no platform data, we allocated pdata dynamically. */ > if (!dev_get_platdata(&pdev->dev)) > kfree(pdata); This is incorrect and unnecessary. Since pdata was allocated using devm_ helper it will be freed automatically. As a side note, if you want to explicitly free memory allocated using devm_ helpers, you need to do it using their devm_ free counterparts, not directly. > @@ -796,8 +792,6 @@ static int gpio_keys_remove(struct platform_device > *pdev) if (!dev_get_platdata(&pdev->dev)) > kfree(ddata->pdata); Same here. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tomasz On Tue, Sep 17, 2013 at 4:49 PM, Tomasz Figa <t.figa@samsung.com> wrote: > Hi Manish, > > Thanks for the patch. I have few comments inline, though. > > On Sunday 15 of September 2013 01:22:23 Manish Badarkhe wrote: >> Update the code to use devm_* API so that driver core will manage >> resources. >> >> Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com> >> --- >> >> :100644 100644 440ce32... b4db721... M drivers/input/keyboard/gpio_keys.c >> >> drivers/input/keyboard/gpio_keys.c | 18 ++++++------------ >> 1 file changed, 6 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/input/keyboard/gpio_keys.c >> b/drivers/input/keyboard/gpio_keys.c index 440ce32..b4db721 100644 >> --- a/drivers/input/keyboard/gpio_keys.c >> +++ b/drivers/input/keyboard/gpio_keys.c >> @@ -588,7 +588,7 @@ gpio_keys_get_devtree_pdata(struct device *dev) >> goto err_out; >> } >> >> - pdata = kzalloc(sizeof(*pdata) + nbuttons * (sizeof *button), >> + pdata = devm_kzalloc(dev, sizeof(*pdata) + nbuttons * >> (sizeof(*button)), GFP_KERNEL); >> if (!pdata) { >> error = -ENOMEM; >> @@ -618,7 +618,7 @@ gpio_keys_get_devtree_pdata(struct device *dev) >> dev_err(dev, >> "Failed to get gpio flags, error: %d\n", >> error); >> - goto err_free_pdata; >> + goto err_out; > > Since the only thing done after err_out label is returning the error, you > can simply return the error here. Agreed, will do that. > >> } >> >> button = &pdata->buttons[i++]; >> @@ -630,7 +630,7 @@ gpio_keys_get_devtree_pdata(struct device *dev) >> dev_err(dev, "Button without keycode: 0x%x\n", >> button->gpio); >> error = -EINVAL; >> - goto err_free_pdata; >> + goto err_out; > > Ditto. Ok > >> } >> >> button->desc = of_get_property(pp, "label", NULL); >> @@ -647,13 +647,11 @@ gpio_keys_get_devtree_pdata(struct device *dev) >> >> if (pdata->nbuttons == 0) { >> error = -EINVAL; >> - goto err_free_pdata; >> + goto err_out; > > Ditto. Ok > >> } >> >> return pdata; >> >> -err_free_pdata: >> - kfree(pdata); >> err_out: >> return ERR_PTR(error); > > Then the whole error path here can be dropped. Ok, I will drop this error path. > >> } >> @@ -699,10 +697,10 @@ static int gpio_keys_probe(struct platform_device >> *pdev) return PTR_ERR(pdata); >> } >> >> - ddata = kzalloc(sizeof(struct gpio_keys_drvdata) + >> + ddata = devm_kzalloc(&pdev->dev, sizeof(struct gpio_keys_drvdata) + >> pdata->nbuttons * sizeof(struct gpio_button_data), >> GFP_KERNEL); >> - input = input_allocate_device(); >> + input = devm_input_allocate_device(&pdev->dev); >> if (!ddata || !input) { >> dev_err(dev, "failed to allocate state\n"); >> error = -ENOMEM; >> @@ -768,8 +766,6 @@ static int gpio_keys_probe(struct platform_device >> *pdev) gpio_remove_key(&ddata->data[i]); >> >> fail1: >> - input_free_device(input); >> - kfree(ddata); >> /* If we have no platform data, we allocated pdata dynamically. */ >> if (!dev_get_platdata(&pdev->dev)) >> kfree(pdata); > > This is incorrect and unnecessary. Since pdata was allocated using devm_ > helper it will be freed automatically. As a side note, if you want to > explicitly free memory allocated using devm_ helpers, you need to do it > using their devm_ free counterparts, not directly. Ok, I will remove this as "pdata" is getting freed automatically. >> @@ -796,8 +792,6 @@ static int gpio_keys_remove(struct platform_device >> *pdev) if (!dev_get_platdata(&pdev->dev)) >> kfree(ddata->pdata); > > Same here. Ok, I will remove this as "pdata" is getting freed automatically. Regards Manish Badarkhe -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Manish, On Sun, Sep 15, 2013 at 01:22:23AM +0530, Manish Badarkhe wrote: > Update the code to use devm_* API so that driver core will manage > resources. > And the benefit of this would be...? There are still resources that are managed in traditional way and I really dislike mixing the 2 styles. I can see applying patch that converts the driver to use devm_ for all its resources and gets rid of the remove() method altogether, but I am not sure how beneficial this kind of changes are for existing drivers. Thanks.
Hi Dmitry, On Wed, Sep 18, 2013 at 12:22 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Hi Manish, > > On Sun, Sep 15, 2013 at 01:22:23AM +0530, Manish Badarkhe wrote: >> Update the code to use devm_* API so that driver core will manage >> resources. >> > > And the benefit of this would be...? > > There are still resources that are managed in traditional way and I > really dislike mixing the 2 styles. I can see applying patch that > converts the driver to use devm_ for all its resources and gets rid of > the remove() method altogether, but I am not sure how beneficial this > kind of changes are for existing drivers. IMO devm_ makes us to manage resources properly without much care about freeing it ( as devm_handles freeing automatically). But, stiil there are use cases which makes us to mix this devm_ with traditionally managed resources. Not much sure about any other benefits. Regards Manish Badarkhe -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 18, 2013 at 12:41:11AM +0530, Manish Badarkhe wrote: > Hi Dmitry, > > On Wed, Sep 18, 2013 at 12:22 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > Hi Manish, > > > > On Sun, Sep 15, 2013 at 01:22:23AM +0530, Manish Badarkhe wrote: > >> Update the code to use devm_* API so that driver core will manage > >> resources. > >> > > > > And the benefit of this would be...? > > > > There are still resources that are managed in traditional way and I > > really dislike mixing the 2 styles. I can see applying patch that > > converts the driver to use devm_ for all its resources and gets rid of > > the remove() method altogether, but I am not sure how beneficial this > > kind of changes are for existing drivers. > > IMO devm_ makes us to manage resources properly without much care about freeing > it ( as devm_handles freeing automatically). Are the resources released improperly in the current version of the driver? IOW I do not see the point in applying this patch as it does not make the driver materially better. Thanks.
Hi Dmitry, On Fri, Sep 20, 2013 at 2:52 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Wed, Sep 18, 2013 at 12:41:11AM +0530, Manish Badarkhe wrote: >> Hi Dmitry, >> >> On Wed, Sep 18, 2013 at 12:22 AM, Dmitry Torokhov >> <dmitry.torokhov@gmail.com> wrote: >> > Hi Manish, >> > >> > On Sun, Sep 15, 2013 at 01:22:23AM +0530, Manish Badarkhe wrote: >> >> Update the code to use devm_* API so that driver core will manage >> >> resources. >> >> >> > >> > And the benefit of this would be...? >> > >> > There are still resources that are managed in traditional way and I >> > really dislike mixing the 2 styles. I can see applying patch that >> > converts the driver to use devm_ for all its resources and gets rid of >> > the remove() method altogether, but I am not sure how beneficial this >> > kind of changes are for existing drivers. >> >> IMO devm_ makes us to manage resources properly without much care about freeing >> it ( as devm_handles freeing automatically). > > Are the resources released improperly in the current version of the > driver? IOW I do not see the point in applying this patch as it does not > make the driver materially better. Resources are released properly in current version of driver but using a traditional way. This patch is just a clean up and to use new devm_ variant. Yes, This patch does not make a driver materially better but make use of "devm_" variant as like any other drivers. Thanks Manish Badarkhe -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index 440ce32..b4db721 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -588,7 +588,7 @@ gpio_keys_get_devtree_pdata(struct device *dev) goto err_out; } - pdata = kzalloc(sizeof(*pdata) + nbuttons * (sizeof *button), + pdata = devm_kzalloc(dev, sizeof(*pdata) + nbuttons * (sizeof(*button)), GFP_KERNEL); if (!pdata) { error = -ENOMEM; @@ -618,7 +618,7 @@ gpio_keys_get_devtree_pdata(struct device *dev) dev_err(dev, "Failed to get gpio flags, error: %d\n", error); - goto err_free_pdata; + goto err_out; } button = &pdata->buttons[i++]; @@ -630,7 +630,7 @@ gpio_keys_get_devtree_pdata(struct device *dev) dev_err(dev, "Button without keycode: 0x%x\n", button->gpio); error = -EINVAL; - goto err_free_pdata; + goto err_out; } button->desc = of_get_property(pp, "label", NULL); @@ -647,13 +647,11 @@ gpio_keys_get_devtree_pdata(struct device *dev) if (pdata->nbuttons == 0) { error = -EINVAL; - goto err_free_pdata; + goto err_out; } return pdata; -err_free_pdata: - kfree(pdata); err_out: return ERR_PTR(error); } @@ -699,10 +697,10 @@ static int gpio_keys_probe(struct platform_device *pdev) return PTR_ERR(pdata); } - ddata = kzalloc(sizeof(struct gpio_keys_drvdata) + + ddata = devm_kzalloc(&pdev->dev, sizeof(struct gpio_keys_drvdata) + pdata->nbuttons * sizeof(struct gpio_button_data), GFP_KERNEL); - input = input_allocate_device(); + input = devm_input_allocate_device(&pdev->dev); if (!ddata || !input) { dev_err(dev, "failed to allocate state\n"); error = -ENOMEM; @@ -768,8 +766,6 @@ static int gpio_keys_probe(struct platform_device *pdev) gpio_remove_key(&ddata->data[i]); fail1: - input_free_device(input); - kfree(ddata); /* If we have no platform data, we allocated pdata dynamically. */ if (!dev_get_platdata(&pdev->dev)) kfree(pdata); @@ -796,8 +792,6 @@ static int gpio_keys_remove(struct platform_device *pdev) if (!dev_get_platdata(&pdev->dev)) kfree(ddata->pdata); - kfree(ddata); - return 0; }
Update the code to use devm_* API so that driver core will manage resources. Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com> --- :100644 100644 440ce32... b4db721... M drivers/input/keyboard/gpio_keys.c drivers/input/keyboard/gpio_keys.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)