From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krzysztof Adamski Subject: Re: [PATCH] regulator: core: fix error path of regulator_ena_gpio_free Date: Wed, 24 Feb 2016 09:26:03 +0100 Message-ID: <20160224082602.GC19954@box2.japko.eu> References: <56CC6979.3050305@nvidia.com> <1456238830-13733-1-git-send-email-krzysztof.adamski@tieto.com> <56CC7863.4080202@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Return-path: Content-Disposition: inline In-Reply-To: <56CC7863.4080202-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Jon Hunter Cc: Mark Brown , "linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Linux Kernel Mailing List List-Id: linux-tegra@vger.kernel.org On Tue, Feb 23, 2016 at 03:18:59PM +0000, Jon Hunter wrote: > >On 23/02/16 14:47, Krzysztof Adamski wrote: >> Signed-off-by: Krzysztof Adamski >> Reported-by: Jon Hunter > >Nit ... I think that order of the above should be reversed. > Couldn't find any reference stating proper order of those tags and briefly looking at other commit messages shows this order as quite common. >> --- >> drivers/regulator/core.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c >> index 6ee9ba4..d1e7859 100644 >> --- a/drivers/regulator/core.c >> +++ b/drivers/regulator/core.c >> @@ -3919,7 +3919,7 @@ regulator_register(const struct regulator_desc *regulator_desc, >> if (ret != 0) { >> rdev_err(rdev, "Failed to request enable GPIO%d: %d\n", >> config->ena_gpio, ret); >> - goto wash; >> + goto clean; >> } >> } >> >> @@ -3942,7 +3942,7 @@ regulator_register(const struct regulator_desc *regulator_desc, >> >> ret = set_machine_constraints(rdev, constraints); >> if (ret < 0) >> - goto scrub; >> + goto wash; >> >> if (init_data && init_data->supply_regulator) >> rdev->supply_name = init_data->supply_regulator; >> @@ -3972,10 +3972,8 @@ out: >> unset_supplies: >> unset_regulator_supplies(rdev); >> >> -scrub: >> - regulator_ena_gpio_free(rdev); >> - >> wash: >> + regulator_ena_gpio_free(rdev); >> device_unregister(&rdev->dev); >> /* device core frees rdev */ >> rdev = ERR_PTR(ret); > >What about the case where device_register() fails? I think you still >call clean and so you will leak the gpio? > >Jon > True. I couldn't find anything more clever than calling regulator_ena_gpio_free() in two paths like in an upcomming v2. Putting it inside of regulator_dev_release() won't entirely fix the problem either as this won't be called in this particular case (device_register() fail). I personally still prefer calling regulator_ena_gpio_free() inside of regulator_register insted of deffering it to regulator_dev_release() as it seems to be clearer to me. Best regards, Krzysztof Adamski From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758148AbcBXI1c (ORCPT ); Wed, 24 Feb 2016 03:27:32 -0500 Received: from mail-wm0-f46.google.com ([74.125.82.46]:37683 "EHLO mail-wm0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758116AbcBXI1a (ORCPT ); Wed, 24 Feb 2016 03:27:30 -0500 Date: Wed, 24 Feb 2016 09:26:03 +0100 From: Krzysztof Adamski To: Jon Hunter Cc: Mark Brown , "linux-tegra@vger.kernel.org" , Linux Kernel Mailing List Subject: Re: [PATCH] regulator: core: fix error path of regulator_ena_gpio_free Message-ID: <20160224082602.GC19954@box2.japko.eu> References: <56CC6979.3050305@nvidia.com> <1456238830-13733-1-git-send-email-krzysztof.adamski@tieto.com> <56CC7863.4080202@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <56CC7863.4080202@nvidia.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-DomainID: tieto.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 23, 2016 at 03:18:59PM +0000, Jon Hunter wrote: > >On 23/02/16 14:47, Krzysztof Adamski wrote: >> Signed-off-by: Krzysztof Adamski >> Reported-by: Jon Hunter > >Nit ... I think that order of the above should be reversed. > Couldn't find any reference stating proper order of those tags and briefly looking at other commit messages shows this order as quite common. >> --- >> drivers/regulator/core.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c >> index 6ee9ba4..d1e7859 100644 >> --- a/drivers/regulator/core.c >> +++ b/drivers/regulator/core.c >> @@ -3919,7 +3919,7 @@ regulator_register(const struct regulator_desc *regulator_desc, >> if (ret != 0) { >> rdev_err(rdev, "Failed to request enable GPIO%d: %d\n", >> config->ena_gpio, ret); >> - goto wash; >> + goto clean; >> } >> } >> >> @@ -3942,7 +3942,7 @@ regulator_register(const struct regulator_desc *regulator_desc, >> >> ret = set_machine_constraints(rdev, constraints); >> if (ret < 0) >> - goto scrub; >> + goto wash; >> >> if (init_data && init_data->supply_regulator) >> rdev->supply_name = init_data->supply_regulator; >> @@ -3972,10 +3972,8 @@ out: >> unset_supplies: >> unset_regulator_supplies(rdev); >> >> -scrub: >> - regulator_ena_gpio_free(rdev); >> - >> wash: >> + regulator_ena_gpio_free(rdev); >> device_unregister(&rdev->dev); >> /* device core frees rdev */ >> rdev = ERR_PTR(ret); > >What about the case where device_register() fails? I think you still >call clean and so you will leak the gpio? > >Jon > True. I couldn't find anything more clever than calling regulator_ena_gpio_free() in two paths like in an upcomming v2. Putting it inside of regulator_dev_release() won't entirely fix the problem either as this won't be called in this particular case (device_register() fail). I personally still prefer calling regulator_ena_gpio_free() inside of regulator_register insted of deffering it to regulator_dev_release() as it seems to be clearer to me. Best regards, Krzysztof Adamski