On Fri, Aug 9, 2024 at 2:57 AM Andi Shyti <andi.shyti@xxxxxxxxxx> wrote: > On Thu, Aug 08, 2024 at 05:14:46AM GMT, 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: First of all, ... > > 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) ...read Submitting Patches and make the above to be ~5-6 significant (useful) lines only. > > 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. This is a step in the right direction, but somewhere in the replies here I would like to hear about roadmap to get rid of the pm_runtime_irq_safe() in all Tegra related code. ... > > * VI I2C device shouldn't be marked as IRQ-safe because VI I2C won't > > * be used for atomic transfers. > > */ Does the comment need an expansion? > > - if (!IS_VI(i2c_dev)) > > + if (!IS_VI(i2c_dev) && !ACPI_HANDLE(i2c_dev->dev)) > > looks good to me, can I have an ack from Andy here? I prefer to see something like is_acpi_node() / is_acpi_device_node() / is_acpi_data_node() / has_acpi_companion() instead depending on the actual ACPI representation of the device. Otherwise no objections. Please, Cc me (andy@xxxxxxxxxx) for the next version. -- With Best Regards, Andy Shevchenko
