[PATCH] [i2c-tegra] Do not mark ACPI devices as irq safe

[PATCH] [i2c-tegra] Do not mark ACPI devices as irq safe

[Breno Leitao]

On ACPI machines, the tegra i2c module encounters an issue due to a
mutex being called inside a spinlock. This leads to the following bug:

	BUG: sleeping function called from invalid context at kernel/locking/mutex.c:585
	in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 1282, name: kssif0010
	preempt_count: 0, expected: 0
	RCU nest depth: 0, expected: 0
	irq event stamp: 0

	Call trace:
	dump_backtrace+0xf0/0x140
	show_stack (./arch/x86/include/asm/current.h:49
		     arch/x86/kernel/dumpstack.c:312)
	dump_stack_lvl (lib/dump_stack.c:89 lib/dump_stack.c:115)
	dump_stack (lib/earlycpio.c:61)
	__might_resched (./arch/x86/include/asm/current.h:49
			 kernel/sched/core.c:10297)
	__might_sleep (./include/linux/lockdep.h:231
			 kernel/sched/core.c:10236)
	__mutex_lock_common+0x5c/0x2190
	mutex_lock_nested (kernel/locking/mutex.c:751)
	acpi_subsys_runtime_resume+0xb8/0x160
	__rpm_callback+0x1cc/0x4b0
	rpm_resume+0xa60/0x1078
	__pm_runtime_resume+0xbc/0x130
	tegra_i2c_xfer+0x74/0x398
	__i2c_transfer (./include/trace/events/i2c.h:122 drivers/i2c/i2c-core-base.c:2258)

The problem arises because during __pm_runtime_resume(), the spinlock
&dev->power.lock is acquired before rpm_resume() is called. Later,
rpm_resume() invokes acpi_subsys_runtime_resume(), which relies on
mutexes, triggering the error.

To address this issue, devices on ACPI are now marked as not IRQ-safe,
considering the dependency of acpi_subsys_runtime_resume() on mutexes.

Co-developed-by: Michael van der Westhuizen <rmikey@meta.com>
Signed-off-by: Michael van der Westhuizen <rmikey@meta.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/i2c/busses/i2c-tegra.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 85b31edc558d..6d783ecc3431 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1804,7 +1804,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	 * VI I2C device shouldn't be marked as IRQ-safe because VI I2C won't
 	 * be used for atomic transfers.
 	 */
-	if (!IS_VI(i2c_dev))
+	if (!IS_VI(i2c_dev) && !ACPI_HANDLE(i2c_dev->dev))
 		pm_runtime_irq_safe(i2c_dev->dev);
 
 	pm_runtime_enable(i2c_dev->dev);
-- 
2.43.0

Re: [PATCH] [i2c-tegra] Do not mark ACPI devices as irq safe

[Andy Shevchenko]

On Thu, Jun 06, 2024 at 06:27:07AM -0700, Breno Leitao wrote:
> On ACPI machines, the tegra i2c module encounters an issue due to a
> mutex being called inside a spinlock. This leads to the following bug:
> 
> 	BUG: sleeping function called from invalid context at kernel/locking/mutex.c:585
> 	in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid: 1282, name: kssif0010
> 	preempt_count: 0, expected: 0
> 	RCU nest depth: 0, expected: 0
> 	irq event stamp: 0
> 
> 	Call trace:
> 	dump_backtrace+0xf0/0x140
> 	show_stack (./arch/x86/include/asm/current.h:49
> 		     arch/x86/kernel/dumpstack.c:312)
> 	dump_stack_lvl (lib/dump_stack.c:89 lib/dump_stack.c:115)
> 	dump_stack (lib/earlycpio.c:61)
> 	__might_resched (./arch/x86/include/asm/current.h:49
> 			 kernel/sched/core.c:10297)
> 	__might_sleep (./include/linux/lockdep.h:231
> 			 kernel/sched/core.c:10236)
> 	__mutex_lock_common+0x5c/0x2190
> 	mutex_lock_nested (kernel/locking/mutex.c:751)
> 	acpi_subsys_runtime_resume+0xb8/0x160
> 	__rpm_callback+0x1cc/0x4b0
> 	rpm_resume+0xa60/0x1078
> 	__pm_runtime_resume+0xbc/0x130
> 	tegra_i2c_xfer+0x74/0x398
> 	__i2c_transfer (./include/trace/events/i2c.h:122 drivers/i2c/i2c-core-base.c:2258)

Please, read
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
and follow the advice on how to improve your commit message.

> The problem arises because during __pm_runtime_resume(), the spinlock
> &dev->power.lock is acquired before rpm_resume() is called. Later,
> rpm_resume() invokes acpi_subsys_runtime_resume(), which relies on
> mutexes, triggering the error.
> 
> To address this issue, devices on ACPI are now marked as not IRQ-safe,
> considering the dependency of acpi_subsys_runtime_resume() on mutexes.

...

While it's a move in the right direction, the real fix is to get rid of
the IRQ safe PM hack completely. Look at how OMAP code was modified for
the last few years and now it's pm_runtime_irq_safe()-free. The main
(ab)users are SH code followed by Tegra drivers.

So, can we fix this once for all?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] [i2c-tegra] Do not mark ACPI devices as irq safe

[Breno Leitao]

Hello Andy,

On Tue, Jun 11, 2024 at 03:49:02PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 06, 2024 at 06:27:07AM -0700, Breno Leitao wrote:

> > The problem arises because during __pm_runtime_resume(), the spinlock
> > &dev->power.lock is acquired before rpm_resume() is called. Later,
> > rpm_resume() invokes acpi_subsys_runtime_resume(), which relies on
> > mutexes, triggering the error.
> > 
> > To address this issue, devices on ACPI are now marked as not IRQ-safe,
> > considering the dependency of acpi_subsys_runtime_resume() on mutexes.
> 
> ...
> 
> While it's a move in the right direction, the real fix is to get rid of
> the IRQ safe PM hack completely.
> Look at how OMAP code was modified for
> the last few years and now it's pm_runtime_irq_safe()-free. The main
> (ab)users are SH code followed by Tegra drivers.

Thanks. 

I think these are two different goals here. This near term goal is just
fix the driver so it can use the pm_runtime_irq_safe() in a saner
way, avoiding calling mutexes inside spinlocks.

Getting rid of the IRQ safe PM seems to me to be more a long term
desirable goal, and unfortunately I cannot afford doing it now.

Laxman, what is your view on this topic?

--breno

Re: [PATCH] [i2c-tegra] Do not mark ACPI devices as irq safe

[Andy Shevchenko]

On Wed, Jun 12, 2024 at 05:12:43AM -0700, Breno Leitao wrote:
> On Tue, Jun 11, 2024 at 03:49:02PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 06, 2024 at 06:27:07AM -0700, Breno Leitao wrote:
> 
> > > The problem arises because during __pm_runtime_resume(), the spinlock
> > > &dev->power.lock is acquired before rpm_resume() is called. Later,
> > > rpm_resume() invokes acpi_subsys_runtime_resume(), which relies on
> > > mutexes, triggering the error.
> > > 
> > > To address this issue, devices on ACPI are now marked as not IRQ-safe,
> > > considering the dependency of acpi_subsys_runtime_resume() on mutexes.
> > 
> > ...
> > 
> > While it's a move in the right direction, the real fix is to get rid of
> > the IRQ safe PM hack completely.
> > Look at how OMAP code was modified for
> > the last few years and now it's pm_runtime_irq_safe()-free. The main
> > (ab)users are SH code followed by Tegra drivers.
> 
> Thanks. 
> 
> I think these are two different goals here. This near term goal is just
> fix the driver so it can use the pm_runtime_irq_safe() in a saner
> way, avoiding calling mutexes inside spinlocks.
> 
> Getting rid of the IRQ safe PM seems to me to be more a long term
> desirable goal, and unfortunately I cannot afford doing it now.
> 
> Laxman, what is your view on this topic?

Yes, please, comment on this. We would like to get rid of the hack named "IRQ
safe PM runtime".

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] [i2c-tegra] Do not mark ACPI devices as irq safe

[Akhil R]

>> I think these are two different goals here. This near term goal is just
>> fix the driver so it can use the pm_runtime_irq_safe() in a saner
>> way, avoiding calling mutexes inside spinlocks.
>> 
>> Getting rid of the IRQ safe PM seems to me to be more a long term
>> desirable goal, and unfortunately I cannot afford doing it now.
>> 
>> Laxman, what is your view on this topic?
>
> Yes, please, comment on this. We would like to get rid of the hack named "IRQ
> safe PM runtime".
>
Any thoughts on how would we handle atomic_xfers without pm_runtime_irq_safe()?

Would the below patch be a good way? I didn't test this though.

@@ -1373,10 +1373,15 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
        struct tegra_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
        int i, ret;

-       ret = pm_runtime_get_sync(i2c_dev->dev);
+       if (i2c_dev->atomic_mode)
+               ret = tegra_i2c_runtime_resume(i2c_dev->dev);
+       else
+               ret = pm_runtime_get_sync(i2c_dev->dev);
+
        if (ret < 0) {
                dev_err(i2c_dev->dev, "runtime resume failed %d\n", ret);
-               pm_runtime_put_noidle(i2c_dev->dev);
+               if (!i2c_dev->atomic_mode)
+                       pm_runtime_put_noidle(i2c_dev->dev);
                return ret;
        }

@@ -1404,7 +1409,10 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
                        break;
        }

-       pm_runtime_put(i2c_dev->dev);
+       if (i2c_dev->atomic_mode)
+               tegra_i2c_runtime_suspend(i2c_dev->dev);
+       else
+               pm_runtime_put(i2c_dev->dev);

        return ret ?: i;
 }