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?
--
Kalle Valo
Joe Perches <[email protected]> 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
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 <[email protected]> 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.
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?
Julian Calaby <[email protected]> writes:
> Hi Kalle,
>
> On Fri, Jan 22, 2016 at 1:58 AM, Kalle Valo <[email protected]> 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?
>
> I'm not experienced or knowledgeable enough
I have seen your comments, you are most certainly knowledgeable enough :)
> to give an ack or formal review of a patch, however I generally read
> all of them. Would it be helpful if I were to give an informal "this
> patch looks sane" for cleanups and other small patches?
That would be _very_ helpful, especially with these cleanup patches from
newbies.
And even better if you can add the official Reviewed-by tag because when
we get the promised patchwork update (crossing my fingers) it will
actually show the review count on the top page. That makes it really
easy to spot what cleanup patches are worth looking at (and which one to
drop). So I very much welcome this practice.
--
Kalle Valo
On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
> Joe Perches <[email protected]> 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
[email protected] might be all we have. Be ready.
Hi Kalle,
On Fri, Jan 22, 2016 at 11:17 PM, Kalle Valo <[email protected]> wrote:
> Julian Calaby <[email protected]> writes:
>
>> Hi Kalle,
>>
>> On Fri, Jan 22, 2016 at 1:58 AM, Kalle Valo <[email protected]> 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?
>>
>> I'm not experienced or knowledgeable enough
>
> I have seen your comments, you are most certainly knowledgeable enough :)
I'm glad one of us has confidence in me. I know general stuff, but I
lack driver / subsystem specific knowledge, i.e. I can tell if
someone's patch looks stupid, but I can't generally tell if the end
result is doing the "right" thing. Thankfully most of the small
cleanup patches aren't generally changing anything that significant.
>> to give an ack or formal review of a patch, however I generally read
>> all of them. Would it be helpful if I were to give an informal "this
>> patch looks sane" for cleanups and other small patches?
>
> That would be _very_ helpful, especially with these cleanup patches from
> newbies.
>
> And even better if you can add the official Reviewed-by tag because when
> we get the promised patchwork update (crossing my fingers) it will
> actually show the review count on the top page. That makes it really
> easy to spot what cleanup patches are worth looking at (and which one to
> drop). So I very much welcome this practice.
Ok, done. I'll give any sane patches my reviewed-by with a brief "this
looks sane" note.
I'll also try to ensure the maintainers of the driver are CC'd on my
reply and ask them to review the more technical stuff if it's
required.
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Sudip Mukherjee <[email protected]> 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
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
Larry Finger <[email protected]> writes:
> We might get new developers, but the cost may be high. In the staging
> tree, things are worse. The tools can be applied in a blind fashion,
> but the results can be really stupid. GregKH has told a few would-be
> contributors to "go away" after a few patches that would not build.
>
> As most of these patches are based on "problems" found by application
> of various standard tools, they will likely be resubmitted over and
> over until the code is "fixed". Whether the patches are useful may not
> be the main question.
>
> My real complaint with these patches is that very few are more than
> compile tested. For example, there are 3 patches for memory leaks in
> b43. One (https://patchwork.kernel.org/patch/7998941) was rejected
> because it missed some such leaks, but there was not a formal NACK.
> The patch was fixed and resubmitted
> (https://patchwork.kernel.org/patch/8014311/), but not yet tested. The
> author then resent it (https://patchwork.kernel.org/patch/8049041/)
> and was chastised for resending as it still had not been tested. Of
> course, the first two of these can be dropped. Unfortunately, there
> are very few devs who have the necessary hardware to test.
>
> Most of the current set do not account for the directory restructuring
> and will not apply.Those can be rejected with the appropriate message
> asking that they be rebased. That should not require too much of your
> time. That will at last clean out the current backlog.
Actually 'git am -3' handles the directory restructuring just fine, so
that's not a problem. And I can also manage the current backlog, it just
takes some time to clear it up. I'm just worried that these cleanup
patches become even a bigger problem in the future.
> Is it possible for us to require the patch author to supply the level
> of testing when that is not obvious? This information should be in the
> comments location after the first ---. I suspect I know the answer for
> non-maintainers, but the formal requirement might be helpful.
I think that's a good idea. If it's only compile tested that should be
properly documented in the commit log so that people are aware of it.
> I also promise to be more diligent in reviewing the patches that are
> directed at the drivers that I maintain.
Thanks, I appreciate that.
--
Kalle Valo
On 01/21/2016 08:58 AM, 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?
Kalle,
We might get new developers, but the cost may be high. In the staging tree,
things are worse. The tools can be applied in a blind fashion, but the results
can be really stupid. GregKH has told a few would-be contributors to "go away"
after a few patches that would not build.
As most of these patches are based on "problems" found by application of various
standard tools, they will likely be resubmitted over and over until the code is
"fixed". Whether the patches are useful may not be the main question.
My real complaint with these patches is that very few are more than compile
tested. For example, there are 3 patches for memory leaks in b43. One
(https://patchwork.kernel.org/patch/7998941) was rejected because it missed some
such leaks, but there was not a formal NACK. The patch was fixed and resubmitted
(https://patchwork.kernel.org/patch/8014311/), but not yet tested. The author
then resent it (https://patchwork.kernel.org/patch/8049041/) and was chastised
for resending as it still had not been tested. Of course, the first two of these
can be dropped. Unfortunately, there are very few devs who have the necessary
hardware to test.
Most of the current set do not account for the directory restructuring and will
not apply.Those can be rejected with the appropriate message asking that they be
rebased. That should not require too much of your time. That will at last clean
out the current backlog.
Is it possible for us to require the patch author to supply the level of testing
when that is not obvious? This information should be in the comments location
after the first ---. I suspect I know the answer for non-maintainers, but the
formal requirement might be helpful. I will start NACKing those patches without
such information.
I also promise to be more diligent in reviewing the patches that are directed at
the drivers that I maintain.
Larry
On Fri, Jan 22, 2016 at 05:54:12PM +0200, Kalle Valo wrote:
> "John W. Linville" <[email protected]> writes:
>
> > On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
> >> Joe Perches <[email protected]> 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
Hi Kalle,
On Fri, Jan 22, 2016 at 1:58 AM, Kalle Valo <[email protected]> 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?
I'm not experienced or knowledgeable enough to give an ack or formal
review of a patch, however I generally read all of them. Would it be
helpful if I were to give an informal "this patch looks sane" for
cleanups and other small patches?
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
"John W. Linville" <[email protected]> writes:
> On Fri, Jan 22, 2016 at 02:21:20PM +0200, Kalle Valo wrote:
>> Joe Perches <[email protected]> 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
Sudip Mukherjee <[email protected]> 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
On Fri, Jan 29, 2016 at 10:08:23AM +0200, Kalle Valo wrote:
> Sudip Mukherjee <[email protected]> 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
Hi Kalle,
On Wed, Mar 16, 2016 at 8:22 PM, Kalle Valo <[email protected]> wrote:
> Julian Calaby <[email protected]> writes:
>
>> Hi Kalle,
>>
>> On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <[email protected]> wrote:
>>> Sudip Mukherjee <[email protected]> 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: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Hi Kalle,
On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <[email protected]> wrote:
> Sudip Mukherjee <[email protected]> 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: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Julian Calaby <[email protected]> writes:
> Hi Kalle,
>
> On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <[email protected]> wrote:
>> Sudip Mukherjee <[email protected]> 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
Hi Kalle,
On Wed, Mar 16, 2016 at 8:42 PM, Julian Calaby <[email protected]> wrote:
> Hi Kalle,
>
> On Wed, Mar 16, 2016 at 8:22 PM, Kalle Valo <[email protected]> wrote:
>> Julian Calaby <[email protected]> writes:
>>
>>> Hi Kalle,
>>>
>>> On Mon, Feb 1, 2016 at 7:21 PM, Kalle Valo <[email protected]> wrote:
>>>> Sudip Mukherjee <[email protected]> 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: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/