From: Santosh Nayak <redacted>
Instead of "in_atomic()", we can use in_interrupt() to check whether
its an interrupt context.
Signed-off-by: Santosh Nayak <redacted>
---
drivers/video/amba-clcd.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
From: richard -rw- weinberger <hidden> Date: 2012-03-11 13:23:00
On Sun, Mar 11, 2012 at 2:14 PM, santosh nayak
[off-list ref] wrote:
From: Santosh Nayak <redacted>
Instead of "in_atomic()", we can use in_interrupt() to check whether
its an interrupt context.
What is the benefit?
Your change turns clcdfb_sleep() into a ticking bomb.
If clcdfb_sleep() is called from an atomic section (e.g. under a
spinlock) it will call msleep()
and the whole thing explodes...
--
Thanks,
//richard
From: Russell King - ARM Linux <hidden> Date: 2012-03-11 13:28:00
On Sun, Mar 11, 2012 at 06:44:25PM +0530, santosh nayak wrote:
From: Santosh Nayak <redacted>
Instead of "in_atomic()", we can use in_interrupt() to check whether
its an interrupt context.
What are you trying to fix?
Your description is an example of a bad commit comment. It merely
describes the change, which anyone can see by looking at the diff.
What it totally and utterly fails to do is to describe _why_ the
change is necessary or what the problem is.
So, until it does, this patch gets a definite NAK.
Not to use in_atomic() in driver code.
Following article inspired me to do the change.
http://lwn.net/Articles/274695/
"in_atomic() is for core kernel use only. Because in special
circumstances (ie: kmap_atomic()) we run inc_preempt_count() even on
non-preemptible kernels to tell the per-arch fault handler that it was
invoked by copy_*_user() inside kmap_atomic(), and it must fail.
In other words, in_atomic() works in a specific low-level situation,
but it was never meant to be used in a wider context. Its placement in
hardirq.h next to macros which can be used elsewhere was, thus, almost
certainly a mistake. As Alan Stern pointed out, the fact that Linux
Device Drivers recommends the use of in_atomic() will not have helped
the situation. Your editor recommends that the authors of that book be
immediately sacked. "
In the present case, we just check whether its an IRQ context or user
context. So for that
we can use "in_interrupt()".
Greg also mentions the same in the following mail.
http://www.spinics.net/lists/newbies/msg43402.html
Regards
Santosh
On Sun, Mar 11, 2012 at 6:57 PM, Russell King - ARM Linux
[off-list ref] wrote:
On Sun, Mar 11, 2012 at 06:44:25PM +0530, santosh nayak wrote:
quoted
From: Santosh Nayak <redacted>
Instead of "in_atomic()", we can use in_interrupt() to check whether
its an interrupt context.
What are you trying to fix?
Your description is an example of a bad commit comment. It merely
describes the change, which anyone can see by looking at the diff.
What it totally and utterly fails to do is to describe _why_ the
change is necessary or what the problem is.
So, until it does, this patch gets a definite NAK.
From: Russell King - ARM Linux <hidden> Date: 2012-03-11 14:36:35
On Sun, Mar 11, 2012 at 07:47:27PM +0530, santosh prasad nayak wrote:
Not to use in_atomic() in driver code.
Following article inspired me to do the change.
http://lwn.net/Articles/274695/
"in_atomic() is for core kernel use only. Because in special
circumstances (ie: kmap_atomic()) we run inc_preempt_count() even on
non-preemptible kernels to tell the per-arch fault handler that it was
invoked by copy_*_user() inside kmap_atomic(), and it must fail.
In other words, in_atomic() works in a specific low-level situation,
but it was never meant to be used in a wider context. Its placement in
hardirq.h next to macros which can be used elsewhere was, thus, almost
certainly a mistake. As Alan Stern pointed out, the fact that Linux
Device Drivers recommends the use of in_atomic() will not have helped
the situation. Your editor recommends that the authors of that book be
immediately sacked. "
In the present case, we just check whether its an IRQ context or user
context. So for that
we can use "in_interrupt()".
Greg also mentions the same in the following mail.
http://www.spinics.net/lists/newbies/msg43402.html
In which case, we'll just have to do mdelay() and forget about allowing
anything else to run for the 20ms that we need to sleep. Sucky but
that's the way things are.
On Sun, Mar 11, 2012 at 8:06 PM, Russell King - ARM Linux
[off-list ref] wrote:
On Sun, Mar 11, 2012 at 07:47:27PM +0530, santosh prasad nayak wrote:
quoted
Not to use in_atomic() in driver code.
Following article inspired me to do the change.
http://lwn.net/Articles/274695/
"in_atomic() is for core kernel use only. Because in special
circumstances (ie: kmap_atomic()) we run inc_preempt_count() even on
non-preemptible kernels to tell the per-arch fault handler that it was
invoked by copy_*_user() inside kmap_atomic(), and it must fail.
In other words, in_atomic() works in a specific low-level situation,
but it was never meant to be used in a wider context. Its placement in
hardirq.h next to macros which can be used elsewhere was, thus, almost
certainly a mistake. As Alan Stern pointed out, the fact that Linux
Device Drivers recommends the use of in_atomic() will not have helped
the situation. Your editor recommends that the authors of that book be
immediately sacked. "
In the present case, we just check whether its an IRQ context or user
context. So for that
we can use "in_interrupt()".
Greg also mentions the same in the following mail.
http://www.spinics.net/lists/newbies/msg43402.html
In which case, we'll just have to do mdelay() and forget about allowing
anything else to run for the 20ms that we need to sleep. Sucky but
that's the way things are.
mdelay() or msleep() are there before. I did not change that.
my point is : in_atomic() vs "in_interrupt()".
We should avoid to use "in_atomic()" in driver code.
In the present case to check IRQ context "in_interrupt()" should be preferred.
regards
Santosh
regards
Santosh
From: Russell King - ARM Linux <hidden> Date: 2012-03-11 15:03:28
On Sun, Mar 11, 2012 at 08:18:32PM +0530, santosh prasad nayak wrote:
On Sun, Mar 11, 2012 at 8:06 PM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
On Sun, Mar 11, 2012 at 07:47:27PM +0530, santosh prasad nayak wrote:
quoted
Not to use in_atomic() in driver code.
Following article inspired me to do the change.
http://lwn.net/Articles/274695/
"in_atomic() is for core kernel use only. Because in special
circumstances (ie: kmap_atomic()) we run inc_preempt_count() even on
non-preemptible kernels to tell the per-arch fault handler that it was
invoked by copy_*_user() inside kmap_atomic(), and it must fail.
In other words, in_atomic() works in a specific low-level situation,
but it was never meant to be used in a wider context. Its placement in
hardirq.h next to macros which can be used elsewhere was, thus, almost
certainly a mistake. As Alan Stern pointed out, the fact that Linux
Device Drivers recommends the use of in_atomic() will not have helped
the situation. Your editor recommends that the authors of that book be
immediately sacked. "
In the present case, we just check whether its an IRQ context or user
context. So for that
we can use "in_interrupt()".
Greg also mentions the same in the following mail.
http://www.spinics.net/lists/newbies/msg43402.html
In which case, we'll just have to do mdelay() and forget about allowing
anything else to run for the 20ms that we need to sleep. Sucky but
that's the way things are.
mdelay() or msleep() are there before. I did not change that.
my point is : in_atomic() vs "in_interrupt()".
We should avoid to use "in_atomic()" in driver code.
In the present case to check IRQ context "in_interrupt()" should be preferred.
in_interrupt() won't tell us if we're being called with spinlocks held,
which _is_ a possibility because this can be called from printk(), for
oops dumps and the like.
in_interrupt() just means that we're inside a hard or soft interrupt,
or nmi. It says nothing about whether msleep() is possible.
On Sun, Mar 11, 2012 at 8:33 PM, Russell King - ARM Linux
[off-list ref] wrote:
On Sun, Mar 11, 2012 at 08:18:32PM +0530, santosh prasad nayak wrote:
quoted
On Sun, Mar 11, 2012 at 8:06 PM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
On Sun, Mar 11, 2012 at 07:47:27PM +0530, santosh prasad nayak wrote:
quoted
Not to use in_atomic() in driver code.
Following article inspired me to do the change.
http://lwn.net/Articles/274695/
"in_atomic() is for core kernel use only. Because in special
circumstances (ie: kmap_atomic()) we run inc_preempt_count() even on
non-preemptible kernels to tell the per-arch fault handler that it was
invoked by copy_*_user() inside kmap_atomic(), and it must fail.
In other words, in_atomic() works in a specific low-level situation,
but it was never meant to be used in a wider context. Its placement in
hardirq.h next to macros which can be used elsewhere was, thus, almost
certainly a mistake. As Alan Stern pointed out, the fact that Linux
Device Drivers recommends the use of in_atomic() will not have helped
the situation. Your editor recommends that the authors of that book be
immediately sacked. "
In the present case, we just check whether its an IRQ context or user
context. So for that
we can use "in_interrupt()".
Greg also mentions the same in the following mail.
http://www.spinics.net/lists/newbies/msg43402.html
In which case, we'll just have to do mdelay() and forget about allowing
anything else to run for the 20ms that we need to sleep. Sucky but
that's the way things are.
mdelay() or msleep() are there before. I did not change that.
my point is : in_atomic() vs "in_interrupt()".
We should avoid to use "in_atomic()" in driver code.
In the present case to check IRQ context "in_interrupt()" should be preferred.
in_interrupt() won't tell us if we're being called with spinlocks held,
which _is_ a possibility because this can be called from printk(), for
oops dumps and the like.
in_interrupt() just means that we're inside a hard or soft interrupt,
or nmi. It says nothing about whether msleep() is possible.
in_atomic() is also not error free. I found following comment in
include/linux/hardirq.h. How do you handle it in non-preemptible
kernel ?
/*
* Are we running in atomic context? WARNING: this macro cannot
* always detect atomic context; in particular, it cannot know about
* held spinlocks in non-preemptible kernels. Thus it should not be
* used in the general case to determine whether sleeping is possible.
* Do not use in_atomic() in driver code.
*/
#define in_atomic() ((preempt_count() & ~PREEMPT_ACTIVE) != 0)
regards
Santosh
From: Russell King - ARM Linux <hidden> Date: 2012-03-11 15:49:00
On Sun, Mar 11, 2012 at 08:49:27PM +0530, santosh prasad nayak wrote:
On Sun, Mar 11, 2012 at 8:33 PM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
in_interrupt() won't tell us if we're being called with spinlocks held,
which _is_ a possibility because this can be called from printk(), for
oops dumps and the like.
in_interrupt() just means that we're inside a hard or soft interrupt,
or nmi. It says nothing about whether msleep() is possible.
in_atomic() is also not error free. I found following comment in
include/linux/hardirq.h. How do you handle it in non-preemptible
kernel ?
/*
* Are we running in atomic context? WARNING: this macro cannot
* always detect atomic context; in particular, it cannot know about
* held spinlocks in non-preemptible kernels. Thus it should not be
* used in the general case to determine whether sleeping is possible.
* Do not use in_atomic() in driver code.
*/
#define in_atomic() ((preempt_count() & ~PREEMPT_ACTIVE) != 0)
That may be, but the fact of the matter is that no one has *ever*
reported an incident where this has failed at this point - and when
it does people will end up with a might_sleep() warning from msleep().
Maybe those who are saying people should not use this should instead
be analysing why people use this, and suggest an alternative solution
to the problem instead of a basic and uninformative "you shouldn't use
this" statement.
As I've said, if we aren't going to use this, then the only solution is
to completely omit the msleep() there and just say "sod you to running
anything else for 20ms while this driver busy-spins." That's
ultimately the safe thing to do, and at the moment I see no other
alternative there.
On Sun, Mar 11, 2012 at 9:18 PM, Russell King - ARM Linux
[off-list ref] wrote:
On Sun, Mar 11, 2012 at 08:49:27PM +0530, santosh prasad nayak wrote:
quoted
On Sun, Mar 11, 2012 at 8:33 PM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
in_interrupt() won't tell us if we're being called with spinlocks held,
which _is_ a possibility because this can be called from printk(), for
oops dumps and the like.
in_interrupt() just means that we're inside a hard or soft interrupt,
or nmi. It says nothing about whether msleep() is possible.
in_atomic() is also not error free. I found following comment in
include/linux/hardirq.h. How do you handle it in non-preemptible
kernel ?
/*
* Are we running in atomic context? WARNING: this macro cannot
* always detect atomic context; in particular, it cannot know about
* held spinlocks in non-preemptible kernels. Thus it should not be
* used in the general case to determine whether sleeping is possible.
* Do not use in_atomic() in driver code.
*/
#define in_atomic() ((preempt_count() & ~PREEMPT_ACTIVE) != 0)
That may be, but the fact of the matter is that no one has *ever*
reported an incident where this has failed at this point - and when
it does people will end up with a might_sleep() warning from msleep().
Maybe those who are saying people should not use this should instead
be analysing why people use this, and suggest an alternative solution
to the problem instead of a basic and uninformative "you shouldn't use
this" statement.
The reason is given in the article.
http://lwn.net/Articles/274695/
"The in_atomic() macro works by checking whether preemption is
disabled, which seems like the right thing to do. Handlers for events
like hardware interrupts will disable preemption, but so will the
acquisition of a spinlock. So this test appears to catch all of the
cases where sleeping would be a bad idea. Certainly a number of people
who have looked at this macro have come to that conclusion.
But if preemption has not been configured into the kernel in the first
place, the kernel does not raise the "preemption count" when spinlocks
are acquired. So, in this situation (which is common - many
distributors still do not enable preemption in their kernels),
in_atomic() has no way to know if the calling code holds any spinlocks
or not. So it will return zero (indicating process context) even when
spinlocks are held. And that could lead to kernel code thinking that
it is running in process context (and acting accordingly) when, in
fact, it is not."
regards
Santosh
As I've said, if we aren't going to use this, then the only solution is
to completely omit the msleep() there and just say "sod you to running
anything else for 20ms while this driver busy-spins." That's
ultimately the safe thing to do, and at the moment I see no other
alternative there.
From: Russell King - ARM Linux <hidden> Date: 2012-03-11 16:42:56
On Sun, Mar 11, 2012 at 10:07:18PM +0530, santosh prasad nayak wrote:
On Sun, Mar 11, 2012 at 9:18 PM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
On Sun, Mar 11, 2012 at 08:49:27PM +0530, santosh prasad nayak wrote:
quoted
On Sun, Mar 11, 2012 at 8:33 PM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
in_interrupt() won't tell us if we're being called with spinlocks held,
which _is_ a possibility because this can be called from printk(), for
oops dumps and the like.
in_interrupt() just means that we're inside a hard or soft interrupt,
or nmi. It says nothing about whether msleep() is possible.
in_atomic() is also not error free. I found following comment in
include/linux/hardirq.h. How do you handle it in non-preemptible
kernel ?
/*
* Are we running in atomic context? WARNING: this macro cannot
* always detect atomic context; in particular, it cannot know about
* held spinlocks in non-preemptible kernels. Thus it should not be
* used in the general case to determine whether sleeping is possible.
* Do not use in_atomic() in driver code.
*/
#define in_atomic() ((preempt_count() & ~PREEMPT_ACTIVE) != 0)
That may be, but the fact of the matter is that no one has *ever*
reported an incident where this has failed at this point - and when
it does people will end up with a might_sleep() warning from msleep().
Maybe those who are saying people should not use this should instead
be analysing why people use this, and suggest an alternative solution
to the problem instead of a basic and uninformative "you shouldn't use
this" statement.
The reason is given in the article.
At this point I'm just going to restate what I said above and below, so
I'm not even going to bother doing that, and instead just say that. I'm
not arguing whether it's right or wrong. I'm just stating that the only
solution I see is to get rid of msleep() in there entirely.
http://lwn.net/Articles/274695/
"The in_atomic() macro works by checking whether preemption is
disabled, which seems like the right thing to do. Handlers for events
like hardware interrupts will disable preemption, but so will the
acquisition of a spinlock. So this test appears to catch all of the
cases where sleeping would be a bad idea. Certainly a number of people
who have looked at this macro have come to that conclusion.
But if preemption has not been configured into the kernel in the first
place, the kernel does not raise the "preemption count" when spinlocks
are acquired. So, in this situation (which is common - many
distributors still do not enable preemption in their kernels),
in_atomic() has no way to know if the calling code holds any spinlocks
or not. So it will return zero (indicating process context) even when
spinlocks are held. And that could lead to kernel code thinking that
it is running in process context (and acting accordingly) when, in
fact, it is not."
regards
Santosh
quoted
As I've said, if we aren't going to use this, then the only solution is
to completely omit the msleep() there and just say "sod you to running
anything else for 20ms while this driver busy-spins." That's
ultimately the safe thing to do, and at the moment I see no other
alternative there.
Russel,
Is this what you want ?
static inline void clcdfb_sleep(unsigned int ms)
{
- if (in_atomic()) {
mdelay(ms);
- } else {
- msleep(ms);
- }
}
Regards
Santosh
On Sun, Mar 11, 2012 at 10:12 PM, Russell King - ARM Linux
[off-list ref] wrote:
On Sun, Mar 11, 2012 at 10:07:18PM +0530, santosh prasad nayak wrote:
quoted
On Sun, Mar 11, 2012 at 9:18 PM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
On Sun, Mar 11, 2012 at 08:49:27PM +0530, santosh prasad nayak wrote:
quoted
On Sun, Mar 11, 2012 at 8:33 PM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
in_interrupt() won't tell us if we're being called with spinlocks held,
which _is_ a possibility because this can be called from printk(), for
oops dumps and the like.
in_interrupt() just means that we're inside a hard or soft interrupt,
or nmi. It says nothing about whether msleep() is possible.
in_atomic() is also not error free. I found following comment in
include/linux/hardirq.h. How do you handle it in non-preemptible
kernel ?
/*
* Are we running in atomic context? WARNING: this macro cannot
* always detect atomic context; in particular, it cannot know about
* held spinlocks in non-preemptible kernels. Thus it should not be
* used in the general case to determine whether sleeping is possible.
* Do not use in_atomic() in driver code.
*/
#define in_atomic() ((preempt_count() & ~PREEMPT_ACTIVE) != 0)
That may be, but the fact of the matter is that no one has *ever*
reported an incident where this has failed at this point - and when
it does people will end up with a might_sleep() warning from msleep().
Maybe those who are saying people should not use this should instead
be analysing why people use this, and suggest an alternative solution
to the problem instead of a basic and uninformative "you shouldn't use
this" statement.
The reason is given in the article.
At this point I'm just going to restate what I said above and below, so
I'm not even going to bother doing that, and instead just say that. I'm
not arguing whether it's right or wrong. I'm just stating that the only
solution I see is to get rid of msleep() in there entirely.
quoted
http://lwn.net/Articles/274695/
"The in_atomic() macro works by checking whether preemption is
disabled, which seems like the right thing to do. Handlers for events
like hardware interrupts will disable preemption, but so will the
acquisition of a spinlock. So this test appears to catch all of the
cases where sleeping would be a bad idea. Certainly a number of people
who have looked at this macro have come to that conclusion.
But if preemption has not been configured into the kernel in the first
place, the kernel does not raise the "preemption count" when spinlocks
are acquired. So, in this situation (which is common - many
distributors still do not enable preemption in their kernels),
in_atomic() has no way to know if the calling code holds any spinlocks
or not. So it will return zero (indicating process context) even when
spinlocks are held. And that could lead to kernel code thinking that
it is running in process context (and acting accordingly) when, in
fact, it is not."
regards
Santosh
quoted
As I've said, if we aren't going to use this, then the only solution is
to completely omit the msleep() there and just say "sod you to running
anything else for 20ms while this driver busy-spins." That's
ultimately the safe thing to do, and at the moment I see no other
alternative there.
From: Russell King - ARM Linux <hidden> Date: 2012-03-11 17:06:13
On Sun, Mar 11, 2012 at 10:27:12PM +0530, santosh prasad nayak wrote:
Russel,
Is this what you want ?
static inline void clcdfb_sleep(unsigned int ms)
{
- if (in_atomic()) {
mdelay(ms);
- } else {
- msleep(ms);
- }
}
"want" is a strong word - I would not say "want" because I don't want the
system busy-spinning for 20ms at a time during normal blank and unblank
events preventing any other thread in the system from running.
Looking at do_unblank_screen() in drivers/tty/vt/vt.c:
/*
* Called by timer as well as from vt_console_driver
*/
void do_unblank_screen(int leaving_gfx)
{
struct vc_data *vc;
/* This should now always be called from a "sane" (read: can schedule)
* context for the sake of the low level drivers, except in the special
* case of oops_in_progress
*/
if (!oops_in_progress)
might_sleep();
There may be another option of changing in_atomic() to be oops_in_progress()
if that comment is to be believed. However, that comment conflicts with
the comment immediately above the function. That said, the code is more
authorative than the comment above it because if that comment were true,
we'd see might_sleep() warnings.
So, I suspect:
static inline void clcdfb_sleep(unsigned int ms)
{
- if (in_atomic()) {
+ if (oops_in_progress) {
mdelay(ms);
} else {
msleep(ms);
is about the best that can be hoped for.
However, I think framebuffer people need to comment about what contexts
a framebuffer driver's fb_blank method would be called from (including
when oops-dumping.)
As I've said, if we aren't going to use this, then the only solution is
to completely omit the msleep() there and just say "sod you to running
anything else for 20ms while this driver busy-spins." That's
ultimately the safe thing to do, and at the moment I see no other
alternative there.
Anyone having this argument is also right now peeing into the wind. Quite
a few console drivers do this including some the big name x86 ones. We
don't seem to be getting any resulting problem reports.
Architecturally we really need a way to help console drivers separate the
civilised acceleration friendly, lock friendly output paths from printk.
That's the real fix, but a whole different matter to solve cleanly.
Alan