Re: [PATCH] checkpatch: Add warnings for use of mdelay()
On 2018/7/6 13:49, Julia Lawall wrote: On Thu, 5 Jul 2018, Dan Carpenter wrote: Neither Smatch nor Coccinelle do a good job tracking when you're in atomic context. I've wanted to add this to Smatch but even then it would be to warn that "We're holding a spinlock so we can't sleep". It's trickier to say for sure when you're not holding a lock... Jia-Ju Bai is working on this. The tool is available on github. It's still being improved, though, so perhaps it's not yet ready for eg 0-day inclusion. He can give more details. Thanks for Julia's recommendation :) I am doing the similar work with Julia, from the beginning of this year. We develop two new LLVM-based tools to find two problems in the Linux kernel: (1) Sleeping in atomic context. The tool is named DSAC. (2) Using non-sleep function calls in non-atomic context. The tool is named DCNS. We handle two common examples of atomic context: (1) Holding a spinlock. (2) In an interrupt handler. DSAC and DCNS can basically work now, and some of the defects found by them have been confirmed and fixed in the Linux kernel. But these tools are still being improved. In fact, I encounter a hard problem when writing the tools, namely how to accurately and completely handle function pointer calls. I have handled the function pointer in form of data structure field, but I do not find a good way to handle the function pointer that is used as a function argument. Can someone give me good advice? We also have made slides introducing DSAC and DCNS tools. If you are interested in our work, I can send you the slides :) Best wishes, Jia-Ju Bai
Re: [PATCH] checkpatch: Add warnings for use of mdelay()
On 2018/7/6 13:49, Julia Lawall wrote: On Thu, 5 Jul 2018, Dan Carpenter wrote: Neither Smatch nor Coccinelle do a good job tracking when you're in atomic context. I've wanted to add this to Smatch but even then it would be to warn that "We're holding a spinlock so we can't sleep". It's trickier to say for sure when you're not holding a lock... Jia-Ju Bai is working on this. The tool is available on github. It's still being improved, though, so perhaps it's not yet ready for eg 0-day inclusion. He can give more details. Thanks for Julia's recommendation :) I am doing the similar work with Julia, from the beginning of this year. We develop two new LLVM-based tools to find two problems in the Linux kernel: (1) Sleeping in atomic context. The tool is named DSAC. (2) Using non-sleep function calls in non-atomic context. The tool is named DCNS. We handle two common examples of atomic context: (1) Holding a spinlock. (2) In an interrupt handler. DSAC and DCNS can basically work now, and some of the defects found by them have been confirmed and fixed in the Linux kernel. But these tools are still being improved. In fact, I encounter a hard problem when writing the tools, namely how to accurately and completely handle function pointer calls. I have handled the function pointer in form of data structure field, but I do not find a good way to handle the function pointer that is used as a function argument. Can someone give me good advice? We also have made slides introducing DSAC and DCNS tools. If you are interested in our work, I can send you the slides :) Best wishes, Jia-Ju Bai
Re: [PATCH] checkpatch: Add warnings for use of mdelay()
On Thu, 5 Jul 2018, Dan Carpenter wrote: > Neither Smatch nor Coccinelle do a good job tracking when you're in > atomic context. I've wanted to add this to Smatch but even then it > would be to warn that "We're holding a spinlock so we can't sleep". > It's trickier to say for sure when you're not holding a lock... Jia-Ju Bai is working on this. The tool is available on github. It's still being improved, though, so perhaps it's not yet ready for eg 0-day inclusion. He can give more details. julia
Re: [PATCH] checkpatch: Add warnings for use of mdelay()
On Thu, 5 Jul 2018, Dan Carpenter wrote: > Neither Smatch nor Coccinelle do a good job tracking when you're in > atomic context. I've wanted to add this to Smatch but even then it > would be to warn that "We're holding a spinlock so we can't sleep". > It's trickier to say for sure when you're not holding a lock... Jia-Ju Bai is working on this. The tool is available on github. It's still being improved, though, so perhaps it's not yet ready for eg 0-day inclusion. He can give more details. julia
Re: [PATCH] checkpatch: Add warnings for use of mdelay()
Neither Smatch nor Coccinelle do a good job tracking when you're in atomic context. I've wanted to add this to Smatch but even then it would be to warn that "We're holding a spinlock so we can't sleep". It's trickier to say for sure when you're not holding a lock... regards, dan carpenter
Re: [PATCH] checkpatch: Add warnings for use of mdelay()
Neither Smatch nor Coccinelle do a good job tracking when you're in atomic context. I've wanted to add this to Smatch but even then it would be to warn that "We're holding a spinlock so we can't sleep". It's trickier to say for sure when you're not holding a lock... regards, dan carpenter
Re: [PATCH] checkpatch: Add warnings for use of mdelay()
On Wed, 2018-07-04 at 11:18 -0700, Prakruthi Deepak Heragu wrote:
> mdelay() is not a preferred API to be used to insert delay in the kernel
> code unless the context is atomic. Instead, msleep() API can be used.
> This patch introduces this warning.
[]
> Signed-off-by: Israel Schlesinger
> Signed-off-by: Stepan Moskovchenko
> Signed-off-by: Prakruthi Deepak Heragu
Really? 3 sign-offs for one trivial patch while
getting simple whitespace issues wrong?
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -5572,6 +5572,12 @@ sub process {
>"Comparing get_jiffies_64() is almost always
> wrong; prefer time_after64, time_before64 and friends\n" . $herecurr);
> }
>
> +# check the patch for use of mdelay
> + if ($line =~ /\bmdelay\s*\(/) {
> + WARN("MDELAY",
> + "use of mdelay() found: msleep() is the preferred
> API.\n" . $herecurr );
No space after $herecurr
> + }
> +
NACK - I think this is unreasonable.
checkpatch is stupid and can only remain that way.
It cannot check for any particular use in atomic
that is appropriate and should not warn with a
false positive when the use is appropriate.
$ git grep -w mdelay | wc -l
2700
How many of those are correct?
If you want a check like this to be useful, write
something for coccinelle or smatch.
Re: [PATCH] checkpatch: Add warnings for use of mdelay()
On Wed, 2018-07-04 at 11:18 -0700, Prakruthi Deepak Heragu wrote:
> mdelay() is not a preferred API to be used to insert delay in the kernel
> code unless the context is atomic. Instead, msleep() API can be used.
> This patch introduces this warning.
[]
> Signed-off-by: Israel Schlesinger
> Signed-off-by: Stepan Moskovchenko
> Signed-off-by: Prakruthi Deepak Heragu
Really? 3 sign-offs for one trivial patch while
getting simple whitespace issues wrong?
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -5572,6 +5572,12 @@ sub process {
>"Comparing get_jiffies_64() is almost always
> wrong; prefer time_after64, time_before64 and friends\n" . $herecurr);
> }
>
> +# check the patch for use of mdelay
> + if ($line =~ /\bmdelay\s*\(/) {
> + WARN("MDELAY",
> + "use of mdelay() found: msleep() is the preferred
> API.\n" . $herecurr );
No space after $herecurr
> + }
> +
NACK - I think this is unreasonable.
checkpatch is stupid and can only remain that way.
It cannot check for any particular use in atomic
that is appropriate and should not warn with a
false positive when the use is appropriate.
$ git grep -w mdelay | wc -l
2700
How many of those are correct?
If you want a check like this to be useful, write
something for coccinelle or smatch.
[PATCH] checkpatch: Add warnings for use of mdelay()
mdelay() is not a preferred API to be used to insert delay in the kernel
code unless the context is atomic. Instead, msleep() API can be used.
This patch introduces this warning.
Signed-off-by: Israel Schlesinger
Signed-off-by: Stepan Moskovchenko
Signed-off-by: Prakruthi Deepak Heragu
---
scripts/checkpatch.pl | 6 ++
1 file changed, 6 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a9c0550..14bba3f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5572,6 +5572,12 @@ sub process {
"Comparing get_jiffies_64() is almost always
wrong; prefer time_after64, time_before64 and friends\n" . $herecurr);
}
+# check the patch for use of mdelay
+ if ($line =~ /\bmdelay\s*\(/) {
+ WARN("MDELAY",
+"use of mdelay() found: msleep() is the preferred
API.\n" . $herecurr );
+ }
+
# warn about #ifdefs in C files
# if ($line =~ /^.\s*\#\s*if(|n)def/ && ($realfile =~ /\.c$/)) {
# print "#ifdef in C files should be avoided\n";
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH] checkpatch: Add warnings for use of mdelay()
mdelay() is not a preferred API to be used to insert delay in the kernel
code unless the context is atomic. Instead, msleep() API can be used.
This patch introduces this warning.
Signed-off-by: Israel Schlesinger
Signed-off-by: Stepan Moskovchenko
Signed-off-by: Prakruthi Deepak Heragu
---
scripts/checkpatch.pl | 6 ++
1 file changed, 6 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a9c0550..14bba3f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5572,6 +5572,12 @@ sub process {
"Comparing get_jiffies_64() is almost always
wrong; prefer time_after64, time_before64 and friends\n" . $herecurr);
}
+# check the patch for use of mdelay
+ if ($line =~ /\bmdelay\s*\(/) {
+ WARN("MDELAY",
+"use of mdelay() found: msleep() is the preferred
API.\n" . $herecurr );
+ }
+
# warn about #ifdefs in C files
# if ($line =~ /^.\s*\#\s*if(|n)def/ && ($realfile =~ /\.c$/)) {
# print "#ifdef in C files should be avoided\n";
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
