| Message ID | 20191111090357.13903-2-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/4] pwm: omap-dmtimer: remove pwmchip in .remove before making it unfunctional | expand |
> Implementation note: The put: label was never reached without a goto and > ret being unequal to 0, so the removed return statement is fine. This can look fine (in principle) because the label was repositioned here. Do you really want to call the function “of_node_put” at two places now? > +++ b/drivers/pwm/pwm-omap-dmtimer.c … > omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL); > if (!omap) { > - pdata->free(dm_timer); > - return -ENOMEM; > + ret = -ENOMEM; > + goto err_alloc_omap; > } … I suggest to reconsider your label name selection according to the Linux coding style. > @@ -339,13 +334,28 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev) … > +err_pwmchip_add: > + > + /* > + * *omap is allocated using devm_kzalloc, > + * so no free necessary here > + */ > +err_alloc_omap: > + > + pdata->free(dm_timer); Would the use of the label “free_dm_timer” be more appropriate? > +put: > + of_node_put(timer); … Can the label “put_node” be nicer? Regards, Markus
On Mon, Nov 11, 2019 at 02:32:30PM +0100, Markus Elfring wrote: > > Implementation note: The put: label was never reached without a goto and > > ret being unequal to 0, so the removed return statement is fine. > > This can look fine (in principle) because the label was repositioned here. > Do you really want to call the function “of_node_put” at two places now? Yes, this is in my eyes more sensible. Either you have the expected path and the error path interwinded, or you have to duplicate some cleanup. IMHO the latter variant is the one that is easier to understand and the one where it's less likely to oversee a needed cleanup. > > +++ b/drivers/pwm/pwm-omap-dmtimer.c > … > > omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL); > > if (!omap) { > > - pdata->free(dm_timer); > > - return -ENOMEM; > > + ret = -ENOMEM; > > + goto err_alloc_omap; > > } > … > > I suggest to reconsider your label name selection according to > the Linux coding style. Documentation/process/coding-style.rst states: "Choose label names which say what the goto does or why the goto exists." So I'd say my names are perfectly fine. > > @@ -339,13 +334,28 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev) > … > > +err_pwmchip_add: > > + > > + /* > > + * *omap is allocated using devm_kzalloc, > > + * so no free necessary here > > + */ > > +err_alloc_omap: > > + > > + pdata->free(dm_timer); > > Would the use of the label “free_dm_timer” be more appropriate? Either you name your labels after what the code at the label does (then "free_dm_timer" is good) or you name it after why you are here (and then err_alloc_omap is fine). I prefer the latter style and then the label name always has to correspond to the action just above it (if any). That's why I grouped the "err_alloc_omap" label to a comment saying that *omap doesn't need to be freed. > > +put: > > + of_node_put(timer); > … > > Can the label “put_node” be nicer? I agree that the label name is bad. I kept the name here and after the 3rd patch the label names are consistent. Best regards Uwe
>> Do you really want to call the function “of_node_put” at two places now? > > Yes, this is in my eyes more sensible. Thanks for this explanation. > Either you have the expected path and the error path interwinded, > and the error path interwinded, This is also reasonable then. This design approach provides the possibility to release a few resources earlier before using additional functionality. > or you have to duplicate some cleanup. * This can be required. * I imagine that specific software infrastructures can help to avoid such duplication, can't they? > IMHO the latter variant is the one that is easier to understand and the > one where it's less likely to oversee a needed cleanup. I am curious on how the clarification will be continued. >>> +++ b/drivers/pwm/pwm-omap-dmtimer.c >> … >>> omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL); >>> if (!omap) { >>> - pdata->free(dm_timer); >>> - return -ENOMEM; >>> + ret = -ENOMEM; >>> + goto err_alloc_omap; >>> } >> … >> >> I suggest to reconsider your label name selection according to >> the Linux coding style. > > Documentation/process/coding-style.rst states: "Choose label names which > say what the goto does or why the goto exists." So I'd say my names are > perfectly fine. The guidance from the example after this quotation might be still too terse so far, isn't it? >>> @@ -339,13 +334,28 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev) >> … >>> +err_pwmchip_add: >>> + >>> + /* >>> + * *omap is allocated using devm_kzalloc, >>> + * so no free necessary here >>> + */ >>> +err_alloc_omap: >>> + >>> + pdata->free(dm_timer); >> >> Would the use of the label “free_dm_timer” be more appropriate? > > Either you name your labels after what the code at the label does > (then "free_dm_timer" is good) I got used to this approach. > you name it after why you are here (and then err_alloc_omap is fine). This choice can trigger special software design consequences. > I prefer the latter style and then the label > name always has to correspond to the action just above it (if any). > That's why I grouped the "err_alloc_omap" label to a comment saying that > *omap doesn't need to be freed. I am also curious if any other contributors would like to share more views around this choice. >>> +put: >>> + of_node_put(timer); >> … >> >> Can the label “put_node” be nicer? > > I agree that the label name is bad. I find your agreement on this aspect interesting then. > I kept the name here and after the 3rd patch the label names are consistent. Can such an evolution be questionable? Regards, Markus
diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c index bdf94c78655f..e36fcad668a6 100644 --- a/drivers/pwm/pwm-omap-dmtimer.c +++ b/drivers/pwm/pwm-omap-dmtimer.c @@ -298,15 +298,10 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev) goto put; } -put: - of_node_put(timer); - if (ret < 0) - return ret; - omap = devm_kzalloc(&pdev->dev, sizeof(*omap), GFP_KERNEL); if (!omap) { - pdata->free(dm_timer); - return -ENOMEM; + ret = -ENOMEM; + goto err_alloc_omap; } omap->pdata = pdata; @@ -339,13 +334,28 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev) ret = pwmchip_add(&omap->chip); if (ret < 0) { dev_err(&pdev->dev, "failed to register PWM\n"); - omap->pdata->free(omap->dm_timer); - return ret; + goto err_pwmchip_add; } + of_node_put(timer); + platform_set_drvdata(pdev, omap); return 0; + +err_pwmchip_add: + + /* + * *omap is allocated using devm_kzalloc, + * so no free necessary here + */ +err_alloc_omap: + + pdata->free(dm_timer); +put: + of_node_put(timer); + + return ret; } static int pwm_omap_dmtimer_remove(struct platform_device *pdev)
Instead of doing error handling in the middle of pwm_omap_dmtimer_probe() move error handling and freeing the reference to timer to the end. This fixes a resource leak as dm_timer wasn't freed when allocating *omap failed. Implementation note: The put: label was never reached without a goto and ret being unequal to 0, so the removed return statement is fine. Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/pwm-omap-dmtimer.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)