Re: [PATCH v2 0/2] get_maintainer: add patch-only keyword matching
On Fri, 2023-09-29 at 11:07 +0900, Justin Stitt wrote: > On Fri, Sep 29, 2023 at 12:52 AM Nick Desaulniers > wrote: > > > > On Wed, Sep 27, 2023 at 11:09 PM Joe Perches wrote: > > > > > > On Thu, 2023-09-28 at 14:31 +0900, Justin Stitt wrote: > > > > On Thu, Sep 28, 2023 at 2:01 PM Joe Perches wrote: > > > > > > > > > > On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > > > > > > Changes in v2: > > > > > > - remove formatting pass (thanks Joe) (but seriously the formatting > > > > > > is > > > > > > bad, is there opportunity to get a formatting pass in here at some > > > > > > point?) > > > > > LG G7 Battery Replacement | Guide | Is it actually a Samsung? I t > > > > > Why? What is it that makes you believe the formatting is bad? > > > > > > > > > > > > > Investigating further, it looked especially bad in my editor. There is > > > > a mixture of > > > > tabs and spaces and my vim tabstop is set to 4 for pl files. Setting > > > > this to > > > > 8 is a whole lot better. But I still see some weird spacing > > > > > > > > > > Yes, it's a bit odd indentation. > > > It's emacs default perl format. > > > 4 space indent with 8 space tabs, maximal tab fill. > > > > > > > Oh! What?! That's the most surprising convention I've ever heard of > > (after the GNU C coding style). Yet another thing to hold against > > perl I guess. :P > > > > I have my editor setup to highlight tabs vs spaces via visual cues, so > > that I don't mess up kernel coding style. (`git clang-format HEAD~` > > after a commit helps). scripts/get_maintainer.pl has some serious > > inconsistencies to the point where I'm not sure what it should or was > > meant to be. Now that you mention it, I see it, and it does seem > > consistent in that regard. > > > > Justin, is your formatter configurable to match that convention? > > Maybe it's still useful, as long as you configure it to stick to the > > pre-existing convention. > > Negative, all the perl formatters I've tried will convert everything to > spaces. > The best I've seen is perltidy. > > https://gist.github.com/JustinStitt/347385921c80a5212c2672075aa769b6 emacs with cperl mode works fine. I don't know much about vim, but when I open this file in vim it looks perfectly normal and it's apparently properly syntax highlighted.
Re: [PATCH v2 0/2] get_maintainer: add patch-only keyword matching
On Thu, 2023-09-28 at 14:31 +0900, Justin Stitt wrote: > On Thu, Sep 28, 2023 at 2:01 PM Joe Perches wrote: > > > > On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > > > Changes in v2: > > > - remove formatting pass (thanks Joe) (but seriously the formatting is > > > bad, is there opportunity to get a formatting pass in here at some > > > point?) > > > > Why? What is it that makes you believe the formatting is bad? > > > > Investigating further, it looked especially bad in my editor. There is > a mixture of > tabs and spaces and my vim tabstop is set to 4 for pl files. Setting this to > 8 is a whole lot better. But I still see some weird spacing > Yes, it's a bit odd indentation. It's emacs default perl format. 4 space indent with 8 space tabs, maximal tab fill.
Re: [PATCH v2 1/2] get_maintainer: add patch-only keyword-matching
On Thu, 2023-09-28 at 14:03 +0900, Justin Stitt wrote: > On Thu, Sep 28, 2023 at 1:46 PM Joe Perches wrote: > > > > On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > > > Add the "D:" type which behaves the same as "K:" but will only match > > > content present in a patch file. [] > > > My opinion: Nack. > > > > I think something like this would be better > > as it avoids duplication of K and D content. > > If I understand correctly, this puts the onus on the get_maintainer users > to select the right argument whereas adding "D:", albeit with some > duplicate code, allows maintainers themselves to decide in exactly > which context they receive mail. Maybe, but I doubt it'll be significantly different. > This could all be a moot point, though, as I believe Konstantin > is trying to separate out the whole idea of a patch-sender needing > to specify the recipients of a patch. As I understand it, by using get_maintainer.
Re: [PATCH v2 0/2] get_maintainer: add patch-only keyword matching
On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > Changes in v2: > - remove formatting pass (thanks Joe) (but seriously the formatting is > bad, is there opportunity to get a formatting pass in here at some > point?) Why? What is it that makes you believe the formatting is bad?
Re: [PATCH v2 2/2] MAINTAINERS: migrate some K to D
On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote: > Let's get the ball rolling with some changes from K to D. > > Ultimately, if it turns out that 100% of K users want to change to D > then really the behavior of K could just be changed. Given my suggestion to 1/2, this would be unnecessary. > > Signed-off-by: Justin Stitt > Original-author: Kees Cook > --- > MAINTAINERS | 16 +--- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 94e431daa7c2..80ffdaa8f044 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5038,7 +5038,7 @@ F: Documentation/kbuild/llvm.rst > F: include/linux/compiler-clang.h > F: scripts/Makefile.clang > F: scripts/clang-tools/ > -K: \b(?i:clang|llvm)\b > +D: \b(?i:clang|llvm)\b > > CLK API > M: Russell King > @@ -8149,7 +8149,7 @@ F: lib/strcat_kunit.c > F: lib/strscpy_kunit.c > F: lib/test_fortify/* > F: scripts/test_fortify.sh > -K: \b__NO_FORTIFY\b > +D: \b__NO_FORTIFY\b > > FPGA DFL DRIVERS > M: Wu Hao > @@ -11405,8 +11405,10 @@ F: > Documentation/ABI/testing/sysfs-kernel-warn_count > F: include/linux/overflow.h > F: include/linux/randomize_kstack.h > F: mm/usercopy.c > -K: \b(add|choose)_random_kstack_offset\b > -K: \b__check_(object_size|heap_object)\b > +D: \b(add|choose)_random_kstack_offset\b > +D: \b__check_(object_size|heap_object)\b > +D: \b__counted_by\b > + > > KERNEL JANITORS > L: kernel-janit...@vger.kernel.org > @@ -17290,7 +17292,7 @@ F:drivers/acpi/apei/erst.c > F: drivers/firmware/efi/efi-pstore.c > F: fs/pstore/ > F: include/linux/pstore* > -K: \b(pstore|ramoops) > +D: \b(pstore|ramoops) > > PTP HARDWARE CLOCK SUPPORT > M: Richard Cochran > @@ -19231,8 +19233,8 @@ F:include/uapi/linux/seccomp.h > F: kernel/seccomp.c > F: tools/testing/selftests/kselftest_harness.h > F: tools/testing/selftests/seccomp/* > -K: \bsecure_computing > -K: \bTIF_SECCOMP\b > +D: \bsecure_computing > +D: \bTIF_SECCOMP\b > > SECURE DIGITAL HOST CONTROLLER INTERFACE (SDHCI) Broadcom BRCMSTB DRIVER > M: Kamal Dasu >
Re: [PATCH v2 1/2] get_maintainer: add patch-only keyword-matching
On Thu, 2023-09-28 at 04:23 +, Justin Stitt wrote:
> Add the "D:" type which behaves the same as "K:" but will only match
> content present in a patch file.
>
> To illustrate:
>
> Imagine this entry in MAINTAINERS:
>
> NEW REPUBLIC
> M: Han Solo
> W: https://www.jointheresistance.org
> D: \bstrncpy\b
>
> Our maintainer, Han, will only be added to the recipients if a patch
> file is passed to get_maintainer (like what b4 does):
> $ ./scripts/get_maintainer.pl 0004-some-change.patch
>
> If the above patch has a `strncpy` present in the subject, commit log or
> diff then Han will be to/cc'd.
>
> However, in the event of a file from the tree given like:
> $ ./scripts/get_maintainer.pl ./lib/string.c
>
> Han will not be noisily to/cc'd (like a K: type would in this
> circumstance)
>
> Signed-off-by: Justin Stitt
> ---
> MAINTAINERS | 5 +
> scripts/get_maintainer.pl | 12 ++--
> 2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b19995690904..94e431daa7c2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -59,6 +59,11 @@ Descriptions of section entries and preferred order
> matches patches or files that contain one or more of the words
> printk, pr_info or pr_err
> One regex pattern per line. Multiple K: lines acceptable.
> + D: *Diff content regex* (perl extended) pattern match that applies only to
> + patches and not entire files (e.g. when using the get_maintainers.pl
> + script).
> + Usage same as K:.
> +
>
> Maintainers List
>
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index ab123b498fd9..a3e697926ddd 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -342,6 +342,7 @@ if ($tree && !top_of_kernel_tree($lk_path)) {
>
> my @typevalue = ();
> my %keyword_hash;
> +my %patch_keyword_hash;
> my @mfiles = ();
> my @self_test_info = ();
>
> @@ -369,8 +370,10 @@ sub read_maintainer_file {
> $value =~ s@([^/])$@$1/@;
> }
> } elsif ($type eq "K") {
> - $keyword_hash{@typevalue} = $value;
> - }
> + $keyword_hash{@typevalue} = $value;
> + } elsif ($type eq "D") {
> + $patch_keyword_hash{@typevalue} = $value;
> + }
> push(@typevalue, "$type:$value");
> } elsif (!(/^\s*$/ || /^\s*\#/)) {
> push(@typevalue, $line);
> @@ -607,6 +610,11 @@ foreach my $file (@ARGV) {
> push(@keyword_tvi, $line);
> }
> }
> +foreach my $line(keys %patch_keyword_hash) {
> + if ($patch_line =~ m/${patch_prefix}$patch_keyword_hash{$line}/x) {
> +push(@keyword_tvi, $line);
> + }
> +}
> }
> }
> close($patch);
>
My opinion: Nack.
I think something like this would be better
as it avoids duplication of K and D content.
---
scripts/get_maintainer.pl | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index ab123b498fd9..07e7d744cadb 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -57,6 +57,7 @@ my $subsystem = 0;
my $status = 0;
my $letters = "";
my $keywords = 1;
+my $keywords_in_file = 0;
my $sections = 0;
my $email_file_emails = 0;
my $from_filename = 0;
@@ -272,6 +273,7 @@ if (!GetOptions(
'letters=s' => \$letters,
'pattern-depth=i' => \$pattern_depth,
'k|keywords!' => \$keywords,
+ 'kf|keywords-in-file!' => \$keywords_in_file,
'sections!' => \$sections,
'fe|file-emails!' => \$email_file_emails,
'f|file' => \$from_filename,
@@ -318,6 +320,7 @@ if ($sections || $letters ne "") {
$subsystem = 0;
$web = 0;
$keywords = 0;
+$keywords_in_file = 0;
$interactive = 0;
} else {
my $selections = $email + $scm + $status + $subsystem + $web;
@@ -548,16 +551,14 @@ foreach my $file (@ARGV) {
$file =~ s/^\Q${cur_path}\E//; #strip any absolute path
$file =~ s/^\Q${lk_path}\E//; #or the path to the lk tree
push(@files, $file);
- if ($file ne "MAINTAINERS" && -f $file && $keywords) {
+ if ($file ne "MAINTAINERS" && -f $file && $keywords &&
$keywords_in_file) {
open(my $f, '<', $file)
or die "$P: Can't open $file: $!\n";
my $text = do { local($/) ; <$f> };
close($f);
- if ($keywords) {
- foreach my $line (keys %keyword_hash) {
- if ($text =~ m/$keyword_hash{$line}/x) {
- push(@keyword_tvi, $line);
- }
+ foreach my $line (keys %keyword_hash) {
+ if ($text =~ m/$keyword_hash{$line}/x) {
+ push(@keyword_tvi, $line);
}
}
}
@@
Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type
On Wed, 2023-09-27 at 09:15 -0700, Kees Cook wrote: > On Wed, Sep 27, 2023 at 03:19:16AM +, Justin Stitt wrote: > > Add the "D:" type which behaves the same as "K:" but will only match > > content present in a patch file. > > > > To illustrate: > > > > Imagine this entry in MAINTAINERS: > > > > NEW REPUBLIC > > M: Han Solo > > W: https://www.jointheresistance.org > > D: \bstrncpy\b > > > > Our maintainer, Han, will only be added to the recipients if a patch > > file is passed to get_maintainer (like what b4 does): > > $ ./scripts/get_maintainer.pl 0004-some-change.patch > > > > If the above patch has a `strncpy` present in the subject, commit log or > > diff then Han will be to/cc'd. > > > > However, in the event of a file from the tree given like: > > $ ./scripts/get_maintainer.pl ./lib/string.c > > > > Han will not be noisily to/cc'd (like a K: type would in this > > circumstance) > > > > Note that folks really shouldn't be using get_maintainer on tree files > > anyways [1]. > > > > [1]: https://lore.kernel.org/all/20230726151515.1650519-1-k...@kernel.org/ > > As Greg suggested, please drop the above paragraph and link. Then this > looks good to me. > > I would immediately want to send this patch too, so please feel free to > add this to your series (and I bet many other hints on "git grep 'K:.\\b'" > would want to switch from K: to D: too): > > diff --git a/MAINTAINERS b/MAINTAINERS [] > @@ -5057,7 +5057,7 @@ F: Documentation/kbuild/llvm.rst > F: include/linux/compiler-clang.h > F: scripts/Makefile.clang > F: scripts/clang-tools/ > -K: \b(?i:clang|llvm)\b > +D: \b(?i:clang|llvm)\b etc... My assumption is that the K: --file use is just unnecessary and it'd be better to only use the K: lookup on patches. (and I've somehow stuffed up the receiving side of my email configuration so please ignore any emails to me that bounce for a while)
Re: [PATCH 3/3] get_maintainer: add patch-only pattern matching type
On Wed, 2023-09-27 at 03:19 +, Justin Stitt wrote: > Add the "D:" type which behaves the same as "K:" but will only match > content present in a patch file. Likely it'd be less aggravating just to document that K: is only for patches and add a !$file test.
Re: [PATCH 2/3] get_maintainer: run perltidy
On Wed, 2023-09-27 at 03:19 +, Justin Stitt wrote: > I'm a first time contributor to get_maintainer.pl and the formatting is > suspicious. I am not sure if there is a particular reason it is the way > it is but I let my editor format it and submitted the diff here in this > patch. Capital NACK. Completely unnecessary and adds no value.
Re: [PATCH 1/3] MAINTAINERS: add documentation for D:
On Wed, 2023-09-27 at 03:19 +, Justin Stitt wrote: > Document what "D:" does. > > This is more or less the same as what "K:" does but only works for patch > files. Nack. I'd rather just add a !$file test to K: patterns.
Re: [PATCH][next] checkpatch: add a couple new alloc functions to alloc with multiplies check
On Tue, 2023-09-12 at 11:04 -0600, Gustavo A. R. Silva wrote:
> vmalloc() and vzalloc() functions have now 2-factor multiplication
> argument forms vmalloc_array() and vcalloc(), correspondingly.
> Add alloc-with-multiplies checks for these new functions.
>
> Link: https://github.com/KSPP/linux/issues/342
> Signed-off-by: Gustavo A. R. Silva
> ---
> scripts/checkpatch.pl | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7d16f863edf1..45265d0eee1b 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -7207,17 +7207,19 @@ sub process {
> "Prefer $3(sizeof(*$1)...) over $3($4...)\n" .
> $herecurr);
> }
>
> -# check for (kv|k)[mz]alloc with multiplies that could be
> kmalloc_array/kvmalloc_array/kvcalloc/kcalloc
> +# check for (kv|k|v)[mz]alloc with multiplies that could be
> kmalloc_array/kvmalloc_array/vmalloc_array/kvcalloc/kcalloc/vcalloc
> if ($perl_version_ok &&
> defined $stat &&
> - $stat =~
> /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
> {
> + $stat =~
> /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k|v)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,?/)
> {
> my $oldfunc = $3;
> my $a1 = $4;
> my $a2 = $10;
> my $newfunc = "kmalloc_array";
> $newfunc = "kvmalloc_array" if ($oldfunc eq "kvmalloc");
> + $newfunc = "vmalloc_array" if ($oldfunc eq "vmalloc");
> $newfunc = "kvcalloc" if ($oldfunc eq "kvzalloc");
> $newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
> + $newfunc = "vcalloc" if ($oldfunc eq "vzalloc");
> my $r1 = $a1;
> my $r2 = $a2;
> if ($a1 =~ /^sizeof\s*\S/) {
> @@ -7233,7 +7235,7 @@ sub process {
>"Prefer $newfunc over $oldfunc with
> multiply\n" . $herectx) &&
> $cnt == 1 &&
> $fix) {
> - $fixed[$fixlinenr] =~
> s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1
> . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
> + $fixed[$fixlinenr] =~
> s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k|v)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1
> . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
> }
> }
> }
Seems ok. Dunno how many more of these there might be like
devm_ variants, but maybe it'd be better style to use a hash
with replacement instead of the repeated if block
Something like:
---
scripts/checkpatch.pl | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7d16f863edf1c..bacb63518be14 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -854,6 +854,23 @@ foreach my $entry (keys %deprecated_apis) {
}
$deprecated_apis_search = "(?:${deprecated_apis_search})";
+our %alloc_with_multiply_apis = (
+ "kmalloc" => "kmalloc_array",
+ "kvmalloc" => "kvmalloc_array",
+ "vmalloc" => "vmalloc_array",
+ "kvzalloc" => "kvcalloc",
+ "kzalloc" => "kcalloc",
+ "vzalloc" => "vcalloc",
+);
+
+#Create a search pattern for all these strings to speed up a loop below
+our $alloc_with_multiply_search = "";
+foreach my $entry (keys %alloc_with_multiply_apis) {
+ $alloc_with_multiply_search .= '|' if ($alloc_with_multiply_search ne
"");
+ $alloc_with_multiply_search .= $entry;
+}
+$alloc_with_multiply_search = "(?:${alloc_with_multiply_search})";
+
our $mode_perms_world_writable = qr{
S_IWUGO |
S_IWOTH |
@@ -7210,14 +7227,11 @@ sub process {
# check for (kv|k)[mz]alloc with multiplies that could be
kmalloc_array/kvmalloc_array/kvcalloc/kcalloc
if ($perl_version_ok &&
defined $stat &&
- $stat =~
/^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
{
+ $stat =~
/^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*($alloc_with_multiply_search)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
{
my $oldfunc = $3;
+ my $newfunc = $alloc_with_multiply_apis{$oldfunc};
my $a1 = $4;
my $a2 = $10;
- my $newfunc = "kmalloc_array";
-
Re: [PATCH 1/1] video: hyperv_fb: Add ratelimit on error message
On Tue, 2021-04-20 at 08:44 -0700, Michael Kelley wrote:
> Due to a full ring buffer, the driver may be unable to send updates to
> the Hyper-V host. But outputing the error message can make the problem
> worse because console output is also typically written to the frame
> buffer. As a result, in some circumstances the error message is output
> continuously.
>
> Break the cycle by rate limiting the error message. Also output
> the error code for additional diagnosability.
>
> Signed-off-by: Michael Kelley
None of the callers of this function ever check the return status.
Why is important/useful to emit this message at all?
> ---
> drivers/video/fbdev/hyperv_fb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index 4dc9077..a7e6eea 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -308,7 +308,7 @@ static inline int synthvid_send(struct hv_device *hdev,
> VM_PKT_DATA_INBAND, 0);
>
>
> if (ret)
> - pr_err("Unable to send packet via vmbus\n");
> + pr_err_ratelimited("Unable to send packet via vmbus; error
> %d\n", ret);
>
>
> return ret;
> }
Re: [PATCH v2] iommu/amd: Fix extended features logging
On Mon, 2021-04-19 at 22:23 +0300, Alexander Monakov wrote:
> On Sun, 11 Apr 2021, Joe Perches wrote:
>
> > > v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer
> > > solution
> > >
> > > drivers/iommu/amd/init.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> > > index 596d0c413473..62913f82a21f 100644
> > > --- a/drivers/iommu/amd/init.c
> > > +++ b/drivers/iommu/amd/init.c
> > > @@ -1929,8 +1929,8 @@ static void print_iommu_info(void)
> > > pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
> > >
> > >
> > > if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
> > > - pci_info(pdev, "Extended features (%#llx):",
> > > - iommu->features);
> > > + pr_info("Extended features (%#llx):", iommu->features);
> > > +
> > > for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
> > > if (iommu_feature(iommu, (1ULL << i)))
> > > pr_cont(" %s", feat_str[i]);
> >
> > How about avoiding all of this by using a temporary buffer
> > and a single pci_info.
>
> I think it is mostly up to the maintainers, but from my perspective, it's not
> good to conflate such a simple bugfix with the substantial rewrite you are
> proposing (which also increases code complexity).
You and I have _significant_ differences in the definition of substantial.
Buffering the output is the preferred code style in preference to
pr_cont.
Do remember pr_cont should _only_ be used when absolutely necessary
as interleaving of other messages from other processes/threads can
and does occur.
Re: [PATCH v5] printk: Userspace format enumeration support
On Mon, 2021-04-19 at 11:53 +0200, Greg Kroah-Hartman wrote: > Hm, 12734 of the pr_err() calls do live in drivers/, so most of those > should be dev_err(). Might be something good to throw at interns... That depends on how much churn you want to have in old drivers that generally don't have any users because the hardware is ancient or no longer manufactured. I suggest not changing those. But I believe a coccinelle script was written quite awhile ago to convert pr_ to dev_ when a struct device * is available. http://btrlinux.inria.fr/staging-media-replace-pr_-with-dev_/
Re: [PATCH] base: power: runtime.c: Remove a unnecessary space
On Sun, 2021-04-18 at 09:11 +, Sebastian Fricke wrote: > Hey Joe, Hi Sebastian. > On 18.04.2021 00:09, Joe Perches wrote: > > On Sun, 2021-04-18 at 06:08 +, Sebastian Fricke wrote: > > > Remove a redundant space to improve the quality of the comment. > > I think this patch is not useful. > > It's not redundant. > > Thank you, I actually found this pattern a few more times but I wanted > to check first if this is a mistake or chosen consciously. [] > > For drivers/base/power/runtime.c, that 2 space after period style is used > > dozens of times and changing a single instance of it isn't very useful. Even in that single file it's not consistent. It's something like 3:1 for 2 spaces over 1 space after period. I believe it's done more by habit and author age than anything. If you learned to type using a typewriter and not a keyboard, then you likely still use 2 spaces after a period. > True and if I understand you correctly you would rather keep it as is > right? Yes. IMO: Whitespace in comments like this should not be changed unless there's some other significant benefit like better alignment. cheers, Joe > > > --- > > > Side-note: > > > I found this while reading the code, I don't believe it is important but > > > I thought it doesn't hurt to fix it. [] > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c [] > > > @@ -786,7 +786,7 @@ static int rpm_resume(struct device *dev, int > > > rpmflags) > > > } > > > > > > /* > > > - * See if we can skip waking up the parent. This is safe only if > > > + * See if we can skip waking up the parent. This is safe only if > > > * power.no_callbacks is set, because otherwise we don't know whether > > > * the resume will actually succeed. > > > */
[PATCH] net/wireless/bcom: constify ieee80211_get_response_rate return
It's not modified so make it const with the eventual goal of moving
data to text for various static struct ieee80211_rate arrays.
Signed-off-by: Joe Perches
---
drivers/net/wireless/broadcom/b43/main.c | 2 +-
drivers/net/wireless/broadcom/b43legacy/main.c | 2 +-
include/net/cfg80211.h | 2 +-
net/wireless/util.c| 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/broadcom/b43/main.c
b/drivers/net/wireless/broadcom/b43/main.c
index 150a366e8f62..17bcec5f3ff7 100644
--- a/drivers/net/wireless/broadcom/b43/main.c
+++ b/drivers/net/wireless/broadcom/b43/main.c
@@ -4053,7 +4053,7 @@ static void b43_update_basic_rates(struct b43_wldev *dev,
u32 brates)
{
struct ieee80211_supported_band *sband =
dev->wl->hw->wiphy->bands[b43_current_band(dev->wl)];
- struct ieee80211_rate *rate;
+ const struct ieee80211_rate *rate;
int i;
u16 basic, direct, offset, basic_offset, rateptr;
diff --git a/drivers/net/wireless/broadcom/b43legacy/main.c
b/drivers/net/wireless/broadcom/b43legacy/main.c
index 7692a2618c97..f64ebff68308 100644
--- a/drivers/net/wireless/broadcom/b43legacy/main.c
+++ b/drivers/net/wireless/broadcom/b43legacy/main.c
@@ -2762,7 +2762,7 @@ static void b43legacy_update_basic_rates(struct
b43legacy_wldev *dev, u32 brates
{
struct ieee80211_supported_band *sband =
dev->wl->hw->wiphy->bands[NL80211_BAND_2GHZ];
- struct ieee80211_rate *rate;
+ const struct ieee80211_rate *rate;
int i;
u16 basic, direct, offset, basic_offset, rateptr;
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 911fae42b0c0..ed07590bc72d 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5606,7 +5606,7 @@ static inline bool cfg80211_channel_is_psc(struct
ieee80211_channel *chan)
* which is, for this function, given as a bitmap of indices of
* rates in the band's bitrate table.
*/
-struct ieee80211_rate *
+const struct ieee80211_rate *
ieee80211_get_response_rate(struct ieee80211_supported_band *sband,
u32 basic_rates, int bitrate);
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 1bf0200f562a..382c5262d997 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -24,7 +24,7 @@
#include "rdev-ops.h"
-struct ieee80211_rate *
+const struct ieee80211_rate *
ieee80211_get_response_rate(struct ieee80211_supported_band *sband,
u32 basic_rates, int bitrate)
{
Re: [PATCH] brcmsmac: fix shift on 4 bit masked value
On Sun, 2021-04-18 at 06:10 +, Kalle Valo wrote:
> Colin King wrote:
>
> > From: Colin Ian King
> >
> > The calculation of offtune_val seems incorrect, the u16 value in
> > pi->tx_rx_cal_radio_saveregs[2] is being masked with 0xf0 and then
> > shifted 8 places right so that always ends up as a zero result. I
> > believe the intended shift was 4 bits to the right. Fix this.
> >
> > [Note: not tested, I don't have the H/W]
> >
> > Addresses-Coverity: ("Operands don't affect result")
> > Fixes: 5b435de0d786 ("net: wireless: add brcm80211 drivers")
> > Signed-off-by: Colin Ian King
>
> I think this needs review from someone familiar with the hardware.
>
> Patch set to Changes Requested.
What "change" are you requesting here?
Likely there needs to be some other setting for the patch.
Perhaps "deferred" as you seem to be requesting a review
and there's no actual change necessary, just approval from
someone with the hardware and that someone test the patch.
Re: [PATCH] base: power: runtime.c: Remove a unnecessary space
On Sun, 2021-04-18 at 06:08 +, Sebastian Fricke wrote: > Remove a redundant space to improve the quality of the comment. I think this patch is not useful. It's not redundant. Two spaces after a period is commonly used to separate sentences. It's especially common when used with fixed pitch fonts. A trivial grep seems to show it's used about 50K times in comments. Though single space after period may be used about twice as often. $ git grep '^\s*\*.*\. [A-Z]' | wc -l 54439 $ git grep '^\s*\*.*\. [A-Z]' | wc -l 110003 For drivers/base/power/runtime.c, that 2 space after period style is used dozens of times and changing a single instance of it isn't very useful. > --- > Side-note: > I found this while reading the code, I don't believe it is important but > I thought it doesn't hurt to fix it. > --- > drivers/base/power/runtime.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 18b82427d0cb..499434b84171 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -786,7 +786,7 @@ static int rpm_resume(struct device *dev, int rpmflags) > } > > > /* > - * See if we can skip waking up the parent. This is safe only if > + * See if we can skip waking up the parent. This is safe only if > * power.no_callbacks is set, because otherwise we don't know whether > * the resume will actually succeed. > */
Re: [PATCH RESEND][next] rtl8xxxu: Fix fall-through warnings for Clang
On Sat, 2021-04-17 at 14:30 -0400, Jes Sorensen wrote: > On 4/17/21 1:52 PM, Kalle Valo wrote: > > "Gustavo A. R. Silva" wrote: > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, fix > > > multiple warnings by replacing /* fall through */ comments with > > > the new pseudo-keyword macro fallthrough; instead of letting the > > > code fall through to the next case. > > > > > > Notice that Clang doesn't recognize /* fall through */ comments as > > > implicit fall-through markings. > > > > > > Link: https://github.com/KSPP/linux/issues/115 > > > Signed-off-by: Gustavo A. R. Silva > > > > Patch applied to wireless-drivers-next.git, thanks. > > > > bf3365a856a1 rtl8xxxu: Fix fall-through warnings for Clang > > > > Sorry this junk patch should not have been applied. I don't believe it's a junk patch. I believe your characterization of it as such is flawed. You don't like the style, that's fine, but: Any code in the kernel should not be a unique style of your own choosing when it could cause various compilers to emit unnecessary warnings. Please remember the kernel code base is a formed by a community with a nominally generally accepted style. There is a real desire in that community to both enable compiler warnings that might show defects and simultaneously avoid unnecessary compiler warnings. This particular change just avoids a possible compiler warning.
Re: [PATCH 1/5] scsi: BusLogic: Fix missing `pr_cont' use
On Fri, 2021-04-16 at 14:41 -0600, Khalid Aziz wrote: > On 4/15/21 8:08 PM, Joe Perches wrote: > > And while it's a lot more code, I'd prefer a solution that looks more > > like the other commonly used kernel logging extension mechanisms > > where adapter is placed before the format, ... in the argument list. > > Hi Joe, > > I don't mind making these changes. It is quite a bit of code but > consistency with other kernel code is useful. Would you like to finalize > this patch, or would you prefer that I take this patch as starting point > and finalize it? Probably better to apply Maciej's patches first and then something like this. btw: the coccinelle script was @@ expression a, b; @@ \(blogic_announce\|blogic_info\|blogic_notice\|blogic_warn\|blogic_err\)( - a, b + b, a , ...)
Re: [PATCH v2] checkpatch: Improve ALLOC_ARRAY_ARGS test
On Fri, 2021-04-16 at 19:57 +0200, Christophe JAILLET wrote:
> The devm_ variant of 'kcalloc()' and 'kmalloc_array()' are not tested
> Add the corresponding check.
>
> Signed-off-by: Christophe JAILLET
> ---
> v2: use a cleaner regex as proposed by Joe Perches
Acked-by: Joe Perches
> ---
> scripts/checkpatch.pl | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 44b9dc330ac6..23697a6b1eaa 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -7006,7 +7006,7 @@ sub process {
> }
>
>
> # check for alloc argument mismatch
> - if ($line =~ /\b(kcalloc|kmalloc_array)\s*\(\s*sizeof\b/) {
> + if ($line =~
> /\b((?:devm_)?(?:kcalloc|kmalloc_array))\s*\(\s*sizeof\b/) {
> WARN("ALLOC_ARRAY_ARGS",
> "$1 uses number as first arg, sizeof is generally
> wrong\n" . $herecurr);
> }
Re: [PATCH] checkpatch: Improve ALLOC_ARRAY_ARGS test
On Fri, 2021-04-16 at 18:51 +0200, Christophe JAILLET wrote:
> Le 16/04/2021 à 18:11, Joe Perches a écrit :
> > On Fri, 2021-04-16 at 17:58 +0200, Christophe JAILLET wrote:
> > > The devm_ variant of 'kcalloc()' and 'kmalloc_array()' are not tested
> > > Add the corresponding check.
> > []
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > > @@ -7006,9 +7006,9 @@ sub process {
> > > }
> > >
> > >
> > >
> > > # check for alloc argument mismatch
> > > - if ($line =~ /\b(kcalloc|kmalloc_array)\s*\(\s*sizeof\b/) {
> > > + if ($line =~
> > > /\b(devm_|)(kcalloc|kmalloc_array)\s*\(\s*sizeof\b/) {
> >
> > Perhaps nicer using
> I'll send a V2.
>
> Thx for the feedback.
>
> CJ
>
> >
> > if ($line =~
> > /\b((?:devm_)?(?:kcalloc|kmalloc_array))\s*\*\s*sizeof\b/) {
The \* above should be \(.
I can't type and apparently I don't proofread either.
I offer the excuse that the * and ( are adjacent on my keyboard...
cheers, Joe
Re: [PATCH] checkpatch: Improve ALLOC_ARRAY_ARGS test
On Fri, 2021-04-16 at 17:58 +0200, Christophe JAILLET wrote:
> The devm_ variant of 'kcalloc()' and 'kmalloc_array()' are not tested
> Add the corresponding check.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7006,9 +7006,9 @@ sub process {
> }
>
>
> # check for alloc argument mismatch
> - if ($line =~ /\b(kcalloc|kmalloc_array)\s*\(\s*sizeof\b/) {
> + if ($line =~
> /\b(devm_|)(kcalloc|kmalloc_array)\s*\(\s*sizeof\b/) {
Perhaps nicer using
if ($line =~
/\b((?:devm_)?(?:kcalloc|kmalloc_array))\s*\*\s*sizeof\b/) {
> WARN("ALLOC_ARRAY_ARGS",
> - "$1 uses number as first arg, sizeof is generally
> wrong\n" . $herecurr);
> + "$1$2 uses number as first arg, sizeof is
> generally wrong\n" . $herecurr);
So there's only one capture group and this line doesn't need to be changed.
Re: [PATCH 1/5] scsi: BusLogic: Fix missing `pr_cont' use
On Fri, 2021-04-16 at 16:28 +0200, Maciej W. Rozycki wrote: > On Fri, 16 Apr 2021, Joe Perches wrote: > > > > I'm not sure if that complex message > > > routing via `blogic_msg' is worth having even, rather than calling > > > `printk' or suitable variants directly. > > > > It's to allow the message content to be added to the internal > > >msgbuf[adapter->msgbuflen] > > with strcpy for later use with blogic_show_info()/seq_write. > > I know, but it's not clear to me if it's worth it (a potential buffer > overrun there too, BTW). It's seq_ output so it's nominally an ABI. But then again, I don't use this at all so I don't care much either. It's also odd/bad form that one output KERN_ does not match its blogic_ (blogic_info is emitted at KERN_NOTICE)
Re: [PATCH v5] printk: Userspace format enumeration support
On Fri, 2021-04-16 at 14:56 +0100, Chris Down wrote: > Any better suggestions? :-) A gcc plugin that looks for functions marked __printf(fmt, pos) so any const fmt is stored.
Re: [PATCH 1/5] scsi: BusLogic: Fix missing `pr_cont' use
On Fri, 2021-04-16 at 12:48 +0200, Maciej W. Rozycki wrote: > I'm not sure if that complex message > routing via `blogic_msg' is worth having even, rather than calling > `printk' or suitable variants directly. It's to allow the message content to be added to the internal >msgbuf[adapter->msgbuflen] with strcpy for later use with blogic_show_info()/seq_write.
Re: [PATCH 1/5] scsi: BusLogic: Fix missing `pr_cont' use
On Thu, 2021-04-15 at 00:39 +0200, Maciej W. Rozycki wrote:
> Update BusLogic driver's messaging system to use `pr_cont' for
> continuation lines, bringing messy output:
>
> pci :00:13.0: PCI->APIC IRQ transform: INT A -> IRQ 17
> scsi: * BusLogic SCSI Driver Version 2.1.17 of 12 September 2013 *
> scsi: Copyright 1995-1998 by Leonard N. Zubkoff
> scsi0: Configuring BusLogic Model BT-958 PCI Wide Ultra SCSI Host Adapter
> scsi0: Firmware Version: 5.07B, I/O Address: 0x7000, IRQ Channel: 17/Level
> scsi0: PCI Bus: 0, Device: 19, Address:
> 0xE0012000,
> Host Adapter SCSI ID: 7
> scsi0: Parity Checking: Enabled, Extended Translation: Enabled
> scsi0: Synchronous Negotiation: Ultra, Wide Negotiation: Enabled
> scsi0: Disconnect/Reconnect: Enabled, Tagged Queuing: Enabled
> scsi0: Scatter/Gather Limit: 128 of 8192 segments, Mailboxes: 211
> scsi0: Driver Queue Depth: 211, Host Adapter Queue Depth: 192
> scsi0: Tagged Queue Depth:
> Automatic
> , Untagged Queue Depth: 3
> scsi0: SCSI Bus Termination: Both Enabled
> , SCAM: Disabled
>
> scsi0: *** BusLogic BT-958 Initialized Successfully ***
> scsi host0: BusLogic BT-958
>
> back to order:
>
> pci :00:13.0: PCI->APIC IRQ transform: INT A -> IRQ 17
> scsi: * BusLogic SCSI Driver Version 2.1.17 of 12 September 2013 *
> scsi: Copyright 1995-1998 by Leonard N. Zubkoff
> scsi0: Configuring BusLogic Model BT-958 PCI Wide Ultra SCSI Host Adapter
> scsi0: Firmware Version: 5.07B, I/O Address: 0x7000, IRQ Channel: 17/Level
> scsi0: PCI Bus: 0, Device: 19, Address: 0xE0012000, Host Adapter SCSI ID: 7
> scsi0: Parity Checking: Enabled, Extended Translation: Enabled
> scsi0: Synchronous Negotiation: Ultra, Wide Negotiation: Enabled
> scsi0: Disconnect/Reconnect: Enabled, Tagged Queuing: Enabled
> scsi0: Scatter/Gather Limit: 128 of 8192 segments, Mailboxes: 211
> scsi0: Driver Queue Depth: 211, Host Adapter Queue Depth: 192
> scsi0: Tagged Queue Depth: Automatic, Untagged Queue Depth: 3
> scsi0: SCSI Bus Termination: Both Enabled, SCAM: Disabled
> scsi0: *** BusLogic BT-958 Initialized Successfully ***
> scsi host0: BusLogic BT-958
>
> Also diagnostic output such as with the `BusLogic=TraceConfiguration'
> parameter is affected and becomes vertical and therefore hard to read.
> This has now been corrected, e.g.:
>
> pci :00:13.0: PCI->APIC IRQ transform: INT A -> IRQ 17
> blogic_cmd(86) Status = 30: 4 ==> 4: FF 05 93 00
> blogic_cmd(95) Status = 28: (Modify I/O Address)
> blogic_cmd(91) Status = 30: 1 ==> 1: 01
> blogic_cmd(04) Status = 30: 4 ==> 4: 41 41 35 30
> blogic_cmd(8D) Status = 30: 14 ==> 14: 45 DC 00 20 00 00 00 00 00 40 30 37 42
> 1D
> scsi: * BusLogic SCSI Driver Version 2.1.17 of 12 September 2013 *
> scsi: Copyright 1995-1998 by Leonard N. Zubkoff
> blogic_cmd(04) Status = 30: 4 ==> 4: 41 41 35 30
> blogic_cmd(0B) Status = 30: 3 ==> 3: 00 08 07
> blogic_cmd(0D) Status = 30: 34 ==> 34: 03 01 07 04 00 00 00 00 00 00 00 00 00
> 00 00 00 FF 42 44 46 FF 00 00 00 00 00 00 00 00 00 FF 00 FF 00
> blogic_cmd(8D) Status = 30: 14 ==> 14: 45 DC 00 20 00 00 00 00 00 40 30 37 42
> 1D
> blogic_cmd(84) Status = 30: 1 ==> 1: 37
> blogic_cmd(8B) Status = 30: 5 ==> 5: 39 35 38 20 20
> blogic_cmd(85) Status = 30: 1 ==> 1: 42
> blogic_cmd(86) Status = 30: 4 ==> 4: FF 05 93 00
> blogic_cmd(91) Status = 30: 64 ==> 64: 41 46 3E 20 39 35 38 20 20 00 C4 00 04
> 01 07 2F 07 04 35 FF FF FF FF FF FF FF FF FF FF 01 00 FE FF 08 FF FF 00 00 00
> 00 00 00 00 01 00 01 00 00 FF FF 00 00 00 00 00 00 00 00 00 00 00 00 00 FC
> scsi0: Configuring BusLogic Model BT-958 PCI Wide Ultra SCSI Host Adapter
>
> etc.
In patch 2, vscnprintf should probably be used to make sure it's
0 terminated.
And while it's a lot more code, I'd prefer a solution that looks more
like the other commonly used kernel logging extension mechanisms
where adapter is placed before the format, ... in the argument list.
Today it's:
void blogic_msg(enum, fmt, adapter, ...);
without the __printf marking so there is one format/arg mismatch.
fyi: in the suggested patch below it's
- blogic_info("BIOS Address: 0x%lX, ", adapter,
- adapter->bios_addr);
+ blogic_cont(adapter, "BIOS Address: 0x%X, ",
+ adapter->bios_addr);
I'd prefer
__printf(3, 4)
void blogic_msg(enum, adapter, fmt, ...)
(or maybe void blogic_msg(adapter, enum, fmt, ...))
And there's a simple addition of a blogic_cont macro and extension
to blogic_msg to simplify the logic and obviousness of the logging
extension lines too.
I suggest this done with coccinelle and a little typing:
---
drivers/scsi/BusLogic.c | 496 +++-
drivers/scsi/BusLogic.h | 32 ++--
2 files changed, 341 insertions(+), 187 deletions(-)
diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c
index
Re: [PATCH 0/3] Detect suspicious indentation after conditional
On Wed, 2021-04-14 at 14:18 -0700, Julius Werner wrote: > *friendly ping* > > Hi Andy, Joe, > > Any comments on this patch series? Are you guys the right point of > contact for checkpatch changes? I don't have any issue with this patch set, but Andy is really the person that should approve any changes to this block of code. > On Thu, Mar 25, 2021 at 8:50 PM Julius Werner wrote: > > > > This patch series is adding functionality to checkpatch.pl to test for > > incorrect code indentation after a conditional statement, like this: > > > > if (a) > > b; > > c; > > > > (Indentation implies that `c;` was guarded by the conditional, but it > > isn't.) The main part is re-sending a patch from Ivo Sieben that was > > already proposed in 2014 [1]. I don't know why it was never merged -- > > it seems that there was no discussion on it. I hope that it was only > > overlooked, because it works great, and I think this is a very important > > class of common error to catch. > > > > I have tested it extensively on the kernel tree and in the course of > > that found a few more edge cases that get fixed by the other two > > patches. With all these applied, the vast majority of hits I get from > > this check on the kernel tree are actual indentation errors or other > > code style violations (e.g. case label and statement on the same line). > > The only significant remaining group of false positives I found are > > cases of macros being defined within a function, which are overall very > > rare. I think the benefit of adding this check would far outweigh the > > remaining amount of noise. > > > > [1]: https://lore.kernel.org/patchwork/patch/465116 > > > > Ivo Sieben (1): > > Suspicious indentation detection after conditional statement > > > > Julius Werner (2): > > checkpatch: ctx_statement_block: Fix preprocessor guard tracking > > checkpatch: Ignore labels when checking indentation > > > > scripts/checkpatch.pl | 56 +++ > > 1 file changed, 52 insertions(+), 4 deletions(-) > > > > -- > > 2.29.2 > >
Re: [PATCH 2/2] Input: evbug - Use 'pr_debug()' instead of hand-writing it
On Thu, 2021-04-15 at 22:58 +0200, Christophe JAILLET wrote:
> 'printk(KERN_DEBUG pr_fmt(...))' can be replaced by a much less verbose
> 'pr_debug()'.
This is not really true because
printk(KERN_DEBUG ...);
will _always_ be emitted if the console level allows
pr_debug(...);
will _only_ be emitted if the console level allows _and_
DEBUG is defined or dynamic_debug is enabled
(and for dynamic_debug, only if specifically enabled)
DEBUG is defined and dynamic_debug is enabled
> diff --git a/drivers/input/evbug.c b/drivers/input/evbug.c
[]
> @@ -21,8 +21,8 @@ MODULE_LICENSE("GPL");
>
>
> static void evbug_event(struct input_handle *handle, unsigned int type,
> unsigned int code, int value)
> {
> - printk(KERN_DEBUG pr_fmt("Event. Dev: %s, Type: %d, Code: %d, Value:
> %d\n"),
> -dev_name(>dev->dev), type, code, value);
> + pr_debug("Event. Dev: %s, Type: %d, Code: %d, Value: %d\n",
> + dev_name(>dev->dev), type, code, value);
> }
Re: [PATCH] staging: greybus: Match parentheses alignment
On Wed, 2021-04-14 at 09:35 -0500, Alex Elder wrote: > On 4/14/21 9:29 AM, Joe Perches wrote: > > On Wed, 2021-04-14 at 08:17 -0500, Alex Elder wrote: > > > Perhaps (like the -W options for GCC) there > > > could be a way to specify in a Makefile which checkpatch > > > messages are reported/not reported? I don't claim that's > > > a good suggestion, but if I could optionally indicate > > > somewhere that "two consecutive blank lines is OK for > > > Greybus" (one example that comes to mind) I might do so. > > > > checkpatch already has --ignore= and --types= > > for the various classes of messages it emits. > > > > see: $ ./scripts/checkpatch.pl --list-types --verbose > > > > Dwaipayan Ray (cc'd) is supposedly working on expanding > > the verbose descriptions of each type. > > > > That's awesome, I wasn't aware of that. > > Any suggestions on a standardized way to say "in this > subtree, please provide these arguments to checkpatch.pl"? > > I can probably stick it in a README file or something, > but is there an existing best practice? There is no standardized mechanism for this checkpatch use. Putting something in a staging README is in general a good way for it to _not_ be read by people doing 'my first kernel patch'. I still think emitting a message for overly long identifiers could be a decent checkpatch test. https://lore.kernel.org/lkml/1518801207.13169.15.ca...@perches.com/
Re: [PATCH] staging: greybus: Match parentheses alignment
On Wed, 2021-04-14 at 08:17 -0500, Alex Elder wrote: > Perhaps (like the -W options for GCC) there > could be a way to specify in a Makefile which checkpatch > messages are reported/not reported? I don't claim that's > a good suggestion, but if I could optionally indicate > somewhere that "two consecutive blank lines is OK for > Greybus" (one example that comes to mind) I might do so. checkpatch already has --ignore= and --types= for the various classes of messages it emits. see: $ ./scripts/checkpatch.pl --list-types --verbose Dwaipayan Ray (cc'd) is supposedly working on expanding the verbose descriptions of each type.
Re: [PATCH 12/19] staging: rtl8723bs: remove unnecessary bracks on DBG_871X removal sites
On Tue, 2021-04-13 at 17:52 +0300, Dan Carpenter wrote:
> On Wed, Apr 07, 2021 at 03:49:36PM +0200, Fabio Aiuto wrote:
> > @@ -2586,11 +2583,9 @@ static int rtw_dbg_port(struct net_device *dev,
> >
> >
> > plist =
> > get_next(plist);
> >
> >
> > - if (extra_arg
> > == psta->aid) {
> > - for (j
> > = 0; j < 16; j++) {
> > + if (extra_arg
> > == psta->aid)
> > + for (j
> > = 0; j < 16; j++)
> >
> > preorder_ctrl = >recvreorder_ctrl[j];
> > - }
> > - }
>
> I think Greg already applied this so no stress (don't bother fixing),
> but you removed a bit too much on this one. Multi-line indents normally
> get curly braces for readability. In other words:
>
> if (extra_arg == psta->aid) {
> for (j = 0; j < 16; j++)
> preorder_ctrl =
> >recvreorder_ctrl[j];
> }
>
> regards,
> dan carpenter
>
Re: [PATCH] staging: media: tegra-vde: Align line break to match with the open parenthesis in file trace.h
On Tue, 2021-04-13 at 21:35 +0530, Dwaipayan Ray wrote:
> On Tue, Apr 13, 2021 at 8:59 PM Thierry Reding
> wrote:
> >
> > On Mon, Apr 12, 2021 at 07:20:40PM -0300, Aline Santana Cordeiro wrote:
> > > Align line break to match with the open parenthesis.
> > > Issue detected by checkpatch.pl.
> > > It consequently solved a few end lines with a '(',
> > > issue also detected by checkpatch.pl
> > >
> > > Signed-off-by: Aline Santana Cordeiro
> > > ---
> > > drivers/staging/media/tegra-vde/trace.h | 111
> > > ++--
> > > 1 file changed, 50 insertions(+), 61 deletions(-)
> >
> > Ugh... I'd say this is one case where checkpatch.pl really shouldn't be
> > complaining since this isn't a function call or declaration. It's more
> > like a code snippet written with macros, so I don't think the same rules
> > should apply.
> >
> > Adding checkpatch folks (hence quoting in full): is there anything we
> > can do about this without too much effort? Or should we just accept this
> > the way it is?
> >
>
> While it may be possible to add exceptions for trace headers on the
> alignment rules, I don't know how many more such exceptions we will
> end up adding. Such fine grained checks might be unnecessarily complex.
> So I think we should just accept the way it is for now.
>
> Joe might have a different opinion?
Tracing uses a different style.
Maybe just suppress various messages for complete code blocks
of DECLARE_EVENT_CLASS, DEFINE_EVENT, and TRACE_EVENT
Re: [PATCH v4 3/3] staging: rtl8192e: remove unnecessary parentheses
On Mon, 2021-04-12 at 16:52 +0530, Mitali Borkar wrote: > Removed unnecessary parentheses because they must be used only when it > is necessary or they improve readability. > Reported by checkpatch. I gave you feedback about the memset changes. Please use it. https://lore.kernel.org/lkml/f5fe04d62b22eb5e09c299e769d9b9d8917f119c.ca...@perches.com/ > Changes from v3:- No changes. > Changes from v2:- Rectified spelling mistake in subject description. > Changes has been made in v3. > Changes from v1:- No changes. [] > diff --git a/drivers/staging/rtl8192e/rtl819x_HTProc.c > b/drivers/staging/rtl8192e/rtl819x_HTProc.c [] > @@ -646,13 +646,13 @@ void HTInitializeHTInfo(struct rtllib_device *ieee) > pHTInfo->CurrentMPDUDensity = pHTInfo->MPDU_Density; > pHTInfo->CurrentAMPDUFactor = pHTInfo->AMPDU_Factor; > > > - memset((void *)(&(pHTInfo->SelfHTCap)), 0, > + memset((void *)(>SelfHTCap), 0, > sizeof(pHTInfo->SelfHTCap)); memset(>SelfHTCap, 0, sizeof(pHTInfo->SelfHTCap); etc...
Re: [PATCH v2] iommu/amd: Fix extended features logging
On Mon, 2021-04-12 at 00:13 +0300, Alexander Monakov wrote:
> print_iommu_info prints the EFR register and then the decoded list of
> features on a separate line:
>
> pci :00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade):
> PPR X2APIC NX GT IA GA PC GA_vAPIC
>
> The second line is emitted via 'pr_cont', which causes it to have a
> different ('warn') loglevel compared to the previous line ('info').
>
> Commit 9a295ff0ffc9 attempted to rectify this by removing the newline
> from the pci_info format string, but this doesn't work, as pci_info
> calls implicitly append a newline anyway.
>
> Printing the decoded features on the same line would make it quite long.
> Instead, change pci_info() to pr_info() to omit PCI bus location info,
> which is shown in the preceding message anyway. This results in:
>
> pci :00:00.2: AMD-Vi: Found IOMMU cap 0x40
> AMD-Vi: Extended features (0x206d73ef22254ade): PPR X2APIC NX GT IA GA PC
> GA_vAPIC
> AMD-Vi: Interrupt remapping enabled
>
> Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix
> divergent log levels")
> Link:
> https://lore.kernel.org/lkml/alpine.lnx.2.20.13.2104112326460.11...@monopod.intra.ispras.ru
> Signed-off-by: Alexander Monakov
> Cc: Paul Menzel
> Cc: Joerg Roedel
> Cc: Suravee Suthikulpanit
> Cc: io...@lists.linux-foundation.org
> ---
>
> v2: avoid pr_info(""), change pci_info() to pr_info() for a nicer
> solution
>
> drivers/iommu/amd/init.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 596d0c413473..62913f82a21f 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -1929,8 +1929,8 @@ static void print_iommu_info(void)
> pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
>
>
> if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
> - pci_info(pdev, "Extended features (%#llx):",
> - iommu->features);
> + pr_info("Extended features (%#llx):", iommu->features);
> +
> for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
> if (iommu_feature(iommu, (1ULL << i)))
> pr_cont(" %s", feat_str[i]);
How about avoiding all of this by using a temporary buffer
and a single pci_info.
Miscellanea:
o Move the feat_str and i declarations into the if block for locality
---
drivers/iommu/amd/init.c | 29 ++---
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 321f5906e6ed..0d219044726e 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1943,30 +1943,37 @@ static int __init iommu_init_pci(struct amd_iommu
*iommu)
static void print_iommu_info(void)
{
- static const char * const feat_str[] = {
- "PreF", "PPR", "X2APIC", "NX", "GT", "[5]",
- "IA", "GA", "HE", "PC"
- };
struct amd_iommu *iommu;
for_each_iommu(iommu) {
struct pci_dev *pdev = iommu->dev;
- int i;
pci_info(pdev, "Found IOMMU cap 0x%x\n", iommu->cap_ptr);
if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
- pci_info(pdev, "Extended features (%#llx):",
-iommu->features);
+ static const char * const feat_str[] = {
+ "PreF", "PPR", "X2APIC", "NX", "GT", "[5]",
+ "IA", "GA", "HE", "PC"
+ };
+ int i;
+ char features[128] = "";
+ int len = 0;
+
for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
- if (iommu_feature(iommu, (1ULL << i)))
- pr_cont(" %s", feat_str[i]);
+ if (!iommu_feature(iommu, BIT_ULL(i)))
+ continue;
+ len += scnprintf(features + len,
+sizeof(features) - len,
+" %s", feat_str[i]);
}
if (iommu->features & FEATURE_GAM_VAPIC)
- pr_cont(" GA_vAPIC");
+ len += scnprintf(features + len,
+sizeof(features) - len,
+" %s", "GA_vPIC");
- pr_cont("\n");
+ pci_info(pdev, "Extended features (%#llx):%s\n",
+iommu->features, features);
}
}
if (irq_remapping_enabled) {
Re: [PATCH] iommu/amd: Fix extended features logging
On Sun, 2021-04-11 at 21:52 +0200, John Ogness wrote: > I'd rather fix dev_info callers to allow pr_cont and then fix any code > that is using this workaround. Assuming you mean all dev_() uses, me too. > And if the print maintainers agree it is OK to encourage > pr_cont(LOGLEVEL "...") usage, then people should really start using > that if the loglevel on those pieces is important. I have no stong feeling about the use of pr_cont( as valuable or not. I think it's just a trivial bit that could be somewhat useful when interleaving occurs. A somewhat better mechanism would be to have an explicit cookie use like: cookie = printk_multipart_init(KERN_LEVEL, fmt, ...); while () printk_multipart_cont(cookie, fmt, ...); printk_multipark_end(cookie, fmt, ...); And separately, there should be a pr_debug_cont or equivalent.
Re: [PATCH] iommu/amd: Fix extended features logging
(cc'ing the printk maintainers)
On Sun, 2021-04-11 at 00:11 +0300, Alexander Monakov wrote:
> print_iommu_info prints the EFR register and then the decoded list of
> features on a separate line:
>
> pci :00:00.2: AMD-Vi: Extended features (0x206d73ef22254ade):
> PPR X2APIC NX GT IA GA PC GA_vAPIC
>
> The second line is emitted via 'pr_cont', which causes it to have a
> different ('warn') loglevel compared to the previous line ('info').
>
> Commit 9a295ff0ffc9 attempted to rectify this by removing the newline
> from the pci_info format string, but this doesn't work, as pci_info
> calls implicitly append a newline anyway.
>
> Restore the newline, and call pr_info with empty format string to set
> the loglevel for subsequent pr_cont calls. The same solution is used in
> EFI and uvesafb drivers.
>
> Fixes: 9a295ff0ffc9 ("iommu/amd: Print extended features in one line to fix
> divergent log levels")
> Signed-off-by: Alexander Monakov
> Cc: Paul Menzel
> Cc: Joerg Roedel
> Cc: Suravee Suthikulpanit
> Cc: io...@lists.linux-foundation.org
> ---
> drivers/iommu/amd/init.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 596d0c413473..a25e241eff1c 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -1929,8 +1929,11 @@ static void print_iommu_info(void)
> pci_info(pdev, "Found IOMMU cap 0x%hx\n", iommu->cap_ptr);
>
>
> if (iommu->cap & (1 << IOMMU_CAP_EFR)) {
> - pci_info(pdev, "Extended features (%#llx):",
> + pci_info(pdev, "Extended features (%#llx):\n",
> iommu->features);
> +
> + pr_info("");
> +
> for (i = 0; i < ARRAY_SIZE(feat_str); ++i) {
> if (iommu_feature(iommu, (1ULL << i)))
> pr_cont(" %s", feat_str[i]);
This shouldn't be necessary.
If this is true then a lot of output logging code broke.
Re: drivers/parport/parport_cs.c:147 parport_config() warn: inconsistent indenting
On Sun, 2021-04-11 at 02:02 +0800, kernel test robot wrote: > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > master > head: d4961772226de3b48a395a26c076d450d7044c76 > commit: decf26f6ec25dac868782dc1751623a87d147831 parport: Convert > printk(KERN_ to pr_( > date: 12 months ago > config: x86_64-randconfig-m001-20210410 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > > smatch warnings: > drivers/parport/parport_cs.c:147 parport_config() warn: inconsistent indenting False positive. The whole thing is inconsistently indented.
Re: [PATCH 4/6] staging: rtl8192e: matched alignment with open parenthesis
On Sat, 2021-04-10 at 07:55 +0530, Mitali Borkar wrote: > On Fri, Apr 09, 2021 at 07:07:09PM -0700, Joe Perches wrote: > > On Sat, 2021-04-10 at 07:05 +0530, Mitali Borkar wrote: > > > Matched the alignment with open parenthesis to meet linux kernel coding > > > style. > > > Reported by checkpatch. > > [] > > > diff --git a/drivers/staging/rtl8192e/rtl819x_HTProc.c > > > b/drivers/staging/rtl8192e/rtl819x_HTProc.c > > [] > > > @@ -154,7 +154,7 @@ bool IsHTHalfNmodeAPs(struct rtllib_device *ieee) > > > (net->ralink_cap_exist)) > > > retValue = true; > > > else if (!memcmp(net->bssid, UNKNOWN_BORADCOM, 3) || > > > - !memcmp(net->bssid, LINKSYSWRT330_LINKSYSWRT300_BROADCOM, 3) || > > > + !memcmp(net->bssid, LINKSYSWRT330_LINKSYSWRT300_BROADCOM, 3) || > > > !memcmp(net->bssid, LINKSYSWRT350_LINKSYSWRT150_BROADCOM, 3) || > > > (net->broadcom_cap_exist)) > > > > checkpatch is a stupid script. > > Look further at the code not just at what checkpatch reports. > > Align all the contination lines, not just the first one. > > > Sir, I have aligned them in last patch of this patchset. This sort of change should not require an additional patch.
Re: [PATCH 4/6] staging: rtl8192e: matched alignment with open parenthesis
On Sat, 2021-04-10 at 07:05 +0530, Mitali Borkar wrote:
> Matched the alignment with open parenthesis to meet linux kernel coding
> style.
> Reported by checkpatch.
[]
> diff --git a/drivers/staging/rtl8192e/rtl819x_HTProc.c
> b/drivers/staging/rtl8192e/rtl819x_HTProc.c
[]
> @@ -154,7 +154,7 @@ bool IsHTHalfNmodeAPs(struct rtllib_device *ieee)
> (net->ralink_cap_exist))
> retValue = true;
> else if (!memcmp(net->bssid, UNKNOWN_BORADCOM, 3) ||
> - !memcmp(net->bssid, LINKSYSWRT330_LINKSYSWRT300_BROADCOM, 3) ||
> + !memcmp(net->bssid, LINKSYSWRT330_LINKSYSWRT300_BROADCOM, 3) ||
> !memcmp(net->bssid, LINKSYSWRT350_LINKSYSWRT150_BROADCOM, 3) ||
> (net->broadcom_cap_exist))
checkpatch is a stupid script.
Look further at the code not just at what checkpatch reports.
Align all the contination lines, not just the first one.
It might be sensible to add a generic function like
static inline bool ether_oui_equal(const u8 *addr, const u8 *oui)
{
return addr[0] == oui[0] && addr[1] == oui[1] && addr[2] == oui[2];
}
to include/linux/etherdevice.h
(Maybe use & instead of && if it's speed sensitive)
so this would read
else if (ether_oui_equal(net->bssid, UNKNOWN_BORADCOM) ||
ether_oui_equal(net->bssid,
LINKSYSWRT330_LINKSYSWRT300_BROADCOM) ||
ether_oui_equal(net->bssid,
LINKSYSWRT350_LINKSYSWRT150_BROADCOM) ||
net->broacom_cap_exist)
and it'd also be good to correct the typo of UNKNOWN_BORADCOM globally.
> @@ -654,13 +654,13 @@ void HTInitializeHTInfo(struct rtllib_device *ieee)
> pHTInfo->CurrentAMPDUFactor = pHTInfo->AMPDU_Factor;
>
>
> memset((void *)(&(pHTInfo->SelfHTCap)), 0,
> - sizeof(pHTInfo->SelfHTCap));
> +sizeof(pHTInfo->SelfHTCap));
Doesn't need casts or parentheses.
memset(>SelfHTCap, 0, sizeof(pHTInfo->SelfHCap));
> memset((void *)(&(pHTInfo->SelfHTInfo)), 0,
> - sizeof(pHTInfo->SelfHTInfo));
> +sizeof(pHTInfo->SelfHTInfo));
etc...
> @@ -815,7 +815,7 @@ void HTUseDefaultSetting(struct rtllib_device *ieee)
> HTFilterMCSRate(ieee, ieee->Regdot11TxHTOperationalRateSet,
> ieee->dot11HTOperationalRateSet);
> ieee->HTHighestOperaRate = HTGetHighestMCSRate(ieee,
> -ieee->dot11HTOperationalRateSet,
> +
> ieee->dot11HTOperationalRateSet,
> MCS_FILTER_ALL);
multi line statement alignment etc...
Re: [PATCH v5 2/6] w1: ds2438: fixed if brackets coding style issue
On Fri, 2021-04-09 at 00:09 -0300, Luiz Sampaio wrote:
> Since there is only one statement inside the if clause, no brackets are
> required.
[]
> diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c
[]
> @@ -287,9 +287,9 @@ static ssize_t iad_read(struct file *filp, struct kobject
> *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_current(sl, ) == 0) {
> + if (w1_ds2438_get_current(sl, ) == 0)
> ret = snprintf(buf, count, "%i\n", voltage);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
to me this would look better using a style like the below:
(and it might be better using sysfs_emit and not snprintf too)
---
drivers/w1/slaves/w1_ds2438.c | 36
1 file changed, 12 insertions(+), 24 deletions(-)
diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c
index 5cfb0ae23e91..9115c5a9bc4f 100644
--- a/drivers/w1/slaves/w1_ds2438.c
+++ b/drivers/w1/slaves/w1_ds2438.c
@@ -279,7 +279,6 @@ static ssize_t iad_read(struct file *filp, struct kobject
*kobj,
loff_t off, size_t count)
{
struct w1_slave *sl = kobj_to_w1_slave(kobj);
- int ret;
int16_t voltage;
if (off != 0)
@@ -287,12 +286,10 @@ static ssize_t iad_read(struct file *filp, struct kobject
*kobj,
if (!buf)
return -EINVAL;
- if (w1_ds2438_get_current(sl, ) == 0) {
- ret = snprintf(buf, count, "%i\n", voltage);
- } else
- ret = -EIO;
+ if (w1_ds2438_get_current(sl, ))
+ return -EIO;
- return ret;
+ return snprintf(buf, count, "%i\n", voltage);
}
static ssize_t page0_read(struct file *filp, struct kobject *kobj,
@@ -330,7 +327,6 @@ static ssize_t temperature_read(struct file *filp, struct
kobject *kobj,
loff_t off, size_t count)
{
struct w1_slave *sl = kobj_to_w1_slave(kobj);
- int ret;
int16_t temp;
if (off != 0)
@@ -338,12 +334,10 @@ static ssize_t temperature_read(struct file *filp, struct
kobject *kobj,
if (!buf)
return -EINVAL;
- if (w1_ds2438_get_temperature(sl, ) == 0) {
- ret = snprintf(buf, count, "%i\n", temp);
- } else
- ret = -EIO;
+ if (w1_ds2438_get_temperature(sl, ))
+ return -EIO;
- return ret;
+ return snprintf(buf, count, "%i\n", temp);
}
static ssize_t vad_read(struct file *filp, struct kobject *kobj,
@@ -351,7 +345,6 @@ static ssize_t vad_read(struct file *filp, struct kobject
*kobj,
loff_t off, size_t count)
{
struct w1_slave *sl = kobj_to_w1_slave(kobj);
- int ret;
uint16_t voltage;
if (off != 0)
@@ -359,12 +352,10 @@ static ssize_t vad_read(struct file *filp, struct kobject
*kobj,
if (!buf)
return -EINVAL;
- if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, ) == 0) {
- ret = snprintf(buf, count, "%u\n", voltage);
- } else
- ret = -EIO;
+ if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, ))
+ return -EIO;
- return ret;
+ return snprintf(buf, count, "%u\n", voltage);
}
static ssize_t vdd_read(struct file *filp, struct kobject *kobj,
@@ -372,7 +363,6 @@ static ssize_t vdd_read(struct file *filp, struct kobject
*kobj,
loff_t off, size_t count)
{
struct w1_slave *sl = kobj_to_w1_slave(kobj);
- int ret;
uint16_t voltage;
if (off != 0)
@@ -380,12 +370,10 @@ static ssize_t vdd_read(struct file *filp, struct kobject
*kobj,
if (!buf)
return -EINVAL;
- if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, ) == 0) {
- ret = snprintf(buf, count, "%u\n", voltage);
- } else
- ret = -EIO;
+ if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, ))
+ return -EIO;
- return ret;
+ return snprintf(buf, count, "%u\n", voltage);
}
static BIN_ATTR(iad, S_IRUGO | S_IWUSR | S_IWGRP, iad_read, iad_write, 0);
Re: [PATCH][next] mlxsw: spectrum_router: remove redundant initialization of variable force
On Mon, 2021-03-29 at 09:04 +0300, Dan Carpenter wrote:
> On Sat, Mar 27, 2021 at 10:33:34PM +, Colin King wrote:
> > From: Colin Ian King
> >
> > The variable force is being initialized with a value that is
> > never read and it is being updated later with a new value. The
> > initialization is redundant and can be removed.
> >
> > Addresses-Coverity: ("Unused value")
> > Signed-off-by: Colin Ian King
> > ---
> > drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > index 6ccaa194733b..ff240e3c29f7 100644
> > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
> > @@ -5059,7 +5059,7 @@ mlxsw_sp_nexthop_obj_bucket_adj_update(struct
> > mlxsw_sp *mlxsw_sp,
> > {
> > u16 bucket_index = info->nh_res_bucket->bucket_index;
> > struct netlink_ext_ack *extack = info->extack;
> > - bool force = info->nh_res_bucket->force;
> > + bool force;
> > char ratr_pl_new[MLXSW_REG_RATR_LEN];
> > char ratr_pl[MLXSW_REG_RATR_LEN];
> > u32 adj_index;
>
> Reverse Christmas tree.
Is still terrible style.
Re: [PATCH] staging: media: meson: vdec: matched alignment with parenthesis
On Fri, 2021-04-09 at 09:30 +0200, Hans Verkuil wrote:
> On 09/04/2021 00:19, Mitali Borkar wrote:
> > Matched alignment with open parenthesis to meet linux kernel coding
> > style.
> > Reported by checkpatch
> >
> > Signed-off-by: Mitali Borkar
> > ---
> > drivers/staging/media/meson/vdec/codec_mpeg12.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/meson/vdec/codec_mpeg12.c
> > b/drivers/staging/media/meson/vdec/codec_mpeg12.c
> > index 48869cc3d973..21e93a13356c 100644
> > --- a/drivers/staging/media/meson/vdec/codec_mpeg12.c
> > +++ b/drivers/staging/media/meson/vdec/codec_mpeg12.c
> > @@ -81,7 +81,7 @@ static int codec_mpeg12_start(struct amvdec_session *sess)
> > }
> >
> >
> > ret = amvdec_set_canvases(sess, (u32[]){ AV_SCRATCH_0, 0 },
> > - (u32[]){ 8, 0 });
> > + (u32[]){ 8, 0 });
>
> The alignment here is because the 2nd and 3rd arguments belong together, so
> the alignment indicates that. In order to keep that I would add a newline
> after 'sess,' as well. Same as is done in meson/vdec/codec_h264.c.
Perhaps better as:
---
drivers/staging/media/meson/vdec/codec_mpeg12.c | 5 +++--
drivers/staging/media/meson/vdec/vdec_helpers.c | 2 +-
drivers/staging/media/meson/vdec/vdec_helpers.h | 2 +-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/meson/vdec/codec_mpeg12.c
b/drivers/staging/media/meson/vdec/codec_mpeg12.c
index 48869cc3d973..933f1cd16ce1 100644
--- a/drivers/staging/media/meson/vdec/codec_mpeg12.c
+++ b/drivers/staging/media/meson/vdec/codec_mpeg12.c
@@ -65,6 +65,8 @@ static int codec_mpeg12_start(struct amvdec_session *sess)
struct amvdec_core *core = sess->core;
struct codec_mpeg12 *mpeg12;
int ret;
+ static const u32 canvas1[] = { AV_SCRATCH_0, 0 };
+ static const u32 canvas2[] = { 8, 0 };
mpeg12 = kzalloc(sizeof(*mpeg12), GFP_KERNEL);
if (!mpeg12)
@@ -80,8 +82,7 @@ static int codec_mpeg12_start(struct amvdec_session *sess)
goto free_mpeg12;
}
- ret = amvdec_set_canvases(sess, (u32[]){ AV_SCRATCH_0, 0 },
- (u32[]){ 8, 0 });
+ ret = amvdec_set_canvases(sess, canvas1, canvas2);
if (ret)
goto free_workspace;
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.c
b/drivers/staging/media/meson/vdec/vdec_helpers.c
index 7f07a9175815..df5c27266c44 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.c
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.c
@@ -177,7 +177,7 @@ static int set_canvas_nv12m(struct amvdec_session *sess,
}
int amvdec_set_canvases(struct amvdec_session *sess,
- u32 reg_base[], u32 reg_num[])
+ const u32 reg_base[], const u32 reg_num[])
{
struct v4l2_m2m_buffer *buf;
u32 pixfmt = sess->pixfmt_cap;
diff --git a/drivers/staging/media/meson/vdec/vdec_helpers.h
b/drivers/staging/media/meson/vdec/vdec_helpers.h
index cfaed52ab526..ace8897c34fe 100644
--- a/drivers/staging/media/meson/vdec/vdec_helpers.h
+++ b/drivers/staging/media/meson/vdec/vdec_helpers.h
@@ -17,7 +17,7 @@
* @reg_num: number of contiguous registers after each reg_base (including it)
*/
int amvdec_set_canvases(struct amvdec_session *sess,
- u32 reg_base[], u32 reg_num[]);
+ const u32 reg_base[], const u32 reg_num[]);
/* Helpers to read/write to the various IPs (DOS, PARSER) */
u32 amvdec_read_dos(struct amvdec_core *core, u32 reg);
Re: [PATCH v5 2/6] w1: ds2438: fixed if brackets coding style issue
On Fri, 2021-04-09 at 00:09 -0300, Luiz Sampaio wrote:
> Since there is only one statement inside the if clause, no brackets are
> required.
>
> Signed-off-by: Luiz Sampaio
> ---
> drivers/w1/slaves/w1_ds2438.c | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/w1/slaves/w1_ds2438.c b/drivers/w1/slaves/w1_ds2438.c
> index 148921fb9702..56e53a748059 100644
> --- a/drivers/w1/slaves/w1_ds2438.c
> +++ b/drivers/w1/slaves/w1_ds2438.c
> @@ -287,9 +287,9 @@ static ssize_t iad_read(struct file *filp, struct kobject
> *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_current(sl, ) == 0) {
> + if (w1_ds2438_get_current(sl, ) == 0)
> ret = snprintf(buf, count, "%i\n", voltage);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
> @@ -338,9 +338,9 @@ static ssize_t temperature_read(struct file *filp, struct
> kobject *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_temperature(sl, ) == 0) {
> + if (w1_ds2438_get_temperature(sl, ) == 0)
> ret = snprintf(buf, count, "%i\n", temp);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
> @@ -359,9 +359,9 @@ static ssize_t vad_read(struct file *filp, struct kobject
> *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, ) == 0) {
> + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VAD, ) == 0)
> ret = snprintf(buf, count, "%u\n", voltage);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
> @@ -380,9 +380,9 @@ static ssize_t vdd_read(struct file *filp, struct kobject
> *kobj,
> if (!buf)
> return -EINVAL;
>
>
> - if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, ) == 0) {
> + if (w1_ds2438_get_voltage(sl, DS2438_ADC_INPUT_VDD, ) == 0)
> ret = snprintf(buf, count, "%u\n", voltage);
> - } else
> + else
> ret = -EIO;
>
>
> return ret;
Re: [RESEND PATCH v1] checkpatch: exclude four preprocessor sub-expressions from MACRO_ARG_REUSE
On Wed, 2021-04-07 at 19:50 +0900, Vincent Mailhol wrote:
> __must_be_array, offsetof, sizeof_field and __stringify are all
> preprocessor macros and do not evaluate their arguments. As such, it
> is safe not to warn when arguments are being reused in those four
> sub-expressions.
>
> Exclude those so that they can pass checkpatch.
>
> Signed-off-by: Vincent Mailhol
Acked-by: Joe Perches
> ---
> scripts/checkpatch.pl | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index df8b23dc1eb0..25ee4fd5b118 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5736,7 +5736,7 @@ sub process {
> next if ($arg =~ /\.\.\./);
> next if ($arg =~ /^type$/i);
> my $tmp_stmt = $define_stmt;
> - $tmp_stmt =~
> s/\b(sizeof|typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g;
> + $tmp_stmt =~
> s/\b(__must_be_array|offsetof|sizeof|sizeof_field|__stringify|typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g;
> $tmp_stmt =~ s/\#+\s*$arg\b//g;
> $tmp_stmt =~ s/\b$arg\s*\#\#//g;
> my $use_cnt = () = $tmp_stmt =~ /\b$arg\b/g;
Re: [PATCH][next] erofs: fix uninitialized variable i used in a while-loop
On Wed, 2021-04-07 at 07:54 +0800, Gao Xiang wrote:
> Hi Colin,
>
> On Tue, Apr 06, 2021 at 05:27:18PM +0100, Colin King wrote:
> > From: Colin Ian King
> >
> > The while-loop iterates until src is non-null or i is 3, however, the
> > loop counter i is not intinitialied to zero, causing incorrect iteration
> > counts. Fix this by initializing it to zero.
> >
> > Addresses-Coverity: ("Uninitialized scalar variable")
> > Fixes: 1aa5f2e2feed ("erofs: support decompress big pcluster for lz4
> > backend")
> > Signed-off-by: Colin Ian King
>
> Thank you very much for catching this! It looks good to me,
> Reviewed-by: Gao Xiang
>
> (btw, may I fold this into the original patchset? since such big pcluster
> patchset is just applied to for-next for further integration testing, and
> the commit id is not stable yet..)
>
> Thanks,
> Gao Xiang
I think this code is odd and would be more intelligible using
a for loop like:
---
fs/erofs/decompressor.c | 20
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index 27aa6a99b371..5a64f4649414 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -286,28 +286,24 @@ static int z_erofs_decompress_generic(struct
z_erofs_decompress_req *rq,
}
ret = alg->prepare_destpages(rq, pagepool);
- if (ret < 0) {
+ if (ret < 0)
return ret;
- } else if (ret) {
+ if (ret) {
dst = page_address(*rq->out);
dst_maptype = 1;
goto dstmap_out;
}
- i = 0;
- while (1) {
+ for (i = 0; i < 3; i++) {
dst = vm_map_ram(rq->out, nrpages_out, -1);
-
+ if (dst) {
+ dst_maptype = 2;
+ goto dstmap_out;
+ }
/* retry two more times (totally 3 times) */
- if (dst || ++i >= 3)
- break;
vm_unmap_aliases();
}
-
- if (!dst)
- return -ENOMEM;
-
- dst_maptype = 2;
+ return -ENOMEM;
dstmap_out:
ret = alg->decompress(rq, dst + rq->pageofs_out);
Re: [PATCH] staging: greybus: Match parentheses alignment
On Tue, 2021-04-06 at 15:27 +0200, Greg KH wrote:
> On Tue, Apr 06, 2021 at 06:42:59PM +0600, Zhansaya Bagdauletkyzy wrote:
> > Match next line with open parentheses by adding tabs/spaces
> > to conform with Linux kernel coding style.
> > Reported by checkpatch.
[]
> > diff --git a/drivers/staging/greybus/camera.c
> > b/drivers/staging/greybus/camera.c
[]
> > @@ -378,8 +378,8 @@ struct ap_csi_config_request {
> > #define GB_CAMERA_CSI_CLK_FREQ_MARGIN 15000U
> >
> >
> > static int gb_camera_setup_data_connection(struct gb_camera *gcam,
> > - struct gb_camera_configure_streams_response *resp,
> > - struct gb_camera_csi_params *csi_params)
> > + struct
> > gb_camera_configure_streams_response *resp,
> > + struct gb_camera_csi_params
> > *csi_params)
>
> And now you violate another coding style requirement, which means
> someone will send another patch to fix that up and around and around it
> goes...
None of the coding style document is an actual requirement Greg.
It's all rules of thumb. Useful rules, but not hard and fast right?
To me, the biggest issue with this code isn't whether or not the
code is aligned at open parentheses or stays within 80 columns,
but is the use of 30+ character length identifiers.
Using identifiers of that length makes using 80 column, or even
100 column length lines infeasible.
Perhaps seeing if include/linux/greybus/greybus_protocols.h
could be updated to use shorter length identifiers might be useful.
The median length identifier there is ~25 chars long and the
maximum length identifier is ~50 chars.
Re: [PATCH v3 00/30] staging: rtl8723bs: remove RT_TRACE logs in core/*
On Sat, 2021-04-03 at 19:28 +0200, Fabio Aiuto wrote: > On Sat, Apr 03, 2021 at 09:17:37AM -0700, Joe Perches wrote: > > On Sat, 2021-04-03 at 17:21 +0200, Fabio Aiuto wrote: > > > On Sat, Apr 03, 2021 at 08:02:25AM -0700, Joe Perches wrote: > > > > On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote: > > > > > This patchset removes all RT_TRACE usages in core/ files. > > > > > > > > and hal and include and os_dep > > > > > > Hi, > > > > > > I was just about to send the second patchset relative to hal/ files. > > > The whole has been split up in directories in order to reduce the > > > number of patch per patchset > > > > > It's a good idea, but the patches relative to RT_TRACE removal > > > could be huge > > > > That's really not a significant issue. > > Simplicity in review is also important. > > Mechanization of patch creation can reduce review efforts. > > Maybe I wrongly associated simplicity with patch dimensions, but maybe > for patches this simple have expert reviewers some tool for > automatic review? Coccinelle is a relatively trusted tool and using it as a scripting mechanism where the script is shown as part of the commit message gives confidence that the change it produces can be applied without significant doubt. To improve confidence that any change that does not have an output object code delta, comparing the object code produced before and after the change is useful. Showing that the code has been both compiled and compared in the commit message also improves confidence that the change is useful and can be applied.
Re: [PATCH v3 00/30] staging: rtl8723bs: remove RT_TRACE logs in core/*
On Sat, 2021-04-03 at 17:21 +0200, Fabio Aiuto wrote: > On Sat, Apr 03, 2021 at 08:02:25AM -0700, Joe Perches wrote: > > On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote: > > > This patchset removes all RT_TRACE usages in core/ files. > > > > and hal and include and os_dep > > Hi, > > I was just about to send the second patchset relative to hal/ files. > The whole has been split up in directories in order to reduce the > number of patch per patchset > It's a good idea, but the patches relative to RT_TRACE removal > could be huge That's really not a significant issue. Simplicity in review is also important. Mechanization of patch creation can reduce review efforts. Few people are actively working on this particular codebase. As far as I can tell no logical defect is being corrected here. None of this is likely to be backported. Applying each individual patch has a 'cost' in maintainer time and review effort. Fewer patches create lower overall costs. Good luck...
Re: [PATCH v3 23/30] staging: rtl8723bs: fix comparison in if condition in core/rtw_recv.c
On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote:
> fix post-commit checkpatch issue:
>
> CHECK: Using comparison to false is error prone
> 27: FILE: drivers/staging/rtl8723bs/core/rtw_recv.c:381:
> + if (psecuritypriv->
> bcheck_grpkey == false &&
>
> Signed-off-by: Fabio Aiuto
> ---
> drivers/staging/rtl8723bs/core/rtw_recv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_recv.c
> b/drivers/staging/rtl8723bs/core/rtw_recv.c
> index cd4324a93275..21949925ec77 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_recv.c
> @@ -378,7 +378,7 @@ static signed int recvframe_chkmic(struct adapter
> *adapter, union recv_frame *p
>
>
> } else {
> /* mic checked ok */
> - if (psecuritypriv->bcheck_grpkey == false &&
> + if (!psecuritypriv->bcheck_grpkey &&
> (IS_MCAST(prxattrib->ra) == true))
> psecuritypriv->bcheck_grpkey = true;
> }
Re: [PATCH v3 00/30] staging: rtl8723bs: remove RT_TRACE logs in core/*
On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote:
> This patchset removes all RT_TRACE usages in core/ files.
and hal and include and os_dep
>
> This is the first of a series aimed at removing RT_TRACE macro.
>
> The whole private tracing system is not tied to a configuration
> symbol and the default behaviour is _trace nothing_. It's verbose
> and relies on a private log level tracing doomed to be
> removed.
It's nice, but individual patches per file done by hand are difficult
to review because you are interleaving removal patches with cleanup
patches.
I believe this should be a patch series with a single patch to remove
all RT_TRACE macro uses using coccinelle and then use separate patches
to do whatever cleanups around these removals you want to do.
All of these below should be done for all files in drivers/staging/rtl8723bs
at once instead of submitting per-file patches.
IMO something like:
Cover-letter: Explain why you are doing this
Patch 1 of N: Remove all RT_TRACE macro uses using a coccinelle script
and include the coccinelle script in the commit message
Patch 2 of N: Remove commented out RT_TRACE uses
Patch 3 of N: Remove RT_TRACE macro definition
Patch 4 of N: Cleanup coccinelle generated {} uses, if/else braces and
the now unnecessary if tests around the RT_TRACE removals
Patch 5 of N: Cleanup whitespace
Patcn x of N: Whatever else related to these RT_TRACE sites...
https://lore.kernel.org/lkml/c845d8ea7d0d8e7a613471edb53d780d660142a9.ca...@perches.com/
Using a sequence like the above would be much easier to review and
would be a significant shorter patch set.
Re: [PATCH 14/16] staging: rtl8723bs: remove all RT_TRACE logs in core/rtw_wlan_util.c
On Fri, 2021-04-02 at 19:40 +0200, Fabio Aiuto wrote: > On Fri, Apr 02, 2021 at 08:20:17AM -0700, Joe Perches wrote: > > On Fri, 2021-04-02 at 14:51 +0200, Fabio Aiuto wrote: > > > On Fri, Apr 02, 2021 at 03:37:57AM -0700, Joe Perches wrote: > > > > On Fri, 2021-04-02 at 12:01 +0200, Fabio Aiuto wrote: > > > > > remove all RT_TRACE logs > > > > > > > > > > fix patch-related checkpatch issues [] > > > > Lastly, another suggestion would be to just submit a single patch > > > > removing _ALL_ the RT_TRACE uses not intermixing various other cleanups > > > > with the series and then do those other cleanups. > > > > > > > > Using a coccinelle script like: > > > > > > > > $ cat RT_TRACE.cocci > > > > @@ > > > > expression a, b, c; > > > > @@ > > > > > > > > - RT_TRACE(a, b, (c)); > > > > > > > > $ spatch -sp-file RT_TRACE.cocci drivers/staging/rtl8723bs/ > > > > > > > > And then clean up the various bits you think are inappropriately done. [] > > > thank you Joe, I tried with (RT_TRACE.cocci in parent folder) > > > > > > user@host:~/src/git/kernels/staging$ spatch -sp-file ../RT_TRACE.cocci > > > drivers/staging/rtl8723bs/ > > > init_defs_builtins: /usr/local/bin/../lib/coccinelle/standard.h > > > 0 files match > > > > Likely you are running the script on the tree after you have > > applied all your patches. > > > > Try running the cocci script on a fresh copy of -next. > > > > Using the script and adding the script in the commit message helps > > others to verify that the changes you make do not have any other effect. > > > > $ cat RT_TRACE.cocci > > @@ > > expression a, b, c; > > @@ > > > > - RT_TRACE(a, b, (c)); > > > > $ git checkout next-20210401 > > $ spatch -sp-file RT_TRACE.cocci --in-place --no-show-diff --very-quiet > > drivers/staging/rtl8723bs/ > > 31 files match > > $ git diff --stat -p > > drivers/staging/rtl8723bs/core/rtw_cmd.c | 34 +-- [] > > 28 files changed, 19 insertions(+), 935 deletions(-) [] > thank you Joe, this mail is so precious ;) I'm not quite sure what you mean by that but you quoted nearly 200k of the previous email. Please remember to trim your replies.
Re: [PATCH 14/16] staging: rtl8723bs: remove all RT_TRACE logs in core/rtw_wlan_util.c
On Fri, 2021-04-02 at 12:01 +0200, Fabio Aiuto wrote:
> remove all RT_TRACE logs
>
> fix patch-related checkpatch issues
>
> Signed-off-by: Fabio Aiuto
> ---
> .../staging/rtl8723bs/core/rtw_wlan_util.c| 26 +--
> 1 file changed, 6 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
> b/drivers/staging/rtl8723bs/core/rtw_wlan_util.c
[]
> @@ -1382,25 +1374,19 @@ int rtw_check_bcn_info(struct adapter *Adapter, u8
> *pframe, u32 packet_len)
> if (encryp_protocol == ENCRYP_PROTOCOL_WPA || encryp_protocol ==
> ENCRYP_PROTOCOL_WPA2) {
> pbuf = rtw_get_wpa_ie(>IEs[12], _ielen,
> bssid->IELength-12);
> if (pbuf && (wpa_ielen > 0)) {
> - if (_SUCCESS == rtw_parse_wpa_ie(pbuf, wpa_ielen+2,
> _cipher, _cipher, _8021x)) {
> - RT_TRACE(_module_rtl871x_mlme_c_, _drv_info_,
> - ("%s pnetwork->pairwise_cipher:
> %d, group_cipher is %d, is_8021x is %d\n", __func__,
> - pairwise_cipher, group_cipher,
> is_8021x));
> - }
> + if (rtw_parse_wpa_ie(pbuf, wpa_ielen + 2, _cipher,
> + _cipher, _8021x) ==
> _SUCCESS)
> + ;
This sort of if is a bit silly.
Better would be to remove the test and just use:
rtw_parse_wpa_ie(pbuf, wpa_ielen + 2, _cipher,
_cipher, _8021x);
> } else {
> pbuf = rtw_get_wpa2_ie(>IEs[12], _ielen,
> bssid->IELength-12);
>
>
> if (pbuf && (wpa_ielen > 0)) {
> - if (_SUCCESS == rtw_parse_wpa2_ie(pbuf,
> wpa_ielen+2, _cipher, _cipher, _8021x)) {
> - RT_TRACE(_module_rtl871x_mlme_c_,
> _drv_info_,
> - ("%s
> pnetwork->pairwise_cipher: %d, pnetwork->group_cipher is %d, is_802x is %d\n",
> - __func__,
> pairwise_cipher, group_cipher, is_8021x));
> - }
> + if (rtw_parse_wpa2_ie(pbuf, wpa_ielen + 2,
> _cipher,
> + _cipher,
> _8021x) == _SUCCESS)
> + ;
rtw_parse_wpa2_ie(pbuf, wpa_ielen + 2,
_cipher,
_cipher, _8021x);
etc...
Lastly, another suggestion would be to just submit a single patch
removing _ALL_ the RT_TRACE uses not intermixing various other cleanups
with the series and then do those other cleanups.
Using a coccinelle script like:
$ cat RT_TRACE.cocci
@@
expression a, b, c;
@@
- RT_TRACE(a, b, (c));
$ spatch -sp-file RT_TRACE.cocci drivers/staging/rtl8723bs/
And then clean up the various bits you think are inappropriately done.
Re: [PATCH 49/49] staging: rtl8723bs: remove obsolete macro
On Thu, 2021-04-01 at 11:21 +0200, Fabio Aiuto wrote:
> remove obsolete macro RT_TRACE
[]
> diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h
> b/drivers/staging/rtl8723bs/include/rtw_debug.h
[]
> -#ifdef DEBUG_RTL871X
> -
> -#if defined(_dbgdump) && defined(_MODULE_DEFINE_)
> -
> - #undef RT_TRACE
> - #define RT_TRACE(_Comp, _Level, Fmt)\
> - do {\
> - if ((_Comp & GlobalDebugComponents) && (_Level <=
> GlobalDebugLevel)) {\
> - _dbgdump("%s [0x%08x,%d]", DRIVER_PREFIX, (unsigned
> int)_Comp, _Level);\
> - _dbgdump Fmt;\
> - } \
> - } while (0)
> -
> -#endif /* defined(_dbgdump) && defined(_MODULE_DEFINE_) */
> -#endif /* DEBUG_RTL871X */
Maybe remove all of these too
$ git grep DEBUG_RTL871X
drivers/staging/rtl8723bs/core/rtw_debug.c:#ifdef DEBUG_RTL871X
drivers/staging/rtl8723bs/core/rtw_debug.c:#endif /* DEBUG_RTL871X */
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:#ifdef DEBUG_RTL871X
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:#ifdef DEBUG_RTL871X
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:#ifdef DEBUG_RTL871X
drivers/staging/rtl8723bs/include/autoconf.h:/*#define DEBUG_RTL871X */
drivers/staging/rtl8723bs/include/rtw_debug.h:#ifdef DEBUG_RTL871X
drivers/staging/rtl8723bs/include/rtw_debug.h:#endif /* DEBUG_RTL871X */
Re: [PATCH v1 1/7] Add Driver for SUNIX PCI(e) I/O expansion board
On Tue, 2021-03-30 at 16:23 +0800, Moriis Ku wrote:
> From: Morris
>
> Signed-off-by: Morris
> ---
> spi_pack.c | 1506
You might try to use scripts/checkpatch.pl on this and see
if there is anything you want to change to have the code look
more like the rest of the kernel.
Using
./scripts/checkpatch.pl --strict --fix
from the top of the kernel tree should help quite a bit with
making the code layout look more like typical kernel style.
> diff --git a/spi_pack.c b/spi_pack.c
[]
> +static void get_info(struct sunix_sdc_spi_channel *spi_chl, unsigned int
> incomeLength, unsigned int * translateLength)
> +{
[]
> + do
> + {
> + Address = spi_chl->info.phy2_base_start +
> spi_chl->info.memoffset;
> +
> + } while (false);
That's an odd and unnecessary use of a do while.
> + memset(TrBuff, 0, SUNIX_SDC_SPI_BUFF);
> + TrLength = 0;
> +
> + pTrHeader->Version = 0x01;
> + pTrHeader->CmdResponseEventData = pRxHeader->CmdResponseEventData |
> 0x8000;
> + pTrHeader->ResponseStatus = nStatus;
> + pTrHeader->Length = (nStatus == SDCSPI_STATUS_SUCCESS)?(31 +
> (cib_info->spi_number_of_device * 12)):0;
> + TrLength = sizeof(SPI_HEADER);
Perhaps reorder the patch submission to define the structs first.
This can't compile as SPI_HEADER isn't defined
SPI_HEADER and PSPI_HEADER should use not use typedefs and use the typical
struct spi_header
and
struct spi_header *
> + if (pTrHeader->ResponseStatus == SDCSPI_STATUS_SUCCESS)
Hungarian isn't generally used in the kernel.
CamelCase isn't generally used either.
> + {
> + memcpy([TrLength], spi_chl->info.model_name, 16);
> + TrLength += 16;
> + TrBuff[TrLength++] = spi_chl->info.bus_number;
> + TrBuff[TrLength++] = spi_chl->info.dev_number;
> + TrBuff[TrLength++] = spi_chl->info.line;
> + TrBuff[TrLength++] = (unsigned char)((Address & 0xff00) >>
> 24);
> + TrBuff[TrLength++] = (unsigned char)((Address & 0x00ff) >>
> 16);
> + TrBuff[TrLength++] = (unsigned char)((Address & 0xff00) >>
> 8);
> + TrBuff[TrLength++] = (unsigned char)((Address & 0x00ff));
> + TrBuff[TrLength++] = (unsigned char)(spi_chl->info.irq);
You might consider using a single char * and increment that and not use
TrLength at all and use p - TrBuff when necessary.
*p++ = spi_chl->info.bus_number;
*p++ = spi_chl->info.dev_number;
...
> + TrBuff[TrLength++] = (unsigned
> char)((cib_info->spi_significand_of_clock & 0xff00) >> 24);
> + TrBuff[TrLength++] = (unsigned
> char)((cib_info->spi_significand_of_clock & 0x00ff) >> 16);
> + TrBuff[TrLength++] = (unsigned
> char)((cib_info->spi_significand_of_clock & 0xff00) >> 8);
> + TrBuff[TrLength++] = (unsigned
> char)((cib_info->spi_significand_of_clock & 0x00ff));
Perhaps
put_unaligned_le32(cib_info->spi_significant_of_clock, p);
p += 4;
etc...
[PATCH] checkpatch: Warn when missing newline in return sysfs_emit() formats
return sysfs_emit() uses should include a newline.
Suggest adding a newline when one is missing.
Add one using --fix too.
Signed-off-by: Joe Perches
---
scripts/checkpatch.pl | 11 +++
1 file changed, 11 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d67146d0b33c..4dbda85fd7e5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7198,6 +7198,17 @@ sub process {
"Using $1 should generally have parentheses around
the comparison\n" . $herecurr);
}
+# return sysfs_emit(foo, fmt, ...) fmt without newline
+ if ($line =~
/\breturn\s+sysfs_emit\s*\(\s*$FuncArg\s*,\s*($String)/ &&
+ substr($rawline, $-[6], $+[6] - $-[6]) !~ /\\n"$/) {
+ my $offset = $+[6] - 1;
+ if (WARN("SYSFS_EMIT",
+"return sysfs_emit(...) formats should include
a terminating newline\n" . $herecurr) &&
+ $fix) {
+ substr($fixed[$fixlinenr], $offset, 0) = '\\n';
+ }
+ }
+
# nested likely/unlikely calls
if ($line =~
/\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
WARN("LIKELY_MISUSE",
Re: [PATCH] mtd: intel-spi: add is_protected and is_bios_locked knobs
On Tue, 2021-03-30 at 18:54 +0300, Tomas Winkler wrote:
> From: Tamar Mashiah
[]
> the region protection status is exposed via sysfs file
> as the manufacturing will need the both files in order to validate
> that the device is properly sealed.
[]
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi.c
> b/drivers/mtd/spi-nor/controllers/intel-spi.c
[]
> +static ssize_t intel_spi_is_protected_show(struct device *dev,
> +struct device_attribute *attr, char
> *buf)
> +{
> + struct intel_spi *ispi = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d", ispi->is_protected);
These should also include a newline in the format. i.e.:
return sysfs_emit(buf, "%d\n", ispi->is_protected);
> +static ssize_t intel_spi_bios_lock_show(struct device *dev,
> + struct device_attribute *attr, char
> *buf)
> +{
> + struct intel_spi *ispi = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d", ispi->is_bios_locked);
etc...
Re: [PATCH v2 3/5] crypto: hisilicon/sgl - add some dfx logs
On Tue, 2021-03-30 at 15:39 +0800, Kai Ye wrote:
> Add some dfx logs in some abnormal exit situations.
[]
> diff --git a/drivers/crypto/hisilicon/sgl.c b/drivers/crypto/hisilicon/sgl.c
[]
> @@ -87,8 +87,10 @@ struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct
> device *dev,
> block[i].sgl = dma_alloc_coherent(dev, block_size,
> [i].sgl_dma,
> GFP_KERNEL);
> - if (!block[i].sgl)
> + if (!block[i].sgl) {
> + dev_err(dev, "Fail to allocate hw SG buffer!\n");
This doesn't seem useful as dma_alloc_coherent does a dump_stack
by default on OOM.
Re: [PATCH v2 1/5] crypto: hisilicon/sgl - fixup coding style
On Tue, 2021-03-30 at 15:39 +0800, Kai Ye wrote:
> use a macro replace of a magic number.
Given the use of 32 in the same test, this
seems more obfuscating that useful to me.
> diff --git a/drivers/crypto/hisilicon/sgl.c b/drivers/crypto/hisilicon/sgl.c
[]
> @@ -9,6 +9,7 @@
> #define HISI_ACC_SGL_NR_MAX 256
> #define HISI_ACC_SGL_ALIGN_SIZE 64
> #define HISI_ACC_MEM_BLOCK_NR5
> +#define HISI_ACC_BLOCK_SIZE_MAX_SHIFT31
>
>
> struct acc_hw_sge {
> dma_addr_t buf;
> @@ -67,7 +68,8 @@ struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct
> device *dev,
> sgl_size = sizeof(struct acc_hw_sge) * sge_nr +
> sizeof(struct hisi_acc_hw_sgl);
> block_size = 1 << (PAGE_SHIFT + MAX_ORDER <= 32 ?
> -PAGE_SHIFT + MAX_ORDER - 1 : 31);
> +PAGE_SHIFT + MAX_ORDER - 1 :
> +HISI_ACC_BLOCK_SIZE_MAX_SHIFT);
> sgl_num_per_block = block_size / sgl_size;
> block_num = count / sgl_num_per_block;
> remain_sgl = count % sgl_num_per_block;
Re: [PATCH 2/2] gpiolib: Allow drivers to return EOPNOTSUPP from config
On Mon, 2021-03-29 at 14:59 +0300, Andy Shevchenko wrote:
> On Mon, Mar 29, 2021 at 2:43 PM Matti Vaittinen
> wrote:
> >
> > The checkpacth instructs to switch from ENOSUPP to EOPNOTSUPP.
> > > WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP
> >
> > Make the gpiolib allow drivers to return both so driver developers
> > can avoid one of the checkpatch complaints.
>
> Internally we are fine to use the ENOTSUPP.
> Checkpatch false positives there.
>
> I doubt we need this change. Rather checkpatch should rephrase this to
> point out that this is only applicable to _user-visible_ error path.
> Cc'ed Joe.
Adding CC for Jakub Kicinski who added that particular rule/test.
And the output message report of the rule is merely a suggestion indicating
a preference. It's always up to an individual to accept/reject.
At best, perhaps wordsmithing the checkpatch message might be an OK option.
+# ENOTSUPP is not a standard error code and should be avoided in new patches.
+# Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP.
+# Similarly to ENOSYS warning a small number of false positives is expected.
+ if (!$file && $line =~ /\bENOTSUPP\b/) {
+ if (WARN("ENOTSUPP",
+"ENOTSUPP is not a SUSV4 error code, prefer
EOPNOTSUPP\n" . $herecurr) &&
+ $fix) {
+ $fixed[$fixlinenr] =~
s/\bENOTSUPP\b/EOPNOTSUPP/;
+ }
+ }
+
Re: [PATCH] sched,psi: fix typo in comment
On Mon, 2021-03-29 at 09:35 +0200, Peter Zijlstra wrote: > On Sat, Mar 27, 2021 at 08:46:10PM +0800, Xie XiuQi wrote: > > s/exceution/execution/ > > s/possibe/possible/ > > s/manupulations/manipulations/ > > > > Signed-off-by: Xie XiuQi > > Xie, if you'd have bothered to check the development tree of the code > you're patching, you'd have found it's long since fixed. March 18, 2021 doesn't seem a long time ago to me. commit 3b03706fa621ce31a3e9ef6307020fde4e6aae16 Author: Ingo Molnar Date: Thu Mar 18 13:38:50 2021 +0100 sched: Fix various typos > I'm getting sick and tired of people wasting bandwidth with this > nonsense. This seems more like a large overreaction IMO.
Re: [PATCH] kconfig: nconf: stop endless search-up loops
On Sun, 2021-03-28 at 11:27 +0200, Mihai Moldovan wrote:
> * On 3/27/21 11:26 PM, Randy Dunlap wrote:
> > There is a test for it in checkpatch.pl but I also used checkpatch.pl
> > without it complaining, so I don't know what it takes to make the script
> > complain.
> >
> > if ($lead !~ /(?:$Operators|\.)\s*$/ &&
> > $to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/ &&
> > WARN("CONSTANT_COMPARISON",
> > "Comparisons should place the constant on the
> > right side of the test\n" . $herecurr) &&
>
> There are multiple issues, err, "challenges" there:
> - literal "Constant" instead of "$Constant"
> - the left part is misinterpreted as an operation due to the minus sign
> (arithmetic operator)
> - $Constant is defined as "qr{$Float|$Binary|$Octal|$Hex|$Int}" (which is
> okay), but all these types do not include their negative range.
>
> As far as I can tell, the latter is intentional. Making these types compatible
> with negative values causes a lot of other places to break, so I'm not keen on
> changing this.
>
> The minus sign being misinterpreted as an operator is highly difficult to fix
> in a sane manner. The original intention was to avoid misinterpreting
> expressions like (var - CONSTANT real_op ...) as a constant-on-left expression
> (and, more importantly, to not misfix it when --fix is given).
>
> The general idea is sane and we probably shouldn't change that, but it would
> be good to handle negative values as well.
>
> At first, I was thinking of overriding this detection by checking if the
> leading part matches "(-\s*$", which should only be true for negative values,
> assuming that there is always an opening parenthesis as part of a conditional
> statement/loop (if, while). After playing around with this and composing this
> message for a few hours, it dawned on me that there can easily be free-
> standing forms (for instance as part of for loops or assignment lines), so
> that wouldn't cut it.
>
> It really goes downhill from here.
>
> I assume that false negatives are nuisances due to stylistic errors in the
> code, but false positives actually harmful since a lot of them make code
> review by maintainers very tedious.
>
> So far, minus signs were always part of the leading capture group. I'd
> actually like to have them in the constant capture group instead:
>
> - $line =~
> /^\+(.*)\b($Constant|[A-Z_][A-Z0-9_]*)\s*($Compare)\s*($LvalOrFunc)/) {
> + $line =~
> /^\+(.*)(-?\s*$Constant|[A-Z_][A-Z0-9_]*)\s*($Compare)\s*($LvalOrFunc)/) {
>
> With that sorted, the next best thing I could come up with to weed out
> preceding variables was this (which shouldn't influence non-negative
> constants):
>
> - if ($lead !~ /(?:$Operators|\.)\s*$/ &&
> + if ($lead !~ /(?:$Operators|\.|[a-z])\s*$/ &&
>
>
> There still are a lot of expressions that won't match this, like
> "-1 + 0 == var" (i.e., "CONSTANT CONSTANT ...") or
> constellations like a simple "(CONSTANT) ..." (e.g.,
> "(1) == var").
>
> This is all fuzzy and getting this right would involve moving away from
> trying to make sense of C code with regular expressions in Perl, but actually
> parsing it to extract the semantics. Not exactly something I'd like to do...
>
> Thoughts on my workaround for this issue?
Making $Constant not include negative values was very intentional.
> Did I miss anything crucial or introduce a new bug inadvertently?
Not as far as I can tell from a trivial read.
My best guess as to what is best or necessary to do is nothing.
checkpatch isn't a real parser and never will be.
It can miss stuff. It's imperfect. It doesn't bother me.
Re: [PATCH] kconfig: nconf: stop endless search-up loops
On Sat, 2021-03-27 at 15:26 -0700, Randy Dunlap wrote:
> On 3/27/21 3:12 PM, Mihai Moldovan wrote:
> > * On 3/27/21 4:58 PM, Randy Dunlap wrote:
> > > On 3/27/21 5:01 AM, Mihai Moldovan wrote:
> > > > + if ((-1 == index) && (index == match_start))
> > >
> > > checkpatch doesn't complain about this (and I wonder how it's missed), but
> > > kernel style is (mostly) "constant goes on right hand side of comparison",
> > > so
> > > if ((index == -1) &&
> >
> > I can naturally send a V2 with that swapped.
> >
> > To my rationale: I made sure to use checkpatch, saw that it was accepted and
> > even went for a quick git grep -- '-1 ==', which likewise returned enough
> > results for me to call this consistent with the current code style.
> >
> > Maybe those matches were just frowned-upon, but forgotten-to-be-critized
> > examples of this pattern being used.
>
> There is a test for it in checkpatch.pl but I also used checkpatch.pl
> without it complaining, so I don't know what it takes to make the script
> complain.
>
> if ($lead !~ /(?:$Operators|\.)\s*$/ &&
> $to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/ &&
> WARN("CONSTANT_COMPARISON",
>"Comparisons should place the constant on the
> right side of the test\n" . $herecurr) &&
Negative values aren't parsed well by the silly script as checkpatch
isn't a real parser.
Basically, checkpatch only recognizes positive ints as constants.
So it doesn't recognize uses like:
a * -5 > b
It doesn't parse -5 as a negative constant.
Though here it does seem the line with
$to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/ &&
should be
$to !~ /^(?:$Constant|[A-Z_][A-Z0-9_]*)$/ &&
You are welcome to try to improve checkpatch, but it seems non-trivial
and a relatively uncommon use in the kernel, so I won't.
Most all of the existing uses seem to be in drivers/scsi/pm8001/pm8001_hwi.c
$ git grep -P 'if\s*\(\s*\-\d+\s*(?:<=|>=|==|<|>)' -- '*.[ch]'
drivers/media/i2c/msp3400-driver.c: if (-1 == scarts[out][in + 1])
drivers/media/pci/bt8xx/bttv-driver.c: if (-1 == formats[i].fourcc)
drivers/media/pci/saa7134/saa7134-tvaudio.c:if (-1 == secondary)
drivers/media/pci/saa7146/mxb.c:if (-1 ==
mxb_saa7740_init[i].length)
drivers/media/usb/s2255/s2255drv.c: if (-1 == formats[i].fourcc)
drivers/net/ieee802154/mrf24j40.c: } else if (-1000 >= mbm && mbm > -2000)
{
drivers/net/ieee802154/mrf24j40.c: } else if (-2000 >= mbm && mbm > -3000)
{
drivers/net/ieee802154/mrf24j40.c: } else if (-3000 >= mbm && mbm > -4000)
{
drivers/net/wireless/broadcom/brcm80211/brcmsmac/phy/phy_lcn.c: if (-1 ==
*gain_index) {
drivers/parisc/eisa_enumerator.c: if (-1==init_slot(i+1, es)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha, GSM_SM_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == check_fw_ready(pm8001_ha)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha, GSM_SM_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha, RB6_ACCESS_REG)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
MBIC_AAP1_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
MBIC_IOP_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
GSM_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
GPIO_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
SPC_TOP_LEVEL_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
GSM_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
SPC_TOP_LEVEL_ADDR_BASE)) {
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 ==
pm80xx_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm8001_hwi.c: if (-1 == pm8001_bar4_shift(pm8001_ha,
0))
drivers/scsi/pm8001/pm8001_sas.c: if (-1 ==
pm8001_bar4_shift(pm8001_ha,
drivers/scsi/pm8001/pm80xx_hwi.c: if (-1 == check_fw_ready(pm8001_ha)) {
drivers/scsi/pm8001/pm80xx_hwi.c: if (-1 == check_fw_ready(pm8001_ha)) {
drivers/scsi/scsi_debug.c: if (-1 == res)
drivers/scsi/scsi_debug.c: if (-1 == ret) {
Re: [PATCH v2 09/20] staging: rtl8723bs: put parentheses on macros with complex values in include/rtw_debug.h
On Sat, 2021-03-27 at 15:24 +0100, Fabio Aiuto wrote:
> fix the following checkpatch warning:
>
> ERROR: Macros starting with if should be enclosed by a
> do - while loop to avoid possible if/else logic defects
> + #define RT_PRINT_DATA(_Comp, _Level,
> _TitleString, _HexData, _HexDataLen)\
>
> Signed-off-by: Fabio Aiuto
It's good to use checkpatch as a guide to improve code, but this
particular code is just a mess to begin with and it makes a
complete mess of the the dmesg log if it's actually enabled.
Try substituting print_hex_dump_debug for this instead.
> ---
> drivers/staging/rtl8723bs/include/rtw_debug.h | 28 ++-
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/rtl8723bs/include/rtw_debug.h
> b/drivers/staging/rtl8723bs/include/rtw_debug.h
> index d1c557818305..b00f8a6c4312 100644
> --- a/drivers/staging/rtl8723bs/include/rtw_debug.h
> +++ b/drivers/staging/rtl8723bs/include/rtw_debug.h
> @@ -236,19 +236,21 @@
> #if defined(_dbgdump)
> #undef RT_PRINT_DATA
> #define RT_PRINT_DATA(_Comp, _Level, _TitleString, _HexData,
> _HexDataLen) \
> - if (((_Comp) & GlobalDebugComponents) && (_Level <=
> GlobalDebugLevel)) \
> - {
> \
> - int __i;
> \
> - u8 *ptr = (u8 *)_HexData;
> \
> - _dbgdump("%s", DRIVER_PREFIX);
> \
> - _dbgdump(_TitleString);
> \
> - for (__i = 0; __i < (int)_HexDataLen; __i++)
> \
> - {
> \
> - _dbgdump("%02X%s", ptr[__i], (((__i + 1) % 4)
> == 0)?" ":" "); \
> - if (((__i + 1) % 16) == 0) _dbgdump("\n");
> \
> - }
> \
> - _dbgdump("\n");
> \
> - }
> + do { \
> + if (((_Comp) & GlobalDebugComponents) && (_Level <=
> GlobalDebugLevel)) \
> + {
> \
> + int __i;
> \
> + u8 *ptr = (u8 *)_HexData;
> \
> + _dbgdump("%s", DRIVER_PREFIX);
> \
> + _dbgdump(_TitleString);
> \
> + for (__i = 0; __i < (int)_HexDataLen; __i++)
> \
> + {
> \
> + _dbgdump("%02X%s", ptr[__i], (((__i +
> 1) % 4) == 0)?" ":" "); \
> + if (((__i + 1) % 16) == 0)
> _dbgdump("\n"); \
> + }
> \
> + _dbgdump("\n");
> \
> + } \
> + } while (0)
> #endif /* defined(_dbgdump) */
> #endif /* DEBUG_RTL871X */
>
>
Re: [PATCH v2 06/15] ACPI: LPSS: fix some coding style issues
On Sat, 2021-03-27 at 10:19 +0200, Andy Shevchenko wrote: > On Saturday, March 27, 2021, Xiaofei Tan wrote: > > > Fix some coding style issues reported by checkpatch.pl, including > > following types: > > > > WARNING: simple_strtol is obsolete, use kstrtol instead > > > And one more thing, the above message is bogus. Read what the comments in > the code says about use cases for simple_*() vs. kstrto*() ones. > > Joe? This check and message is nearly 10 years old and was appropriate for when it was implemented. kernel.h currently has: * Use these functions if and only if you cannot use kstrto, because So the message could be changed to something like: WARNING: simple_strtol should be used only when kstrtol can not be used.
Re: [PATCH 2/2] streamline_config.pl: Add softtabstop=4 for vim users
On Thu, 2021-03-25 at 09:50 -0400, Steven Rostedt wrote: > On Thu, 25 Mar 2021 15:20:13 +0900 Masahiro Yamada > wrote: > > > > The root cause of inconsistency is that > > you mix up space-indentation and tab-indentation. > > I do not know if it is a standard way either. > > This is the default way emacs has edited perl files for as long as I can > remember (back to 1996). It became my standard of editing perl files just > because of that. For everything else, I use tabs. [] > > For example, scripts/checkpatch.pl uses only tabs, > > which I think is more robust. > > Probably because Joe probably uses vim ;-) I generally use emacs. Maybe Andy Whitcroft uses vim. For checkpatch.pl I just followed Andy's style. get_maintainer.pl uses the 4 spaces then 1 tab style like Steven uses. perl code can be pretty long left to right so using smaller indentation seems useful there.
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 23:36 +0100, Rasmus Villemoes wrote: > On 24/03/2021 23.18, Joe Perches wrote: > > There's no silly game here. %pe would either print a string or a value. > > A hashed value, that is, never the raw value. There is value in printing the raw value. As discussed, it can simplify the code. The worry about exposing a ptr value is IMO overstated. It's trivial to inspect the uses and _all_ %p uses need inspection and validation at acceptance anyway.
Re: [PATCH] Fix fnic driver to remove bogus ratelimit messages.
On Tue, 2021-03-23 at 10:27 -0700, ldun...@suse.com wrote:
> From: Lee Duncan
>
> Commit b43abcbbd5b1 ("scsi: fnic: Ratelimit printks to avoid
> looding when vlan is not set by the switch.i") added
> printk_ratelimit() in front of a couple of debug-mode
> messages, to reduce logging overrun when debugging the
> driver. The code:
>
> > if (printk_ratelimit())
> > FNIC_FCS_DBG(KERN_DEBUG, fnic->lport->host,
> > "Start VLAN Discovery\n");
>
> ends up calling printk_ratelimit() quite often, triggering
> many kernel messages about callbacks being surpressed.
>
> The fix is to decompose FNIC_FCS_DBG(), then change the order
> of checks so that printk_ratelimit() is only called if
> driver debugging is enabled.
Please make sure to cc the fnic maintainers when submitting patches
to their drivers.
$ ./scripts/get_maintainer.pl drivers/scsi/fnic/
Satish Kharat (supporter:CISCO FCOE HBA DRIVER)
Sesidhar Baddela (supporter:CISCO FCOE HBA DRIVER)
Karan Tilak Kumar (supporter:CISCO FCOE HBA DRIVER)
"James E.J. Bottomley" (maintainer:SCSI SUBSYSTEM)
"Martin K. Petersen" (maintainer:SCSI SUBSYSTEM)
linux-s...@vger.kernel.org (open list:CISCO FCOE HBA DRIVER)
linux-kernel@vger.kernel.org (open list)
My preference would be to rewrite the FNIC__DBG macros to use
a single fnic_dbg macro and add a new fnic_dbg_ratelimited macro.
Something like the below:
This converts a few uses at KERN_INFO to KERN_DEBUG, only uses fnic
and adds a macro concatenation instead of multiple macros.
Miscellanea:
o Add missing newlines
o Coalesce formats
o Align arguments
---
compile tested only
drivers/scsi/fnic/fnic.h | 45 +++
drivers/scsi/fnic/fnic_fcs.c | 92 +-
drivers/scsi/fnic/fnic_isr.c | 9 +-
drivers/scsi/fnic/fnic_main.c | 9 +-
drivers/scsi/fnic/fnic_scsi.c | 280 --
5 files changed, 162 insertions(+), 273 deletions(-)
diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
index 69f373b53132..f12cd6ed9296 100644
--- a/drivers/scsi/fnic/fnic.h
+++ b/drivers/scsi/fnic/fnic.h
@@ -130,36 +130,27 @@
extern unsigned int fnic_log_level;
extern unsigned int io_completions;
-#define FNIC_MAIN_LOGGING 0x01
-#define FNIC_FCS_LOGGING 0x02
-#define FNIC_SCSI_LOGGING 0x04
-#define FNIC_ISR_LOGGING 0x08
-
-#define FNIC_CHECK_LOGGING(LEVEL, CMD) \
-do { \
- if (unlikely(fnic_log_level & LEVEL)) \
- do {\
- CMD;\
- } while (0);\
+#define FNIC_DBG_MAIN 0x01
+#define FNIC_DBG_FCS 0x02
+#define FNIC_DBG_SCSI 0x04
+#define FNIC_DBG_ISR 0x08
+
+#define fnic_dbg(fnic, TYPE, fmt, ...) \
+do { \
+ if (unlikely(fnic_log_level & FNIC_DBG_##TYPE)) \
+ shost_printk(KERN_DEBUG, (fnic)->lport->host, \
+fmt, ##__VA_ARGS__); \
} while (0)
-#define FNIC_MAIN_DBG(kern_level, host, fmt, args...) \
- FNIC_CHECK_LOGGING(FNIC_MAIN_LOGGING, \
-shost_printk(kern_level, host, fmt, ##args);)
-
-#define FNIC_FCS_DBG(kern_level, host, fmt, args...) \
- FNIC_CHECK_LOGGING(FNIC_FCS_LOGGING,\
-shost_printk(kern_level, host, fmt, ##args);)
-
-#define FNIC_SCSI_DBG(kern_level, host, fmt, args...) \
- FNIC_CHECK_LOGGING(FNIC_SCSI_LOGGING, \
-shost_printk(kern_level, host, fmt, ##args);)
-
-#define FNIC_ISR_DBG(kern_level, host, fmt, args...) \
- FNIC_CHECK_LOGGING(FNIC_ISR_LOGGING,\
-shost_printk(kern_level, host, fmt, ##args);)
+#define fnic_dbg_ratelimited(fnic, TYPE, fmt, ...) \
+do { \
+ if (unlikely(fnic_log_level & FNIC_DBG_##TYPE) && \
+ printk_ratelimit()) \
+ shost_printk(KERN_DEBUG, (fnic)->lport->host, \
+fmt, ##__VA_ARGS__); \
+} while (0)
-#define FNIC_MAIN_NOTE(kern_level, host, fmt, args...) \
+#define FNIC_MAIN_NOTE(kern_level, host, fmt, args...) \
shost_printk(kern_level, host, fmt, ##args)
extern const char *fnic_state_str[];
diff --git a/drivers/scsi/fnic/fnic_fcs.c b/drivers/scsi/fnic/fnic_fcs.c
index 881c4823d7e2..81eb278ea025 100644
--- a/drivers/scsi/fnic/fnic_fcs.c
+++ b/drivers/scsi/fnic/fnic_fcs.c
@@ -75,9 +75,8 @@ void fnic_handle_link(struct work_struct
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 22:27 +0100, Rasmus Villemoes wrote:
> On 24/03/2021 20.24, Joe Perches wrote:
> > On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
> > > On 24/03/2021 18.20, Joe Perches wrote:
> > >
> > > >
> > > > Maybe it's better to output non PTR_ERR %pe uses as decimal so this
> > > > sort of code would work.
> > >
> > > No, because that would leak the pointer value when somebody has
> > > accidentally passed a real kernel pointer to %pe.
> >
> > I think it's not really an issue.
> >
> > _All_ code that uses %p extensions need inspection anyway.
>
> There are now a bunch of sanity checks in place that catch e.g. an
> ERR_PTR passed to an extension that would derefence the pointer;
> enforcing that only ERR_PTRs are passed to %pe (or falling back to %p)
> is another of those safeguards.
>
> > It's already possible to intentionally 'leak' the ptr value
> > by using %pe, -ptr so I think that's not really an issue.
> >
>
> Huh, what? I assume -ptr is shorthand for (void*)-(unsigned long)ptr.
> How would that leak the value if ptr is an ordinary kernel pointer?
> That's not an ERR_PTR unless (unsigned long)ptr is < 4095 or so.
You are confusing ERR_PTR with IS_ERR
ERR_PTR is just
include/linux/err.h:static inline void * __must_check ERR_PTR(long error)
include/linux/err.h-{
include/linux/err.h-return (void *) error;
include/linux/err.h-}f
> If you want to print the pointer value just do %px. No need for silly
> games.
There's no silly game here. %pe would either print a string or a value.
It already does that in 2 cases.
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
> On 24/03/2021 18.20, Joe Perches wrote:
> > On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
> > > On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
> > > > On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
> > > []
> > > > > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c
> > > > > > b/drivers/gpu/drm/imx/imx-ldb.c
> > > > > []
> > > > > > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> > > > > > drm_encoder *encoder)
> > > > > > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > > > > > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child,
> > > > > > encoder);
> > > > > >
> > > > > > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > > > > > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > > > > + __func__, ERR_PTR(mux));
> > > > >
> > > > > This does not compile without warnings.
> > > > >
> > > > > drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> > > > > drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects
> > > > > argument of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> > > > > 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > > > | ^~
> > > > >
> > > > > If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> > > > > is converting an int a void * to decode the error type and
> > > > > emit it as a string.
> > > >
> > > > Sorry about that.
> > > >
> > > > I decided against using ERR_PTR() in order to also check for
> > > > positive array overflow, but the version I tested was different from
> > > > the version I sent.
> > > >
> > > > v3 coming.
> > >
> > > Thanks. No worries.
> > >
> > > Up to you, vsprintf would emit the positive mux as a funky hashed
> > > hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
> > > perhaps %d without the ERR_PTR use makes the most sense.
> > >
>
> >
> > Maybe it's better to output non PTR_ERR %pe uses as decimal so this
> > sort of code would work.
>
> No, because that would leak the pointer value when somebody has
> accidentally passed a real kernel pointer to %pe.
I think it's not really an issue.
_All_ code that uses %p extensions need inspection anyway.
It's already possible to intentionally 'leak' the ptr value
by using %pe, -ptr so I think that's not really an issue.
>
> If the code wants a cute -EFOO string explaining what's wrong, what
> about "%pe", ERR_PTR(mux < 0 : mux : -ERANGE)? Or two separate error
> messages
>
> if (mux < 0)
> ...
> else if (mux >= ARRAY_SIZE())
> ...
Multiple tests, more unnecessary code, multiple format strings, etc...
[RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
> On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
> > On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
> []
> > > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c
> > > > b/drivers/gpu/drm/imx/imx-ldb.c
> > > []
> > > > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> > > > drm_encoder *encoder)
> > > > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > > > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child,
> > > > encoder);
> > > >
> > > > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > > > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > > + __func__, ERR_PTR(mux));
> > >
> > > This does not compile without warnings.
> > >
> > > drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> > > drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects
> > > argument of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> > > 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > | ^~
> > >
> > > If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> > > is converting an int a void * to decode the error type and
> > > emit it as a string.
> >
> > Sorry about that.
> >
> > I decided against using ERR_PTR() in order to also check for
> > positive array overflow, but the version I tested was different from
> > the version I sent.
> >
> > v3 coming.
>
> Thanks. No worries.
>
> Up to you, vsprintf would emit the positive mux as a funky hashed
> hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
> perhaps %d without the ERR_PTR use makes the most sense.
>
>
Maybe it's better to output non PTR_ERR %pe uses as decimal so this
sort of code would work.
---
lib/vsprintf.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3600db686fa4..debdd1c62038 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -619,19 +619,23 @@ static char *string_nocheck(char *buf, char *end, const
char *s,
return widen_string(buf, len, end, spec);
}
-static char *err_ptr(char *buf, char *end, void *ptr,
-struct printf_spec spec)
+static noinline_for_stack
+char *err_ptr(char *buf, char *end, void *ptr, struct printf_spec spec)
{
int err = PTR_ERR(ptr);
- const char *sym = errname(err);
- if (sym)
- return string_nocheck(buf, end, sym, spec);
+ if (IS_ERR(ptr)) {
+ const char *sym = errname(err);
+
+ if (sym)
+ return string_nocheck(buf, end, sym, spec);
+ }
/*
-* Somebody passed ERR_PTR(-1234) or some other non-existing
-* Efoo - or perhaps CONFIG_SYMBOLIC_ERRNAME=n. Fall back to
-* printing it as its decimal representation.
+* Somebody passed ERR_PTR(-1234) or some other non-existing -E
+* or perhaps CONFIG_SYMBOLIC_ERRNAME=n
+* or perhaps a positive number like an array index
+* Fall back to printing it as its decimal representation.
*/
spec.flags |= SIGN;
spec.base = 10;
@@ -2407,9 +2411,7 @@ char *pointer(const char *fmt, char *buf, char *end, void
*ptr,
case 'x':
return pointer_string(buf, end, ptr, spec);
case 'e':
- /* %pe with a non-ERR_PTR gets treated as plain %p */
- if (!IS_ERR(ptr))
- break;
+ /* %pe with a non-ERR_PTR(ptr) gets treated as %ld */
return err_ptr(buf, end, ptr, spec);
case 'u':
case 'k':
---
Re: [PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
> On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
[]
> > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> > []
> > > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> > > drm_encoder *encoder)
> > > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
> > >
> > > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > + __func__, ERR_PTR(mux));
> >
> > This does not compile without warnings.
> >
> > drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> > drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects argument
> > of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> > 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > | ^~
> >
> > If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> > is converting an int a void * to decode the error type and
> > emit it as a string.
>
> Sorry about that.
>
> I decided against using ERR_PTR() in order to also check for
> positive array overflow, but the version I tested was different from
> the version I sent.
>
> v3 coming.
Thanks. No worries.
Up to you, vsprintf would emit the positive mux as a funky hashed
hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
perhaps %d without the ERR_PTR use makes the most sense.
Re: [PATCH] amdgpu: fix gcc -Wrestrict warning
On Tue, 2021-03-23 at 14:04 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> gcc warns about an sprintf() that uses the same buffer as source
> and destination, which is undefined behavior in C99:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c: In function
> 'amdgpu_securedisplay_debugfs_write':
> drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:141:6: error: 'sprintf'
> argument 3 overlaps destination object 'i2c_output' [-Werror=restrict]
> 141 | sprintf(i2c_output, "%s 0x%X", i2c_output,
> | ^~
> 142 |
> securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]);
> |
> ~
> drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:97:7: note: destination
> object referenced by 'restrict'-qualified argument 1 was declared here
> 97 | char i2c_output[256];
> | ^~
>
> Rewrite it to remember the current offset into the buffer instead.
>
> Signed-off-by: Arnd Bergmann
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> index 834440ab9ff7..69d7f6bff5d4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> @@ -136,9 +136,10 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct
> file *f, const char __u
> ret = psp_securedisplay_invoke(psp,
> TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);
> if (!ret) {
> if (securedisplay_cmd->status ==
> TA_SECUREDISPLAY_STATUS__SUCCESS) {
> + int pos = 0;
> memset(i2c_output, 0, sizeof(i2c_output));
> for (i = 0; i <
> TA_SECUREDISPLAY_I2C_BUFFER_SIZE; i++)
> - sprintf(i2c_output, "%s 0x%X",
> i2c_output,
> + pos += sprintf(i2c_output + pos, "
> 0x%X",
>
> securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]);
> dev_info(adev->dev, "SECUREDISPLAY: I2C buffer
> out put is :%s\n", i2c_output);
Perhaps use a hex output like:
---
drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
index 9cf856c94f94..25bb34c72d20 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
@@ -97,13 +97,12 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct
file *f, const char __u
uint32_t op;
int i;
char str[64];
- char i2c_output[256];
int ret;
if (*pos || size > sizeof(str) - 1)
return -EINVAL;
- memset(str, 0, sizeof(str));
+ memset(str, 0, sizeof(str));
ret = copy_from_user(str, buf, size);
if (ret)
return -EFAULT;
@@ -139,11 +138,9 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct
file *f, const char __u
ret = psp_securedisplay_invoke(psp,
TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);
if (!ret) {
if (securedisplay_cmd->status ==
TA_SECUREDISPLAY_STATUS__SUCCESS) {
- memset(i2c_output, 0, sizeof(i2c_output));
- for (i = 0; i <
TA_SECUREDISPLAY_I2C_BUFFER_SIZE; i++)
- sprintf(i2c_output, "%s 0x%X",
i2c_output,
-
securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]);
- dev_info(adev->dev, "SECUREDISPLAY: I2C buffer
out put is :%s\n", i2c_output);
+ dev_info(adev->dev, "SECUREDISPLAY: I2C buffer
output is: %*ph\n",
+(int)TA_SECUREDISPLAY_I2C_BUFFER_SIZE,
+
securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf);
} else {
psp_securedisplay_parse_resp_status(psp,
securedisplay_cmd->status);
}
Re: [PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
On Wed, 2021-03-24 at 13:17 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> When CONFIG_OF is disabled, building with 'make W=1' produces warnings
> about out of bounds array access:
>
> drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_set_clock.constprop':
> drivers/gpu/drm/imx/imx-ldb.c:186:8: error: array subscript -22 is below
> array bounds of 'struct clk *[4]' [-Werror=array-bounds]
>
> Add an error check before the index is used, which helps with the
> warning, as well as any possible other error condition that may be
> triggered at runtime.
>
> The warning could be fixed by adding a Kconfig depedency on CONFIG_OF,
> but Liu Ying points out that the driver may hit the out-of-bounds
> problem at runtime anyway.
>
> Signed-off-by: Arnd Bergmann
> ---
> v2: fix subject line
> expand patch description
> print mux number
> check upper bound as well
[]
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
[]
> @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct drm_encoder
> *encoder)
> int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
>
> + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> + __func__, ERR_PTR(mux));
This does not compile without warnings.
drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects argument of
type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
| ^~
If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
is converting an int a void * to decode the error type and
emit it as a string.
Re: [PATCH] video: mmp: Few typo fixes
On Mon, 2021-03-22 at 12:36 -0700, Randy Dunlap wrote:
> On 3/22/21 6:02 AM, Bhaskar Chowdhury wrote:
> >
> > s/configed/configured/
> > s/registed/registered/
> > s/defintions/definitions/
> >
> > Signed-off-by: Bhaskar Chowdhury
>
> Acked-by: Randy Dunlap
[]
> > diff --git a/include/video/mmp_disp.h b/include/video/mmp_disp.h
> > index 77252cb46361..ea8b4331b7a1 100644
> > --- a/include/video/mmp_disp.h
> > +++ b/include/video/mmp_disp.h
> > @@ -172,7 +172,7 @@ struct mmp_panel {
> > /* use node to register to list */
> > struct list_head node;
> > const char *name;
> > - /* path name used to connect to proper path configed */
> > + /* path name used to connect to proper path configured */
The spelling is now correct, but the word order doesn't make much sense.
> > @@ -291,7 +291,7 @@ static inline int mmp_overlay_set_addr(struct
> > mmp_overlay *overlay,
> > * it defined a common interface that plat driver need to implement
> > */
> > struct mmp_path_info {
> > - /* driver data, set when registed*/
> > + /* driver data, set when registered*/
should have a space before */
Re: [PATCH] drm/imx: fix out of bounds array access warning
On Tue, 2021-03-23 at 14:05 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> When CONFIG_OF is disabled, building with 'make W=1' produces warnings
> about out of bounds array access:
>
> drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_set_clock.constprop':
> drivers/gpu/drm/imx/imx-ldb.c:186:8: error: array subscript -22 is below
> array bounds of 'struct clk *[4]' [-Werror=array-bounds]
>
> Add an error check before the index is used, which helps with the
> warning, as well as any possible other error condition that may be
> triggered at runtime.
[]
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
[]
> @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct drm_encoder
> *encoder)
> int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
>
> + if (mux < 0) {
> + dev_warn(ldb->dev,
> + "%s: invalid mux\n", __func__);
trivia:
Any real reason to make this 2 lines? It fits nicely in 80 chars. Maybe:
dev_warn(ldb->dev, "%s: invalid mux: %d\n", __func__, mux);
or maybe:
dev_warn(ldb->dev, "%s: invalid mux: %pe\n",
__func__, ERR_PTR(mux));
> @@ -255,6 +261,12 @@ imx_ldb_encoder_atomic_mode_set(struct drm_encoder
> *encoder,
[]
> + if (mux < 0) {
> + dev_warn(ldb->dev,
> + "%s: invalid mux\n", __func__);
etc...
Re: CHECKPATCH: missing a warning soon after include files decl -c
On Sat, 2021-03-20 at 11:59 +0100, Greg KH wrote:
> On Sat, Mar 20, 2021 at 11:54:24AM +0100, Fabio Aiuto wrote:
> > Hi,
> >
> > here's an issue in checkpatch.pl
> >
> > $ perl script/checkpatch.pl -f drivers/staging/rtl8723bs/core/rtw_ap.c
> >
> > I get three warning related to an extern declaration
> >
> > WARNING: externs should be avoided in .c files
> > #14: FILE: drivers/staging/rtl8723bs/core/rtw_ap.c:14:
> > +extern unsigned char WMM_OUI[];
> > --
> > WARNING: externs should be avoided in .c files
> > #15: FILE: drivers/staging/rtl8723bs/core/rtw_ap.c:15:
> > +extern unsigned char WPS_OUI[];
> > --
> > WARNING: externs should be avoided in .c files
> > #16: FILE: drivers/staging/rtl8723bs/core/rtw_ap.c:16:
> > +extern unsigned char P2P_OUI[];
> > --
> >
> > but the file checked has 4 extern declaration:
> > -
> > #define _RTW_AP_C_
> >
> > #include
> > #include
> > #include
> >
> > extern unsigned char RTW_WPA_OUI[];
> > extern unsigned char WMM_OUI[];
> > extern unsigned char WPS_OUI[];
> > extern unsigned char P2P_OUI[];
> >
> > void init_mlme_ap_info(struct adapter *padapter)
> > ---
> >
> > If I add a ';' this way:
> >
> > #define _RTW_AP_C_
> >
> > #include
> > #include
> > #include
> > ;
> > extern unsigned char RTW_WPA_OUI[];
> > extern unsigned char WMM_OUI[];
> > extern unsigned char WPS_OUI[];
> > extern unsigned char P2P_OUI[];
> >
> > void init_mlme_ap_info(struct adapter *padapter)
> >
>
> Wait, why would you do the above?
It is rather an ugly hack.
> Don't try to trick a perl script that has a hard time parsing C files,
> try to resolve the original issue here.
>
> And that is that the above definitions should be in a .h file somewhere.
> If you make that move then all should be fine.
Actually, these would seem to be better as one or multiple functions with
local statics or even as static inlines functions in the .h file
$ git grep -w RTW_WPA_OUI drivers/staging/rtl8723bs/core
drivers/staging/rtl8723bs/core/rtw_ap.c:extern unsigned char RTW_WPA_OUI[];
drivers/staging/rtl8723bs/core/rtw_ap.c:if (!memcmp(RTW_WPA_OUI, oui,
4))
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:unsigned char RTW_WPA_OUI[] =
{0x00, 0x50, 0xf2, 0x01};
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c: if
((!memcmp(pIE->data, RTW_WPA_OUI, 4)) ||
drivers/staging/rtl8723bs/core/rtw_wlan_util.c:extern unsigned char
RTW_WPA_OUI[];
drivers/staging/rtl8723bs/core/rtw_wlan_util.c: if
((!memcmp(pIE->data, RTW_WPA_OUI, 4)) && (!memcmp((pIE->data + 12),
WPA_TKIP_CIPHER, 4)))
$ git grep -w WMM_OUI drivers/staging/rtl8723bs/core
drivers/staging/rtl8723bs/core/rtw_ap.c:extern unsigned char WMM_OUI[];
drivers/staging/rtl8723bs/core/rtw_ap.c:else if (!memcmp(WMM_OUI, oui,
4))
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:unsigned char WMM_OUI[] = {0x00,
0x50, 0xf2, 0x02};
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:
(!memcmp(pIE->data, WMM_OUI, 4)) ||
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c: if
(!memcmp(pIE->data, WMM_OUI, 4))
drivers/staging/rtl8723bs/include/rtw_mlme_ext.h:extern unsigned char WMM_OUI[];
$ git grep -w WPS_OUI drivers/staging/rtl8723bs/core
drivers/staging/rtl8723bs/core/rtw_ap.c:extern unsigned char WPS_OUI[];
drivers/staging/rtl8723bs/core/rtw_ap.c:else if (!memcmp(WPS_OUI, oui,
4))
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:unsigned char WPS_OUI[] = {0x00,
0x50, 0xf2, 0x04};
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:
(!memcmp(pIE->data, WPS_OUI, 4))) {
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c: if
((!padapter->registrypriv.wifi_spec) && (!memcmp(pIE->data, WPS_OUI, 4))) {
$ git grep -w P2P_OUI drivers/staging/rtl8723bs/core
drivers/staging/rtl8723bs/core/rtw_ap.c:extern unsigned char P2P_OUI[];
drivers/staging/rtl8723bs/core/rtw_ap.c:else if (!memcmp(P2P_OUI, oui,
4))
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:unsigned char P2P_OUI[] = {0x50,
0x6F, 0x9A, 0x09};
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c: if (!memcmp(frame_body + 2,
P2P_OUI, 4)) {
So maybe something like the below (written in email client, maybe typos)
enum oui_type {
RTW_WPA,
WMM,
WPS,
P2P
};
bool is_oui_type(const u8 *mem, enum oui_type type)
{
static const u8 rtw_wpa[] = {0x00, 0x50, 0xf2, 0x01};
static const u8 wmm[] = {0x00, 0x50, 0xf2, 0x02};
static const u8 wps[] = {0x00, 0x50, 0xf2, 0x04};
static const u8 p2p[] = {0x50, 0x6F, 0x9A, 0x09};
const u8 *oui;
if (type == RTW_WPA)
oui = rtw_wpa;
else if (type == WMM)
oui = wmm;
else if (type == WPS)
oui = wps;
else if (type == P2P)
oui = p2p;
Re: [PATCH] clang-format: Update ColumnLimit
On Fri, 2021-03-19 at 19:48 +0100, Miguel Ojeda wrote: > On Fri, Mar 19, 2021 at 7:45 PM Ansuel Smith wrote: > > > > Sorry, didn't notice that. Considering that checkpatch complains and > > some reviewers actually state that 100 is the new limit, I think it's > > time to update the file. > > IIUC, 80 is still the soft limit, but 100 is now the hard limit. 80 columns is still the strongly preferred limit. >From coding-style.rst: --- The preferred limit on the length of a single line is 80 columns. Statements longer than 80 columns should be broken into sensible chunks, unless exceeding 80 columns significantly increases readability and does not hide information. --- IMO: clang-format is mechanical and, like checkpatch, doesn't have much 'taste'. Ideally, 100 columns would only be used when long length identifiers exist with some mechanism that determines statement complexity. Today it's fairly easy to go beyond 80 columns even if a statement is similar to a = b + c; when identifier lengths are relatively long. There are many existing 25+ character length identifiers, so the trivial statement above if used with all identifiers of 25 characters or more exceeds 80 columns. So for some things, clang-format (and checkpatch) should allow > 80 column lines for trivial statements like the above. It's not a trivial implementation problem though.
Re: [PATCH] drm: Few typo fixes
On Thu, 2021-03-18 at 16:07 +0530, Bhaskar Chowdhury wrote: > s/instatiated/instantiated/ > s/unreference/unreferenced/ []> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c [] > @@ -644,7 +644,7 @@ EXPORT_SYMBOL(drm_property_blob_get); > * @id: id of the blob property > * > * If successful, this takes an additional reference to the blob property. > - * callers need to make sure to eventually unreference the returned property > + * callers need to make sure to eventually unreferenced the returned property I think this is worse.
Re: [Linuxarm] Re: [PATCH 2/3] drivers/perf: convert sysfs scnprintf family to sysfs_emit_at
On Thu, 2021-03-18 at 17:33 +0800, liuqi (BA) wrote:
> On 2021/3/17 22:57, Joe Perches wrote:
> > On Wed, 2021-03-17 at 17:41 +0800, Qi Liu wrote:
> > > Use the generic sysfs_emit_at() function take place of scnprintf()
> > []
> > > diff --git a/drivers/perf/arm-ccn.c b/drivers/perf/arm-ccn.c
> > []
> > > @@ -328,41 +328,37 @@ static ssize_t arm_ccn_pmu_event_show(struct device
> > > *dev,
> > > struct arm_ccn_pmu_event, attr);
> > > ssize_t res;
> > >
> > >
> > >
> > > - res = scnprintf(buf, PAGE_SIZE, "type=0x%x", event->type);
> > > + res = sysfs_emit(buf, "type=0x%x", event->type);
> > > if (event->event)
> > > - res += scnprintf(buf + res, PAGE_SIZE - res, ",event=0x%x",
> > > + res += sysfs_emit_at(buf + res, res, ",event=0x%x",
> > > event->event);
> >
> > sysfs_emit_at should always use buf, not buf + offset.
> > res should be int and is the offset from buf for the output
> >
> > so the form should be similar to
> >
> > int len;
> >
> > len = sysfs_emit(buf, "type=0x%x", event->type);
> > if (event->event) {
> > len += sysfs_emit_at(buf, len, ",event=0x%x", event->event);
> >
> > etc...
> >
> Hi Joe,
>
> I'll fix the use of sysfs_emit_at in next version, thanks.
> But I think it's better to keep the res as ssize_t, as the return value
> of this function is ssize_t.
The 2nd arg of sysfs_emit_at is int.
On 64 bit platforms, ssize_t is 64 bit while int is 32.
If res (or len) is ssize_t, there could be a lot of -Wconversion warnings
like this produced when using make W=
warning: conversion from ‘ssize_t’ {aka ‘long int’} to ‘int’ may change value
[-Wconversion]
262 | len += sysfs_emit_at(buf, len, "\n");
Re: [PATCH 2/3] drivers/perf: convert sysfs scnprintf family to sysfs_emit_at
On Wed, 2021-03-17 at 17:41 +0800, Qi Liu wrote:
> Use the generic sysfs_emit_at() function take place of scnprintf()
[]
> diff --git a/drivers/perf/arm-ccn.c b/drivers/perf/arm-ccn.c
[]
> @@ -328,41 +328,37 @@ static ssize_t arm_ccn_pmu_event_show(struct device
> *dev,
> struct arm_ccn_pmu_event, attr);
> ssize_t res;
>
>
> - res = scnprintf(buf, PAGE_SIZE, "type=0x%x", event->type);
> + res = sysfs_emit(buf, "type=0x%x", event->type);
> if (event->event)
> - res += scnprintf(buf + res, PAGE_SIZE - res, ",event=0x%x",
> + res += sysfs_emit_at(buf + res, res, ",event=0x%x",
> event->event);
sysfs_emit_at should always use buf, not buf + offset.
res should be int and is the offset from buf for the output
so the form should be similar to
int len;
len = sysfs_emit(buf, "type=0x%x", event->type);
if (event->event) {
len += sysfs_emit_at(buf, len, ",event=0x%x", event->event);
etc...
Re: [PATCH 13/13] MAINTAINERS: Add entry for the bitmap API
On Tue, 2021-03-16 at 21:47 -0700, Yury Norov wrote: > [CC Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn] > > On Tue, Mar 16, 2021 at 01:45:51PM +0200, Andy Shevchenko wrote: > > On Mon, Mar 15, 2021 at 06:54:24PM -0700, Yury Norov wrote: > > > Add myself as maintainer for bitmap API and Andy and Rasmus as reviewers. > > > > > > I'm an author of current implementation of lib/find_bit and an active > > > contributor to lib/bitmap. It was spotted that there's no maintainer for > > > bitmap API. I'm willing to maintain it. > > > > > > Signed-off-by: Yury Norov > > > Acked-by: Andy Shevchenko > > > Acked-by: Rasmus Villemoes > > > --- > > > MAINTAINERS | 16 > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index 3dd20015696e..44f94cdd5a20 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -3151,6 +3151,22 @@ F: Documentation/filesystems/bfs.rst > > > F: fs/bfs/ > > > F: include/uapi/linux/bfs_fs.h > > > > > > > > > +BITMAP API > > > +M: Yury Norov > > > +R: Andy Shevchenko > > > +R: Rasmus Villemoes > > > +S: Maintained > > > +F: include/asm-generic/bitops/find.h > > > +F: include/linux/bitmap.h > > > +F: lib/bitmap.c > > > +F: lib/find_bit.c > > > > > +F: lib/find_find_bit_benchmark.c > > > > Does this file exist? > > I guess checkpatch.pl nowadays has a MAINTAINER data base validation. > > No lib/find_find_bit_benchmark.c doesn't exist. It's a typo, it should > be lib/find_bit_benchmark.c. Checkpatch doesn't warn: > > yury:linux$ scripts/checkpatch.pl > 0013-MAINTAINERS-Add-entry-for-the-bitmap-API.patch > total: 0 errors, 0 warnings, 22 lines checked checkpatch does not validate filenames for each patch. checkpatch does have a --self-test=patterns capability that does validate file accessibility.
Re: [PATCH] staging: rtl8723bs/core: add spaces between operators
On Tue, 2021-03-16 at 20:05 +0800, Qiang Ma wrote:
> Add spaces between operators for a better readability
> in function 'rtw_seccalctkipmic'.
Perhaps better would be to refactor it a bit to
follow the comments. Something like:
---
drivers/staging/rtl8723bs/core/rtw_security.c | 24 ++--
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/rtl8723bs/core/rtw_security.c
b/drivers/staging/rtl8723bs/core/rtw_security.c
index a311595deafb..a30e1fa717af 100644
--- a/drivers/staging/rtl8723bs/core/rtw_security.c
+++ b/drivers/staging/rtl8723bs/core/rtw_security.c
@@ -405,30 +405,26 @@ void rtw_secgetmic(struct mic_data *pmicdata, u8 *dst)
void rtw_seccalctkipmic(u8 *key, u8 *header, u8 *data, u32 data_len, u8
*mic_code, u8 pri)
{
-
struct mic_data micdata;
u8 priority[4] = {0x0, 0x0, 0x0, 0x0};
+ int da_offset;
+ int sa_offset;
rtw_secmicsetkey(, key);
priority[0] = pri;
/* Michael MIC pseudo header: DA, SA, 3 x 0, Priority */
- if (header[1]&1) { /* ToDS == 1 */
- rtw_secmicappend(, [16], 6); /* DA */
- if (header[1]&2) /* From Ds == 1 */
- rtw_secmicappend(, [24], 6);
- else
- rtw_secmicappend(, [10], 6);
- } else {/* ToDS == 0 */
- rtw_secmicappend(, [4], 6); /* DA */
- if (header[1]&2) /* From Ds == 1 */
- rtw_secmicappend(, [16], 6);
- else
- rtw_secmicappend(, [10], 6);
+ if (header[1] & 1) { /* ToDS == 1 */
+ da_offset = 16;
+ sa_offset = (header[1] & 2) ? 24 : 10;
+ } else {/* ToDS == 0 */
+ da_offset = 4;
+ sa_offset = (header[1] & 2) ? 16 : 10;
}
+ rtw_secmicappend(, [da_offset], 6); /* DA */
+ rtw_secmicappend(, [sa_offset], 6); /* SA */
rtw_secmicappend(, [0], 4);
-
rtw_secmicappend(, data, data_len);
rtw_secgetmic(, mic_code);
Re: [PATCH v2 5/5] mtd: spi-nor: swp: Drop 'else' after 'return'
On Mon, 2021-03-15 at 11:24 +, tudor.amba...@microchip.com wrote: > On 3/15/21 8:53 AM, Joe Perches wrote: > > On Mon, 2021-03-08 at 11:58 +0530, Pratyush Yadav wrote: > > > On 06/03/21 11:50AM, Tudor Ambarus wrote: > > > > else is not generally useful after a break or return. > > > > > > > > Signed-off-by: Tudor Ambarus > > > > > > Reviewed-by: Pratyush Yadav > > > > > > > I don't think this improves the code. > > > > Generally, checkpatch is a stupid little script. > > > > This code uses a form like: > > if (foo) > > return bar; > > else > > return baz; > > Isn't else redundant? What are the benefits of keeping the else? Visual consistency and it's a widely used style. A long time ago Al Viro wrote: https://lore.kernel.org/lkml/20140925032215.gk7...@zeniv.linux.org.uk/ which resulted in the patch to checkpatch that tries to ignore that style. https://lore.kernel.org/lkml/1411621434.4026.9.camel@joe-AO725/ > > I think better would be to change the code to use temporaries > > and convert the functions to bool. > returning one is wrong indeed, would you submit a patch for the conversion > of the functions to bool? Just a suggestion...
Re: [PATCH v2 5/5] mtd: spi-nor: swp: Drop 'else' after 'return'
On Mon, 2021-03-08 at 11:58 +0530, Pratyush Yadav wrote:
> On 06/03/21 11:50AM, Tudor Ambarus wrote:
> > else is not generally useful after a break or return.
> >
> > Signed-off-by: Tudor Ambarus
>
> Reviewed-by: Pratyush Yadav
>
I don't think this improves the code.
Generally, checkpatch is a stupid little script.
This code uses a form like:
if (foo)
return bar;
else
return baz;
which checkpatch recognizes as OK and so checkpatch does not
emit any warning message, but this code just adds comments
before each return which confuses checkpatch.
I think better would be to change the code to use temporaries
and convert the functions to bool.
Something like:
---
drivers/mtd/spi-nor/core.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 0522304f52fa..e174a2f1d621 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1798,36 +1798,41 @@ static void spi_nor_get_locked_range_sr(struct spi_nor
*nor, u8 sr, loff_t *ofs,
}
/*
- * Return 1 if the entire region is locked (if @locked is true) or unlocked (if
- * @locked is false); 0 otherwise
+ * Return true if the entire region is locked
+ * (if @locked is true) or unlocked (if @locked is false); false otherwise
*/
-static int spi_nor_check_lock_status_sr(struct spi_nor *nor, loff_t ofs,
+static bool spi_nor_check_lock_status_sr(struct spi_nor *nor, loff_t ofs,
uint64_t len, u8 sr, bool locked)
{
loff_t lock_offs;
uint64_t lock_len;
+ uint64_t lock_max;
+ uint64_t ofs_max;
if (!len)
- return 1;
+ return true;
spi_nor_get_locked_range_sr(nor, sr, _offs, _len);
+ lock_max = lock_offs + lock_len;
+ ofs_max = ofs + len;
+
if (locked)
/* Requested range is a sub-range of locked range */
- return (ofs + len <= lock_offs + lock_len) && (ofs >=
lock_offs);
+ return (ofs_max <= lock_max) && (ofs >= lock_offs);
else
/* Requested range does not overlap with locked range */
- return (ofs >= lock_offs + lock_len) || (ofs + len <=
lock_offs);
+ return (ofs >= lock_max) || (ofs_max <= lock_offs);
}
-static int spi_nor_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
- u8 sr)
+static bool spi_nor_is_locked_sr(struct spi_nor *nor, loff_t ofs, uint64_t len,
+u8 sr)
{
return spi_nor_check_lock_status_sr(nor, ofs, len, sr, true);
}
-static int spi_nor_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t
len,
- u8 sr)
+static bool spi_nor_is_unlocked_sr(struct spi_nor *nor, loff_t ofs, uint64_t
len,
+ u8 sr)
{
return spi_nor_check_lock_status_sr(nor, ofs, len, sr, false);
}
Re: [PATCH] staging: rtl8723bs: align comments
On Wed, 2021-03-10 at 20:48 +0300, Dan Carpenter wrote:
> You need to have a space character after the '*'.
Perhaps YA checkpatch test...
---
scripts/checkpatch.pl | 15 +++
1 file changed, 15 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f42e5ba16d9b..0de553d52605 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3876,6 +3876,21 @@ sub process {
}
}
+# Independent comment lines should have a space after the comment initiator
+ if ($line =~ /^\+[ \t]*($;+)/) {#leading comment
+ my $comment = trim(substr($rawline, $-[1], $+[1] -
$-[1]));
+ if ($comment =~ m{^(/\*|\*/|\*|//)(.*)}) {
+ my $init = $1;
+ my $rest = $2;
+ if ($init =~ m{^(?:/\*|\*|//)$} &&
+ $rest ne '' &&
+ $rest !~ /^[\s\*=\-]/) {
+ WARN("COMMENT_STYLE",
+"Comments generally use whitespace
after the comment initiator\n" . $herecurr);
+ }
+ }
+ }
+
# check for missing blank lines after struct/union declarations
# with exceptions for various attributes and macros
if ($prevline =~ /^[\+ ]};?\s*$/ &&
Re: linux-kernel janitorial RFP: Mark static arrays as const
On Sun, 2021-03-07 at 20:14 +0100, Julia Lawall wrote:
>
> On Wed, 3 Mar 2021, Joe Perches wrote:
>
> > On Wed, 2021-03-03 at 10:41 +0100, Rasmus Villemoes wrote:
> > > On 02/03/2021 18.42, Joe Perches wrote:
> > > > Here is a possible opportunity to reduce data usage in the kernel.
> > > >
> > > > $ git grep -P -n
> > > > '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' drivers/ | \
> > > > grep -v __initdata | \
> > > > wc -l
> > > > 3250
> > > >
> > > > Meaning there are ~3000 declarations of arrays with what appears to be
> > > > file static const content that are not marked const.
> > > >
> > > > So there are many static arrays that could be marked const to move the
> > > > compiled object code from data to text minimizing the total amount of
> > > > exposed r/w data.
> > >
> > > You can add const if you like, but it will rarely change the generated
> > > code. gcc is already smart enough to take a static array whose contents
> > > are provably never modified within the TU and put it in .rodata:
> >
> > At least some or perhaps even most of the time, true, but the gcc compiler
> > from v5 through at least v10 seems inconsistent about when it does the
> > appropriate conversion.
> >
> > See the example I posted:
> > https://lore.kernel.org/lkml/6b8b250a06a98ce42120a14824531a8641f5e8aa.ca...@perches.com/
> >
> > It was a randomly chosen source file conversion btw, I had no prior
> > knowledge of whether the text/data use would change.
> >
> > I'm unsure about clang consistently moving static but provably const arrays
> > from data to text. I rarely use clang. At least for v11 it seems to be
> > better though. I didn't try 10.1.
>
> I tried the relevnt drivers in drivers/input/joystick. I got only one
> driver that changed with gcc 9.3, which was
> drivers/input/joystick/analog.c. It actually got larger:
>
> original:
>
> textdata bss dec hex filename
> 22607 10560 320 3348782cf drivers/input/joystick/analog.o
>
> after adding const:
>
> textdata bss dec hex filename
> 22728 10816 320 338648448 drivers/input/joystick/analog.o
>
> This was the only case where bss was not 0, but I don't know if there is a
> connection.
You really need consider using defconfig so whatever object code
does not have tracing/debugging support.
For instance, this code with defconfig and analog joystick:
Original:
$ size drivers/input/joystick/analog.o
textdata bss dec hex filename
8115 261 22486002198 drivers/input/joystick/analog.o
with const:
$ size drivers/input/joystick/analog.o
textdata bss dec hex filename
8179 201 2248604219c drivers/input/joystick/analog.o
Re: linux-kernel janitorial RFP: Mark static arrays as const
On Wed, 2021-03-03 at 10:41 +0100, Rasmus Villemoes wrote:
> On 02/03/2021 18.42, Joe Perches wrote:
> > Here is a possible opportunity to reduce data usage in the kernel.
> >
> > $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]'
> > drivers/ | \
> > grep -v __initdata | \
> > wc -l
> > 3250
> >
> > Meaning there are ~3000 declarations of arrays with what appears to be
> > file static const content that are not marked const.
> >
> > So there are many static arrays that could be marked const to move the
> > compiled object code from data to text minimizing the total amount of
> > exposed r/w data.
>
> You can add const if you like, but it will rarely change the generated
> code. gcc is already smart enough to take a static array whose contents
> are provably never modified within the TU and put it in .rodata:
At least some or perhaps even most of the time, true, but the gcc compiler
from v5 through at least v10 seems inconsistent about when it does the
appropriate conversion.
See the example I posted:
https://lore.kernel.org/lkml/6b8b250a06a98ce42120a14824531a8641f5e8aa.ca...@perches.com/
It was a randomly chosen source file conversion btw, I had no prior
knowledge of whether the text/data use would change.
I'm unsure about clang consistently moving static but provably const arrays
from data to text. I rarely use clang. At least for v11 it seems to be
better though. I didn't try 10.1.
Re: [PATCH v2 08/10] fsdax: Dedup file range to use a compare function
On Fri, 2021-02-26 at 08:20 +0800, Shiyang Ruan wrote:
> With dax we cannot deal with readpage() etc. So, we create a dax
> comparison funciton which is similar with
> vfs_dedupe_file_range_compare().
> And introduce dax_remap_file_range_prep() for filesystem use.
[]
> diff --git a/fs/dax.c b/fs/dax.c
[]
> @@ -1856,3 +1856,54 @@ vm_fault_t dax_finish_sync_fault(struct vm_fault *vmf,l
> return dax_insert_pfn_mkwrite(vmf, pfn, order);
> }
> EXPORT_SYMBOL_GPL(dax_finish_sync_fault);
> +
> +static loff_t dax_range_compare_actor(struct inode *ino1, loff_t pos1,
> + struct inode *ino2, loff_t pos2, loff_t len, void *data,
> + struct iomap *smap, struct iomap *dmap)
> +{
> + void *saddr, *daddr;
> + bool *same = data;
> + int ret;
> +
> + while (len) {
> + if (smap->type == IOMAP_HOLE && dmap->type == IOMAP_HOLE)
> + goto next;
> +
> + if (smap->type == IOMAP_HOLE || dmap->type == IOMAP_HOLE) {
> + *same = false;
> + break;
> + }
> +
> + ret = dax_iomap_direct_access(smap, pos1,
> + ALIGN(pos1 + len, PAGE_SIZE), , NULL);
> + if (ret < 0)
> + return -EIO;
> +
> + ret = dax_iomap_direct_access(dmap, pos2,
> + ALIGN(pos2 + len, PAGE_SIZE), , NULL);
> + if (ret < 0)
> + return -EIO;
> +
> + *same = !memcmp(saddr, daddr, len);
> + if (!*same)
> + break;
> +next:
> + len -= len;
> + }
> +
> + return 0;
> +}
This code looks needlessly complex.
len is never decremented inside the while loop so the while loop
itself looks unnecessary. Is there some missing decrement of len
or some other reason to use a while loop?
Is dax_iomap_direct_access some ugly macro that modifies a hidden len?
Why not remove the while loop and use straightforward code without
unnecessary indentatation?
{
void *saddr;
void *daddr;
bool *same = data;
int ret;
if (!len ||
(smap->type == IOMAP_HOLE && dmap->type == IOMAP_HOLE))
return 0;
if (smap->type == IOMAP_HOLE || dmap->type == IOMAP_HOLE) {
*same = false;
return 0;
}
ret = dax_iomap_direct_access(smap, pos1, ALIGN(pos1 + len, PAGE_SIZE),
, NULL);
if (ret < 0)
return -EIO;
ret = dax_iomap_direct_access(dmap, pos2, ALIGN(pos2 + len, PAGE_SIZE),
, NULL);
if (ret < 0)
return -EIO;
*same = !memcmp(saddr, daddr, len);
return 0;
}
I didn't look at the rest.
Re: [PATCH V3 XRT Alveo 00/18] XRT Alveo driver overview
On Sun, 2021-02-21 at 12:43 -0800, Moritz Fischer wrote: > On Wed, Feb 17, 2021 at 10:40:01PM -0800, Lizhi Hou wrote: > > This is V3 of patch series which adds management physical function driver > > for Xilinx > > Alveo PCIe accelerator cards, > > https://www.xilinx.com/products/boards-and-kits/alveo.html > > This driver is part of Xilinx Runtime (XRT) open source stack. [] > Please fix the indents all across this patchset. Doesn't checkpatch with > --strict complain about this? I glanced at a couple bits of these patches and didn't notice any of what I consider poor indentation style. What indent is wrong here?
Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
On Tue, 2021-03-02 at 22:41 +0100, Julia Lawall wrote:
>
> On Tue, 2 Mar 2021, Joe Perches wrote:
>
> > Here is a possible opportunity to reduce data usage in the kernel.
>
> Does it actually reduce data usage?
Yes, at least for gcc. For instance:
$ gcc --version
gcc (Ubuntu 10.2.0-13ubuntu1) 10.2.0
And with the diff below (x86-64 defconfig with hwmon/pmbus and max20730)
$ diff --git a/drivers/hwmon/pmbus/max20730.c b/drivers/hwmon/pmbus/max20730.c
index 9dd3dd79bc18..b824eab95456 100644
--- a/drivers/hwmon/pmbus/max20730.c
+++ b/drivers/hwmon/pmbus/max20730.c
@@ -434,7 +434,7 @@ static long direct_to_val(u16 w, enum pmbus_sensor_classes >
return d;
}
-static u32 max_current[][5] = {
+static const u32 max_current[][5] = {
[max20710] = { 6200, 8000, 9700, 11600 },
[max20730] = { 13000, 16600, 20100, 23600 },
[max20734] = { 21000, 27000, 32000, 38000 },
$ size drivers/hwmon/pmbus/max20730.o*
textdata bss dec hex filename
9245 256 09501251d drivers/hwmon/pmbus/max20730.o.gcc.new
9149 352 09501251d drivers/hwmon/pmbus/max20730.o.gcc.old
with clang-11 it doesn't seem to make a difference:
$ /usr/bin/clang --version
Ubuntu clang version 11.0.0-2
$ size drivers/hwmon/pmbus/max20730.o*
textdata bss dec hex filename
9166 256 1942324cf
drivers/hwmon/pmbus/max20730.o.clang-11.new
9166 256 1942324cf
drivers/hwmon/pmbus/max20730.o.clang-11.old
linux-kernel janitorial RFP: Mark static arrays as const
Here is a possible opportunity to reduce data usage in the kernel.
$ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]'
drivers/ | \
grep -v __initdata | \
wc -l
3250
Meaning there are ~3000 declarations of arrays with what appears to be
file static const content that are not marked const.
So there are many static arrays that could be marked const to move the
compiled object code from data to text minimizing the total amount of
exposed r/w data.
However, I do not know of a mechanism using coccinelle to determine
whether or not any of these static declarations are ever modified.
So it appears that each instance of these declarations might need
manual inspection.
But for arrays declared inside functions, it's much more likely that
the static declaration without const is done with the intent to modify
the array:
(note the difference in the git grep with a leading '^\s+')
$ git grep -Pn '^\s+static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]'
drivers/ | \
grep -v __initdata | \
wc -l
323
- For instance: (head -10 of the git grep for file statics)
drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32, 16,
8, 4, 2, 1 };
drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {
drivers/accessibility/speakup/main.c:2059:static spkup_hand spkup_handler[] = {
drivers/accessibility/speakup/speakup_acntpc.c:35:static unsigned int
synth_portlist[] = { 0x2a8, 0 };
drivers/accessibility/speakup/speakup_decpc.c:133:static int synth_portlist[] =
{ 0x340, 0x350, 0x240, 0x250, 0 };
drivers/accessibility/speakup/speakup_dectlk.c:110:static int ap_defaults[] =
{122, 89, 155, 110, 208, 240, 200, 106, 306};
drivers/accessibility/speakup/speakup_dectlk.c:111:static int g5_defaults[] =
{86, 81, 86, 84, 81, 80, 83, 83, 73};
drivers/accessibility/speakup/speakup_dtlk.c:34:static unsigned int
synth_portlist[] = {
drivers/accessibility/speakup/speakup_keypc.c:34:static unsigned int
synth_portlist[] = { 0x2a8, 0 };
drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {
For drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32,
16, 8, 4, 2, 1 };
masks is only used in static function say_key and should be const and
perhaps the declaration might be better moved into that function.
For drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {
funcvals is only used in static function spk_handle_help and should be const
and perhaps the declaration might be better moved into that function.
For drivers/accessibility/speakup/main.c:2059:static spkup_hand spkup_handler[]
= {
spkup_handler is only used in static function do_spkup and should be const
and perhaps the declaration might be better moved into that function.
etc... for speakup
For drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {
array ac_props is assigned as a reference in acpi_ac_add as a
"const enum power_supply_property *" member of a struct power_supply_desc.
- For instance: (head -10 of the git grep for function statics)
drivers/acpi/apei/apei-base.c:781: static u8 whea_uuid_str[] =
"ed855e0c-6c90-47bf-a62a-26de0fc5ad5c";
drivers/block/amiflop.c:1051: static unsigned char CRCTable1[] = {
drivers/block/amiflop.c:1070: static unsigned char CRCTable2[] = {
drivers/block/drbd/drbd_nl.c:872: static char units[] = { 'K', 'M', 'G',
'T', 'P', 'E' };
drivers/block/drbd/drbd_proc.c:224: static char write_ordering_chars[] = {
drivers/block/drbd/drbd_receiver.c:4363:static enum drbd_conns c_tab[]
= {
drivers/char/pcmcia/synclink_cs.c:3717: static unsigned char patterns[] =
drivers/cpufreq/intel_pstate.c:1515:static int silvermont_freq_table[] = {
drivers/cpufreq/intel_pstate.c:1530:static int airmont_freq_table[] = {
drivers/dma/xgene-dma.c:360:static u8 flyby_type[] = {
Some of these could be const, but some could not. For instance:
For drivers/acpi/apei/apei-base.c:781: static u8 whea_uuid_str[] =
"ed855e0c-6c90-47bf-a62a-26de0fc5ad5c";
whea_uuid_str is assigned as a reference in "int apei_osc_setup(void)"
a struct acpi_osc_context where .uuid_str is not declared as const char *.
Re: [PATCH v8 0/3] checkpatch: add verbose mode
On Mon, 2021-03-01 at 14:22 -0700, Jonathan Corbet wrote: > Dwaipayan Ray writes: > > > Add a new verbose mode to checkpatch. The verbose test > > descriptions are read from the new checkpatch documentation > > file at `Documentation/dev-tools/checkpatch.rst`, which > > is also added by this series. > > So I can certainly take the doc change, as requested. Remind me, > though...should I apply the whole set, or will the checkpatch changes go > via another path? There's no dedicated upstream path for checkpatch. So please take the checkpatch changes too.
Re: [PATCH 11/20] hwmon: Manual replacement of the deprecated strlcpy() with return values
On Mon, 2021-02-22 at 07:46 -0800, Guenter Roeck wrote: > On 2/22/21 7:12 AM, Romain Perier wrote: > > The strlcpy() reads the entire source buffer first, it is dangerous if > > the source buffer lenght is unbounded or possibility non NULL-terminated. > > length > > > It can lead to linear read overflows, crashes, etc... > > > Not here. This description is misleading. > > > As recommended in the deprecated interfaces [1], it should be replaced > > by strscpy. > > > > This commit replaces all calls to strlcpy that handle the return values > > by the corresponding strscpy calls with new handling of the return > > values (as it is quite different between the two functions). > > > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > > > > Signed-off-by: Romain Perier > > This patch just adds pain to injury, as the source 'buffers' are all fixed > strings and their length will never exceed the maximum buffer length. > I really don't see the point of using strscpy() in this situation. Might as well just use strcpy (I'd still prefer stracpy) https://lore.kernel.org/lkml/24bb53c57767c1c2a8f266c305a67...@sk2.org/T/
Re: [PATCH 0/2] tracing: Detect unsafe dereferencing of pointers from trace events
On Fri, 2021-02-26 at 18:33 -0500, Steven Rostedt wrote: > On Fri, 26 Feb 2021 14:21:00 -0800 > Linus Torvalds wrote: > > > > The second patch handles strings "%s" [..] > > > > Doing this at runtime really feels like the wrong thing to do. > > > > It won't even protect us from what happened - people like me and > > Andrew won't even run those tracepoints in the first place, so we > > won't notice. > > > > It really would be much better in every respect to have this done by > > checkpatch, I think. > > They are not mutually exclusive. We could have both. One thing that's nice > about this patch is that it removes the possibility of a real bug. That is, > it will catch the dereferencing of a string that is not valid, WARN about > it, but it wont try to dereference it (outside of the > strcpy_from_kernel_nofault()). And hopefully the warning and lack of data > they want, will have this get caught during development. > > Also, there's cases that %s is allowed to reference data that I don't know > if checkpatch would be able to differentiate. It's not obvious to me how to do that.
Re: [PATCH v7 0/2] checkpatch: add verbose mode
On Thu, 2021-02-25 at 21:55 +0100, Mauro Carvalho Chehab wrote: > Em Thu, 25 Feb 2021 23:38:03 +0530 > Dwaipayan Ray escreveu: > > > On Thu, Feb 25, 2021 at 11:03 PM Joe Perches wrote: > > > > > > On Mon, 2021-02-22 at 13:22 +0530, Dwaipayan Ray wrote: > > > > Add a new verbose mode to checkpatch. The verbose test > > > > descriptions are read from the checkpatch documentation > > > > file at `Documentation/dev-tools/checkpatch.rst`. > > > > > > > > The verbose mode is optional and can be enabled by the > > > > flag -v or --verbose. > > > > > > > > The documentation file is only parsed by checkpatch.pl > > > > if the verbose mode is enabled. The verbose mode can > > > > not be used together with the --terse option. > > > > > > I don't have any real objection to this patch set, but as this > > > might be added to the Documentation tree and in .rst format, > > > perhaps Jonathan Corbet and/or Mauro Carvalho Chehab might have > > > some opinion. > > > > > > Also I do not want to be a maintainer of this .rst file and > > > likely neither Jon nor Mauro would either. Perhaps you? > > > > > > > > > > I could take it up if everybody is okay with it! > > > > > Ideally, the patch order would be reversed so the .rst file > > > is added first, then checkpatch updated to use it. > > > > > > > > > > Sure, if Jonathan or Mauro has no objections to it, I will be happy > > to resend it so that it can be picked up properly. > > I don't have any objections, provided that I won't be maintaining > it :-) > > - > > Just my two cents: > > IMO, maintaining this on a separate file can be a maintenance nightmare, > as this is the kind of thing that can become obsolete real soon. > > One alternative would be to use Pod::Usage module, just like > this script does: > > scripts/get_abi.pl > > with something similar to that, calling > > $ checkpatch --man > > Could generate a man-page style with all options, while: > > $ checkpatch --help > > would print the current help page. > > Yet, this would generate more work for Joe, as, for every new > type, the corresponding help text would be needed. Does this get integrated into the .rst output? I see: Documentation/Makefile:$(shell $(srctree)/scripts/get_abi.pl validate --dir $(srctree)/Documentation/ABI) But no obvious mechanism that emits .rst files for Pod::Usage And no, I'm not much interested in maintaining those docs either.
Re: [PATCH v7 0/2] checkpatch: add verbose mode
On Mon, 2021-02-22 at 13:22 +0530, Dwaipayan Ray wrote: > Add a new verbose mode to checkpatch. The verbose test > descriptions are read from the checkpatch documentation > file at `Documentation/dev-tools/checkpatch.rst`. > > The verbose mode is optional and can be enabled by the > flag -v or --verbose. > > The documentation file is only parsed by checkpatch.pl > if the verbose mode is enabled. The verbose mode can > not be used together with the --terse option. I don't have any real objection to this patch set, but as this might be added to the Documentation tree and in .rst format, perhaps Jonathan Corbet and/or Mauro Carvalho Chehab might have some opinion. Also I do not want to be a maintainer of this .rst file and likely neither Jon nor Mauro would either. Perhaps you? Ideally, the patch order would be reversed so the .rst file is added first, then checkpatch updated to use it. And _a lot_ more types and descriptive content should be added. > > Changes in v7: > - Add color coding support to --list-types option > > Changes in v6: > - Allow using verbose mode with --list-types option > > Changes in v5: > - Change the reference format to use absolute links. > - Print verbose descriptions only for the first time > a message type is encountered. > > Changes in v4: > - Change the type description format > - Group the message types by usage > - Make handling of --terse with --verbose simpler > > Changes in v3: > - Simplify documentation file parsing in checkpatch > - Document a total of 33 message types for checkpatch > > Changes in v2: > - Use .rst Field Lists to specify the type descriptions. > - Add a few more type descriptions to documentation. > > Dwaipayan Ray (2): > checkpatch: add verbose mode > docs: add documentation for checkpatch > > Documentation/dev-tools/checkpatch.rst | 525 + > Documentation/dev-tools/index.rst | 1 + > scripts/checkpatch.pl | 133 ++- > 3 files changed, 639 insertions(+), 20 deletions(-) > create mode 100644 Documentation/dev-tools/checkpatch.rst >
Re: checkpatch warnings for references to earlier commits
On Mon, 2021-02-22 at 13:14 -0800, Luck, Tony wrote:
> Would it be possible to teach checkpatch not to warn about
> canonical references to earlier commits? E.g.
>
> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per
> line)
> #7:
> commit e80634a75aba ("EDAC, skx: Retrieve and print retry_rd_err_log
> registers")
No, not really. It's possible and typical for commit descriptions
to span multiple lines. checkpatch accepts that.
Re: [PATCH v6 1/2] checkpatch: add verbose mode
On Mon, 2021-02-22 at 00:05 +0530, Dwaipayan Ray wrote: > On Sun, Feb 21, 2021 at 11:36 PM Joe Perches wrote: > > > > On Sun, 2021-02-21 at 17:28 +0530, Dwaipayan Ray wrote: > > > Add a new verbose mode to checkpatch.pl to emit additional verbose > > > test descriptions. The verbose mode is optional and can be enabled > > > by the flag -v or --verbose. > > > > OK, maybe add color coding to the list_types output. > Okay, nice idea! [] > Sure, I will do something like this. > Are there any other improvements you can see right now > or will the coloring thing suffice? A lot more descriptive output for the .rst file and of course a lot more of the types documented...
Re: [PATCH v6 1/2] checkpatch: add verbose mode
On Sun, 2021-02-21 at 17:28 +0530, Dwaipayan Ray wrote:
> Add a new verbose mode to checkpatch.pl to emit additional verbose
> test descriptions. The verbose mode is optional and can be enabled
> by the flag -v or --verbose.
OK, maybe add color coding to the list_types output.
Something like:
---
scripts/checkpatch.pl | 61 ---
1 file changed, 43 insertions(+), 18 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bbff13e0ca7e..ccd4a1bd73cb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -163,14 +163,39 @@ sub list_types {
my $text = <$script>;
close($script);
- my @types = ();
+ my %types = ();
# Also catch when type or level is passed through a variable
- for ($text =~
/(?:(?:\bCHK|\bWARN|\bERROR|&\{\$msg_level})\s*\(|\$msg_type\s*=)\s*"([^"]+)"/g)
{
- push (@types, $_);
+ while ($text =~ /\b(CHK|WARN|ERROR|&\{\$msg_level})\s*\(\s*"([^"]+)"/g)
{
+ if (exists($types{$2})) {
+ $types{$2} .= ",$1" if ($types{$2} ne $1);
+ } else {
+ $types{$2} = $1;
+ }
+ }
+ print("#\tMessage type");
+ if ($color) {
+ print(" (color coding: ");
+ print(RED . "ERROR" . RESET);
+ print("|");
+ print(YELLOW . "WARNING" . RESET);
+ print("|");
+ print(GREEN . "CHECK" . RESET);
+ print("|");
+ print("Multiple levels" . RESET);
+ print(")");
}
- @types = sort(uniq(@types));
- print("#\tMessage type\n\n");
- foreach my $type (@types) {
+ print("\n\n");
+ foreach my $type (sort keys %types) {
+ if ($color) {
+ my $level = $types{$type};
+ if ($level eq 'ERROR') {
+ $type = RED . $type . RESET;
+ } elsif ($level eq 'WARN') {
+ $type = YELLOW . $type . RESET;
+ } elsif ($level eq 'CHK') {
+ $type = GREEN . $type . RESET;
+ }
+ }
print(++$count . "\t" . $type . "\n");
if ($verbose && exists($verbose_messages{$type})) {
my $message = $verbose_messages{$type};
@@ -301,6 +326,18 @@ help(0) if ($help);
die "$P: --git cannot be used with --file or --fix\n" if ($git && ($file ||
$fix));
die "$P: --verbose cannot be used with --terse\n" if ($verbose && $terse);
+if ($color =~ /^[01]$/) {
+ $color = !$color;
+} elsif ($color =~ /^always$/i) {
+ $color = 1;
+} elsif ($color =~ /^never$/i) {
+ $color = 0;
+} elsif ($color =~ /^auto$/i) {
+ $color = (-t STDOUT);
+} else {
+ die "$P: Invalid color mode: $color\n";
+}
+
load_docs() if ($verbose);
list_types(0) if ($list_types);
@@ -321,18 +358,6 @@ if ($#ARGV < 0) {
push(@ARGV, '-');
}
-if ($color =~ /^[01]$/) {
- $color = !$color;
-} elsif ($color =~ /^always$/i) {
- $color = 1;
-} elsif ($color =~ /^never$/i) {
- $color = 0;
-} elsif ($color =~ /^auto$/i) {
- $color = (-t STDOUT);
-} else {
- die "$P: Invalid color mode: $color\n";
-}
-
# skip TAB size 1 to avoid additional checks on $tabsize - 1
die "$P: Invalid TAB size: $tabsize\n" if ($tabsize < 2);
