Re: [PATCH] regulator: reorder initialization steps in regulator_register()

[Caesar Wang]

在 2016年04月12日 18:31, Boris Brezillon 写道:
> device_register() is calling ->get_voltage() as part of is sysfs attribute
> initialization process, and this functions might need to know the regulator
> constraints to return a valid value.
> This is at least true for the pwm regulator driver (when operating in
> continuous mode) which needs to know the minimum and maximum voltage values
> to calculate the current voltage:
>
> min_uV + (((max_uV - min_uV) * dutycycle) / 100);
>
> Move device_register() after set_machine_constraints() to make sure those
> constraints are correctly initialized when ->get_voltage() is called.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> Reported-by: Stephen Barber <smbarber@xxxxxxxxxxxx>
Tested-by: Caesar Wang <wxt@xxxxxxxxxxxxxx>

Yep, do many reboot tests on my rk3399 evb board.
that's seem fix the crash issue.

[ 0.744584] task: ffffffc0edab8000 ti: ffffffc0edac0000 task.ti: 
ffffffc0edac0000 [ 0.752803]
PC is at pwm_regulator_get_voltage+0x28/0x54 [ 0.758628]
LR is at pwm_regulator_get_voltage+0x14/0x54
....

As the same reported on https://patchwork.kernel.org/patch/8702441/

> ---
>   drivers/regulator/core.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index e0b7642..8258568 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3950,13 +3950,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
>   	rdev->dev.parent = dev;
>   	dev_set_name(&rdev->dev, "regulator.%lu",
>   		    (unsigned long) atomic_inc_return(&regulator_no));
> -	ret = device_register(&rdev->dev);
> -	if (ret != 0) {
> -		put_device(&rdev->dev);
> -		goto wash;
> -	}
> -
> -	dev_set_drvdata(&rdev->dev, rdev);
>   
>   	/* set regulator constraints */
>   	if (init_data)
> @@ -3964,7 +3957,15 @@ regulator_register(const struct regulator_desc *regulator_desc,
>   
>   	ret = set_machine_constraints(rdev, constraints);
>   	if (ret < 0)
> -		goto scrub;
> +		goto wash;
> +
> +	ret = device_register(&rdev->dev);
> +	if (ret != 0) {
> +		put_device(&rdev->dev);
> +		goto wash;
> +	}
> +
> +	dev_set_drvdata(&rdev->dev, rdev);
>   
>   	if (init_data && init_data->supply_regulator)
>   		rdev->supply_name = init_data->supply_regulator;
> @@ -3993,8 +3994,6 @@ out:
>   
>   unset_supplies:
>   	unset_regulator_supplies(rdev);
> -
> -scrub:
>   	regulator_ena_gpio_free(rdev);
>   	device_unregister(&rdev->dev);
>   	/* device core frees rdev */
> @@ -4002,6 +4001,7 @@ scrub:
>   	goto out;
>   
>   wash:
> +	kfree(rdev->constraints);
>   	regulator_ena_gpio_free(rdev);
>   clean:
>   	kfree(rdev);
>
> --
> Thanks,
> Caesar