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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux