Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933149AbcDLLCs (ORCPT ); Tue, 12 Apr 2016 07:02:48 -0400 Received: from mail-pf0-f174.google.com ([209.85.192.174]:34909 "EHLO mail-pf0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932393AbcDLLCq (ORCPT ); Tue, 12 Apr 2016 07:02:46 -0400 Subject: Re: [PATCH] regulator: reorder initialization steps in regulator_register() To: Boris Brezillon , Mark Brown , Liam Girdwood References: <1460457060-3946-1-git-send-email-boris.brezillon@free-electrons.com> Cc: linux-kernel@vger.kernel.org, =?UTF-8?Q?Heiko_St=c3=bcbner?= , Stephen Barber , Douglas Anderson , Brian Norris From: Caesar Wang Message-ID: <570CD5CA.5040400@gmail.com> Date: Tue, 12 Apr 2016 19:02:34 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <1460457060-3946-1-git-send-email-boris.brezillon@free-electrons.com> Content-Type: text/plain; charset=gbk; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2745 Lines: 90 ?? 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 > Reported-by: Stephen Barber Tested-by: Caesar Wang 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(®ulator_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