Hi, On 08/04/2014 05:24 PM, Alan Stern wrote: > On Mon, 4 Aug 2014, Mathias Nyman wrote: > >>> --- a/drivers/usb/host/xhci-ring.c >>> +++ b/drivers/usb/host/xhci-ring.c >>> @@ -1718,10 +1718,12 @@ cleanup: >>> * TRB in this TD, this function returns that TRB's segment. Otherwise it >>> * returns 0. >>> */ >>> -struct xhci_segment *trb_in_td(struct xhci_segment *start_seg, >>> +struct xhci_segment *trb_in_td(struct xhci_hcd *xhci, >>> + struct xhci_segment *start_seg, >>> union xhci_trb *start_trb, >>> union xhci_trb *end_trb, >>> - dma_addr_t suspect_dma) >>> + dma_addr_t suspect_dma, >>> + bool debug) >> >> The added debug information is useful, but I'm not a big fan of the boolean debug function parameter. >> >> I get that we only want to print the information when we really expect the trb to be in the TD, and fail. >> This is a simple way of doing it, but reading the code with lots of true / false function arguments is difficult. >> (xhci has too many of them already) >> >> I haven't got a better solution, all other variants seems to have their own drawbacks. >> New suggestions are welcome > > This function could be made private. Then there could be two separate > public functions to call it, one with the debug parameter set to true > and one with the parameter set to false. Not sure that kind of indirection makes things better, it feels like obfuscation to me. > The parameter's name could be changed to something more readable, such > as warn_if_not_found. But that is not what it does, it prints debug information *while* it is searching. If things were as simple as warn if not found, we could simply always warn. The problem is that by the time we know the TRB is not found in the TD, we may have already gone through the loop multiple times, and we want a log message for each iteration of the loop when this happens. So one way or the other we need to go through the loop twice. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
