On Wed, 8 Nov 2006, Mariusz Kozlowski wrote:
> Hello,
>
> > I encourage you to go ahead and change code sequences like that one also,
> > whenever it's safe to do so.
>
> Ok I'll make another approach when those 33 get merged.
>
> The thing is there are simple code sequences that can be fixed:
>
> if (urb) {
> usb_kill_urb(urb);
> usb_free_urb(urb);
> }
>
> then there are more complex ones like this one:
>
> if (urb) {
> dbg("we hit this and blah blah ...");
> usb_kill_urb(urb);
> usb_free_urb(urb);
> urb = NULL;
> }
>
> which probably should not be touched as it would cause them to emit those
> debug messages all the time the code sequence is executed even if urb == NULL.
It depends... In my experience, code like that almost always executes
with urb != NULL. NULL values usually occur only in error pathways.
You'd have to check each individual case to make sure, of course.
On the other hand, if the dbg() statement actually tries to dereference
urb then you can't pull it outside the conditional. But you could still
remove usb_kill_urb and usb_free_urb from the conditional's body. That
way a non-debug build would eliminate the test entirely, since the body
would be empty.
> The other story are sequences executed in a loop:
>
> for (i = 0; ...; i++) {
> if (urb) {
> usb_kill_urb(urb);
> usb_free_urb(urb);
> urb = NULL;
> }
> }
>
> where removing 'if' causes calling all three lines each time even if urb ==
> NULL
> thus probably resulting in some slowdown(?). Ayway I don't think the slowdown
> is
> important here and those 'if's can be safely removed.
You'd have to have an awful lot of loop iterations before that would make
a noticeable difference in speed.
> The next thing found when digging the code is this common sequence:
>
> if (mc->data)
> usb_buffer_free(dev, URB_INT_SIZE, mc->data, mc->data_dma);
>
> which is also redundant. This is from 2.6.19-rc5-mm2:
>
> void usb_buffer_free (struct usb_device *dev, size_t size, void *addr,
> dma_addr_t dma)
> {
> if (!dev || !dev->bus)
> return;
> if (!addr)
> return;
> hcd_buffer_free (dev->bus, size, addr, dma);
> }
>
> The only thing is that 'addr' is not checked at the beggining so
> usb_buffer_free()
> might be rewritten to:
>
> void usb_buffer_free (struct usb_device *dev, size_t size, void *addr,
> dma_addr_t dma)
> {
> if (!addr || !dev || !dev->bus)
> return;
> hcd_buffer_free (dev->bus, size, addr, dma);
> }
>
> what makes sense to me because the 'addr' pointer is what this function is
> about so it
> should be checked first.
Either way would be okay.
> I could do that in one series of patches. It is simple enough not to break
> things and
> hopefuly remove lots of redundant code. What do you thik of these (proposed)
> changes?
They all sound good.
Alan Stern
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
