LKML Archive on lore.kernel.org
 help / color / Atom feed
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Markus Elfring <Markus.Elfring@web.de>
Cc: linux-pwm@vger.kernel.org,
	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 21:08:00 +0100
Message-ID: <20191111200800.7njfdz7w3offt6we@pengutronix.de> (raw)
In-Reply-To: <a7a4656d-98b4-6bbb-e389-fe7028a38f97@web.de>

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

  reply index

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 [this message]
2019-11-11 21:30           ` Markus Elfring
     [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 publically 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=20191111200800.7njfdz7w3offt6we@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=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 \
    /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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git