LinuxLists.cc - Re: [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline.

2020-12-04 20:23:28

Subject: Re: [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline.

On Thu, Dec 03, 2020 at 07:04:00PM +0000, [email protected] wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=202453
>
> --- Comment #7 from Thomas Gleixner ([email protected]) ---
> On Tue, Aug 06 2019 at 14:07, bugzilla-daemon wrote:
> > Jean Delvare ([email protected]) changed:
> >
> > Is this happening with vanilla kernels or gentoo kernels?
> >
> > Thomas, I'm not sure if this is a bug in the i2c-i801 driver or something
> > more
> > general in how we handle the interrupts under threadirqs. Any suggestion how
> > to
> > investigate this further?
>
> Bah. What happens is that the i2c-i801 driver interrupt handler does:
>
> i801_isr()
>
> ...
> return i801_host_notify_isr(priv);
>
> which invokes:
>
> i2c_handle_smbus_host_notify()
>
> which in turn invokes
>
> generic_handle_irq()
>
> and that explodes with forced interrupt threading because it's called
> with interrupts enabled.
>
> The root of all evil is the usage of generic_handle_irq() under the
> assumption that this is always called from hard interrupt context. That
> assumption is not true since 8d32a307e4fa ("genirq: Provide forced
> interrupt threading") which went into 2.6.39 almost 10 years ago.
>
> Seems you got lucky that since 10 years someone no user of this uses a
> threaded interrupt handler, like some of the i2c drivers actually do. :)
>
> So there are a couple of options to fix this:
>
> 1) Set the IRQF_NO_THREAD flag and replace the waitqueue as that
> won't work on RT.
>
> Looking at the usage it's a single waiter wakeup and a single
> waiter at a time because the xfer is fully serialized by the
> core. So we can switch it to rcuwait, if there would be
> rcu_wait_event_timeout(), but that's fixable.
>
> 2) Have a wrapper around handle_generic_irq() which ensures that
> interrupts are disabled before invoking it.
>
> 3) Make the interrupt which is dispatched there to be requested with
> [devm_]request_any_context_irq(). That also requires that the
> NESTED_THREAD flag is set on the irq descriptor.
>
> That's exactly made for the use case where the dispatching
> interrupt can be either threaded or in hard interrupt context.
>
> But that's lots of churn.
>
> And because we have so many options, here is the question:
>
> Is i2c_handle_smbus_host_notify() guaranteed to be called from hard
> interrupt handlers (assumed that we use #1 above)?
>
> I can't tell because there is also i2c_slave_host_notify_cb() which
> invokes it and my i2c foo is not good enough to figure that out.
>
> If that's the case the #1 would be the straight forward solution. If
> not, then you want #2 because then the problem will just pop up via the
> slave thing and that might be not as trivial to fix as this one.
>
> If you can answer that, I can send you the proper patch :)

tglx suggested moving this to the appropriate mailing lists, so I'mm
Cc'ing those.

Jean, Wolfram, Benjamin, or someone else, could you please check Thomas'
questions above and let us know what you think?

I'll copy-paste my attempt to answer this in bugzilla below:

```
As far as I can grep through bus drivers, yes, it is called from hard
interrupt handlers only.

i2c_handle_smbus_host_notify() is indeed called from
i2c_slave_host_notify_cb(), which, in its turn, is set to be called as
->slave_cb() via i2c_slave_event() wrapper only.

Also, check [1], slide #9. I'm not sure about that "usually" word
though since I couldn't find any examples of "unusual" usage.

/* not an i2c guru here either, just looking around the code */

[1] https://elinux.org/images/f/f6/ELCE15-WolframSang-ShinyNewI2CSlaveFramework.pdf
```

and also tglx' follow-up question:

```
The question is whether it's guaranteed under all circumstances
including forced irq threading. The i801 driver has assumptions about
this, so I wouldn't be surprised if there are more.
```

Thanks.

--
Oleksandr Natalenko (post-factum)

2020-12-05 18:35:53

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline.

On Fri, Dec 04 2020 at 21:19, Oleksandr Natalenko wrote:
> On Thu, Dec 03, 2020 at 07:04:00PM +0000, [email protected] wrote:
>> 2) Have a wrapper around handle_generic_irq() which ensures that
>> interrupts are disabled before invoking it.

> The question is whether it's guaranteed under all circumstances
> including forced irq threading. The i801 driver has assumptions about
> this, so I wouldn't be surprised if there are more.

Assuming that a final answer might take some time, the below which
implements #2 will make it at least work for now.

Thanks,

tglx
---
Subject: genirq, i2c: Provide and use generic_dispatch_irq()
From: Thomas Gleixner <[email protected]>
Date: Thu, 03 Dec 2020 19:12:24 +0100

Carlos reported that on his system booting with 'threadirqs' on the command
line result in the following warning:

irq 31 handler irq_default_primary_handler+0x0/0x10 enabled interrupts
WARNING: CPU: 2 PID: 989 at kernel/irq/handle.c:153 __handle_irq_event_percpu+0x19f/0x1b0

The reason is in the i2c stack:

i801_isr()
i801_host_notify_isr()
i2c_handle_smbus_host_notify()
generic_handle_irq()

and that explodes with forced interrupt threading because it's called with
interrupts enabled.

It would be possible to set IRQF_NO_THREAD on the i801 interrupt to exclude
it from force threading, but that would break on RT and require a larger
update.

It's also unclear whether there are other drivers which can reach that code
path via i2c_slave_host_notify_cb(). As there are enough i2c drivers which
use threaded interrupt handlers by default it seems not completely
impossible that this can happen even without force threaded interrupts.

For a quick fix provide a wrapper around generic_handle_irq() which has a
local_irq_save/restore() around the invocation and use it in the i2c code.

Reported-by: Carlos Jimenez <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=202453
---
drivers/i2c/i2c-core-base.c | 2 +-
include/linux/irqdesc.h | 1 +
kernel/irq/irqdesc.c | 20 ++++++++++++++++++++
3 files changed, 22 insertions(+), 1 deletion(-)

--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1385,7 +1385,7 @@ int i2c_handle_smbus_host_notify(struct
if (irq <= 0)
return -ENXIO;

- generic_handle_irq(irq);
+ generic_dispatch_irq(irq);

return 0;
}
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -153,6 +153,7 @@ static inline void generic_handle_irq_de
}

int generic_handle_irq(unsigned int irq);
+int generic_dispatch_irq(unsigned int irq);

#ifdef CONFIG_HANDLE_DOMAIN_IRQ
/*
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -652,6 +652,26 @@ int generic_handle_irq(unsigned int irq)
}
EXPORT_SYMBOL_GPL(generic_handle_irq);

+/**
+ * generic_dispatch_irq - Dispatch an interrupt from an interrupt handler
+ * @irq: The irq number to handle
+ *
+ * A wrapper around generic_handle_irq() which ensures that interrupts are
+ * disabled when the primary handler of the dispatched irq is invoked.
+ * This is useful for interrupt handlers with dispatching to be safe for
+ * the forced threaded case.
+ */
+int generic_dispatch_irq(unsigned int irq)
+{
+ unsigned long flags;
+ int ret;
+
+ local_irq_save(&flags);
+ ret = generic_handle_irq(irq);
+ local_irq_restore(&flags);
+ return ret;
+}
+
#ifdef CONFIG_HANDLE_DOMAIN_IRQ
/**
* __handle_domain_irq - Invoke the handler for a HW irq belonging to a domain

2020-12-05 18:45:03

by Oleksandr Natalenko

[permalink] [raw]

Subject: Re: [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline.

On Sat, Dec 05, 2020 at 05:19:18PM +0100, Thomas Gleixner wrote:
> On Fri, Dec 04 2020 at 21:19, Oleksandr Natalenko wrote:
> > On Thu, Dec 03, 2020 at 07:04:00PM +0000, [email protected] wrote:
> >> 2) Have a wrapper around handle_generic_irq() which ensures that
> >> interrupts are disabled before invoking it.
>
> > The question is whether it's guaranteed under all circumstances
> > including forced irq threading. The i801 driver has assumptions about
> > this, so I wouldn't be surprised if there are more.
>
> Assuming that a final answer might take some time, the below which
> implements #2 will make it at least work for now.
>
> Thanks,
>
> tglx
> ---
> Subject: genirq, i2c: Provide and use generic_dispatch_irq()
> From: Thomas Gleixner <[email protected]>
> Date: Thu, 03 Dec 2020 19:12:24 +0100
>
> Carlos reported that on his system booting with 'threadirqs' on the command
> line result in the following warning:
>
> irq 31 handler irq_default_primary_handler+0x0/0x10 enabled interrupts
> WARNING: CPU: 2 PID: 989 at kernel/irq/handle.c:153 __handle_irq_event_percpu+0x19f/0x1b0
>
> The reason is in the i2c stack:
>
> i801_isr()
> i801_host_notify_isr()
> i2c_handle_smbus_host_notify()
> generic_handle_irq()
>
> and that explodes with forced interrupt threading because it's called with
> interrupts enabled.
>
> It would be possible to set IRQF_NO_THREAD on the i801 interrupt to exclude
> it from force threading, but that would break on RT and require a larger
> update.
>
> It's also unclear whether there are other drivers which can reach that code
> path via i2c_slave_host_notify_cb(). As there are enough i2c drivers which
> use threaded interrupt handlers by default it seems not completely
> impossible that this can happen even without force threaded interrupts.
>
> For a quick fix provide a wrapper around generic_handle_irq() which has a
> local_irq_save/restore() around the invocation and use it in the i2c code.
>
> Reported-by: Carlos Jimenez <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=202453
> ---
> drivers/i2c/i2c-core-base.c | 2 +-
> include/linux/irqdesc.h | 1 +
> kernel/irq/irqdesc.c | 20 ++++++++++++++++++++
> 3 files changed, 22 insertions(+), 1 deletion(-)
>
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1385,7 +1385,7 @@ int i2c_handle_smbus_host_notify(struct
> if (irq <= 0)
> return -ENXIO;
>
> - generic_handle_irq(irq);
> + generic_dispatch_irq(irq);
>
> return 0;
> }
> --- a/include/linux/irqdesc.h
> +++ b/include/linux/irqdesc.h
> @@ -153,6 +153,7 @@ static inline void generic_handle_irq_de
> }
>
> int generic_handle_irq(unsigned int irq);
> +int generic_dispatch_irq(unsigned int irq);
>
> #ifdef CONFIG_HANDLE_DOMAIN_IRQ
> /*
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -652,6 +652,26 @@ int generic_handle_irq(unsigned int irq)
> }
> EXPORT_SYMBOL_GPL(generic_handle_irq);
>
> +/**
> + * generic_dispatch_irq - Dispatch an interrupt from an interrupt handler
> + * @irq: The irq number to handle
> + *
> + * A wrapper around generic_handle_irq() which ensures that interrupts are
> + * disabled when the primary handler of the dispatched irq is invoked.
> + * This is useful for interrupt handlers with dispatching to be safe for
> + * the forced threaded case.
> + */
> +int generic_dispatch_irq(unsigned int irq)
> +{
> + unsigned long flags;
> + int ret;
> +
> + local_irq_save(&flags);
> + ret = generic_handle_irq(irq);
> + local_irq_restore(&flags);

FWIW, for me &flags explodes build on v5.10-rc6. I had to change it to local_irq_save/restore(flags) (without taking an address via &).

> + return ret;
> +}
> +
> #ifdef CONFIG_HANDLE_DOMAIN_IRQ
> /**
> * __handle_domain_irq - Invoke the handler for a HW irq belonging to a domain

--
Oleksandr Natalenko (post-factum)

2020-12-05 19:04:52

by Thomas Gleixner

[permalink] [raw]

Subject: Re: [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline.

On Sat, Dec 05 2020 at 17:24, Oleksandr Natalenko wrote:
> On Sat, Dec 05, 2020 at 05:19:18PM +0100, Thomas Gleixner wrote:
>> +/**
>> + * generic_dispatch_irq - Dispatch an interrupt from an interrupt handler
>> + * @irq: The irq number to handle
>> + *
>> + * A wrapper around generic_handle_irq() which ensures that interrupts are
>> + * disabled when the primary handler of the dispatched irq is invoked.
>> + * This is useful for interrupt handlers with dispatching to be safe for
>> + * the forced threaded case.
>> + */
>> +int generic_dispatch_irq(unsigned int irq)
>> +{
>> + unsigned long flags;
>> + int ret;
>> +
>> + local_irq_save(&flags);
>> + ret = generic_handle_irq(irq);
>> + local_irq_restore(&flags);
>
> FWIW, for me &flags explodes build on v5.10-rc6. I had to change it to local_irq_save/restore(flags) (without taking an address via &).

That's right. Don't know what I was thinking when writing it and then
compiling with the patch removed (just checked history ...) Oh, well

2020-12-05 20:23:47

by kernel test robot

[permalink] [raw]

Subject: Re: genirq, i2c: Provide and use generic_dispatch_irq()

Attachments:

(No filename) (7.07 kB)
.config.gz (25.09 kB)
Download all attachments

2020-12-05 20:25:24

by kernel test robot

[permalink] [raw]

Subject: Re: genirq, i2c: Provide and use generic_dispatch_irq()

Attachments:

(No filename) (5.65 kB)
.config.gz (36.77 kB)
Download all attachments

2020-12-05 21:31:36

by kernel test robot

[permalink] [raw]

Subject: Re: genirq, i2c: Provide and use generic_dispatch_irq()

Hi Thomas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on tip/irq/core linux/master linus/master v5.10-rc6 next-20201204]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Thomas-Gleixner/genirq-i2c-Provide-and-use-generic_dispatch_irq/20201206-023308
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: parisc-randconfig-s032-20201206 (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.3-179-ga00755aa-dirty
# https://github.com/0day-ci/linux/commit/79d4e8b739f9018dc2d95a25baf1199fbcf1fb03
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Thomas-Gleixner/genirq-i2c-Provide-and-use-generic_dispatch_irq/20201206-023308
git checkout 79d4e8b739f9018dc2d95a25baf1199fbcf1fb03
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=parisc

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from include/linux/spinlock.h:50,
from include/linux/irq.h:14,
from kernel/irq/irqdesc.c:10:
kernel/irq/irqdesc.c: In function 'generic_dispatch_irq':
include/linux/typecheck.h:12:18: warning: comparison of distinct pointer types lacks a cast
12 | (void)(&__dummy == &__dummy2); \
| ^~
include/linux/irqflags.h:159:3: note: in expansion of macro 'typecheck'
159 | typecheck(unsigned long, flags); \
| ^~~~~~~~~
include/linux/irqflags.h:202:3: note: in expansion of macro 'raw_local_irq_save'
202 | raw_local_irq_save(flags); \
| ^~~~~~~~~~~~~~~~~~
kernel/irq/irqdesc.c:669:2: note: in expansion of macro 'local_irq_save'
669 | local_irq_save(&flags);
| ^~~~~~~~~~~~~~
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/parisc/include/asm/cmpxchg.h:89,
from arch/parisc/include/asm/atomic.h:10,
from include/linux/atomic.h:7,
from arch/parisc/include/asm/bitops.h:13,
from include/linux/bitops.h:29,
from include/linux/kernel.h:12,
from include/linux/list.h:9,
from include/linux/preempt.h:11,
from include/linux/spinlock.h:51,
from include/linux/irq.h:14,
from kernel/irq/irqdesc.c:10:
include/linux/irqflags.h:160:9: error: lvalue required as left operand of assignment
160 | flags = arch_local_irq_save(); \
| ^
include/linux/irqflags.h:202:3: note: in expansion of macro 'raw_local_irq_save'
202 | raw_local_irq_save(flags); \
| ^~~~~~~~~~~~~~~~~~
kernel/irq/irqdesc.c:669:2: note: in expansion of macro 'local_irq_save'
669 | local_irq_save(&flags);
| ^~~~~~~~~~~~~~
In file included from include/linux/spinlock.h:50,
from include/linux/irq.h:14,
from kernel/irq/irqdesc.c:10:
include/linux/typecheck.h:12:18: warning: comparison of distinct pointer types lacks a cast
12 | (void)(&__dummy == &__dummy2); \
| ^~
include/linux/irqflags.h:174:3: note: in expansion of macro 'typecheck'
174 | typecheck(unsigned long, flags); \
| ^~~~~~~~~
include/linux/irqflags.h:203:8: note: in expansion of macro 'raw_irqs_disabled_flags'
203 | if (!raw_irqs_disabled_flags(flags)) \
| ^~~~~~~~~~~~~~~~~~~~~~~
kernel/irq/irqdesc.c:669:2: note: in expansion of macro 'local_irq_save'
669 | local_irq_save(&flags);
| ^~~~~~~~~~~~~~
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/parisc/include/asm/cmpxchg.h:89,
from arch/parisc/include/asm/atomic.h:10,
from include/linux/atomic.h:7,
from arch/parisc/include/asm/bitops.h:13,
from include/linux/bitops.h:29,
from include/linux/kernel.h:12,
from include/linux/list.h:9,
from include/linux/preempt.h:11,
from include/linux/spinlock.h:51,
from include/linux/irq.h:14,
from kernel/irq/irqdesc.c:10:
>> kernel/irq/irqdesc.c:669:17: warning: passing argument 1 of 'arch_irqs_disabled_flags' makes integer from pointer without a cast [-Wint-conversion]
669 | local_irq_save(&flags);
| ^~~~~~
| |
| long unsigned int *
include/linux/irqflags.h:175:28: note: in definition of macro 'raw_irqs_disabled_flags'
175 | arch_irqs_disabled_flags(flags); \
| ^~~~~
kernel/irq/irqdesc.c:669:2: note: in expansion of macro 'local_irq_save'
669 | local_irq_save(&flags);
| ^~~~~~~~~~~~~~
In file included from include/linux/irqflags.h:16,
from include/asm-generic/cmpxchg-local.h:6,
from arch/parisc/include/asm/cmpxchg.h:89,
from arch/parisc/include/asm/atomic.h:10,
from include/linux/atomic.h:7,
from arch/parisc/include/asm/bitops.h:13,
from include/linux/bitops.h:29,
from include/linux/kernel.h:12,
from include/linux/list.h:9,
from include/linux/preempt.h:11,
from include/linux/spinlock.h:51,
from include/linux/irq.h:14,
from kernel/irq/irqdesc.c:10:
arch/parisc/include/asm/irqflags.h:37:59: note: expected 'long unsigned int' but argument is of type 'long unsigned int *'
37 | static inline bool arch_irqs_disabled_flags(unsigned long flags)
| ~~~~~~~~~~~~~~^~~~~
In file included from include/linux/spinlock.h:50,
from include/linux/irq.h:14,
from kernel/irq/irqdesc.c:10:
include/linux/typecheck.h:12:18: warning: comparison of distinct pointer types lacks a cast
12 | (void)(&__dummy == &__dummy2); \
| ^~
include/linux/irqflags.h:174:3: note: in expansion of macro 'typecheck'
174 | typecheck(unsigned long, flags); \
| ^~~~~~~~~
include/linux/irqflags.h:209:8: note: in expansion of macro 'raw_irqs_disabled_flags'
209 | if (!raw_irqs_disabled_flags(flags)) \
| ^~~~~~~~~~~~~~~~~~~~~~~
kernel/irq/irqdesc.c:671:2: note: in expansion of macro 'local_irq_restore'
671 | local_irq_restore(&flags);
| ^~~~~~~~~~~~~~~~~
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/parisc/include/asm/cmpxchg.h:89,
from arch/parisc/include/asm/atomic.h:10,
from include/linux/atomic.h:7,
from arch/parisc/include/asm/bitops.h:13,
from include/linux/bitops.h:29,
from include/linux/kernel.h:12,
from include/linux/list.h:9,
from include/linux/preempt.h:11,
from include/linux/spinlock.h:51,
from include/linux/irq.h:14,
from kernel/irq/irqdesc.c:10:
kernel/irq/irqdesc.c:671:20: warning: passing argument 1 of 'arch_irqs_disabled_flags' makes integer from pointer without a cast [-Wint-conversion]
671 | local_irq_restore(&flags);
| ^~~~~~
| |
| long unsigned int *
include/linux/irqflags.h:175:28: note: in definition of macro 'raw_irqs_disabled_flags'
175 | arch_irqs_disabled_flags(flags); \
| ^~~~~
kernel/irq/irqdesc.c:671:2: note: in expansion of macro 'local_irq_restore'
671 | local_irq_restore(&flags);
| ^~~~~~~~~~~~~~~~~
In file included from include/linux/irqflags.h:16,
from include/asm-generic/cmpxchg-local.h:6,
from arch/parisc/include/asm/cmpxchg.h:89,
from arch/parisc/include/asm/atomic.h:10,
from include/linux/atomic.h:7,
from arch/parisc/include/asm/bitops.h:13,
from include/linux/bitops.h:29,
from include/linux/kernel.h:12,
from include/linux/list.h:9,
from include/linux/preempt.h:11,
from include/linux/spinlock.h:51,
from include/linux/irq.h:14,
from kernel/irq/irqdesc.c:10:
arch/parisc/include/asm/irqflags.h:37:59: note: expected 'long unsigned int' but argument is of type 'long unsigned int *'
37 | static inline bool arch_irqs_disabled_flags(unsigned long flags)
| ~~~~~~~~~~~~~~^~~~~
In file included from include/linux/spinlock.h:50,
from include/linux/irq.h:14,
from kernel/irq/irqdesc.c:10:
include/linux/typecheck.h:12:18: warning: comparison of distinct pointer types lacks a cast
12 | (void)(&__dummy == &__dummy2); \
| ^~
include/linux/irqflags.h:164:3: note: in expansion of macro 'typecheck'
164 | typecheck(unsigned long, flags); \
| ^~~~~~~~~
include/linux/irqflags.h:211:3: note: in expansion of macro 'raw_local_irq_restore'
211 | raw_local_irq_restore(flags); \
| ^~~~~~~~~~~~~~~~~~~~~
kernel/irq/irqdesc.c:671:2: note: in expansion of macro 'local_irq_restore'
671 | local_irq_restore(&flags);
| ^~~~~~~~~~~~~~~~~
In file included from include/asm-generic/cmpxchg-local.h:6,
from arch/parisc/include/asm/cmpxchg.h:89,
from arch/parisc/include/asm/atomic.h:10,
from include/linux/atomic.h:7,
from arch/parisc/include/asm/bitops.h:13,

vim +/arch_irqs_disabled_flags +669 kernel/irq/irqdesc.c

654
655 /**
656 * generic_dispatch_irq - Dispatch an interrupt from an interrupt handler
657 * @irq: The irq number to handle
658 *
659 * A wrapper around generic_handle_irq() which ensures that interrupts are
660 * disabled when the primary handler of the dispatched irq is invoked.
661 * This is useful for interrupt handlers with dispatching to be safe for
662 * the forced threaded case.
663 */
664 int generic_dispatch_irq(unsigned int irq)
665 {
666 unsigned long flags;
667 int ret;
668
> 669 local_irq_save(&flags);
670 ret = generic_handle_irq(irq);
671 local_irq_restore(&flags);
672 return ret;
673 }
674

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (11.62 kB)
.config.gz (33.93 kB)
Download all attachments

2021-01-04 21:01:15

by Wolfram Sang

[permalink] [raw]

Subject: Re: [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline.

> Jean, Wolfram, Benjamin, or someone else, could you please check Thomas'
> questions above and let us know what you think?

Sending this message here again because Bugzilla didn't know about the
mailing lists. Sorry for the noise!

===

Okay, here is some context about HostNotify. It is a rarely used SMBus
extension which allows clients to send a notification to the host. The
host must be able to listen to the I2C address 0x08. The only host
controller which has implemented this natively is the i801 because the
hardware sets a bit when this happened. (Sidenote, the only clients I am
aware of which send notifications are some touchscreen controllers.)
This is why the i801 driver calls i2c_handle_smbus_host_notify()
directly. And only that one, so far.

But recently, Alain Volmat got the idea that we can use the generic
slave framework to make host controllers listen on address 0x08 and
check for a valid HostNotification. This is why the generic
i2c_slave_host_notify_cb() calls i2c_handle_smbus_host_notify(), too.
This allows all I2C host controllers which select I2C_SLAVE to use
HostNotify. Those are few currently, but their number is steadily
increasing.

And as it looks to me, currently all drivers selecting I2C_SLAVE check
their interrupts which handle the slave state (i.e. managing I2C address
0x08) in a non-threaded context. But there is no guarantee for that.
Unless we formulate one. However, my gut feeling is that option #3 might
be not so much churn for this case, but I need to double check if I am
overlooking something.

Given that only some touchscreen controllers send HostNotify and you
need to enforce threaded irqs for this WARN, this might explain why it
went unnoticed for 10 years.

I hope this helps. Thank you everyone for the input provided so far!

Attachments:

(No filename) (1.79 kB)
signature.asc (849.00 B)
Download all attachments