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