kernelnewbies.kernelnewbies.org archive mirror
 help / color / mirror / Atom feed
* msleep() in interrupt handler?
@ 2015-08-19 23:47 Woody Wu
  2015-08-19 23:52 ` Jeff Haran
  0 siblings, 1 reply; 14+ messages in thread
From: Woody Wu @ 2015-08-19 23:47 UTC (permalink / raw)
  To: kernelnewbies

Hi List,

What will happen if call msleep() in an interrupt handler? I knew the
msleep() will cause the calling process to sleep, but in the context of
interruption, there is no process, so I am wondering what is getting
'sleep' in this situation.

-woody


-- 
Sent from Gmail Mobile
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150820/8a47bab1/attachment.html 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* msleep() in interrupt handler?
  2015-08-19 23:47 msleep() in interrupt handler? Woody Wu
@ 2015-08-19 23:52 ` Jeff Haran
  2015-08-20  5:45   ` Woody Wu
  0 siblings, 1 reply; 14+ messages in thread
From: Jeff Haran @ 2015-08-19 23:52 UTC (permalink / raw)
  To: kernelnewbies



From: kernelnewbies-bounces@kernelnewbies.org [mailto:kernelnewbies-bounces at kernelnewbies.org] On Behalf Of Woody Wu
Sent: Wednesday, August 19, 2015 4:47 PM
To: kernelnewbies
Subject: msleep() in interrupt handler?

Hi List,

What will happen if call msleep() in an interrupt handler? I knew the msleep() will cause the calling process to sleep, but in the context of interruption, there is no process, so I am wondering what is getting 'sleep' in this situation.

-woody


--
Sent from Gmail Mobile

If you are lucky you?ll get a message along the lines of ?scheduling while atomic? on your console to let you know you?ve blown it. At least that?s what I?ve seen when I?ve blown it like that. 8^)

Jeff Haran
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150819/c7b988fa/attachment.html 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* msleep() in interrupt handler?
  2015-08-19 23:52 ` Jeff Haran
@ 2015-08-20  5:45   ` Woody Wu
  2015-08-20  6:17     ` John de la Garza
  0 siblings, 1 reply; 14+ messages in thread
From: Woody Wu @ 2015-08-20  5:45 UTC (permalink / raw)
  To: kernelnewbies

On Thursday, August 20, 2015, Jeff Haran <Jeff.Haran@citrix.com> wrote:

>
>
>
>
> *From:* kernelnewbies-bounces at kernelnewbies.org
> <javascript:_e(%7B%7D,'cvml','kernelnewbies-bounces@kernelnewbies.org');>
> [mailto:kernelnewbies-bounces at kernelnewbies.org
> <javascript:_e(%7B%7D,'cvml','kernelnewbies-bounces@kernelnewbies.org');>]
> *On Behalf Of *Woody Wu
> *Sent:* Wednesday, August 19, 2015 4:47 PM
> *To:* kernelnewbies
> *Subject:* msleep() in interrupt handler?
>
>
>
> Hi List,
>
>
>
> What will happen if call msleep() in an interrupt handler? I knew the
> msleep() will cause the calling process to sleep, but in the context of
> interruption, there is no process, so I am wondering what is getting
> 'sleep' in this situation.
>
>
>
> -woody
>
>
>
> --
> Sent from Gmail Mobile
>
>
>
> If you are lucky you?ll get a message along the lines of ?scheduling while
> atomic? on your console to let you know you?ve blown it. At least that?s
> what I?ve seen when I?ve blown it like that. 8^)
>
>
>
> Jeff Haran
>
>
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?



-- 
Sent from Gmail Mobile
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150820/d6efd1fe/attachment.html 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* msleep() in interrupt handler?
  2015-08-20  5:45   ` Woody Wu
@ 2015-08-20  6:17     ` John de la Garza
  2015-08-20 11:02       ` Woody Wu
  0 siblings, 1 reply; 14+ messages in thread
From: John de la Garza @ 2015-08-20  6:17 UTC (permalink / raw)
  To: kernelnewbies

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?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* msleep() in interrupt handler?
  2015-08-20  6:17     ` John de la Garza
@ 2015-08-20 11:02       ` Woody Wu
  2015-08-20 12:35         ` victorascroft at gmail.com
  0 siblings, 1 reply; 14+ messages in thread
From: Woody Wu @ 2015-08-20 11:02 UTC (permalink / raw)
  To: kernelnewbies

On Thursday, August 20, 2015, John de la Garza <john@jjdev.com> 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

Thanks in advance.

-woody


-- 
Sent from Gmail Mobile
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150820/48bbdfa1/attachment.html 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* msleep() in interrupt handler?
  2015-08-20 11:02       ` Woody Wu
@ 2015-08-20 12:35         ` victorascroft at gmail.com
  2015-08-20 13:44           ` Woody Wu
  0 siblings, 1 reply; 14+ messages in thread
From: victorascroft at gmail.com @ 2015-08-20 12:35 UTC (permalink / raw)
  To: kernelnewbies

On 15-08-20 19:02:50, Woody Wu wrote:
> On Thursday, August 20, 2015, John de la Garza <john@jjdev.com> 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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* msleep() in interrupt handler?
  2015-08-20 12:35         ` victorascroft at gmail.com
@ 2015-08-20 13:44           ` Woody Wu
  2015-08-20 16:42             ` victorascroft at gmail.com
  0 siblings, 1 reply; 14+ messages in thread
From: Woody Wu @ 2015-08-20 13:44 UTC (permalink / raw)
  To: kernelnewbies

On Thursday, August 20, 2015, <victorascroft@gmail.com> wrote:

> On 15-08-20 19:02:50, Woody Wu wrote:
> > On Thursday, August 20, 2015, John de la Garza <john@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.

Thanks.
-woody


-- 
Sent from Gmail Mobile
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150820/f74e57b4/attachment.html 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* msleep() in interrupt handler?
  2015-08-20 13:44           ` Woody Wu
@ 2015-08-20 16:42             ` victorascroft at gmail.com
  2015-08-20 16:54               ` Jeff Haran
  0 siblings, 1 reply; 14+ messages in thread
From: victorascroft at gmail.com @ 2015-08-20 16:42 UTC (permalink / raw)
  To: kernelnewbies

On 15-08-20 21:44:14, Woody Wu wrote:
> On Thursday, August 20, 2015, <victorascroft@gmail.com> wrote:
> 
> > On 15-08-20 19:02:50, Woody Wu wrote:
> > > On Thursday, August 20, 2015, John de la Garza <john@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.

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* msleep() in interrupt handler?
  2015-08-20 16:42             ` victorascroft at gmail.com
@ 2015-08-20 16:54               ` Jeff Haran
  2015-08-20 17:49                 ` victorascroft at gmail.com
  0 siblings, 1 reply; 14+ messages in thread
From: Jeff Haran @ 2015-08-20 16:54 UTC (permalink / raw)
  To: kernelnewbies

> -----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@gmail.com> wrote:
> >
> > > On 15-08-20 19:02:50, Woody Wu wrote:
> > > > On Thursday, August 20, 2015, John de la Garza <john@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.
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* msleep() in interrupt handler?
  2015-08-20 16:54               ` Jeff Haran
@ 2015-08-20 17:49                 ` victorascroft at gmail.com
  2015-08-20 18:16                   ` Jeff Haran
  0 siblings, 1 reply; 14+ messages in thread
From: victorascroft at gmail.com @ 2015-08-20 17:49 UTC (permalink / raw)
  To: kernelnewbies

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@gmail.com> wrote:
> > >
> > > > On 15-08-20 19:02:50, Woody Wu wrote:
> > > > > On Thursday, August 20, 2015, John de la Garza <john@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.
> > 
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* msleep() in interrupt handler?
  2015-08-20 17:49                 ` victorascroft at gmail.com
@ 2015-08-20 18:16                   ` Jeff Haran
  2015-08-20 19:15                     ` victorascroft at gmail.com
  0 siblings, 1 reply; 14+ messages in thread
From: Jeff Haran @ 2015-08-20 18:16 UTC (permalink / raw)
  To: kernelnewbies

> -----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@gmail.com> wrote:
> > > >
> > > > > On 15-08-20 19:02:50, Woody Wu wrote:
> > > > > > On Thursday, August 20, 2015, John de la Garza <john@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:

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* msleep() in interrupt handler?
  2015-08-20 18:16                   ` Jeff Haran
@ 2015-08-20 19:15                     ` victorascroft at gmail.com
  2015-08-20 19:36                       ` Jeff Haran
  0 siblings, 1 reply; 14+ messages in thread
From: victorascroft at gmail.com @ 2015-08-20 19:15 UTC (permalink / raw)
  To: kernelnewbies

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@gmail.com> wrote:
> > > > >
> > > > > > On 15-08-20 19:02:50, Woody Wu wrote:
> > > > > > > On Thursday, August 20, 2015, John de la Garza <john@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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* msleep() in interrupt handler?
  2015-08-20 19:15                     ` victorascroft at gmail.com
@ 2015-08-20 19:36                       ` Jeff Haran
  2015-08-20 19:52                         ` victorascroft at gmail.com
  0 siblings, 1 reply; 14+ messages in thread
From: Jeff Haran @ 2015-08-20 19:36 UTC (permalink / raw)
  To: kernelnewbies

> -----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@gmail.com> wrote:
> > > > > >
> > > > > > > On 15-08-20 19:02:50, Woody Wu wrote:
> > > > > > > > On Thursday, August 20, 2015, John de la Garza
> > > > > > > > <john@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.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* msleep() in interrupt handler?
  2015-08-20 19:36                       ` Jeff Haran
@ 2015-08-20 19:52                         ` victorascroft at gmail.com
  0 siblings, 0 replies; 14+ messages in thread
From: victorascroft at gmail.com @ 2015-08-20 19:52 UTC (permalink / raw)
  To: kernelnewbies

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@gmail.com> wrote:
> > > > > > >
> > > > > > > > On 15-08-20 19:02:50, Woody Wu wrote:
> > > > > > > > > On Thursday, August 20, 2015, John de la Garza
> > > > > > > > > <john@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.
> > > > > >
> > > > >

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-08-20 19:52 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-19 23:47 msleep() in interrupt handler? Woody Wu
2015-08-19 23:52 ` Jeff Haran
2015-08-20  5:45   ` Woody Wu
2015-08-20  6:17     ` John de la Garza
2015-08-20 11:02       ` Woody Wu
2015-08-20 12:35         ` victorascroft at gmail.com
2015-08-20 13:44           ` Woody Wu
2015-08-20 16:42             ` victorascroft at gmail.com
2015-08-20 16:54               ` Jeff Haran
2015-08-20 17:49                 ` victorascroft at gmail.com
2015-08-20 18:16                   ` Jeff Haran
2015-08-20 19:15                     ` victorascroft at gmail.com
2015-08-20 19:36                       ` Jeff Haran
2015-08-20 19:52                         ` victorascroft at gmail.com

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).