msleep() in interrupt handler?

[victorascroft at gmail.com]

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.

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