On 15-08-20 18:16:02, Jeff Haran wrote:
> > -----Original Message-----
> > From: [email protected] [mailto:[email protected]]
> > 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: [email protected] [mailto:[email protected]]
> > > > 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, <[email protected]> wrote:
> > > > >
> > > > > > On 15-08-20 19:02:50, Woody Wu wrote:
> > > > > > > On Thursday, August 20, 2015, John de la Garza <[email protected]
> > > > > > <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.
>
> 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.
> > > >
> > >
_______________________________________________
Kernelnewbies mailing list
[email protected]
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
