Fine fine. We're on the same page then. On Wed, Jun 05, 2013 at 01:13:07PM +0300, Xenia Ragiadakou wrote: > >Also read_nic_byte_E() now returns an error code but we don't check > >it so we've just moved the uninitialized code problem around a bit > >but not fixed it. The error handling could be added in a follow on > >patch because that doesn't introduce any bugs that weren't there in > >the original code. > > Yes, i will do it in a following patch but there are a lot of calls > to read_nic_* (maybe i will put if (unlikely(ret))) > If you want to break it up into many patches that's fine too. The point is that now at least the callers can check for errors if they want to so it's an improvement. Don't use unlikely(). It makes the code less readable. The rule is don't use likely/unlikely() in drivers OR don't use likely/unlikely() without a benchmark to show that it improves performance. > >> status = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), > >> RTL8187_REQ_GET_REGS, RTL8187_REQT_READ, > >>- (indx&0xff)|0xff00, (indx>>8)&0x0f,&data, 1, HZ / 2); > > (indx& 0xff) | 0xff00, (indx>> 8)& 0x0f, > > data, 1, HZ / 2); > > > >Also "indx" is a weird way to abreviate. Normally it would be "idx" > >or spelled out all the way "index". > > indx maybe wants to show a relation with the wIndex field of the > setup packet (sent > when performing a control transfer). > Here, there is a bug also, introduced by me (&data should be data), > i overlooked it (sorry!!) No no. Your final code has "data" this is showing the original code. The compiler would have warned about any bugs like that. regards, dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
