[PATCH net-next V2 00/16] net: fec: cleanup and fixes

On 2/24/2016 7:52 PM, Joshua Clayton wrote:

Hello Troy,
I'm replying here instead of to a particular commit because several of
the commit messages seem inadequate.

The first line summaries all look good.

The descriptions should each also include the "user visible impact" of
the patch and the justification for it (i.e. why you made the change).

For instance, patch 3 doesn't include either what will change
 (nothing, I'm guessing?) or why we now pass in the structures
instead of a queue_id. 

I can add to the commit message, that this is in preparation for
patch 4 which depends on it. Or I could squash patches 2/3/4
together, but I think it is easier to review smaller patches.

You've also got a few (e.g. patch 9, patch 14) where the substance
of the patch is in the summary,

but missing from the message.

These kind of descriptions are very hard to review since the expression
is split between the subject of the email and the body of the email, which
are not close
together in some email programs.

Better to reiterate or elaborate on the summary in the message.
In patch 9, for instance, it would be more clear to say:

Move restart test to earlier in fec_txq() which saves one comparison. 

I can do this. And change patch 14 to read


Create subroutine reset_tx_queue to have one place
to release any queued tx skbs.

Any other commit messages you'd like to improve?

P.S I'm a little confused, as I came looking for a v3 of the first 8 patches
and found these instead. I'll try to give your first 8 a look when they show up.

The 1st 8 patches have already been applied. I added a patch to address your review there
at the end of the series. So, that patch will show up in my next set.


Thanks for the review

Troy