[PATCH 4/5] staging: r8712u: Merging Realtek's latest. Mostly types, defines and includes.
ali at internetdog.org
Tue Aug 23 06:50:50 PDT 2011
sorry if I seem a bit reluctant here. I just spent 3 days testing the
Keep the feedback coming. Hopefully, I'll have a bit more
energy in a few days for testing the next version of the patches.
On Tue, Aug 23, 2011 at 02:35:55PM +0300, Dan Carpenter wrote:
> On Tue, Aug 23, 2011 at 04:41:32PM +0800, Ali Bahar wrote:
> > You need to realize that there were 165 files which showed changes!
> Patches 1 and 2 are good. Then there are only 43 files left.
And those 43 took only 3 weeks! :-)
> Run git citool. Highlight all the copyright notices. Right click
> and select "Stage Lines for commit". That brings you down to 35 files.
Yes, this will handle the copyright stuff. (Commit 1)
> Since we already discussed it, the _netdev_priv() to netdev_priv() is
> simple. Make that a single patch. The os_free_netdev() is the same
> and so are the _usb_alloc_urb() and _usb_submit_urb() patches.
Yes, this, too, will compile w/o a headache. (Commit 2)
> Changing #includes can break the build but in this case it's fine to
> just merge all the include changes in one commit. I tested it, and
> it compiles ok.
OK. (Commit 3)
> The rest is probably spaghetti changes like you said. There are some
> one liners here and there.
> LedControl871x() removes an unneeded NULL check. (The address of a
> stack variable is never NULL).
Yes, there are a number of such things in the code. Frankly, amidst
all the whitespace and trivial/cosmetic changes sucking-up time, I
chose not to dwell on them.
There are numerous optimizations which can be done here. But, the
plan for this driver includes eventual rewrite.
> There was an unused macro deleted in osdep_service.h
> -#define MSECS(t) (HZ * ((t) / 1000) + (HZ * ((t) % 1000)) / 1000)
Realtek had moved it to another file. But, since it was not used, I
removed it altogther.
> The changes in rtl8712_cmd.c are purely whitespace changes. It looks
> like the intent was to fix a bug, but the patch doesn't actually do
The cpu_to_le32 call was moved a few lines down, after the if-else.
> The casting changes in r8712_wep_encrypt() are wrong:
> - *((u32 *)crc) = cpu_to_le32(getcrc32(
> + *((unsigned long *)crc) = cpu_to_le32(getcrc32(
> payload, length));
> crc is 32 bits so casting it to long will just cause memory
> corruption on 64 bit systems. I'm not sure what was intended. Same
> in r8712_wep_decrypt().
Ouch! Good catch! This was one of _Realtek's_ changes, so I checked
only the lvalue for pointer-size.
I'll sift thru the code, again, for this sort of thing.
> That still leaves a huge 1000 line patch... But yeah we may not have
> a lot of options.
That's the thing. The sum total of the commits will not change, nor
will the commit logs improve for most of the code; and it's already
a lot prettier than it was (in the development branch), and turned out
far better than it was likely to have.
> dan carpenter
More information about the devel