[PATCH v3 0/4] mfd: tps6586x: register restart handler

[PATCH v3 0/4] 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 [1], 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 on
tegra20-based boards.

During panic(), preemption is disabled.
This should be correctly detected by i2c_in_atomic_xfer_mode() to use
atomic i2c xfer in this late stage. This avoids warnings regarding
"Voluntary context switch within RCU".

[1] 8f0c714ad9be1ef774c98e8819a7a571451cb019
v2: https://lore.kernel.org/all/20230320220345.1463687-1-bbara93@gmail.com/
system_state: https://lore.kernel.org/all/20230320213230.1459532-1-bbara93@gmail.com/
v1: https://lore.kernel.org/all/20230316164703.1157813-1-bbara93@gmail.com/

v3:
- bring system_state back in this series
- do atomic i2c xfer if not preemptible (as suggested by Dmitry)
- fix style issues mentioned by Dmitry
- add cc stable as suggested by Dmitry
- add explanation why this is needed for Jon

v2:
- use devm-based restart handler
- convert the existing power_off handler to a devm-based handler
- handle system_state in extra series

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

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

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

[PATCH v3 1/4] 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.

This e.g. 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.

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 v3 2/4] i2c: core: run atomic i2c xfer when !preemptible

[Benjamin Bara]

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

A panic() calls preempt_disable_notrace() before calling
emergency_restart(). If an i2c device is used for the restart, the xfer
should not schedule out (to avoid 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

Therefore, not only check for disabled IRQs but also for preemptible.

Cc: stable@vger.kernel.org # v5.2+
Suggested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
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 v3 3/4] 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 | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index 2d947f3f606a..93f1bf440191 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,16 @@ 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;
+	struct device *tps6586x_dev = data->cb_data;
+	int ret;
+
+	ret = tps6586x_clr_bits(tps6586x_dev, TPS6586X_SUPPLYENE, EXITSLREQ_BIT);
+	if (ret)
+		return ret;
 
-	tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
+	return tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
 }
 
 static void tps6586x_print_version(struct i2c_client *client, int version)
@@ -559,9 +563,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,
+						      &client->dev);
+		if (ret) {
+			dev_err(&client->dev, "register power off handler failed: %d\n", ret);
+			goto err_add_devs;
+		}
 	}
 
 	return 0;

-- 
2.34.1

[PATCH v3 4/4] 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
8f0c714ad9be1ef774c98e8819a7a571451cb019.
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 (as not in use).

The TPS658629-Q1 (unfortunately the only TPS6586x with public data sheet)
provides a SOFT RST bit in the SUPPLYENE reg to request a (cold) reboot.
This avoids the hang-up.

Tested on a TPS658640.

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

diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index 93f1bf440191..c8adf6a08277 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)
 
@@ -470,6 +471,17 @@ static int tps6586x_power_off_handler(struct sys_off_data *data)
 	return tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
 }
 
+static int tps6586x_restart_handler(struct sys_off_data *data)
+{
+	struct device *tps6586x_dev = data->cb_data;
+
+	/* bring pmic into HARD REBOOT state */
+	tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SOFT_RST_BIT);
+
+	mdelay(500);
+	return 0;
+}
+
 static void tps6586x_print_version(struct i2c_client *client, int version)
 {
 	const char *name;
@@ -570,6 +582,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,
+						    &client->dev);
+		if (ret) {
+			dev_err(&client->dev, "register restart handler failed: %d\n", ret);
+			goto err_add_devs;
+		}
 	}
 
 	return 0;

-- 
2.34.1

Re: [PATCH v3 2/4] i2c: core: run atomic i2c xfer when !preemptible

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 246 bytes --]


> -	return system_state > SYSTEM_RUNNING && irqs_disabled();
> +	return system_state > SYSTEM_RUNNING && !preemptible();

For the !CONFIG_PREEMPT_COUNT case, preemptible() is defined 0. So,
don't we lose the irqs_disabled() check in that case?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/4] i2c: core: run atomic i2c xfer when !preemptible

[Benjamin Bara]

On Mon, 27 Mar 2023 at 16:54, Wolfram Sang <wsa@kernel.org> wrote:
> For the !CONFIG_PREEMPT_COUNT case, preemptible() is defined 0. So,
> don't we lose the irqs_disabled() check in that case?

Thanks for the feedback!
PREEMPT_COUNT is selected by PREEMPTION, so I guess in the case of
!PREEMPT_COUNT,
we should be atomic (anyways)?

Re: [PATCH v3 2/4] i2c: core: run atomic i2c xfer when !preemptible

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 545 bytes --]

On Mon, Mar 27, 2023 at 06:23:24PM +0200, Benjamin Bara wrote:
> On Mon, 27 Mar 2023 at 16:54, Wolfram Sang <wsa@kernel.org> wrote:
> > For the !CONFIG_PREEMPT_COUNT case, preemptible() is defined 0. So,
> > don't we lose the irqs_disabled() check in that case?
> 
> Thanks for the feedback!
> PREEMPT_COUNT is selected by PREEMPTION, so I guess in the case of
> !PREEMPT_COUNT,
> we should be atomic (anyways)?

Could you make sure please? Asking Peter Zijlstra might be a good idea.
He helped me with the current implementation.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/4] i2c: core: run atomic i2c xfer when !preemptible

[Benjamin Bara]

On Wed, 29 Mar 2023 at 21:50, Wolfram Sang <wsa@kernel.org> wrote:
> Could you make sure please?

Sure, I'll try. The check before bae1d3a was:
in_atomic() || irqs_disabled()
which boils down to:
(preempt_count() != 0) || irqs_disabled()
preemptible() is defined as:
(preempt_count() == 0 && !irqs_disabled())

so this patch should behave the same as pre-v5.2, but with the
additional system state check. From my point of view, the additional
value of the in_atomic() check was that it activated atomic i2c xfers
when preemption is disabled, like in the case of panic(). So reverting
that commit would also re-activate atomic i2c transfers during emergency
restarts. However, I think considering the system state makes sense
here.

From my understanding, non-atomic i2c transfers require enabled IRQs,
but atomic i2c transfers do not have any "requirements". So the
irqs_disabled() check is not here to ensure that the following atomic
i2c transfer works correctly, but to use non-atomic i2c xfer as
long/often as possible.

Unfortunately, I am not sure yet about !CONFIG_PREEMPTION. I looked into
some i2c-bus implementations which implement both, atomic and
non-atomic. As far as I saw, the basic difference is that the non-atomic
variants usually utilize the DMA and then call a variant of
wait_for_completion(), like in i2c_imx_dma_write() [1]. However, the
documentation of wait_for_completion [2] states that:
"wait_for_completion() and its variants are only safe in process context
(as they can sleep) but not (...) [if] preemption is disabled".
Therefore, I am not quite sure yet if !CONFIG_PREEMPTION uses the
non-atomic variant at all or if this case is handled differently.

> Asking Peter Zijlstra might be a good idea.
> He helped me with the current implementation.

Thanks for the hint! I wrote an extra email to him and added him to CC.

Thanks & best regards,
Benjamin

[1] drivers/i2c/busses/i2c-imx.c
[2] https://www.kernel.org/doc/Documentation/scheduler/completion.txt

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

[Dmitry Osipenko]

On 3/27/23 16:57, 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 | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
> index 2d947f3f606a..93f1bf440191 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,16 @@ 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;
> +	struct device *tps6586x_dev = data->cb_data;
> +	int ret;
> +
> +	ret = tps6586x_clr_bits(tps6586x_dev, TPS6586X_SUPPLYENE, EXITSLREQ_BIT);
> +	if (ret)
> +		return ret;
>  
> -	tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
> +	return tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
>  }
>  
>  static void tps6586x_print_version(struct i2c_client *client, int version)
> @@ -559,9 +563,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,
> +						      &client->dev);
> +		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 v3 4/4] mfd: tps6586x: register restart handler

[Dmitry Osipenko]

On 3/27/23 16:57, 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
> 8f0c714ad9be1ef774c98e8819a7a571451cb019.
> 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 (as not in use).
> 
> The TPS658629-Q1 (unfortunately the only TPS6586x with public data sheet)
> provides a SOFT RST bit in the SUPPLYENE reg to request a (cold) reboot.
> This avoids the hang-up.
> 
> Tested on a TPS658640.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  drivers/mfd/tps6586x.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
> index 93f1bf440191..c8adf6a08277 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)
>  
> @@ -470,6 +471,17 @@ static int tps6586x_power_off_handler(struct sys_off_data *data)
>  	return tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SLEEP_MODE_BIT);
>  }
>  
> +static int tps6586x_restart_handler(struct sys_off_data *data)
> +{
> +	struct device *tps6586x_dev = data->cb_data;
> +
> +	/* bring pmic into HARD REBOOT state */
> +	tps6586x_set_bits(tps6586x_dev, TPS6586X_SUPPLYENE, SOFT_RST_BIT);
> +
> +	mdelay(500);
Is this delay needed? There is no delay in a case of power off.

-- 
Best regards,
Dmitry

Re: [PATCH v3 4/4] mfd: tps6586x: register restart handler

[Benjamin Bara]

Thanks for the feedback!

The DS states: "When the reboot request state is set an internal timer TWAIT
(10ms typ) is started. The reboot request ends when t > TWAIT."

Therefore, my intention was to wait a little bit before starting the next
handler in the chain. I can do some tests without the mdelay, but otherwise
will reduce it to 15ms in the next patch. What do you think about it?

BR,
Benjamin

Re: [PATCH v3 4/4] mfd: tps6586x: register restart handler

[Dmitry Osipenko]

On 4/3/23 09:50, Benjamin Bara wrote:
> Thanks for the feedback!
> 
> The DS states: "When the reboot request state is set an internal timer TWAIT
> (10ms typ) is started. The reboot request ends when t > TWAIT."
> 
> Therefore, my intention was to wait a little bit before starting the next
> handler in the chain. I can do some tests without the mdelay, but otherwise
> will reduce it to 15ms in the next patch. What do you think about it?

Sounds good. Please add a clarifying comment for the delay to the code.

-- 
Best regards,
Dmitry

Re: [PATCH v3 2/4] i2c: core: run atomic i2c xfer when !preemptible

[Peter Zijlstra]

On Sun, Apr 02, 2023 at 12:04:48PM +0200, Benjamin Bara wrote:
> On Wed, 29 Mar 2023 at 21:50, Wolfram Sang <wsa@kernel.org> wrote:
> > Could you make sure please?
> 
> Sure, I'll try. The check before bae1d3a was:
> in_atomic() || irqs_disabled()
> which boils down to:
> (preempt_count() != 0) || irqs_disabled()
> preemptible() is defined as:
> (preempt_count() == 0 && !irqs_disabled())
> 
> so this patch should behave the same as pre-v5.2, but with the
> additional system state check. From my point of view, the additional
> value of the in_atomic() check was that it activated atomic i2c xfers
> when preemption is disabled, like in the case of panic(). So reverting
> that commit would also re-activate atomic i2c transfers during emergency
> restarts. However, I think considering the system state makes sense
> here.
> 
> From my understanding, non-atomic i2c transfers require enabled IRQs,
> but atomic i2c transfers do not have any "requirements". So the
> irqs_disabled() check is not here to ensure that the following atomic
> i2c transfer works correctly, but to use non-atomic i2c xfer as
> long/often as possible.
> 
> Unfortunately, I am not sure yet about !CONFIG_PREEMPTION. I looked into
> some i2c-bus implementations which implement both, atomic and
> non-atomic. As far as I saw, the basic difference is that the non-atomic
> variants usually utilize the DMA and then call a variant of
> wait_for_completion(), like in i2c_imx_dma_write() [1]. However, the
> documentation of wait_for_completion [2] states that:
> "wait_for_completion() and its variants are only safe in process context
> (as they can sleep) but not (...) [if] preemption is disabled".
> Therefore, I am not quite sure yet if !CONFIG_PREEMPTION uses the
> non-atomic variant at all or if this case is handled differently.
> 
> > Asking Peter Zijlstra might be a good idea.
> > He helped me with the current implementation.
> 
> Thanks for the hint! I wrote an extra email to him and added him to CC.

So yeah, can't call schedule() if non preemptible (which is either
preempt_disable(), local_bh_disable() (true for bh handlers) or
local_irq_disable() (true for IRQ handlers) and mostly rcu_read_lock()).

You can mostly forget about CONFIG_PREEMPT=n (or more specifically
CONFIG_PREMPT_COUNT=n) things that work for PREEMPT typically also work
for !PREEMPT.

The question here seems to be if i2c_in_atomic_xfer_mode() should have
an in_atomic() / !preemptible() check, right? IIUC Wolfram doesn't like
it being used outside of extra special cicumstances?

Re: [PATCH v3 2/4] i2c: core: run atomic i2c xfer when !preemptible

[Benjamin Bara]

On Tue, 4 Apr 2023 at 10:23, Peter Zijlstra <peterz@infradead.org> wrote:
> You can mostly forget about CONFIG_PREEMPT=n (or more specifically
> CONFIG_PREMPT_COUNT=n) things that work for PREEMPT typically also work
> for !PREEMPT.

Thanks for the clarification!

> The question here seems to be if i2c_in_atomic_xfer_mode() should have
> an in_atomic() / !preemptible() check, right? IIUC Wolfram doesn't like
> it being used outside of extra special cicumstances?

Sorry for expressing things more complicated than needed.
Yes, exactly. Wolfram expressed considerations of adding preemptible() as
check, as the now existing irq_disabled() check is lost with !PREEMPT. I tried
to clarify that the check seems to be there for "performance reasons" and that
the check essentially was !preemptible() before v5.2, so nothing should break.

However, what came to my mind during my "research", is that it might make sense
to handle all i2c transfers atomically when in a post RUNNING state (namely
HALT, POWER_OFF, RESTART, SUSPEND). The outcome would basically be the same as
with this patch series applied, but I guess the reasoning behind it would be
simplified: If in a restart handler, the i2c transfer is atomic, independent of
current IRQ or preemption state. Currently, the state depends on from where the
handler is triggered. As you have stated, most of the i2c transfers in the
mentioned states will just update single bits for power-off/sleep/suspend, so
IMHO nothing where not using a DMA would matter from a performance point of
view. But there might be other reasons for not doing so - I guess in this case
the provided patch is fine (at least from my side).