On 24/02/16 08:26, Krzysztof Adamski wrote: > 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 <krzysztof.adamski@xxxxxxxxx> >>> Reported-by: Jon Hunter <jonathanh@xxxxxxxxxx> >> >> 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. Yes if you were to put regulator_ena_gpio_free() inside the regulator_dev_release(), you would still need to call regulator_ena_gpio_free() if the device_register fails. I see the way you have it now there are two places you call regulator_ena_gpio_free() in the error path. It is not a big deal, really either way. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html