msleep() in interrupt handler?

[Jeff Haran]

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

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