lkml.org 
[lkml]   [2019]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/4] pwm: omap-dmtimer: simplify error handling
From
Date
>> 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

\
 
 \ /
  Last update: 2019-11-11 22:31    [W:0.057 / U:0.408 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site