Re: wireless-drivers: random cleanup patches piling up

Re: wireless-drivers: random cleanup patches piling up

[Joe Perches]

On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
> Hi,
> 
> I have quite a lot of random cleanup patches from new developers waiting
> in my queue:
> 
> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
> 
> (Not all of them are cleanup patches, there are also few patches
> deferred due to other reasons, but you get the idea.)
> 
> These cleanup patches usually take quite a lot of my time and I'm
> starting to doubt the benefit, compared to the time needed to dig
> through them and figuring out what to apply. And this is of course time
> away from other patches, so it's slowing down "real" development.
> 
> I really don't know what to do. Part of me is saying that I just should
> drop them unless it's reviewed by a more experienced developer but on
> the other hand this is a good way get new developers onboard.
> 
> What others think? Are these kind of patches useful?

Some yes, mostly not really.

While whitespace style patches have some small value,
very few of the new contributors that use tools like
"scripts/checkpatch.pl -f" on various kernel files 
actually continue on to submit actual defect fixing
or optimization or code clarity patches.

Whitespace patches, where git diff -w does not show
any difference and objdiff on the objects for a few
randconfigs are identical, maybe could be sifted
into a separate category from other patches.

Maybe the kbuild test robot could help identify the
whitespace style
patches that can be easily applied.

Maybe the kernel-janitors list should be used and
also maybe some maintainers that want these style
patches could opt-in to getting these applied after
some milestone like an -rc1.

Fengguang?

Re: wireless-drivers: random cleanup patches piling up

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

On Thu, Jan 21, 2016 at 04:52:45PM -0800, Joe Perches wrote:
> Whitespace patches, where git diff -w does not show
> any difference and objdiff on the objects for a few
> randconfigs are identical, maybe could be sifted
> into a separate category from other patches.
> Maybe the kbuild test robot could help identify the
> whitespace style patches that can be easily applied.

It sort of takes a while to test the randconfig thing...  diff -w
doesn't catch every single issue.  For example:

http://www.spinics.net/lists/linux-driver-devel/msg78707.html

That's the only time I noticed a mistake like that which diff -w missed.

Also here is my rename_rev.pl script.  It's pretty useful.  Pipe
patches to it and it filters out the white space changes.

regards,
dan carpenter


[-- Attachment #2: rename_rev.pl --]
[-- Type: text/x-perl, Size: 10630 bytes --]

#!/usr/bin/perl

# This is a tool to help review variable rename patches. The goal is
# to strip out the automatic sed renames and the white space changes
# and leaves the interesting code changes.
#
# Example 1: A patch renames openInfo to open_info:
#     cat diff | rename_review.pl openInfo open_info
#
# Example 2: A patch swaps the first two arguments to some_func():
#     cat diff | rename_review.pl \
#                    -e 's/some_func\((.*?),(.*?),/some_func\($2, $1,/'
#
# Example 3: A patch removes the xkcd_ prefix from some but not all the
# variables.  Instead of trying to figure out which variables were renamed
# just remove the prefix from them all:
#     cat diff | rename_review.pl -ea 's/xkcd_//g'
#
# Example 4: A patch renames 20 CamelCase variables.  To review this let's
# just ignore all case changes and all '_' chars.
#     cat diff | rename_review -ea 'tr/[A-Z]/[a-z]/' -ea 's/_//g'
#
# The other arguments are:
# -nc removes comments
# -ns removes '\' chars if they are at the end of the line.

use strict;
use File::Temp qw/ :mktemp  /;

sub usage() {
    print "usage: cat diff | $0 old new old new old new...\n";
    print "   or: cat diff | $0 -e 's/old/new/g'\n";
    print " -a : auto";
    print " -e : execute on old lines\n";
    print " -ea: execute on all lines\n";
    print " -nc: no comments\n";
    print " -nb: no unneeded braces\n";
    print " -ns: no slashes at the end of a line\n";
    print " -pull: for function pull.  deletes context.\n";
    print " -r <recipe>: NULL, bool";
    exit(1);
}
my @subs;
my @strict_subs;
my @cmds;
my $strip_comments;
my $strip_braces;
my $strip_slashes;
my $pull_context;
my $auto;

sub filter($) {
    my $_ = shift();
    my $old = 0;
    if ($_ =~ /^-/) {
        $old = 1;
    }
    # remove the first char
    s/^[ +-]//;
    if ($strip_comments) {
        s/\/\*.*?\*\///g;
        s/\/\/.*//;
    }
    foreach my $cmd (@cmds) {
        if ($old || $cmd->[0] =~ /^-ea$/) {
            eval $cmd->[1];
        }
    }
    foreach my $sub (@subs) {
        if ($old) {
            s/$sub->[0]/$sub->[1]/g;
        }
    }
    foreach my $sub (@strict_subs) {
        if ($old) {
            s/\b$sub->[0]\b/$sub->[1]/g;
        }
    }

    # remove the newline so we can move curly braces here if we want.
    s/\n//;
    return $_;
}

while (my $param1 = shift()) {
    if ($param1 =~ /^-a$/) {
        $auto = 1;
        next;
    }
    if ($param1 =~ /^-nc$/) {
        $strip_comments = 1;
        next;
    }
    if ($param1 =~ /^-nb$/) {
        $strip_braces = 1;
        next;
    }
    if ($param1 =~ /^-ns$/) {
        $strip_slashes = 1;
        next;
    }
    if ($param1 =~ /^-pull$/) {
        $pull_context = 1;
        next;
    }
    my $param2 = shift();
    if ($param2 =~ /^$/) {
        usage();
    }
    if ($param1 =~ /^-e(a|)$/) {
        push @cmds, [$param1, $param2];
        next;
    }
    if ($param1 =~ /^-r$/) {
        if ($param2 =~ /bool/) {
            push @cmds, ["-e", "s/== true//"];
            push @cmds, ["-e", "s/true ==//"];
            push @cmds, ["-e", "s/([a-zA-Z\-\>\._]+) == false/!\$1/"];
            next;
        } elsif ($param2 =~ /NULL/) {
            push @cmds, ["-e", "s/ != NULL//"];
            push @cmds, ["-e", "s/([a-zA-Z\-\>\._0-9]+) == NULL/!\$1/"];
            next;
        } elsif ($param2 =~ /BIT/) {
            push @cmds, ["-e", 's/1[uUlL]* *<< *(\d+)/BIT($1)/'];
            push @cmds, ["-e", 's/\(1 *<< *(\w+)\)/BIT($1)/'];
            push @cmds, ["-e", 's/\(BIT\((.*?)\)\)/BIT($1)/'];
            next;
        }
        usage();
    }

    push @subs, [$param1, $param2];
}

my ($oldfh, $oldfile) = mkstemp("/tmp/oldXXXXX");
my ($newfh, $newfile) = mkstemp("/tmp/newXXXXX");

my @input = <STDIN>;

# auto works on the observation that the - line comes before the + line when we
# rename variables.  Take the first - line.  Find the first + line.  Find the
# one word difference.  Test that the old word never occurs in the new text.
if ($auto) {
    my %c_keywords = (  auto => 1,
                        break => 1,
                        case => 1,
                        char => 1,
                        const => 1,
                        continue => 1,
                        default => 1,
                        do => 1,
                        double => 1,
                        else => 1,
                        enum => 1,
                        extern => 1,
                        float => 1,
                        for => 1,
                        goto => 1,
                        if => 1,
                        int => 1,
                        long => 1,
                        register => 1,
                        return => 1,
                        short => 1,
                        signed => 1,
                        sizeof => 1,
                        static => 1,
                        struct => 1,
                        switch => 1,
                        typedef => 1,
                        union => 1,
                        unsigned => 1,
                        void => 1,
                        volatile => 1,
                        while => 1);
    my %old_words;
    my %new_words;
    my %added_cmds;
    my @new_subs;

    my $inside = 0;
    foreach my $line (@input) {
        if ($line =~ /^(---|\+\+\+)/) {
            next;
        }

        if ($line =~ /^@/) {
            $inside = 1;
        }
        if ($inside && !(($_ =~ /^[- @+]/) || ($_ =~ /^$/))) {
            $inside = 0;
        }
        if (!$inside) {
            next;
        }

        if ($line =~ /^-/) {
            s/-//;
            my @words = split(/\W+/, $line);
            foreach my $word (@words) {
                $old_words{$word} = 1;
            }
        } elsif ($line =~ /^\+/) {
            s/\+//;
            my @words = split(/\W+/, $line);
            foreach my $word (@words) {
                $new_words{$word} = 1;
            }
        }
    }

    my $old_line;
    my $new_line;
    $inside = 0;
    foreach my $line (@input) {
        if ($line =~ /^(---|\+\+\+)/) {
            next;
        }

        if ($line =~ /^@/) {
            $inside = 1;
        }
        if ($inside && !(($_ =~ /^[- @+]/) || ($_ =~ /^$/))) {
            $inside = 0;
        }
        if (!$inside) {
            next;
        }


        if ($line =~ /^-/ && !$old_line) {
            s/^-//;
            $old_line = $line;
            next;
        } elsif ($old_line && $line =~ /^\+/) {
            s/^\+//;
            $new_line = $line;
        } else {
            next;
        }

        my @old_words = split(/\W+/, $old_line);
        my @new_words = split(/\W+/, $new_line);
        my @new_cmds;

        my $i;
        my $diff_count = 0;
        for ($i = 0; ; $i++) {
            if (!defined($old_words[$i]) && !defined($new_words[$i])) {
                last;
            }
            if (!defined($old_words[$i]) || !defined($new_words[$i])) {
                $diff_count = 1000;
                last;
            }
            if ($old_words[$i] eq $new_words[$i]) {
                next;
            }
            if ($c_keywords{$old_words[$i]}) {
                $diff_count = 1000;
                last;
            }
            if ($new_words{$old_words[$i]}) {
                $diff_count++;
            }
            push @new_cmds, [$old_words[$i], $new_words[$i]];
        }
        if ($diff_count <= 2) {
            foreach my $sub (@new_cmds) {
                if ($added_cmds{$sub->[0] . $sub->[1]}) {
                    next;
                }
                $added_cmds{$sub->[0] . $sub->[1]} = 1;
                push @new_subs, [$sub->[0] , $sub->[1]];
            }
        }

        $old_line = 0;
    }

    if (@new_subs) {
        print "RENAMES:\n";
        foreach my $sub (@new_subs) {
            print "$sub->[0] => $sub->[1]\n";
            push @strict_subs, [$sub->[0] , $sub->[1]];
        }
        print "---\n";
    }
}

my $output;

#recreate an old file and a new file
my $inside = 0;
foreach (@input) {
    if ($pull_context && !($_ =~ /^[+-@]/)) {
        next;
    }

    if ($_ =~ /^(---|\+\+\+)/) {
        next;
    }

    if ($_ =~ /^@/) {
        $inside = 1;
    }
    if ($inside && !(($_ =~ /^[- @+]/) || ($_ =~ /^$/))) {
        $inside = 0;
    }
    if (!$inside) {
        next;
    }

    $output = filter($_);

    if ($strip_braces && $_ =~ /^(\+|-)\W+{/) {
        $output =~ s/^[\t ]+(.*)/ $1/;
    } else {
        $output = "\n" . $output;
    }

    if ($_ =~ /^-/) {
        print $oldfh $output;
        next;
    }
    if ($_ =~ /^\+/) {
        print $newfh $output;
        next;
    }
    print $oldfh $output;
    print $newfh $output;

}
print $oldfh "\n";
print $newfh "\n";
# git diff puts a -- and version at the end of the diff.  put the -- into the
# new file as well so it's ignored
if ($output =~ /\n-/) {
    print $newfh "-\n";
}

my $hunk;
my $old_txt;
my $new_txt;

open diff, "diff -uw $oldfile $newfile |";
while (<diff>) {
    if ($_ =~ /^(---|\+\+\+)/) {
        next;
    }

    if ($_ =~ /^@/) {

        if ($strip_comments) {
            $old_txt =~ s/\/\*.*?\*\///g;
            $new_txt =~ s/\/\*.*?\*\///g;
        }
        if ($strip_braces) {
            $old_txt =~ s/{([^;{]*?);}/$1;/g;
            $new_txt =~ s/{([^;{]*?);}/$1;/g;
            # this is a hack because i don't know how to replace nested
            # unneeded curly braces.
            $old_txt =~ s/{([^;{]*?);}/$1;/g;
            $new_txt =~ s/{([^;{]*?);}/$1;/g;
        }

        if ($old_txt ne $new_txt) {
            print $hunk;
            print $_;
        }
        $hunk = "";
        $old_txt = "";
        $new_txt = "";
        next;
    }

    $hunk = $hunk . $_;

    if ($strip_slashes) {
        s/\\$//;
    }

    if ($_ =~ /^-/) {
        s/-//;
        s/[ \t\n]//g;
        $old_txt = $old_txt . $_;
        next;
    }
    if ($_ =~ /^\+/) {
        s/\+//;
        s/[ \t\n]//g;
        $new_txt = $new_txt . $_;
        next;
    }
    if ($_ =~ /^ /) {
        s/^ //;
        s/[ \t\n]//g;
        $old_txt = $old_txt . $_;
        $new_txt = $new_txt . $_;
    }
}

if ($old_txt ne $new_txt) {
    if ($strip_comments) {
        $old_txt =~ s/\/\*.*?\*\///g;
        $new_txt =~ s/\/\*.*?\*\///g;
    }
    if ($strip_braces) {
        $old_txt =~ s/{([^;{]*?);}/$1;/g;
        $new_txt =~ s/{([^;{]*?);}/$1;/g;
        $old_txt =~ s/{([^;{]*?);}/$1;/g;
        $new_txt =~ s/{([^;{]*?);}/$1;/g;
    }

    print $hunk;
}

unlink($oldfile);
unlink($newfile);

print "\ndone.\n";

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Joe Perches <joe@perches.com> writes:

> On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
>> Hi,
>> 
>> I have quite a lot of random cleanup patches from new developers waiting
>> in my queue:
>> 
>> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
>> 
>> (Not all of them are cleanup patches, there are also few patches
>> deferred due to other reasons, but you get the idea.)
>> 
>> These cleanup patches usually take quite a lot of my time and I'm
>> starting to doubt the benefit, compared to the time needed to dig
>> through them and figuring out what to apply. And this is of course time
>> away from other patches, so it's slowing down "real" development.
>> 
>> I really don't know what to do. Part of me is saying that I just should
>> drop them unless it's reviewed by a more experienced developer but on
>> the other hand this is a good way get new developers onboard.
>> 
>> What others think? Are these kind of patches useful?
>
> Some yes, mostly not really.
>
> While whitespace style patches have some small value,
> very few of the new contributors that use tools like
> "scripts/checkpatch.pl -f" on various kernel files 
> actually continue on to submit actual defect fixing
> or optimization or code clarity patches.

That's also my experience from maintaining wireless-drivers for a year,
this seems to be a "hit and run" type of phenomenon.

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[John W. Linville]

On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
> Joe Perches <joe@perches.com> writes:
> 
> > On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
> >> Hi,
> >> 
> >> I have quite a lot of random cleanup patches from new developers waiting
> >> in my queue:
> >> 
> >> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
> >> 
> >> (Not all of them are cleanup patches, there are also few patches
> >> deferred due to other reasons, but you get the idea.)
> >> 
> >> These cleanup patches usually take quite a lot of my time and I'm
> >> starting to doubt the benefit, compared to the time needed to dig
> >> through them and figuring out what to apply. And this is of course time
> >> away from other patches, so it's slowing down "real" development.
> >> 
> >> I really don't know what to do. Part of me is saying that I just should
> >> drop them unless it's reviewed by a more experienced developer but on
> >> the other hand this is a good way get new developers onboard.
> >> 
> >> What others think? Are these kind of patches useful?
> >
> > Some yes, mostly not really.
> >
> > While whitespace style patches have some small value,
> > very few of the new contributors that use tools like
> > "scripts/checkpatch.pl -f" on various kernel files 
> > actually continue on to submit actual defect fixing
> > or optimization or code clarity patches.
> 
> That's also my experience from maintaining wireless-drivers for a year,
> this seems to be a "hit and run" type of phenomenon.

Should we be looking for someone to run a "wireless-driver-cleanups"
tree?  They could handle the cleanups and trivial stuff, and send
you a pull request a couple of times per release...?

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

"John W. Linville" <linville@tuxdriver.com> writes:

> On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
>> Joe Perches <joe@perches.com> writes:
>> 
>> > On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
>> >> Hi,
>> >> 
>> >> I have quite a lot of random cleanup patches from new developers waiting
>> >> in my queue:
>> >> 
>> >> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
>> >> 
>> >> (Not all of them are cleanup patches, there are also few patches
>> >> deferred due to other reasons, but you get the idea.)
>> >> 
>> >> These cleanup patches usually take quite a lot of my time and I'm
>> >> starting to doubt the benefit, compared to the time needed to dig
>> >> through them and figuring out what to apply. And this is of course time
>> >> away from other patches, so it's slowing down "real" development.
>> >> 
>> >> I really don't know what to do. Part of me is saying that I just should
>> >> drop them unless it's reviewed by a more experienced developer but on
>> >> the other hand this is a good way get new developers onboard.
>> >> 
>> >> What others think? Are these kind of patches useful?
>> >
>> > Some yes, mostly not really.
>> >
>> > While whitespace style patches have some small value,
>> > very few of the new contributors that use tools like
>> > "scripts/checkpatch.pl -f" on various kernel files 
>> > actually continue on to submit actual defect fixing
>> > or optimization or code clarity patches.
>> 
>> That's also my experience from maintaining wireless-drivers for a year,
>> this seems to be a "hit and run" type of phenomenon.
>
> Should we be looking for someone to run a "wireless-driver-cleanups"
> tree?  They could handle the cleanups and trivial stuff, and send
> you a pull request a couple of times per release...?

Not a bad idea! But I don't think we need a separate tree as applying
patches from patchwork is easy. It should be doable that we add an
account to patchwork and whenever I see a this type of trivial cleanup
patch I'll assign it to the cleanup maintainer and whenever he/she
thinks it's ready he assigns the patch back to me and I'll apply it.

The only difficult part is finding a victim/volunteer to
do that ;)

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[Sudip Mukherjee]

On Fri, Jan 22, 2016 at 05:54:12PM +0200, Kalle Valo wrote:
> "John W. Linville" <linville@tuxdriver.com> writes:
> 
> > On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
> >> Joe Perches <joe@perches.com> writes:
> >> 
> >> > On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
> >> >> Hi,
> >> >> 
> >> >> I have quite a lot of random cleanup patches from new developers waiting
> >> >> in my queue:
> >> >> 
> >> >> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
> >> >> 
> >> >> (Not all of them are cleanup patches, there are also few patches
> >> >> deferred due to other reasons, but you get the idea.)
> >> >> 
> >> >> These cleanup patches usually take quite a lot of my time and I'm
> >> >> starting to doubt the benefit, compared to the time needed to dig
> >> >> through them and figuring out what to apply. And this is of course time
> >> >> away from other patches, so it's slowing down "real" development.
> >> >> 
> >> >> I really don't know what to do. Part of me is saying that I just should
> >> >> drop them unless it's reviewed by a more experienced developer but on
> >> >> the other hand this is a good way get new developers onboard.
> >> >> 
> >> >> What others think? Are these kind of patches useful?
> >> >
> >> > Some yes, mostly not really.
> >> >
> >> > While whitespace style patches have some small value,
> >> > very few of the new contributors that use tools like
> >> > "scripts/checkpatch.pl -f" on various kernel files 
> >> > actually continue on to submit actual defect fixing
> >> > or optimization or code clarity patches.
> >> 
> >> That's also my experience from maintaining wireless-drivers for a year,
> >> this seems to be a "hit and run" type of phenomenon.
> >
> > Should we be looking for someone to run a "wireless-driver-cleanups"
> > tree?  They could handle the cleanups and trivial stuff, and send
> > you a pull request a couple of times per release...?
> 
> Not a bad idea! But I don't think we need a separate tree as applying
> patches from patchwork is easy. It should be doable that we add an
> account to patchwork and whenever I see a this type of trivial cleanup
> patch I'll assign it to the cleanup maintainer and whenever he/she
> thinks it's ready he assigns the patch back to me and I'll apply it.
> 
> The only difficult part is finding a victim/volunteer to
> do that ;)

I can be a volunteer (victim?). Though i donot know much about
wireless-drivers, but I do know a little about cleanup patches.
And maybe, in the process I will start knowing wireless-drivers.

regards
sudip

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:

>> >> That's also my experience from maintaining wireless-drivers for a year,
>> >> this seems to be a "hit and run" type of phenomenon.
>> >
>> > Should we be looking for someone to run a "wireless-driver-cleanups"
>> > tree?  They could handle the cleanups and trivial stuff, and send
>> > you a pull request a couple of times per release...?
>> 
>> Not a bad idea! But I don't think we need a separate tree as applying
>> patches from patchwork is easy. It should be doable that we add an
>> account to patchwork and whenever I see a this type of trivial cleanup
>> patch I'll assign it to the cleanup maintainer and whenever he/she
>> thinks it's ready he assigns the patch back to me and I'll apply it.
>> 
>> The only difficult part is finding a victim/volunteer to
>> do that ;)
>
> I can be a volunteer (victim?). Though i donot know much about
> wireless-drivers, but I do know a little about cleanup patches.
> And maybe, in the process I will start knowing wireless-drivers.

I think it's better that you have prior experience with linux-wireless
before doing something like this. You can start by reviewing patches and
providing Reviewed-by tags.

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[Sudip Mukherjee]

On Fri, Jan 29, 2016 at 10:08:23AM +0200, Kalle Valo wrote:
> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
> 
> >> >> That's also my experience from maintaining wireless-drivers for a year,
> >> >> this seems to be a "hit and run" type of phenomenon.
> >> >
> >> > Should we be looking for someone to run a "wireless-driver-cleanups"
> >> > tree?  They could handle the cleanups and trivial stuff, and send
> >> > you a pull request a couple of times per release...?
> >> 
> >> Not a bad idea! But I don't think we need a separate tree as applying
> >> patches from patchwork is easy. It should be doable that we add an
> >> account to patchwork and whenever I see a this type of trivial cleanup
> >> patch I'll assign it to the cleanup maintainer and whenever he/she
> >> thinks it's ready he assigns the patch back to me and I'll apply it.
> >> 
> >> The only difficult part is finding a victim/volunteer to
> >> do that ;)
> >
> > I can be a volunteer (victim?). Though i donot know much about
> > wireless-drivers, but I do know a little about cleanup patches.
> > And maybe, in the process I will start knowing wireless-drivers.
> 
> I think it's better that you have prior experience with linux-wireless
> before doing something like this. You can start by reviewing patches and
> providing Reviewed-by tags.

Sure, I am starting that way. I checked in patchwork and I do not see
any checkpatch related patch pending (except staging, which Greg will
handle). I think you must have cleared all of them.

regards
sudip

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:

> Sure, I am starting that way. I checked in patchwork and I do not see
> any checkpatch related patch pending (except staging, which Greg will
> handle). I think you must have cleared all of them.

They are in deferred state. The search functionality in patchwork is not
that intuitive and they are not easy to find so here's a direct link:

https://patchwork.kernel.org/project/linux-wireless/list/?state=10&order=date

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[Julian Calaby]

Hi Kalle,

On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
>
>> Sure, I am starting that way. I checked in patchwork and I do not see
>> any checkpatch related patch pending (except staging, which Greg will
>> handle). I think you must have cleared all of them.
>
> They are in deferred state. The search functionality in patchwork is not
> that intuitive and they are not easy to find so here's a direct link:
>
> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&order=date

I'm currently going through that list and producing a bundle of
"applyable" patches.

My criteria is:
1. The change is sane.
2. It's either obviously correct, I can review it, or someone else has
reviewed or acked it.
3. No changes other than rebasing and fixing commit messages are
required to apply it.

Some of these patches need work on their commit messages, some are
complicated enough that I feel I should be providing review notes so
someone else can double check my review, and all of them should be
rebased and compile tested. Also, some are controversial, so I'll be
segregating them from the main set.

How would you like me to communicate this list to you? I'm happy to
provide branches you can pull from or I could just post updated
versions to the list and give reviewed-by tags to those that don't
need more work.

Every patch will get an email on linux-wireless regardless.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: wireless-drivers: random cleanup patches piling up

[Kalle Valo]

Julian Calaby <julian.calaby@gmail.com> writes:

> Hi Kalle,
>
> On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
>>
>>> Sure, I am starting that way. I checked in patchwork and I do not see
>>> any checkpatch related patch pending (except staging, which Greg will
>>> handle). I think you must have cleared all of them.
>>
>> They are in deferred state. The search functionality in patchwork is not
>> that intuitive and they are not easy to find so here's a direct link:
>>
>> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&order=date
>
> I'm currently going through that list and producing a bundle of
> "applyable" patches.

Nice.

> My criteria is:
> 1. The change is sane.
> 2. It's either obviously correct, I can review it, or someone else has
> reviewed or acked it.
> 3. No changes other than rebasing and fixing commit messages are
> required to apply it.

BTW, 'git am -s -3' is the best way to apply a patch. The three way
merge is awesome (if the submitter has sent the patch correctly).

> Some of these patches need work on their commit messages, some are
> complicated enough that I feel I should be providing review notes so
> someone else can double check my review, and all of them should be
> rebased and compile tested. Also, some are controversial, so I'll be
> segregating them from the main set.
>
> How would you like me to communicate this list to you? I'm happy to
> provide branches you can pull from or I could just post updated
> versions to the list and give reviewed-by tags to those that don't
> need more work.
>
> Every patch will get an email on linux-wireless regardless.

I guess posting the patches to linux-wireless is the easiest for
everyone? I have a script which automatically takes patches from
patchwork so that's very easy for me. But remember to use Signed-off-by
instead of Reviewed-by as you are resending the patches.

Thanks you, your help here is very much appreciated.

-- 
Kalle Valo

Re: wireless-drivers: random cleanup patches piling up

[Julian Calaby]

Hi Kalle,

On Wed, Mar 16, 2016 at 8:22 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
> Julian Calaby <julian.calaby@gmail.com> writes:
>
>> Hi Kalle,
>>
>> On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>>> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
>>>
>>>> Sure, I am starting that way. I checked in patchwork and I do not see
>>>> any checkpatch related patch pending (except staging, which Greg will
>>>> handle). I think you must have cleared all of them.
>>>
>>> They are in deferred state. The search functionality in patchwork is not
>>> that intuitive and they are not easy to find so here's a direct link:
>>>
>>> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&order=date
>>
>> I'm currently going through that list and producing a bundle of
>> "applyable" patches.
>
> Nice.

Thanks, I figured that checking the deferred list on patchwork at some
point would be a good plan. After a release seemed like a good time to
do it.

>> My criteria is:
>> 1. The change is sane.
>> 2. It's either obviously correct, I can review it, or someone else has
>> reviewed or acked it.
>> 3. No changes other than rebasing and fixing commit messages are
>> required to apply it.
>
> BTW, 'git am -s -3' is the best way to apply a patch. The three way
> merge is awesome (if the submitter has sent the patch correctly).
>
>> Some of these patches need work on their commit messages, some are
>> complicated enough that I feel I should be providing review notes so
>> someone else can double check my review, and all of them should be
>> rebased and compile tested. Also, some are controversial, so I'll be
>> segregating them from the main set.
>>
>> How would you like me to communicate this list to you? I'm happy to
>> provide branches you can pull from or I could just post updated
>> versions to the list and give reviewed-by tags to those that don't
>> need more work.
>>
>> Every patch will get an email on linux-wireless regardless.
>
> I guess posting the patches to linux-wireless is the easiest for
> everyone? I have a script which automatically takes patches from
> patchwork so that's very easy for me. But remember to use Signed-off-by
> instead of Reviewed-by as you are resending the patches.

If they end up being exactly identical to the original, I'll just add
reviewed-bys to the original patches, otherwise I'll do exactly that.

> Thanks you, your help here is very much appreciated.

No problem!

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: wireless-drivers: random cleanup patches piling up

[Julian Calaby]

Hi Kalle,

On Wed, Mar 16, 2016 at 8:42 PM, Julian Calaby <julian.calaby@gmail.com> wrote:
> Hi Kalle,
>
> On Wed, Mar 16, 2016 at 8:22 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>> Julian Calaby <julian.calaby@gmail.com> writes:
>>
>>> Hi Kalle,
>>>
>>> On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <kvalo@codeaurora.org> wrote:
>>>> Sudip Mukherjee <sudipm.mukherjee@gmail.com> writes:
>>>>
>>>>> Sure, I am starting that way. I checked in patchwork and I do not see
>>>>> any checkpatch related patch pending (except staging, which Greg will
>>>>> handle). I think you must have cleared all of them.
>>>>
>>>> They are in deferred state. The search functionality in patchwork is not
>>>> that intuitive and they are not easy to find so here's a direct link:
>>>>
>>>> https://patchwork.kernel.org/project/linux-wireless/list/?state=10&order=date
>>>
>>> I'm currently going through that list and producing a bundle of
>>> "applyable" patches.
>>
>> Nice.
>
> Thanks, I figured that checking the deferred list on patchwork at some
> point would be a good plan. After a release seemed like a good time to
> do it.
>
>>> My criteria is:
>>> 1. The change is sane.
>>> 2. It's either obviously correct, I can review it, or someone else has
>>> reviewed or acked it.
>>> 3. No changes other than rebasing and fixing commit messages are
>>> required to apply it.
>>
>> BTW, 'git am -s -3' is the best way to apply a patch. The three way
>> merge is awesome (if the submitter has sent the patch correctly).
>>
>>> Some of these patches need work on their commit messages, some are
>>> complicated enough that I feel I should be providing review notes so
>>> someone else can double check my review, and all of them should be
>>> rebased and compile tested. Also, some are controversial, so I'll be
>>> segregating them from the main set.
>>>
>>> How would you like me to communicate this list to you? I'm happy to
>>> provide branches you can pull from or I could just post updated
>>> versions to the list and give reviewed-by tags to those that don't
>>> need more work.
>>>
>>> Every patch will get an email on linux-wireless regardless.
>>
>> I guess posting the patches to linux-wireless is the easiest for
>> everyone? I have a script which automatically takes patches from
>> patchwork so that's very easy for me. But remember to use Signed-off-by
>> instead of Reviewed-by as you are resending the patches.
>
> If they end up being exactly identical to the original, I'll just add
> reviewed-bys to the original patches, otherwise I'll do exactly that.

I'm going to just repost everything as it'll just be easier at my end.

Git tree: https://github.com/SkUrRiEr/wireless-drivers-pending

I've split the pending patches into 4 sets:

1. Cleanup: patches that weren't reviewed or were just forgotten.
2. Detail: patches that needed a detailed review
3. More Work: patches that only partially fix a problem
4. Controversial: patches people hated but fit my criteria

I'll go into a lot more detail in my cover letter.

At this point, everything in patchwork that's deferred is either:
1. Unreviewable by me (I poked the authors of most of the older
patches yesterday)
2. An earlier version of a patch I picked up
3. Too "new" (less than a couple of months old)

I'll start sending stuff shortly.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: wireless-drivers: random cleanup patches piling up

[Joe Perches]

On Fri, 2016-01-22 at 10:12 -0500, John W. Linville wrote:
> On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
> > Joe Perches <joe@perches.com> writes:
> > 
> > > On Thu, 2016-01-21 at 16:58 +0200, Kalle Valo wrote:
> > > > Hi,
> > > > 
> > > > I have quite a lot of random cleanup patches from new developers waiting
> > > > in my queue:
> > > > 
> > > > https://patchwork.kernel.org/project/linux-wireless/list/?state=10&delegate=25621&order=date
> > > > 
> > > > (Not all of them are cleanup patches, there are also few patches
> > > > deferred due to other reasons, but you get the idea.)
> > > > 
> > > > These cleanup patches usually take quite a lot of my time and I'm
> > > > starting to doubt the benefit, compared to the time needed to dig
> > > > through them and figuring out what to apply. And this is of course time
> > > > away from other patches, so it's slowing down "real" development.
> > > > 
> > > > I really don't know what to do. Part of me is saying that I just should
> > > > drop them unless it's reviewed by a more experienced developer but on
> > > > the other hand this is a good way get new developers onboard.
> > > > 
> > > > What others think? Are these kind of patches useful?
> > > 
> > > Some yes, mostly not really.
> > > 
> > > While whitespace style patches have some small value,
> > > very few of the new contributors that use tools like
> > > "scripts/checkpatch.pl -f" on various kernel files 
> > > actually continue on to submit actual defect fixing
> > > or optimization or code clarity patches.
> > 
> > That's also my experience from maintaining wireless-drivers for a year,
> > this seems to be a "hit and run" type of phenomenon.
> 
> Should we be looking for someone to run a "wireless-driver-cleanups"
> tree?  They could handle the cleanups and trivial stuff, and send
> you a pull request a couple of times per release...?

If you are really interested in this sort of code cleanup,
and not in a new developer that might show up because of
a "my first kernel patch" process, maybe it'd be better
to do a preemptive run of something like:

$ git ls-files drivers/net/wireless | \
  while read file ; do \
    ./scripts/checkpatch.pl -f --fix-inplace --types=spacing $file ; \
  done

with git diff -w, compile every modified file, use objdiff, etc.
and a commit per subdirectory or driver.

A problem with that is checkpatch messages really aren't
dicta and there are some things that maybe look nicer
before the script mucks them up.

For instance, in the first file from that pass:

drivers/net/wireless/admtek/adm8211.c
[]
@@ -273,7 +273,7 @@ static void adm8211_write_sram_bytes(struct ieee80211_hw *dev
                }
        } else {
                for (i = 0; i < len; i += 4) {
-                       u32 val = (buf[i + 0] << 0 ) | (buf[i + 1] << 8 ) |
+                       u32 val = (buf[i + 0] << 0) | (buf[i + 1] << 8 ) |
                                  (buf[i + 2] << 16) | (buf[i + 3] << 24);
                        adm8211_write_sram(dev, addr + i / 4, val);
                }

You could reasonably argue that the "<< 0 )" was used
for alignment and doesn't need to be changed.  Perhaps
this should be "<<  0)" instead.

But a better change would be not be a whitespace change
but "get_unaligned_le32(buf + i)", maybe removing the
temporary too.

And there's an equivalent change that could use
get_unaligned_le16 in the preceding block that
checkpatch doesn't flag.

Anyway, a trivial change like the first block I looked
at could be done automatically, but it really doesn't
improve the code much.