On Tue, Aug 23, 2011 at 09:50:50PM +0800, Ali Bahar wrote: > Hi Dan, > > sorry if I seem a bit reluctant here. I just spent 3 days testing the > patches. Sorry for this. I thought you worked for RealTek. Why are those guys hoarding their code and making you reverse engineer it? If they had submitted it to us, we could have fixed some of the bugs in it. Does anyone there have an email where we could at least CC them? Anyway, the last 3 patches will need to be redone. With git citool, it's pretty easy. Highlight what you want in the patch. Stage it. Commit. > > 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) > I would prefer four commits. It's the same amount of work either way isn't it? > > 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. > Larry, can you chime in here? Are we really planning to delete this driver? Could you update the TODO if that's the situation. The point is that with one liners you can easily just commit them as a separate commit without thinking too hard. Low hanging fruit. > > 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. > Yeah. The if condition modifies "blnPending" but that doesn't affect the call to cpu_to_le32() so this is just a white space change. > > 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. > I would drop all the changes to drivers/staging/rtl8712/rtl871x_security.c None of them make sense. > > > > 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 for your work on this. Btw in drivers/staging/rtl8712/usb_ops_linux.c the patch changes pipe to be a an int, when it should be an unsigned int. I don't know why they would do that, but it's wrong. regards, dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
