msleep() in interrupt handler?

[victorascroft at gmail.com]

On 15-08-20 19:36:23, Jeff Haran wrote:
> > -----Original Message-----
> > From: victorascroft at gmail.com [mailto:victorascroft at gmail.com]
> > Sent: Thursday, August 20, 2015 12:16 PM
> > To: Jeff Haran
> > Cc: Woody Wu; John de la Garza; kernelnewbies
> > Subject: Re: msleep() in interrupt handler?
> > 
> > On 15-08-20 18:16:02, Jeff Haran wrote:
> > > > -----Original Message-----
> > > > From: victorascroft at gmail.com [mailto:victorascroft at gmail.com]
> > > > Sent: Thursday, August 20, 2015 10:50 AM
> > > > To: Jeff Haran
> > > > Cc: Woody Wu; John de la Garza; kernelnewbies
> > > > Subject: Re: msleep() in interrupt handler?
> > > >
> > > > On 15-08-20 16:54:26, Jeff Haran wrote:
> > > > > > -----Original Message-----
> > > > > > From: victorascroft at gmail.com [mailto:victorascroft at gmail.com]
> > > > > > Sent: Thursday, August 20, 2015 9:42 AM
> > > > > > To: Woody Wu
> > > > > > Cc: John de la Garza; Jeff Haran; kernelnewbies
> > > > > > Subject: Re: msleep() in interrupt handler?
> > > > > >
> > > > > > On 15-08-20 21:44:14, Woody Wu wrote:
> > > > > > > On Thursday, August 20, 2015, <victorascroft at gmail.com> wrote:
> > > > > > >
> > > > > > > > On 15-08-20 19:02:50, Woody Wu wrote:
> > > > > > > > > On Thursday, August 20, 2015, John de la Garza
> > > > > > > > > <john at jjdev.com
> > > > > > > > <javascript:;>> wrote:
> > > > > > > > >
> > > > > > > > > > On Thu, Aug 20, 2015 at 01:45:34PM +0800, Woody Wu wrote:
> > > > > > > > > > > I did not see the message.  Actually my interrupt
> > > > > > > > > > > handler is calling i2c_transfer which in turn used
> > > > > > > > > > > msleep() somewhere in its code.  Is
> > > > > > > > this
> > > > > > > > > > > normal or dangerous?
> > > > > > > > > >
> > > > > > > > > > Can you have the interrupt handler put the work on a
> > > > > > > > > > workqueue and quickly return?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Yes, that is an option.  But I firstly need to know the
> > > > > > > > > old code is
> > > > > > > > really
> > > > > > > > > bad. The interrupt is triggered by an i2c touchscreen, and
> > > > > > > > > the interrupt handler use the i2c core code to start the
> > > > > > > > > i2c transferring.  I see in
> > > > > > > > the
> > > > > > > > > i2c adapter code a msleep() was invoked at beginning of
> > transfer.
> > > > > > > > > I
> > > > > > > > doubt
> > > > > > > > > that this is a potential problem.  But you know the i2c
> > > > > > > > > touchscreen
> > > > > > > > driver
> > > > > > > > > code is also part of the mainline, so I am not sure my option.
> > > > > > > > > You guys can check the code of atmel_mXT224_ts.c, the i2c
> > > > > > > > > adapter code is
> > > > > > > > i2c_s3c.c
> > > > > > > >
> > > > > > > > I checked the code. The kernel release I am checking in is 4.1.5.
> > > > > > > > From what I can see there is only atmel_mxt_ts.c and not
> > > > > > > > atmel_mXT224_ts.c in drivers/ input/touchscreen. In this
> > > > > > > > code, it is requesting a threaded irq with the top handler
> > > > > > > > being specified as null and the bottom handler specified.
> > > > > > > >
> > > > > > > > Since the bottom handler is being used where i2c_transfer is
> > > > > > > > called and as such though on a quick check I do not see the
> > > > > > > > msleep() call, even if the msleep were called while in the
> > > > > > > > bottom handler context it would be fine.
> > > > > > > >
> > > > > > > > I do not know which code you are referring to but in hard
> > > > > > > > interrupt context atleast you can never ever call any
> > > > > > > > function which can sleep. It is just gonna blow in some way.
> > > > > > > >
> > > > > > > > - Sanchayan.
> > > > > > > >
> > > > > > >
> > > > > > > The file name you said is right.  The kernel version I am
> > > > > > > using is 3.1.x, but I guess it does no much matter to the
> > > > > > > question. The interrupt handler of the atmel_mxt_ts called
> > > > > > > i2c_transfer() which indeed called the actual i2c adapter's
> > > > > > > transfer method. In my platform, the i2c adapter is a s3c i2c
> > > > > > > controller, so I was checking the code in
> > > > > > > i2c/busses/i2c_s3c.c, from this file I saw the msleep() was
> > > > > > > called in  i2c_doxfer()->i2c_set_master() call sequence. I
> > > > > > > think you can
> > > > > > find he similar things in 4.1.5.
> > > > > >
> > > > > > Yes right atmel_mxt_ts does call i2c_transfer. I did not check
> > > > > > myself but even if the call chain results in msleep() getting
> > > > > > called somewhere this would not be in the top irq handler. So
> > > > > > mlseep() is ok. Had this been in top irq handler (which it will
> > > > > > never be in the
> > > > > > kernel) then it will just not work at all as the kernel will crash.
> > > > > >
> > > > > > Check all drivers in touchscreen. All of them do not use the top
> > > > > > irq handler and use a bottom handler specified with threaded irq
> > > > > > request. So it is fine if
> > > > > > msleep() is getting called somewhere down that line.
> > > > >
> > > > > Perhaps I misread the msleep() code, but it appeared to me that it
> > > > > resulted
> > > > in a call to schedule(). If you are executing in bottom half context
> > > > and call a kernel function that results in a scheduling, what
> > > > returns execution to the bottom half after schedule()? There's no
> > > > task control block behind a bottom half to return control to, is there?
> > > >
> > > > Nice question. Thanks for asking. It made me look at that code for
> > > > the first time. Frankly I accept I do not know. But here goes.
> > > >
> > > > The call chain is as follows:
> > > > msleep()
> > > > schedule_timeout_uninterruptible()
> > > > schedule_timeout()
> > > > From here it sets up a timer and calls schedule() In schedule(), we
> > > > take task_struct as the current one submit this task and then call
> > > > __schedule() (The comments above this one are worth reading)
> > > >
> > > > I believe this will definitely return control back from what I
> > > > understand once this task is scheduled back.
> > > >
> > > > -- Sanchayan.
> > >
> > > But the current task_struct will be whatever task happened to be running
> > when the soft IRQ went off. Execution won't return to the soft IRQ code that
> > called msleep() because soft IRQs aren't tasks. When soft IRQs are running
> > under ksoftirqd it might work, but that's the exception. Take a look at this call
> > tree:
> > 
> > The thread handler function which is setup by the request threaded irq is a
> > kernel thread. The kernel thread I talk of here isn't the soft irq you are
> > speaking of. Kernel thread can run in process context as far as I know the last
> > time I looked up. So on sleeping and getting scheduled, it definitiely has a
> > context it can return back to. Now this place I do not have the code to trace
> > and only know in theory or atleast I am not sure if I am looking at the correct
> > place.
> > 
> > request_threaded_irq
> > __setup_irq
> > 
> > This shows a task struct being associated with the irqaction. So the kernel
> > thread definitely has a process context. As long as there is a process context
> > we can definitely return from a msleep()->schedule()->__schedule.
> > 
> > -- Sanchayan.
> > 
> 
> OK, I see the source of my confusion. When I read "bottom handler" in your original post I assumed you were referring to bottom halves as in soft IRQs and missed the threaded IRQ bit.
> 
> My apologies for the distraction and thanks for clarifying this.

No problem at all. It was an interesting discussion and I got to learn
what I wrote. Thanks. Cheers.

- Sanchayan.

> 
> Jeff Haran
> 
> > >
> > > 	schedule() ->
> > > 		__schedule() ->
> > > 			schedule_debug()
> > >
> > > 2631 /*
> > > 2632  * Various schedule()-time debugging checks and statistics:
> > > 2633  */
> > > 2634 static inline void schedule_debug(struct task_struct *prev)
> > > 2635 {
> > > 2636 #ifdef CONFIG_SCHED_STACK_END_CHECK
> > > 2637         BUG_ON(unlikely(task_stack_end_corrupted(prev)));
> > > 2638 #endif
> > > 2639         /*
> > > 2640          * Test if we are atomic. Since do_exit() needs to call into
> > > 2641          * schedule() atomically, we ignore that path. Otherwise whine
> > > 2642          * if we are scheduling when we should not.
> > > 2643          */
> > > 2644         if (unlikely(in_atomic_preempt_off() && prev->state !=
> > TASK_DEAD))
> > > 2645                 __schedule_bug(prev);
> > > 2646         rcu_sleep_check();
> > > 2647
> > > 2648         profile_hit(SCHED_PROFILING, __builtin_return_address(0));
> > > 2649
> > > 2650         schedstat_inc(this_rq(), sched_count);
> > > 2651 }
> > >
> > > The last time I looked at this in any detail (which was admittedly quite a
> > while ago), in_atomic_preempt_off() would return true when called by a
> > soft IRQ, but it could have changed since then.
> > >
> > > Jeff Haran
> > >
> > > > >
> > > > > > Also as far as I know none of the touchscreen drivers in
> > > > > > drivers/input/touchscreen use the irq handler plus workqueue
> > > > > > approach anymore. Also if one were to try and submit such a one
> > > > > > to the mainline you will be just asked to convert to a threaded
> > > > > > irq approach. Just some info since I saw a recommendation on
> > > > > > going with irq + workqueue approach. However I dont know if
> > > > > > threaded irqs existed
> > > > in 3.1.x.
> > > > > >
> > > > > > - Sanchayan.
> > > > > >
> > > > >