Re: [PATCH] regulator: core: fix error path of regulator_ena_gpio_free

Krzysztof Adamski
2016-02-24T08:26:03Z

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-++hxYGjEMp0AvxtiuMwx3w@public.gmane.org>
>> Reported-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
>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