On Mon, 22 Jun 2009 16:44:53 -0700, Sarah Sharp <sarah.a.sharp@xxxxxxxxxxxxxxx> wrote:
This looks good, but if I were doing it, I would simplify it. Look:
> +++ b/drivers/usb/host/xhci-hcd.c
> @@ -1026,6 +1026,46 @@ void xhci_reset_bandwidth(struct usb_hcd *hcd, struct usb_device *udev)
> +void xhci_start_endpoint_reset(struct usb_hcd *hcd,
> + struct endpoint_reset_callback *callback)
> +{
> + reset_completion = kzalloc(sizeof(*reset_completion), GFP_ATOMIC);
> + if (!reset_completion) {
> + xhci_warn(xhci, "Could not allocate reset command completion\n");
> + return;
> + }
This is a bug, right? Looks like the caller will hang forever.
> + INIT_LIST_HEAD(&reset_completion->cmd_list);
While we're at it, why initiate a list head which you're about to add?
> + xhci_dbg(xhci, "Queueing reset endpoint command\n");
> + spin_lock_irqsave(&xhci->lock, flags);
> + ret = xhci_queue_reset_ep(xhci, slot_id, ep_index);
> + if (!ret) {
> + list_add_tail(&reset_completion->cmd_list, &xhci->devs[slot_id]->cmd_list);
> + xhci_ring_cmd_db(xhci);
> + }
And if the ret is nonzero, what happens? A leak of struct
xhci_command_completion?
None of these problems would arise if you placed a list_head
into struct endpoint_reset_callback instead of creating struct
xhci_command_completion. It is a bit of a layering violation,
but nothing major.
I cannot judge adding a return code to handle_cmd_completion(),
maybe it was the right thing to do.
-- Pete
--
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
