Messages in this thread | | | Date | Tue, 22 Nov 2011 12:32:45 -0800 | Subject | Re: [PATCH net-next v1 3/4] net-e1000e: reworked carrier detection logic | From | Maciej Żenczykowski <> |
| |
David,
I know you asked for a review of this on Friday, before you sent this out to netdev, and I was planning on doing so, but I see you've sent it out to a wider audience now. So, I'll attach comments directly here.
You took my patch (which was for 2.6.34) and split it in two (it was erroneously marked author zenczykowski@gmail.com instead of maze@google.com, so that should be fixed) and while you seem to have reduced the amount of lines-of-code-churn, I'm not sure you've actually made the resulting code (as a whole) more readable.
I'm not sure how much of these changes were a result of changes elsewhere in the driver between 2.6.34 and now.
This was very tricky code to get right, and I'm not sure whether the patch as you posted it gets all the corner cases right... Indeed I actually think the changes you've made reduce the robustness wrt. timeouts caused by firmware concurrent accesses. In particular the first read access you make is very likely to fail. I think renaming success to link and iterations to intervals was worthwhile since it improved readability...
For reference, the original patch, cherrypicked onto net-next and compile (but not anything else) tested will be a reply to this email.
| |