cvs commit: src/sys/dev/em if_em.c if_em.h

[Jack Vogel]

On 11/10/06, Gleb Smirnoff <glebius at freebsd.org> wrote:
>   Jack,
>
> On Fri, Nov 10, 2006 at 09:30:27AM +0000, Jack F Vogel wrote:
> J> jfv         2006-11-10 09:30:27 UTC
> J>
> J>   FreeBSD src repository
> J>
> J>   Modified files:        (Branch: RELENG_6)
> J>     sys/dev/em           if_em.c if_em.h
> J>   Log:
> J>   This patch redesigns the watchdog timer. The old version had SMP
> J>   vulnerabilities. It also has the FAST_INTR code as a compile option.
> J>
> J>   Submitted by: jfv
> J>   Reviewed by: scottl
> J>   Approved by: re, scottl
> J>
> J>   Revision   Changes    Path
> J>   1.65.2.21  +297 -87   src/sys/dev/em/if_em.c
> J>   1.32.2.6   +11 -3     src/sys/dev/em/if_em.h
>
> This patch also does a number of other changes, that aren't listed
> in the commit message:
>
> 1) Merges revision 1.159 by jhb, that locks callouts properly.
> 2) Merges revisions 1.160, 1.161 by jhb, that move the
>    allocation/destruction of receive/transmit structures to the
>    device attach and detach methods.
> 3) Backouts revision 1.80 by glebius.
> 4) Fixes mbuf handling error in em_encap() that would let to make
>    manipulations with freed mbufs.
> 5) Removes bogus declaration from if_em.c.
>
> In the FreeBSD CVS there is a habit to list all the changes made,
> don't hide anything. A detailed CVS log, that includes all the
> changes made, including references to original revisions in HEAD,
> makes easier the work of other people in the project and outside
> of it. It is easier to review and understand such changes, easier
> to apply it to outside source trees, easier to backout to check
> for regressions.
>
> Also, it is preferrable not to put a lot of different changes
> into one commit, just dropping your own source tree above the
> CVS checkout and typing "cvs ci". It is better to put effort
> into separating each logical change into one separate patch and
> commit it. Again, this will make easier to understand and to
> make partial backouts in case of regressions.

Not trying to hide anything Gleb, what I checked in was the patch
that was posted to stable. OK, so it wasnt broken down into
each individual piece, or I didnt list them all, I will try to be more
dutiful on that kind of detail.

> Also, AFAIR, this patch was expected to fix kern/104978 problem
> report. In perfect case, you should had assigned the PR to
> yourself as soon as it arrived, because it is a regression
> that was made by your commit, so you take the responsibility
> with this PR. Then, the patch should had been sent to the PR
> originator for testing. And finally, the commit message should
> have referenced the PR.

Still not familiar with dealing with PRs.

> Also, in FreeBSD CVS there is a habit to give credit to all
> parties involved in the work, including people who reported
> the problem, who tested the patch, who reviewed it, commented
> or submitted parts of the patch.

I did give all parties credit back when the patch was posted to
mailing list.

> In the discussed commit you should have added me as reviewer,
> or even as submitter, because I have suggested to piggyback on
> local timer and I've submitted original patch, that does this.
> Also, I have pointed out the mbuf handling error in em_encap(),
> even have written two emails to explain that to you.
> The forward declaration fix was submitted by Ed Maste.
>
> And concerning backing out revision 1.80. I am too tired to
> speak in support of "Glebs beloved infinite loop". This question
> was raised so many times during last year. I will repeat my words
> for the last time, as this mail goes to public list and I will be
> able to just point people at it. A year ago, I have spent several
> weeks fighting with wedging interfaces on my routers and finally
> came to this loop. This loop fixes a PRACTICALLY encountered
> problem, and now you have removed it, because it THEORETICALLY can
> cause problems. Ok... You are maintainer of the driver, you decide.
> I will not insist on this loop anymore, I will just have a patched
> em(4) driver for my job, as many companies already have.

I talked with Scott specifically about this, he supported this change.
You said before that FAST_INTR solves that problem for you, my
intention was that you use that solution.

> Finally, I will just notice that two more FreeBSD policies were
> violated by your last commits: committing directly to STABLE
> branch, omitting mentor's approval.

This was NO violation Gleb, I had email and approval from RE and
Scott to check this code in, thats why my checkin said it was
approved by RE.

I am sorry that this checkin seems to have aggrevated you so
much, I have just been trying to make this release a success.

Jack