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

linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* Re: [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline.
       [not found] ` <bug-202453-19117-0k1QQBMPTi@https.bugzilla.kernel.org/>
@ 2020-12-04 20:19   ` Oleksandr Natalenko
  2020-12-05 16:19     ` Thomas Gleixner
  2021-01-04 20:58     ` [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline Wolfram Sang
  0 siblings, 2 replies; 8+ messages in thread
From: Oleksandr Natalenko @ 2020-12-04 20:19 UTC (permalink / raw)
  To: bugzilla-daemon
  Cc: jdelvare, wsa, benjamin.tissoires, rui.zhang, tglx, linux-i2c,
	linux-kernel

On Thu, Dec 03, 2020 at 07:04:00PM +0000, bugzilla-daemon@bugzilla.kernel.org wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=202453
> 
> --- Comment #7 from Thomas Gleixner (tglx@linutronix.de) ---
> On Tue, Aug 06 2019 at 14:07, bugzilla-daemon wrote:
> > Jean Delvare (jdelvare@suse.de) 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)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline.
  2020-12-04 20:19   ` [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline Oleksandr Natalenko
@ 2020-12-05 16:19     ` Thomas Gleixner
  2020-12-05 16:24       ` Oleksandr Natalenko
                         ` (3 more replies)
  2021-01-04 20:58     ` [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline Wolfram Sang
  1 sibling, 4 replies; 8+ messages in thread
From: Thomas Gleixner @ 2020-12-05 16:19 UTC (permalink / raw)
  To: Oleksandr Natalenko, bugzilla-daemon
  Cc: jdelvare, wsa, benjamin.tissoires, rui.zhang, linux-i2c,
	linux-kernel, Marc Zyngier, Peter Zijlstra, Carlos Jimenez

On Fri, Dec 04 2020 at 21:19, Oleksandr Natalenko wrote:
> On Thu, Dec 03, 2020 at 07:04:00PM +0000, bugzilla-daemon@bugzilla.kernel.org 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 <tglx@linutronix.de>
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 <javashin1986@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline.
  2020-12-05 16:19     ` Thomas Gleixner
@ 2020-12-05 16:24       ` Oleksandr Natalenko
  2020-12-05 18:59         ` Thomas Gleixner
  2020-12-05 20:19       ` genirq, i2c: Provide and use generic_dispatch_irq() kernel test robot
                         ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Oleksandr Natalenko @ 2020-12-05 16:24 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: bugzilla-daemon, jdelvare, wsa, benjamin.tissoires, rui.zhang,
	linux-i2c, linux-kernel, Marc Zyngier, Peter Zijlstra,
	Carlos Jimenez

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, bugzilla-daemon@bugzilla.kernel.org 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 <tglx@linutronix.de>
> 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 <javashin1986@gmail.com>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 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)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline.
  2020-12-05 16:24       ` Oleksandr Natalenko
@ 2020-12-05 18:59         ` Thomas Gleixner
  0 siblings, 0 replies; 8+ messages in thread
From: Thomas Gleixner @ 2020-12-05 18:59 UTC (permalink / raw)
  To: Oleksandr Natalenko
  Cc: bugzilla-daemon, jdelvare, wsa, benjamin.tissoires, rui.zhang,
	linux-i2c, linux-kernel, Marc Zyngier, Peter Zijlstra,
	Carlos Jimenez

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: genirq, i2c: Provide and use generic_dispatch_irq()
  2020-12-05 16:19     ` Thomas Gleixner
  2020-12-05 16:24       ` Oleksandr Natalenko
@ 2020-12-05 20:19       ` kernel test robot
  2020-12-05 20:19       ` kernel test robot
  2020-12-05 21:27       ` kernel test robot
  3 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2020-12-05 20:19 UTC (permalink / raw)
  To: Thomas Gleixner, Oleksandr Natalenko, bugzilla-daemon
  Cc: kbuild-all, jdelvare, wsa, benjamin.tissoires, rui.zhang,
	linux-i2c, linux-kernel, Marc Zyngier, Peter Zijlstra

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

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: nds32-randconfig-r016-20201206 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # 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 ARCH=nds32 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

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:225:36: note: in expansion of macro 'raw_local_irq_save'
     225 | #define local_irq_save(flags) do { raw_local_irq_save(flags); } while (0)
         |                                    ^~~~~~~~~~~~~~~~~~
   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/bitops.h:14,
                    from ./arch/nds32/include/generated/asm/bitops.h:1,
                    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:225:36: note: in expansion of macro 'raw_local_irq_save'
     225 | #define local_irq_save(flags) do { raw_local_irq_save(flags); } while (0)
         |                                    ^~~~~~~~~~~~~~~~~~
   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:164:3: note: in expansion of macro 'typecheck'
     164 |   typecheck(unsigned long, flags); \
         |   ^~~~~~~~~
   include/linux/irqflags.h:226:39: note: in expansion of macro 'raw_local_irq_restore'
     226 | #define local_irq_restore(flags) do { raw_local_irq_restore(flags); } while (0)
         |                                       ^~~~~~~~~~~~~~~~~~~~~
   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/bitops.h:14,
                    from ./arch/nds32/include/generated/asm/bitops.h:1,
                    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_local_irq_restore' makes integer from pointer without a cast [-Wint-conversion]
     671 |  local_irq_restore(&flags);
         |                    ^~~~~~
         |                    |
         |                    long unsigned int *
   include/linux/irqflags.h:165:26: note: in definition of macro 'raw_local_irq_restore'
     165 |   arch_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/linux/irqflags.h:16,
                    from include/asm-generic/bitops.h:14,
                    from ./arch/nds32/include/generated/asm/bitops.h:1,
                    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/nds32/include/asm/irqflags.h:27:57: note: expected 'long unsigned int' but argument is of type 'long unsigned int *'
      27 | static inline void arch_local_irq_restore(unsigned long flags)
         |                                           ~~~~~~~~~~~~~~^~~~~

vim +/arch_local_irq_restore +671 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/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25696 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: genirq, i2c: Provide and use generic_dispatch_irq()
  2020-12-05 16:19     ` Thomas Gleixner
  2020-12-05 16:24       ` Oleksandr Natalenko
  2020-12-05 20:19       ` genirq, i2c: Provide and use generic_dispatch_irq() kernel test robot
@ 2020-12-05 20:19       ` kernel test robot
  2020-12-05 21:27       ` kernel test robot
  3 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2020-12-05 20:19 UTC (permalink / raw)
  To: Thomas Gleixner, Oleksandr Natalenko, bugzilla-daemon
  Cc: kbuild-all, clang-built-linux, jdelvare, wsa, benjamin.tissoires,
	rui.zhang, linux-i2c, linux-kernel, Marc Zyngier, Peter Zijlstra

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

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: x86_64-randconfig-a016-20201206 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project fc7818f5d6906555cebad2c2e7c313a383b9cb82)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # 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=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> kernel/irq/irqdesc.c:669:2: warning: comparison of distinct pointer types ('unsigned long *' and 'typeof (&flags) *' (aka 'unsigned long **')) [-Wcompare-distinct-pointer-types]
           local_irq_save(&flags);
           ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/irqflags.h:225:36: note: expanded from macro 'local_irq_save'
   #define local_irq_save(flags)   do { raw_local_irq_save(flags); } while (0)
                                        ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/irqflags.h:159:3: note: expanded from macro 'raw_local_irq_save'
                   typecheck(unsigned long, flags);        \
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/typecheck.h:12:18: note: expanded from macro 'typecheck'
           (void)(&__dummy == &__dummy2); \
                  ~~~~~~~~ ^  ~~~~~~~~~
   kernel/irq/irqdesc.c:669:2: error: expression is not assignable
           local_irq_save(&flags);
           ^              ~~~~~~
   include/linux/irqflags.h:225:36: note: expanded from macro 'local_irq_save'
   #define local_irq_save(flags)   do { raw_local_irq_save(flags); } while (0)
                                        ^                  ~~~~~
   include/linux/irqflags.h:160:9: note: expanded from macro 'raw_local_irq_save'
                   flags = arch_local_irq_save();          \
                   ~~~~~ ^
   kernel/irq/irqdesc.c:671:2: warning: comparison of distinct pointer types ('unsigned long *' and 'typeof (&flags) *' (aka 'unsigned long **')) [-Wcompare-distinct-pointer-types]
           local_irq_restore(&flags);
           ^~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/irqflags.h:226:39: note: expanded from macro 'local_irq_restore'
   #define local_irq_restore(flags) do { raw_local_irq_restore(flags); } while (0)
                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/irqflags.h:164:3: note: expanded from macro 'raw_local_irq_restore'
                   typecheck(unsigned long, flags);        \
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/typecheck.h:12:18: note: expanded from macro 'typecheck'
           (void)(&__dummy == &__dummy2); \
                  ~~~~~~~~ ^  ~~~~~~~~~
>> kernel/irq/irqdesc.c:671:20: warning: incompatible pointer to integer conversion passing 'unsigned long *' to parameter of type 'unsigned long'; remove & [-Wint-conversion]
           local_irq_restore(&flags);
                             ^~~~~~
   include/linux/irqflags.h:226:61: note: expanded from macro 'local_irq_restore'
   #define local_irq_restore(flags) do { raw_local_irq_restore(flags); } while (0)
                                                               ^~~~~
   include/linux/irqflags.h:165:26: note: expanded from macro 'raw_local_irq_restore'
                   arch_local_irq_restore(flags);          \
                                          ^~~~~
   arch/x86/include/asm/irqflags.h:82:66: note: passing argument to parameter 'flags' here
   static __always_inline void arch_local_irq_restore(unsigned long flags)
                                                                    ^
   3 warnings and 1 error generated.

vim +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/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37656 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: genirq, i2c: Provide and use generic_dispatch_irq()
  2020-12-05 16:19     ` Thomas Gleixner
                         ` (2 preceding siblings ...)
  2020-12-05 20:19       ` kernel test robot
@ 2020-12-05 21:27       ` kernel test robot
  3 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2020-12-05 21:27 UTC (permalink / raw)
  To: Thomas Gleixner, Oleksandr Natalenko, bugzilla-daemon
  Cc: kbuild-all, jdelvare, wsa, benjamin.tissoires, rui.zhang,
	linux-i2c, linux-kernel, Marc Zyngier, Peter Zijlstra

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

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 <lkp@intel.com>

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/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34745 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline.
  2020-12-04 20:19   ` [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline Oleksandr Natalenko
  2020-12-05 16:19     ` Thomas Gleixner
@ 2021-01-04 20:58     ` Wolfram Sang
  1 sibling, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2021-01-04 20:58 UTC (permalink / raw)
  To: Oleksandr Natalenko
  Cc: bugzilla-daemon, jdelvare, benjamin.tissoires, rui.zhang, tglx,
	linux-i2c, linux-kernel

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

> 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!

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-01-04 20:59 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <bug-202453-19117@https.bugzilla.kernel.org/>
     [not found] ` <bug-202453-19117-0k1QQBMPTi@https.bugzilla.kernel.org/>
2020-12-04 20:19   ` [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline Oleksandr Natalenko
2020-12-05 16:19     ` Thomas Gleixner
2020-12-05 16:24       ` Oleksandr Natalenko
2020-12-05 18:59         ` Thomas Gleixner
2020-12-05 20:19       ` genirq, i2c: Provide and use generic_dispatch_irq() kernel test robot
2020-12-05 20:19       ` kernel test robot
2020-12-05 21:27       ` kernel test robot
2021-01-04 20:58     ` [Bug 202453] TRACE irq/18-i801_smb Tainted when enabled threadirqs in kernel commandline Wolfram Sang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).