[PATCH 4/5] staging: r8712u: Merging Realtek's latest. Mostly types, defines and includes.
Ali Bahar
ali at internetdog.org
Tue Aug 23 16:39:04 UTC 2011
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
>
More information about the devel
mailing list