From: Markus Elfring <Markus.Elfring@web.de>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
linux-pwm@vger.kernel.org
Cc: Neil Armstrong <narmstrong@baylibre.com>,
Neil Brown <neilb@suse.de>,
kernel-janitors@vger.kernel.org,
LKML <linux-kernel@vger.kernel.org>,
Thierry Reding <thierry.reding@gmail.com>,
kernel@pengutronix.de
Subject: Re: [PATCH 2/4] pwm: omap-dmtimer: simplify error handling
Date: Mon, 11 Nov 2019 22:30:42 +0100 [thread overview]
Message-ID: <8328c71e-0a9a-31f4-afb5-b4b1bf28b004@web.de> (raw)
In-Reply-To: <20191111200800.7njfdz7w3offt6we@pengutronix.de>
>> 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
next prev parent reply other threads:[~2019-11-11 21:30 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-09 12:26 [PATCH] pwm: omap-dmtimer: Add missing put_device() call in pwm_omap_dmtimer_probe() Markus Elfring
2019-11-11 7:19 ` Uwe Kleine-König
[not found] ` <20191111090357.13903-1-u.kleine-koenig@pengutronix.de>
[not found] ` <20191111091606.qf7gj5gln3khfnsl@pengutronix.de>
2019-11-11 13:28 ` [0/4] pwm: omap-dmtimer: Software improvements Markus Elfring
2019-11-11 13:28 ` Markus Elfring
2019-11-11 19:57 ` Uwe Kleine-König
2019-11-11 13:30 ` [PATCH 1/4] pwm: omap-dmtimer: remove pwmchip in .remove before making it unfunctional Markus Elfring
2019-11-11 20:00 ` Uwe Kleine-König
2019-11-11 21:00 ` [1/4] " Markus Elfring
[not found] ` <20191111090357.13903-2-u.kleine-koenig@pengutronix.de>
2019-11-11 13:32 ` [PATCH 2/4] pwm: omap-dmtimer: simplify error handling Markus Elfring
2019-11-11 20:08 ` Uwe Kleine-König
2019-11-11 21:30 ` Markus Elfring [this message]
[not found] ` <20191111090357.13903-3-u.kleine-koenig@pengutronix.de>
2019-11-11 13:41 ` [PATCH 3/4] pwm: omap-dmtimer: put_device() after of_find_device_by_node() Markus Elfring
2019-11-11 20:09 ` Uwe Kleine-König
2019-11-11 21:38 ` [3/4] " Markus Elfring
2019-11-11 21:46 ` Uwe Kleine-König
2019-11-12 7:40 ` Markus Elfring
[not found] ` <20191111090357.13903-4-u.kleine-koenig@pengutronix.de>
2019-11-11 13:47 ` [PATCH 4/4] pwm: omap-dmtimer: Allow compiling with COMPILE_TEST Markus Elfring
2019-11-11 13:47 ` Markus Elfring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8328c71e-0a9a-31f4-afb5-b4b1bf28b004@web.de \
--to=markus.elfring@web.de \
--cc=kernel-janitors@vger.kernel.org \
--cc=kernel@pengutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=narmstrong@baylibre.com \
--cc=neilb@suse.de \
--cc=thierry.reding@gmail.com \
--cc=u.kleine-koenig@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).