On Tue, Aug 23, 2011 at 06:46:59PM +0300, Dan Carpenter wrote: > 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? I think Larry's the contact. _I_ just volunteered to merge the tarball they have on their site. > 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. OK. > > > 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? OK. > > > 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. I guess what I was tacitly conveying was that only a tiny percentage of the change can be isolated like this; we're still left with one big-ass patch, regardless! Still, I'll re-do the patches. > > > 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. OK. > > > 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. OK. I'm not surprised. > > > 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 OK. > why they would do that, but it's wrong. No shortage of such observations here! thanks, ali > regards, > dan carpenter > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
