| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 25 Aug 2022 13:32:50 +0200
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: Christian Kohlschütter
<christian@...lschutter.com>, broonie@...nel.org
Cc: heiko@...ech.de, lgirdwood@...il.com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-mmc@...r.kernel.org, linux-rockchip@...ts.infradead.org,
m.reichl@...etechno.de, robin.murphy@....com,
vincent.legoll@...il.com, wens@...nel.org
Subject: Re: [PATCH v4] regulator: core: Resolve supply name earlier to
prevent double-init
Hi Christian,
On 18.08.2022 14:46, Christian Kohlschütter wrote:
> From: Christian Kohlschütter <christian@...lschutter.com>
>
> Previously, an unresolved regulator supply reference upon calling
> regulator_register on an always-on or boot-on regulator caused
> set_machine_constraints to be called twice.
>
> This in turn may initialize the regulator twice, leading to voltage
> glitches that are timing-dependent. A simple, unrelated configuration
> change may be enough to hide this problem, only to be surfaced by
> chance.
>
> One such example is the SD-Card voltage regulator in a NanoPI R4S that
> would not initialize reliably unless the registration flow was just
> complex enough to allow the regulator to properly reset between calls.
>
> Fix this by re-arranging regulator_register, trying resolve the
> regulator's supply early enough that set_machine_constraints does not
> need to be called twice.
>
> Signed-off-by: Christian Kohlschütter <christian@...lschutter.com>
This patch landed recently in linux next as commit 8a866d527ac0
("regulator: core: Resolve supply name earlier to prevent double-init").
Unfortunately it breaks booting of Samsung Exynos 5800 based Peach-Pi
(arch/arm/boot/dts/exynos5800-peach-pi.dts) and Peach-Pit
(arch/arm/boot/dts/exynos5420-peach-pit.dts) Chromebooks. The last
message in the kernel log is a message about disabling 'vdd_1v2'
regulator. This regulator is not used directly, however it is a supply
for other critical regulators.
In the kernel log I also noticed lots of the following warnings, which
were not present before this patch:
[ 2.977695] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 2.987715] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 2.997524] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.007560] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.017240] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.026765] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.036501] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.045809] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.055497] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.064765] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.074549] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.085336] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.094815] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.104282] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.113755] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.124446] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.135298] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.144788] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.154562] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
[ 3.164340] debugfs: Directory '(null)-SUPPLY' with parent
'reg-dummy-regulator-dummy' already present!
They are printed when the main PMIC is being probed and registered. This
shows that the supplies for some regulators are not properly found,
probably due to the circular dependencies there ('vdd_1v2' is provided
by the same PMIC, which use it as a supply for other regulators).
Let me know if I can help debugging this issue further, but for now I
would suggest reverting this change.
> ---
> drivers/regulator/core.c | 52 +++++++++++++++++++++++++---------------
> 1 file changed, 33 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index d8373cb04f9..a5033c6ba01 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -5496,7 +5496,39 @@ regulator_register(const struct regulator_desc *regulator_desc,
> BLOCKING_INIT_NOTIFIER_HEAD(&rdev->notifier);
> INIT_DELAYED_WORK(&rdev->disable_work, regulator_disable_work);
>
> - /* preform any regulator specific init */
> + /* set regulator constraints */
> + if (init_data)
> + rdev->constraints = kmemdup(&init_data->constraints,
> + sizeof(*rdev->constraints),
> + GFP_KERNEL);
> + else
> + rdev->constraints = kzalloc(sizeof(*rdev->constraints),
> + GFP_KERNEL);
> + if (!rdev->constraints) {
> + ret = -ENOMEM;
> + goto clean;
> + }
> +
> + if (init_data && init_data->supply_regulator)
> + rdev->supply_name = init_data->supply_regulator;
> + else if (regulator_desc->supply_name)
> + rdev->supply_name = regulator_desc->supply_name;
> +
> + if ((rdev->supply_name && !rdev->supply) &&
> + (rdev->constraints->always_on ||
> + rdev->constraints->boot_on)) {
> + /* Try to resolve the name of the supplying regulator here first
> + * so we prevent double-initializing the regulator, which may
> + * cause timing-specific voltage brownouts/glitches that are
> + * hard to debug.
> + */
> + ret = regulator_resolve_supply(rdev);
> + if (ret)
> + rdev_dbg(rdev, "unable to resolve supply early: %pe\n",
> + ERR_PTR(ret));
> + }
> +
> + /* perform any regulator specific init */
> if (init_data && init_data->regulator_init) {
> ret = init_data->regulator_init(rdev->reg_data);
> if (ret < 0)
> @@ -5522,24 +5554,6 @@ regulator_register(const struct regulator_desc *regulator_desc,
> (unsigned long) atomic_inc_return(®ulator_no));
> dev_set_drvdata(&rdev->dev, rdev);
>
> - /* set regulator constraints */
> - if (init_data)
> - rdev->constraints = kmemdup(&init_data->constraints,
> - sizeof(*rdev->constraints),
> - GFP_KERNEL);
> - else
> - rdev->constraints = kzalloc(sizeof(*rdev->constraints),
> - GFP_KERNEL);
> - if (!rdev->constraints) {
> - ret = -ENOMEM;
> - goto wash;
> - }
> -
> - if (init_data && init_data->supply_regulator)
> - rdev->supply_name = init_data->supply_regulator;
> - else if (regulator_desc->supply_name)
> - rdev->supply_name = regulator_desc->supply_name;
> -
> ret = set_machine_constraints(rdev);
> if (ret == -EPROBE_DEFER) {
> /* Regulator might be in bypass mode and so needs its supply
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Powered by blists - more mailing lists