Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1314673 > unrolled thread

Re: wireless-drivers: random cleanup patches piling up

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: wireless-drivers: random cleanup patches piling up Joe Perches <joe@perches.com> - 2016-01-22 02:00 +0100
    Re: wireless-drivers: random cleanup patches piling up Dan Carpenter <dan.carpenter@oracle.com> - 2016-01-22 08:40 +0100
    Re: wireless-drivers: random cleanup patches piling up Kalle Valo <kvalo@codeaurora.org> - 2016-01-22 13:30 +0100
      Re: wireless-drivers: random cleanup patches piling up "John W. Linville" <linville@tuxdriver.com> - 2016-01-22 16:20 +0100
        Re: wireless-drivers: random cleanup patches piling up Kalle Valo <kvalo@codeaurora.org> - 2016-01-22 17:00 +0100
          Re: wireless-drivers: random cleanup patches piling up Sudip Mukherjee <sudipm.mukherjee@gmail.com> - 2016-01-26 06:30 +0100
        Re: wireless-drivers: random cleanup patches piling up Joe Perches <joe@perches.com> - 2016-01-22 19:10 +0100

#1314673 — Re: wireless-drivers: random cleanup patches piling up

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?

[toc] | [next] | [standalone]

#1314802

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1314950

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

[toc] | [prev] | [next] | [standalone]

#1315071

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.

[toc] | [prev] | [next] | [standalone]

#1315089

"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

[toc] | [prev] | [next] | [standalone]

#1317541

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

[toc] | [prev] | [next] | [standalone]

#1315186

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web