linux-kernel - [PATCH v4 1/2] mfd: da9062: enable being a system-power-controller

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <c6e19670802355222a228061f37aadace2764933.1657104160.git.helmut.grohne@intenta.de>
Date:   Wed, 6 Jul 2022 14:29:48 +0200
From:   Helmut Grohne <helmut.grohne@...enta.de>
To:     Support Opensource <support.opensource@...semi.com>,
        Lee Jones <lee.jones@...aro.org>
CC:     <linux-kernel@...r.kernel.org>,
        Adam Thomson <Adam.Thomson.Opensource@...semi.com>,
        Mark Brown <broonie@...nel.org>,
        Wolfram Sang <wsa@...-dreams.de>
Subject: [PATCH v4 1/2] mfd: da9062: enable being a system-power-controller

The DA9062 can be the device used to power the CPU. In that case, it can
be used to power off the system. In the CONTROL_A register, the M_*_EN
bits must be zero for the corresponding *_EN bits to have an effect. We
zero them all to turn off the system.

Signed-off-by: Helmut Grohne <helmut.grohne@...enta.de>
---
 drivers/mfd/da9062-core.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Compared to v3, this addresses the following concerns raised by Lee Jones:
 * Consistently start error messages upper case.
 * Pull i2c_get_clientdata out of the call again. In his previous review he
   remarked the joint declaration and call as "passable", which I misunderstood
   as "can be passed to regmap_update_bits directly" while the intended meaning
   was more like "acceptable". This is part is now reverted back to the v2
   form.

Adam Thomson's observation about the use of i2c in atomic context is observable
in reality since a few kernel releases. When powering off the system, I now see
this:

reboot: Power down
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/i2c/i2c-core.h:40 i2c_transfer+0x104/0x134
---[ end trace 0000000000000000 ]---

Looking at the actual warning, this is actually about using non-atomic xfers in
atomic context (i.e. he's totally right). Wolfram Sang worked on this problem
on a more fundamental level and added a patch "i2c: core: introduce callbacks
for atomic transfers" (63b96983a5dd). Unfortunately, the relevant i2c driver in
use here (i2c-cadence) does not include atomic transfer callbacks yet, which is
why I see the warning. However, that only confirms the way of using i2c code in
pm_power_off hooks as the "right" way to do it.

In all of my tests, the system powered off reliably.

I still didn't include Adam Thomson's Reviewed-by from v2, because the code is
still noticeably different.

Thanks for bearing with me

Helmut
diff --git a/drivers/mfd/da9062-core.c b/drivers/mfd/da9062-core.c
index 2774b2cbaea6..94d447769da3 100644
--- a/drivers/mfd/da9062-core.c
+++ b/drivers/mfd/da9062-core.c
@@ -620,6 +620,27 @@ static const struct of_device_id da9062_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, da9062_dt_ids);

+static struct i2c_client *da9062_i2c_client;
+
+static void da9062_power_off(void)
+{
+	struct da9062 *chip = i2c_get_clientdata(da9062_i2c_client);
+	int ret;
+
+	ret = regmap_update_bits(
+		chip->regmap,
+		DA9062AA_CONTROL_A,
+		DA9062AA_SYSTEM_EN_MASK | DA9062AA_POWER_EN_MASK |
+			DA9062AA_POWER1_EN_MASK | DA9062AA_M_SYSTEM_EN_MASK |
+			DA9062AA_M_POWER_EN_MASK | DA9062AA_M_POWER1_EN_MASK,
+		0
+	);
+
+	if (ret < 0)
+		dev_err(&da9062_i2c_client->dev,
+			"Failed to power the system off (err=%d)\n", ret);
+}
+
 static int da9062_i2c_probe(struct i2c_client *i2c,
 	const struct i2c_device_id *id)
 {
@@ -720,6 +741,15 @@ static int da9062_i2c_probe(struct i2c_client *i2c,
 		return ret;
 	}

+	if (of_device_is_system_power_controller(i2c->dev.of_node)) {
+		if (!pm_power_off) {
+			da9062_i2c_client = i2c;
+			pm_power_off = da9062_power_off;
+		} else {
+			dev_warn(&i2c->dev, "Poweroff callback already assigned\n");
+		}
+	}
+
 	return ret;
 }

@@ -727,6 +757,11 @@ static int da9062_i2c_remove(struct i2c_client *i2c)
 {
 	struct da9062 *chip = i2c_get_clientdata(i2c);

+	if (pm_power_off == da9062_power_off)
+		pm_power_off = NULL;
+	if (da9062_i2c_client)
+		da9062_i2c_client = NULL;
+
 	mfd_remove_devices(chip->dev);
 	regmap_del_irq_chip(i2c->irq, chip->regmap_irq);

-- 
2.30.2

Dipl.-Inf. Helmut Grohne
Research and Development
Application Engineering

Phone: +49 (371) 24354 0 ∙ Fax:  +49 (371) 24354 020
helmut.grohne@...enta.de ∙ https://www.intenta.de

Intenta GmbH ∙ Ahornstraße 55 ∙ 09112 Chemnitz, Germany
Managing Director: Dr.-Ing. Basel Fardi ∙ VAT/USt-IdNr.: DE 275745394
Commercial register: HRB 26404 Amtsgericht Chemnitz