[PATCH 4/5] staging: r8712u: Merging Realtek's latest. Mostly types, defines and includes.

[Ali Bahar]

Hi Dan,

sorry if I seem a bit reluctant here. I just spent 3 days testing the
patches.  
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
> anything.

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.
Thanks.

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.

thanks,
ali

 
> regards,
> dan carpenter
>