[PATCH v6 0/5] mfd: tps6586x: register restart handler

[PATCH v6 0/5] mfd: tps6586x: register restart handler

[Benjamin Bara]

Hi!

The Tegra20 requires an enabled VDE power domain during startup. As the
VDE is currently not used, it is disabled during runtime.
Since 8f0c714ad9be, there is a workaround for the "normal restart path"
which enables the VDE before doing PMC's warm reboot. This workaround is
not executed in the "emergency restart path", leading to a hang-up
during start.

This series implements and registers a new pmic-based restart handler
for boards with tps6586x. This cold reboot ensures that the VDE power
domain is enabled during startup on tegra20-based boards.

Since bae1d3a05a8b, i2c transfers are non-atomic while preemption is
disabled (which is e.g. done during panic()). This could lead to
warnings ("Voluntary context switch within RCU") in i2c-based restart
handlers during emergency restart. The state of preemption should be
detected by i2c_in_atomic_xfer_mode() to use atomic i2c xfer when
required. Beside the new system_state check, the check is the same as
the one pre v5.2.

v5: https://lore.kernel.org/r/20230327-tegra-pmic-reboot-v5-0-ab090e03284d@skidata.com

v6:
- drop 4/6 to abort restart on unexpected failure (suggested by Dmitry)
- 4,5: fix comments in handlers (suggested by Lee)
- 4,5: same delay for both handlers (suggested by Lee)

v5:
- introduce new 3 & 4, therefore 3 -> 5, 4 -> 6
- 3: provide dev to sys_off handler, if it is known
- 4: return NOTIFY_DONE from sys_off_notify, to avoid skipping
- 5: drop Reviewed-by from Dmitry, add poweroff timeout
- 5,6: return notifier value instead of direct errno from handler
- 5,6: use new dev field instead of passing dev as cb_data
- 5,6: increase timeout values based on error observations
- 6: skip unsupported reboot modes in restart handler

---
Benjamin Bara (5):
      kernel/reboot: emergency_restart: set correct system_state
      i2c: core: run atomic i2c xfer when !preemptible
      kernel/reboot: add device to sys_off_handler
      mfd: tps6586x: use devm-based power off handler
      mfd: tps6586x: register restart handler

 drivers/i2c/i2c-core.h |  2 +-
 drivers/mfd/tps6586x.c | 55 ++++++++++++++++++++++++++++++++++++++++++--------
 include/linux/reboot.h |  3 +++
 kernel/reboot.c        |  4 ++++
 4 files changed, 55 insertions(+), 9 deletions(-)
---
base-commit: 197b6b60ae7bc51dd0814953c562833143b292aa
change-id: 20230327-tegra-pmic-reboot-4175ff814a4b

Best regards,
-- 
Benjamin Bara <benjamin.bara@skidata.com>

[PATCH v6 1/5] kernel/reboot: emergency_restart: set correct system_state

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

As the emergency restart does not call kernel_restart_prepare(), the
system_state stays in SYSTEM_RUNNING.

Since bae1d3a05a8b, this hinders i2c_in_atomic_xfer_mode() from becoming
active, and therefore might lead to avoidable warnings in the restart
handlers, e.g.:

[   12.667612] WARNING: CPU: 1 PID: 1 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch+0x33c/0x6b0
[   12.676926] Voluntary context switch within RCU read-side critical section!
...
[   12.742376]  schedule_timeout from wait_for_completion_timeout+0x90/0x114
[   12.749179]  wait_for_completion_timeout from tegra_i2c_wait_completion+0x40/0x70
...
[   12.994527]  atomic_notifier_call_chain from machine_restart+0x34/0x58
[   13.001050]  machine_restart from panic+0x2a8/0x32c

Avoid these by setting the correct system_state.

Fixes: bae1d3a05a8b ("i2c: core: remove use of in_atomic()")
Cc: stable@vger.kernel.org # v5.2+
Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 kernel/reboot.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/reboot.c b/kernel/reboot.c
index 3bba88c7ffc6..6ebef11c8876 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -74,6 +74,7 @@ void __weak (*pm_power_off)(void);
 void emergency_restart(void)
 {
 	kmsg_dump(KMSG_DUMP_EMERG);
+	system_state = SYSTEM_RESTART;
 	machine_emergency_restart();
 }
 EXPORT_SYMBOL_GPL(emergency_restart);

-- 
2.34.1

[PATCH v6 2/5] i2c: core: run atomic i2c xfer when !preemptible

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

Since bae1d3a05a8b, i2c transfers are non-atomic if preemption is
disabled. However, non-atomic i2c transfers require preemption (e.g. in
wait_for_completion() while waiting for the DMA).

panic() calls preempt_disable_notrace() before calling
emergency_restart(). Therefore, if an i2c device is used for the
restart, the xfer should be atomic. This avoids warnings like:

[   12.667612] WARNING: CPU: 1 PID: 1 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch+0x33c/0x6b0
[   12.676926] Voluntary context switch within RCU read-side critical section!
...
[   12.742376]  schedule_timeout from wait_for_completion_timeout+0x90/0x114
[   12.749179]  wait_for_completion_timeout from tegra_i2c_wait_completion+0x40/0x70
...
[   12.994527]  atomic_notifier_call_chain from machine_restart+0x34/0x58
[   13.001050]  machine_restart from panic+0x2a8/0x32c

Use !preemptible() instead, which is basically the same check as
pre-v5.2.

Fixes: bae1d3a05a8b ("i2c: core: remove use of in_atomic()")
Cc: stable@vger.kernel.org # v5.2+
Suggested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Acked-by: Wolfram Sang <wsa@kernel.org>
Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/i2c/i2c-core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 1247e6e6e975..05b8b8dfa9bd 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -29,7 +29,7 @@ int i2c_dev_irq_from_resources(const struct resource *resources,
  */
 static inline bool i2c_in_atomic_xfer_mode(void)
 {
-	return system_state > SYSTEM_RUNNING && irqs_disabled();
+	return system_state > SYSTEM_RUNNING && !preemptible();
 }
 
 static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)

-- 
2.34.1

[PATCH v6 3/5] kernel/reboot: add device to sys_off_handler

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

If the dev is known (e.g. a devm-based sys_off_handler is used), it can
be passed to the handler's callback to have it available there.
Otherwise, cb_data might be set to the dev in most of the cases.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 include/linux/reboot.h | 3 +++
 kernel/reboot.c        | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/include/linux/reboot.h b/include/linux/reboot.h
index 2b6bb593be5b..c4cc3b89ced1 100644
--- a/include/linux/reboot.h
+++ b/include/linux/reboot.h
@@ -129,11 +129,14 @@ enum sys_off_mode {
  * @cb_data: User's callback data.
  * @cmd: Command string. Currently used only by the sys-off restart mode,
  *       NULL otherwise.
+ * @dev: Device of the sys-off handler. Only if known (devm_register_*),
+ *       NULL otherwise.
  */
 struct sys_off_data {
 	int mode;
 	void *cb_data;
 	const char *cmd;
+	struct device *dev;
 };
 
 struct sys_off_handler *
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 6ebef11c8876..395a0ea3c7a8 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -55,6 +55,7 @@ struct sys_off_handler {
 	enum sys_off_mode mode;
 	bool blocking;
 	void *list;
+	struct device *dev;
 };
 
 /*
@@ -324,6 +325,7 @@ static int sys_off_notify(struct notifier_block *nb,
 	data.cb_data = handler->cb_data;
 	data.mode = mode;
 	data.cmd = cmd;
+	data.dev = handler->dev;
 
 	return handler->sys_off_cb(&data);
 }
@@ -511,6 +513,7 @@ int devm_register_sys_off_handler(struct device *dev,
 	handler = register_sys_off_handler(mode, priority, callback, cb_data);
 	if (IS_ERR(handler))
 		return PTR_ERR(handler);
+	handler->dev = dev;
 
 	return devm_add_action_or_reset(dev, devm_unregister_sys_off_handler,
 					handler);

-- 
2.34.1

[PATCH v6 4/5] mfd: tps6586x: use devm-based power off handler

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

Convert the power off handler to a devm-based power off handler.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/mfd/tps6586x.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index 2d947f3f606a..b12c9e18970a 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -22,6 +22,7 @@
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/platform_device.h>
+#include <linux/reboot.h>
 #include <linux/regmap.h>
 #include <linux/of.h>
 
@@ -457,13 +458,21 @@ static const struct regmap_config tps6586x_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
-static struct device *tps6586x_dev;
-static void tps6586x_power_off(void)
+static int tps6586x_power_off_handler(struct sys_off_data *data)
 {
-	if (tps6586x_clr_bits(tps6586x_dev, TPS6586X_SUPPLYENE, EXITSLREQ_BIT))
-		return;
+	int ret;
+
+	/* Put the PMIC into sleep state. This takes at least 20ms. */
+	ret = tps6586x_clr_bits(data->dev, TPS6586X_SUPPLYENE, EXITSLREQ_BIT);
+	if (ret)
+		return notifier_from_errno(ret);
+
+	ret = tps6586x_set_bits(data->dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
+	if (ret)
+		return notifier_from_errno(ret);
 
-	tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
+	mdelay(50);
+	return notifier_from_errno(-ETIME);
 }
 
 static void tps6586x_print_version(struct i2c_client *client, int version)
@@ -559,9 +568,13 @@ static int tps6586x_i2c_probe(struct i2c_client *client)
 		goto err_add_devs;
 	}
 
-	if (pdata->pm_off && !pm_power_off) {
-		tps6586x_dev = &client->dev;
-		pm_power_off = tps6586x_power_off;
+	if (pdata->pm_off) {
+		ret = devm_register_power_off_handler(&client->dev, &tps6586x_power_off_handler,
+						      NULL);
+		if (ret) {
+			dev_err(&client->dev, "register power off handler failed: %d\n", ret);
+			goto err_add_devs;
+		}
 	}
 
 	return 0;

-- 
2.34.1

[PATCH v6 5/5] mfd: tps6586x: register restart handler

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

There are a couple of boards which use a tps6586x as
"ti,system-power-controller", e.g. the tegra20-tamonten.dtsi.
For these, the only registered restart handler is the warm reboot via
tegra's PMC. As the bootloader of the tegra20 requires the VDE, it must
be ensured that VDE is enabled (which is the case after a cold reboot).
For the "normal reboot", this is basically the case since 8f0c714ad9be.
However, this workaround is not executed in case of an emergency restart.
In case of an emergency restart, the system now simply hangs in the
bootloader, as VDE is not enabled (because it is not used).

The TPS658629-Q1 provides a SOFT RST bit in the SUPPLYENE reg to request
a (cold) reboot, which takes at least 20ms (as the data sheet states).
This avoids the hang-up.

Tested on a TPS658640.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/mfd/tps6586x.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index b12c9e18970a..3b8faa058e59 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -30,6 +30,7 @@
 #include <linux/mfd/tps6586x.h>
 
 #define TPS6586X_SUPPLYENE	0x14
+#define SOFT_RST_BIT		BIT(0)
 #define EXITSLREQ_BIT		BIT(1)
 #define SLEEP_MODE_BIT		BIT(3)
 
@@ -475,6 +476,24 @@ static int tps6586x_power_off_handler(struct sys_off_data *data)
 	return notifier_from_errno(-ETIME);
 }
 
+static int tps6586x_restart_handler(struct sys_off_data *data)
+{
+	int ret;
+
+	/* TPS6586X only provides a hard/cold reboot, skip others. */
+	if (data->mode != REBOOT_UNDEFINED && data->mode != REBOOT_COLD &&
+	    data->mode != REBOOT_HARD)
+		return NOTIFY_DONE;
+
+	/* Put the PMIC into hard reboot state. This takes at least 20ms. */
+	ret = tps6586x_set_bits(data->dev, TPS6586X_SUPPLYENE, SOFT_RST_BIT);
+	if (ret)
+		return notifier_from_errno(ret);
+
+	mdelay(50);
+	return notifier_from_errno(-ETIME);
+}
+
 static void tps6586x_print_version(struct i2c_client *client, int version)
 {
 	const char *name;
@@ -575,6 +594,13 @@ static int tps6586x_i2c_probe(struct i2c_client *client)
 			dev_err(&client->dev, "register power off handler failed: %d\n", ret);
 			goto err_add_devs;
 		}
+
+		ret = devm_register_restart_handler(&client->dev, &tps6586x_restart_handler,
+						    NULL);
+		if (ret) {
+			dev_err(&client->dev, "register restart handler failed: %d\n", ret);
+			goto err_add_devs;
+		}
 	}
 
 	return 0;

-- 
2.34.1

Re: [PATCH v6 4/5] mfd: tps6586x: use devm-based power off handler

[Lee Jones]

On Tue, 09 May 2023, Benjamin Bara wrote:

> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> Convert the power off handler to a devm-based power off handler.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  drivers/mfd/tps6586x.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)

Do the 2 MFD patches depend on the others?

For my own reference (apply this as-is to your sign-off block):

Acked-for-MFD-by: Lee Jones <lee@kernel.org>

-- 
Lee Jones [李琼斯]

Re: [PATCH v6 5/5] mfd: tps6586x: register restart handler

[Lee Jones]

On Tue, 09 May 2023, Benjamin Bara wrote:

> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> There are a couple of boards which use a tps6586x as
> "ti,system-power-controller", e.g. the tegra20-tamonten.dtsi.
> For these, the only registered restart handler is the warm reboot via
> tegra's PMC. As the bootloader of the tegra20 requires the VDE, it must
> be ensured that VDE is enabled (which is the case after a cold reboot).
> For the "normal reboot", this is basically the case since 8f0c714ad9be.
> However, this workaround is not executed in case of an emergency restart.
> In case of an emergency restart, the system now simply hangs in the
> bootloader, as VDE is not enabled (because it is not used).
> 
> The TPS658629-Q1 provides a SOFT RST bit in the SUPPLYENE reg to request
> a (cold) reboot, which takes at least 20ms (as the data sheet states).
> This avoids the hang-up.
> 
> Tested on a TPS658640.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  drivers/mfd/tps6586x.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)

I plan to apply the whole set once you have all required Acks.

For my own reference (apply this as-is to your sign-off block):

Acked-for-MFD-by: Lee Jones <lee@kernel.org>

-- 
Lee Jones [李琼斯]

Re: [PATCH v6 4/5] mfd: tps6586x: use devm-based power off handler

[Benjamin Bara]

On Thu, 18 May 2023 at 11:43, Lee Jones <lee@kernel.org> wrote:
> Do the 2 MFD patches depend on the others?

They depend on 3/5, which is an extension to [1] and makes the
respective device available to its sys-off handler.

1/5 and 2/5 avoid a warning which is shown if the handler is called from
an emergency restart (e.g. panic()). The reason behind it is that the
i2c transfer currently doesn't recognize that it should be atomic in
this phase and utilizes the DMA instead, which schedules out while
waiting for completion ("Voluntary context switch within RCU read-side
critical section!").

[1] https://lore.kernel.org/lkml/20220509233235.995021-4-dmitry.osipenko@collabora.com/

Re: [PATCH v6 1/5] kernel/reboot: emergency_restart: set correct system_state

[Dmitry Osipenko]

On 5/9/23 22:02, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> As the emergency restart does not call kernel_restart_prepare(), the
> system_state stays in SYSTEM_RUNNING.
> 
> Since bae1d3a05a8b, this hinders i2c_in_atomic_xfer_mode() from becoming
> active, and therefore might lead to avoidable warnings in the restart
> handlers, e.g.:
> 
> [   12.667612] WARNING: CPU: 1 PID: 1 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch+0x33c/0x6b0
> [   12.676926] Voluntary context switch within RCU read-side critical section!
> ...
> [   12.742376]  schedule_timeout from wait_for_completion_timeout+0x90/0x114
> [   12.749179]  wait_for_completion_timeout from tegra_i2c_wait_completion+0x40/0x70
> ...
> [   12.994527]  atomic_notifier_call_chain from machine_restart+0x34/0x58
> [   13.001050]  machine_restart from panic+0x2a8/0x32c
> 
> Avoid these by setting the correct system_state.
> 
> Fixes: bae1d3a05a8b ("i2c: core: remove use of in_atomic()")
> Cc: stable@vger.kernel.org # v5.2+
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  kernel/reboot.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index 3bba88c7ffc6..6ebef11c8876 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -74,6 +74,7 @@ void __weak (*pm_power_off)(void);
>  void emergency_restart(void)
>  {
>  	kmsg_dump(KMSG_DUMP_EMERG);
> +	system_state = SYSTEM_RESTART;
>  	machine_emergency_restart();
>  }
>  EXPORT_SYMBOL_GPL(emergency_restart);
> 

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

Re: [PATCH v6 2/5] i2c: core: run atomic i2c xfer when !preemptible

[Dmitry Osipenko]

On 5/9/23 22:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> Since bae1d3a05a8b, i2c transfers are non-atomic if preemption is
> disabled. However, non-atomic i2c transfers require preemption (e.g. in
> wait_for_completion() while waiting for the DMA).
> 
> panic() calls preempt_disable_notrace() before calling
> emergency_restart(). Therefore, if an i2c device is used for the
> restart, the xfer should be atomic. This avoids warnings like:
> 
> [   12.667612] WARNING: CPU: 1 PID: 1 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch+0x33c/0x6b0
> [   12.676926] Voluntary context switch within RCU read-side critical section!
> ...
> [   12.742376]  schedule_timeout from wait_for_completion_timeout+0x90/0x114
> [   12.749179]  wait_for_completion_timeout from tegra_i2c_wait_completion+0x40/0x70
> ...
> [   12.994527]  atomic_notifier_call_chain from machine_restart+0x34/0x58
> [   13.001050]  machine_restart from panic+0x2a8/0x32c
> 
> Use !preemptible() instead, which is basically the same check as
> pre-v5.2.
> 
> Fixes: bae1d3a05a8b ("i2c: core: remove use of in_atomic()")
> Cc: stable@vger.kernel.org # v5.2+
> Suggested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Acked-by: Wolfram Sang <wsa@kernel.org>
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  drivers/i2c/i2c-core.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 1247e6e6e975..05b8b8dfa9bd 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -29,7 +29,7 @@ int i2c_dev_irq_from_resources(const struct resource *resources,
>   */
>  static inline bool i2c_in_atomic_xfer_mode(void)
>  {
> -	return system_state > SYSTEM_RUNNING && irqs_disabled();
> +	return system_state > SYSTEM_RUNNING && !preemptible();
>  }
>  
>  static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
> 

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

Re: [PATCH v6 3/5] kernel/reboot: add device to sys_off_handler

[Dmitry Osipenko]

On 5/9/23 22:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> If the dev is known (e.g. a devm-based sys_off_handler is used), it can
> be passed to the handler's callback to have it available there.
> Otherwise, cb_data might be set to the dev in most of the cases.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  include/linux/reboot.h | 3 +++
>  kernel/reboot.c        | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/include/linux/reboot.h b/include/linux/reboot.h
> index 2b6bb593be5b..c4cc3b89ced1 100644
> --- a/include/linux/reboot.h
> +++ b/include/linux/reboot.h
> @@ -129,11 +129,14 @@ enum sys_off_mode {
>   * @cb_data: User's callback data.
>   * @cmd: Command string. Currently used only by the sys-off restart mode,
>   *       NULL otherwise.
> + * @dev: Device of the sys-off handler. Only if known (devm_register_*),
> + *       NULL otherwise.
>   */
>  struct sys_off_data {
>  	int mode;
>  	void *cb_data;
>  	const char *cmd;
> +	struct device *dev;
>  };
>  
>  struct sys_off_handler *
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index 6ebef11c8876..395a0ea3c7a8 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -55,6 +55,7 @@ struct sys_off_handler {
>  	enum sys_off_mode mode;
>  	bool blocking;
>  	void *list;
> +	struct device *dev;
>  };
>  
>  /*
> @@ -324,6 +325,7 @@ static int sys_off_notify(struct notifier_block *nb,
>  	data.cb_data = handler->cb_data;
>  	data.mode = mode;
>  	data.cmd = cmd;
> +	data.dev = handler->dev;
>  
>  	return handler->sys_off_cb(&data);
>  }
> @@ -511,6 +513,7 @@ int devm_register_sys_off_handler(struct device *dev,
>  	handler = register_sys_off_handler(mode, priority, callback, cb_data);
>  	if (IS_ERR(handler))
>  		return PTR_ERR(handler);
> +	handler->dev = dev;
>  
>  	return devm_add_action_or_reset(dev, devm_unregister_sys_off_handler,
>  					handler);
> 

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

Re: [PATCH v6 4/5] mfd: tps6586x: use devm-based power off handler

[Dmitry Osipenko]

On 5/9/23 22:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> Convert the power off handler to a devm-based power off handler.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  drivers/mfd/tps6586x.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
> index 2d947f3f606a..b12c9e18970a 100644
> --- a/drivers/mfd/tps6586x.c
> +++ b/drivers/mfd/tps6586x.c
> @@ -22,6 +22,7 @@
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/platform_device.h>
> +#include <linux/reboot.h>
>  #include <linux/regmap.h>
>  #include <linux/of.h>
>  
> @@ -457,13 +458,21 @@ static const struct regmap_config tps6586x_regmap_config = {
>  	.cache_type = REGCACHE_RBTREE,
>  };
>  
> -static struct device *tps6586x_dev;
> -static void tps6586x_power_off(void)
> +static int tps6586x_power_off_handler(struct sys_off_data *data)
>  {
> -	if (tps6586x_clr_bits(tps6586x_dev, TPS6586X_SUPPLYENE, EXITSLREQ_BIT))
> -		return;
> +	int ret;
> +
> +	/* Put the PMIC into sleep state. This takes at least 20ms. */
> +	ret = tps6586x_clr_bits(data->dev, TPS6586X_SUPPLYENE, EXITSLREQ_BIT);
> +	if (ret)
> +		return notifier_from_errno(ret);
> +
> +	ret = tps6586x_set_bits(data->dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
> +	if (ret)
> +		return notifier_from_errno(ret);
>  
> -	tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
> +	mdelay(50);
> +	return notifier_from_errno(-ETIME);
>  }
>  
>  static void tps6586x_print_version(struct i2c_client *client, int version)
> @@ -559,9 +568,13 @@ static int tps6586x_i2c_probe(struct i2c_client *client)
>  		goto err_add_devs;
>  	}
>  
> -	if (pdata->pm_off && !pm_power_off) {
> -		tps6586x_dev = &client->dev;
> -		pm_power_off = tps6586x_power_off;
> +	if (pdata->pm_off) {
> +		ret = devm_register_power_off_handler(&client->dev, &tps6586x_power_off_handler,
> +						      NULL);
> +		if (ret) {
> +			dev_err(&client->dev, "register power off handler failed: %d\n", ret);
> +			goto err_add_devs;
> +		}
>  	}
>  
>  	return 0;
> 

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

Re: [PATCH v6 5/5] mfd: tps6586x: register restart handler

[Dmitry Osipenko]

On 5/9/23 22:03, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> There are a couple of boards which use a tps6586x as
> "ti,system-power-controller", e.g. the tegra20-tamonten.dtsi.
> For these, the only registered restart handler is the warm reboot via
> tegra's PMC. As the bootloader of the tegra20 requires the VDE, it must
> be ensured that VDE is enabled (which is the case after a cold reboot).
> For the "normal reboot", this is basically the case since 8f0c714ad9be.
> However, this workaround is not executed in case of an emergency restart.
> In case of an emergency restart, the system now simply hangs in the
> bootloader, as VDE is not enabled (because it is not used).
> 
> The TPS658629-Q1 provides a SOFT RST bit in the SUPPLYENE reg to request
> a (cold) reboot, which takes at least 20ms (as the data sheet states).
> This avoids the hang-up.
> 
> Tested on a TPS658640.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  drivers/mfd/tps6586x.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
> index b12c9e18970a..3b8faa058e59 100644
> --- a/drivers/mfd/tps6586x.c
> +++ b/drivers/mfd/tps6586x.c
> @@ -30,6 +30,7 @@
>  #include <linux/mfd/tps6586x.h>
>  
>  #define TPS6586X_SUPPLYENE	0x14
> +#define SOFT_RST_BIT		BIT(0)
>  #define EXITSLREQ_BIT		BIT(1)
>  #define SLEEP_MODE_BIT		BIT(3)
>  
> @@ -475,6 +476,24 @@ static int tps6586x_power_off_handler(struct sys_off_data *data)
>  	return notifier_from_errno(-ETIME);
>  }
>  
> +static int tps6586x_restart_handler(struct sys_off_data *data)
> +{
> +	int ret;
> +
> +	/* TPS6586X only provides a hard/cold reboot, skip others. */
> +	if (data->mode != REBOOT_UNDEFINED && data->mode != REBOOT_COLD &&
> +	    data->mode != REBOOT_HARD)
> +		return NOTIFY_DONE;

Not sure whether it's worthwhile to care about the reboot mode. If we
would really care, then the supported modes should be a part of sys-off
handler definition. Maybe Rafael could comment on it.

Otherwise looks good.

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

Re: [PATCH v6 2/5] i2c: core: run atomic i2c xfer when !preemptible

[Nishanth Menon]

On 21:03-20230509, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> Since bae1d3a05a8b, i2c transfers are non-atomic if preemption is
> disabled. However, non-atomic i2c transfers require preemption (e.g. in
> wait_for_completion() while waiting for the DMA).
> 
> panic() calls preempt_disable_notrace() before calling
> emergency_restart(). Therefore, if an i2c device is used for the
> restart, the xfer should be atomic. This avoids warnings like:
> 
> [   12.667612] WARNING: CPU: 1 PID: 1 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch+0x33c/0x6b0
> [   12.676926] Voluntary context switch within RCU read-side critical section!
> ...
> [   12.742376]  schedule_timeout from wait_for_completion_timeout+0x90/0x114
> [   12.749179]  wait_for_completion_timeout from tegra_i2c_wait_completion+0x40/0x70
> ...
> [   12.994527]  atomic_notifier_call_chain from machine_restart+0x34/0x58
> [   13.001050]  machine_restart from panic+0x2a8/0x32c
> 
> Use !preemptible() instead, which is basically the same check as
> pre-v5.2.
> 
> Fixes: bae1d3a05a8b ("i2c: core: remove use of in_atomic()")
> Cc: stable@vger.kernel.org # v5.2+
> Suggested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Acked-by: Wolfram Sang <wsa@kernel.org>
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  drivers/i2c/i2c-core.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 1247e6e6e975..05b8b8dfa9bd 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -29,7 +29,7 @@ int i2c_dev_irq_from_resources(const struct resource *resources,
>   */
>  static inline bool i2c_in_atomic_xfer_mode(void)
>  {
> -	return system_state > SYSTEM_RUNNING && irqs_disabled();
> +	return system_state > SYSTEM_RUNNING && !preemptible();
>  }
>  
>  static inline int __i2c_lock_bus_helper(struct i2c_adapter *adap)
> 
> -- 
> 2.34.1
> 
Tested-by: Nishanth Menon <nm@ti.com>

This in addition to a deeper bug in our driver seems to have helped
resolve a report we had been looking at. Tested on beagleplay platform

https://lore.kernel.org/all/ZGeHMjlnob2GFyHF@francesco-nb.int.toradex.com/

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v6 1/5] kernel/reboot: emergency_restart: set correct system_state

[Nishanth Menon]

On 21:02-20230509, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> As the emergency restart does not call kernel_restart_prepare(), the
> system_state stays in SYSTEM_RUNNING.
> 
> Since bae1d3a05a8b, this hinders i2c_in_atomic_xfer_mode() from becoming
> active, and therefore might lead to avoidable warnings in the restart
> handlers, e.g.:
> 
> [   12.667612] WARNING: CPU: 1 PID: 1 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch+0x33c/0x6b0
> [   12.676926] Voluntary context switch within RCU read-side critical section!
> ...
> [   12.742376]  schedule_timeout from wait_for_completion_timeout+0x90/0x114
> [   12.749179]  wait_for_completion_timeout from tegra_i2c_wait_completion+0x40/0x70
> ...
> [   12.994527]  atomic_notifier_call_chain from machine_restart+0x34/0x58
> [   13.001050]  machine_restart from panic+0x2a8/0x32c
> 
> Avoid these by setting the correct system_state.
> 
> Fixes: bae1d3a05a8b ("i2c: core: remove use of in_atomic()")
> Cc: stable@vger.kernel.org # v5.2+
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  kernel/reboot.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index 3bba88c7ffc6..6ebef11c8876 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -74,6 +74,7 @@ void __weak (*pm_power_off)(void);
>  void emergency_restart(void)
>  {
>  	kmsg_dump(KMSG_DUMP_EMERG);
> +	system_state = SYSTEM_RESTART;
>  	machine_emergency_restart();
>  }
>  EXPORT_SYMBOL_GPL(emergency_restart);
> 
> -- 
> 2.34.1
> 

Tested-by: Nishanth Menon <nm@ti.com>

This in addition to a deeper bug in our driver seems to have helped
resolve a report we had been looking at. Tested on beagleplay platform

https://lore.kernel.org/all/ZGeHMjlnob2GFyHF@francesco-nb.int.toradex.com/

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v6 1/5] kernel/reboot: emergency_restart: set correct system_state

[Francesco Dolcini]

Hello Nishanth,

On Wed, Jun 14, 2023 at 07:06:50PM -0500, Nishanth Menon wrote:
> On 21:02-20230509, Benjamin Bara wrote:
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> > 
> > As the emergency restart does not call kernel_restart_prepare(), the
> > system_state stays in SYSTEM_RUNNING.
> > 
> > Since bae1d3a05a8b, this hinders i2c_in_atomic_xfer_mode() from becoming
> > active, and therefore might lead to avoidable warnings in the restart
> > handlers, e.g.:
> > 
> > [   12.667612] WARNING: CPU: 1 PID: 1 at kernel/rcu/tree_plugin.h:318 rcu_note_context_switch+0x33c/0x6b0
> > [   12.676926] Voluntary context switch within RCU read-side critical section!
> > ...
> > [   12.742376]  schedule_timeout from wait_for_completion_timeout+0x90/0x114
> > [   12.749179]  wait_for_completion_timeout from tegra_i2c_wait_completion+0x40/0x70
> > ...
> > [   12.994527]  atomic_notifier_call_chain from machine_restart+0x34/0x58
> > [   13.001050]  machine_restart from panic+0x2a8/0x32c
> > 
> > Avoid these by setting the correct system_state.
> > 
> > Fixes: bae1d3a05a8b ("i2c: core: remove use of in_atomic()")
> > Cc: stable@vger.kernel.org # v5.2+
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> >  kernel/reboot.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/kernel/reboot.c b/kernel/reboot.c
> > index 3bba88c7ffc6..6ebef11c8876 100644
> > --- a/kernel/reboot.c
> > +++ b/kernel/reboot.c
> > @@ -74,6 +74,7 @@ void __weak (*pm_power_off)(void);
> >  void emergency_restart(void)
> >  {
> >  	kmsg_dump(KMSG_DUMP_EMERG);
> > +	system_state = SYSTEM_RESTART;
> >  	machine_emergency_restart();
> >  }
> >  EXPORT_SYMBOL_GPL(emergency_restart);
> > 
> > -- 
> > 2.34.1
> > 
> 
> Tested-by: Nishanth Menon <nm@ti.com>
> 
> This in addition to a deeper bug in our driver seems to have helped
> resolve a report we had been looking at. Tested on beagleplay platform
> 
> https://lore.kernel.org/all/ZGeHMjlnob2GFyHF@francesco-nb.int.toradex.com/

Is this patch going to fix the RCU warning I reported on that email or
it is just part of a more complex solution?

Francesco

Re: [PATCH v6 1/5] kernel/reboot: emergency_restart: set correct system_state

[Nishanth Menon]

On 15:21-20230615, Francesco Dolcini wrote:
> > > 
> > > -- 
> > > 2.34.1
> > > 
> > 
> > Tested-by: Nishanth Menon <nm@ti.com>
> > 
> > This in addition to a deeper bug in our driver seems to have helped
> > resolve a report we had been looking at. Tested on beagleplay platform
> > 
> > https://lore.kernel.org/all/ZGeHMjlnob2GFyHF@francesco-nb.int.toradex.com/
> 
> Is this patch going to fix the RCU warning I reported on that email or
> it is just part of a more complex solution?

From what I see, It is part of the solution.
Problem happens as follows for us:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/ti_sci.c#n421

When i2c is not that frequently used, runtime pm disables the power
domain on our platform. As part of reset or power-off, when i2c is
invoked, it ends up calling into the firmware handler which (no
surprise), attempts to do the wrong thing (and rightly flagged by RCU).

We are in the middle of trying various combinations out to ensure we
are'nt messing things up.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

Re: [PATCH v6 5/5] mfd: tps6586x: register restart handler

[Dmitry Osipenko]

On 5/18/23 14:48, Dmitry Osipenko wrote:
> On 5/9/23 22:03, Benjamin Bara wrote:
>> From: Benjamin Bara <benjamin.bara@skidata.com>
>>
>> There are a couple of boards which use a tps6586x as
>> "ti,system-power-controller", e.g. the tegra20-tamonten.dtsi.
>> For these, the only registered restart handler is the warm reboot via
>> tegra's PMC. As the bootloader of the tegra20 requires the VDE, it must
>> be ensured that VDE is enabled (which is the case after a cold reboot).
>> For the "normal reboot", this is basically the case since 8f0c714ad9be.
>> However, this workaround is not executed in case of an emergency restart.
>> In case of an emergency restart, the system now simply hangs in the
>> bootloader, as VDE is not enabled (because it is not used).
>>
>> The TPS658629-Q1 provides a SOFT RST bit in the SUPPLYENE reg to request
>> a (cold) reboot, which takes at least 20ms (as the data sheet states).
>> This avoids the hang-up.
>>
>> Tested on a TPS658640.
>>
>> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
>> ---
>>  drivers/mfd/tps6586x.c | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
>> index b12c9e18970a..3b8faa058e59 100644
>> --- a/drivers/mfd/tps6586x.c
>> +++ b/drivers/mfd/tps6586x.c
>> @@ -30,6 +30,7 @@
>>  #include <linux/mfd/tps6586x.h>
>>  
>>  #define TPS6586X_SUPPLYENE	0x14
>> +#define SOFT_RST_BIT		BIT(0)
>>  #define EXITSLREQ_BIT		BIT(1)
>>  #define SLEEP_MODE_BIT		BIT(3)
>>  
>> @@ -475,6 +476,24 @@ static int tps6586x_power_off_handler(struct sys_off_data *data)
>>  	return notifier_from_errno(-ETIME);
>>  }
>>  
>> +static int tps6586x_restart_handler(struct sys_off_data *data)
>> +{
>> +	int ret;
>> +
>> +	/* TPS6586X only provides a hard/cold reboot, skip others. */
>> +	if (data->mode != REBOOT_UNDEFINED && data->mode != REBOOT_COLD &&
>> +	    data->mode != REBOOT_HARD)
>> +		return NOTIFY_DONE;
> 
> Not sure whether it's worthwhile to care about the reboot mode. If we
> would really care, then the supported modes should be a part of sys-off
> handler definition. Maybe Rafael could comment on it.
> 
> Otherwise looks good.
> 
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

Benjamin, could you please add acks and review tags to the commits and
send v7? I'd also suggest to drop the "data->mode" checks unless there
is a good practical reason to keep them. There are no other drivers in
kernel that do such checks.

-- 
Best regards,
Dmitry