On Thu, Aug 22, 2013 at 06:20:14PM +0300, Xenia Ragiadakou wrote: > This patch refactors the switch statement in handle_cmd_completion() by > creating a separate function for each branch, named 'xhci_handle_cmd_<type>'. > > Other changes introduced by this patch are: > > - Renaming the already existed functions by adding the prefix > 'xhci_handle_cmd_' for consistency. Also, for handle_stopped_endpoint() > the order of arguments was changed too, so that the prototype of > 'xhci_handle_cmd_' be similar and easy to follow. > - Additional local variables were added, such as cmd_trb, cmd_comp_code, > cmd_type, add_flags and drop_flags, and the label 'update_ring', > mainly to reduce code dublication and line length. > - The variable ep_ring, that was assigned in the case TRB_CONFIG_EP > but never used, was removed. Can you break these changes into several patches, in order to make it easier to review? Something like: 1. Rename existing functions. 2. Change ordering of arguments for consistency. 3. Remove the unused variable from the TRB_CONFIG_EP case. 4. Several patches to refactor the code. For the refactoring, break each refactoring into a separate patch. I.e. refactoring and creating xhci_handle_cmd_enable_slot goes in one patch, xhci_handle_cmd_config_ep goes in another patch. This patch is nearing the 400-line fatigue point of most reviewers, see the article on Greg's google plus post for an interesting analysis: https://plus.google.com/111049168280159033135/posts/2KcFNwzzEgC Sarah Sharp > > Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx> > --- > > Differences from version 1: > > Create separate functions for each switch case branch, instead of a single > function holding the entire switch > > drivers/usb/host/xhci-ring.c | 310 ++++++++++++++++++++++++++----------------- > 1 file changed, 185 insertions(+), 125 deletions(-) > > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index ddbda35..bc9ce03 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -755,8 +755,8 @@ static void xhci_giveback_urb_in_irq(struct xhci_hcd *xhci, > * 2. Otherwise, we turn all the TRBs in the TD into No-op TRBs (with the chain > * bit cleared) so that the HW will skip over them. > */ > -static void handle_stopped_endpoint(struct xhci_hcd *xhci, > - union xhci_trb *trb, struct xhci_event_cmd *event) > +static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, > + struct xhci_event_cmd *event, union xhci_trb *trb) > { > unsigned int slot_id; > unsigned int ep_index; > @@ -1063,9 +1063,8 @@ static void update_ring_for_set_deq_completion(struct xhci_hcd *xhci, > * endpoint doorbell to restart the ring, but only if there aren't more > * cancellations pending. > */ > -static void handle_set_deq_completion(struct xhci_hcd *xhci, > - struct xhci_event_cmd *event, > - union xhci_trb *trb) > +static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, > + struct xhci_event_cmd *event, union xhci_trb *trb) > { > unsigned int slot_id; > unsigned int ep_index; > @@ -1157,9 +1156,8 @@ static void handle_set_deq_completion(struct xhci_hcd *xhci, > ring_doorbell_for_active_rings(xhci, slot_id, ep_index); > } > > -static void handle_reset_ep_completion(struct xhci_hcd *xhci, > - struct xhci_event_cmd *event, > - union xhci_trb *trb) > +static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci, > + struct xhci_event_cmd *event, union xhci_trb *trb) > { > int slot_id; > unsigned int ep_index; > @@ -1372,21 +1370,160 @@ static int handle_stopped_cmd_ring(struct xhci_hcd *xhci, > return cur_trb_is_good; > } > > -static void handle_cmd_completion(struct xhci_hcd *xhci, > +static void xhci_handle_cmd_enable_slot(struct xhci_hcd *xhci, > + struct xhci_event_cmd *event, u32 cmd_comp_code) > +{ > + int slot_id; > + > + slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags)); > + if (cmd_comp_code == COMP_SUCCESS) > + xhci->slot_id = slot_id; > + else > + xhci->slot_id = 0; > + complete(&xhci->addr_dev); > +} > + > +static void xhci_handle_cmd_disable_slot(struct xhci_hcd *xhci, > struct xhci_event_cmd *event) > { > - int slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags)); > - u64 cmd_dma; > - dma_addr_t cmd_dequeue_dma; > - struct xhci_input_control_ctx *ctrl_ctx; > + int slot_id; > struct xhci_virt_device *virt_dev; > + > + slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags)); > + virt_dev = xhci->devs[slot_id]; > + if (!virt_dev) > + return; > + if (xhci->quirks & XHCI_EP_LIMIT_QUIRK) > + /* Delete default control endpoint resources */ > + xhci_free_device_endpoint_resources(xhci, virt_dev, true); > + xhci_free_virt_device(xhci, slot_id); > +} > + > +static void xhci_handle_cmd_config_ep(struct xhci_hcd *xhci, > + struct xhci_event_cmd *event, u32 cmd_comp_code) > +{ > + int slot_id; > + struct xhci_virt_device *virt_dev; > + struct xhci_input_control_ctx *ctrl_ctx; > unsigned int ep_index; > - struct xhci_ring *ep_ring; > unsigned int ep_state; > + u32 add_flags, drop_flags; > + > + slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags)); > + virt_dev = xhci->devs[slot_id]; > + if (handle_cmd_in_cmd_wait_list(xhci, virt_dev, event)) > + return; > + /* > + * Configure endpoint commands can come from the USB core > + * configuration or alt setting changes, or because the HW > + * needed an extra configure endpoint command after a reset > + * endpoint command or streams were being configured. > + * If the command was for a halted endpoint, the xHCI driver > + * is not waiting on the configure endpoint command. > + */ > + ctrl_ctx = xhci_get_input_control_ctx(xhci, virt_dev->in_ctx); > + if (!ctrl_ctx) { > + xhci_warn(xhci, "Could not get input context, bad type.\n"); > + return; > + } > + add_flags = le32_to_cpu(ctrl_ctx->add_flags); > + drop_flags = le32_to_cpu(ctrl_ctx->drop_flags); > + /* Input ctx add_flags are the endpoint index plus one */ > + ep_index = xhci_last_valid_endpoint(add_flags) - 1; > + /* A usb_set_interface() call directly after clearing a halted > + * condition may race on this quirky hardware. Not worth > + * worrying about, since this is prototype hardware. Not sure > + * if this will work for streams, but streams support was > + * untested on this prototype. > + */ > + if (xhci->quirks & XHCI_RESET_EP_QUIRK && > + ep_index != (unsigned int) -1 && > + add_flags - SLOT_FLAG == drop_flags) { > + ep_state = virt_dev->eps[ep_index].ep_state; > + if (!(ep_state & EP_HALTED)) > + goto bandwidth_change; > + xhci_dbg_trace(xhci, trace_xhci_dbg_quirks, > + "Completed config ep cmd - " > + "last ep index = %d, state = %d", > + ep_index, ep_state); > + /* Clear internal halted state and restart ring(s) */ > + virt_dev->eps[ep_index].ep_state &= ~EP_HALTED; > + ring_doorbell_for_active_rings(xhci, slot_id, ep_index); > + return; > + } > +bandwidth_change: > + xhci_dbg_trace(xhci, trace_xhci_dbg_context_change, > + "Completed config ep cmd"); > + virt_dev->cmd_status = cmd_comp_code; > + complete(&virt_dev->cmd_completion); > +} > + > +static void xhci_handle_cmd_eval_ctx(struct xhci_hcd *xhci, > + struct xhci_event_cmd *event, u32 cmd_comp_code) > +{ > + int slot_id; > + struct xhci_virt_device *virt_dev; > + > + slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags)); > + virt_dev = xhci->devs[slot_id]; > + if (handle_cmd_in_cmd_wait_list(xhci, virt_dev, event)) > + return; > + virt_dev->cmd_status = cmd_comp_code; > + complete(&virt_dev->cmd_completion); > +} > + > +static void xhci_handle_cmd_addr_dev(struct xhci_hcd *xhci, > + struct xhci_event_cmd *event, u32 cmd_comp_code) > +{ > + int slot_id; > + > + slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags)); > + xhci->devs[slot_id]->cmd_status = cmd_comp_code; > + complete(&xhci->addr_dev); > +} > + > +static void xhci_handle_cmd_reset_dev(struct xhci_hcd *xhci, > + struct xhci_event_cmd *event) > +{ > + int slot_id; > + struct xhci_virt_device *virt_dev; > + > + xhci_dbg(xhci, "Completed reset device command.\n"); > + slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags)); > + virt_dev = xhci->devs[slot_id]; > + if (virt_dev) > + handle_cmd_in_cmd_wait_list(xhci, virt_dev, event); > + else > + xhci_warn(xhci, "Reset device command completion " > + "for disabled slot %u\n", slot_id); > +} > + > +static void xhci_handle_cmd_nec_get_fw(struct xhci_hcd *xhci, > + struct xhci_event_cmd *event) > +{ > + if (!(xhci->quirks & XHCI_NEC_HOST)) { > + xhci->error_bitmask |= 1 << 6; > + return; > + } > + xhci_dbg_trace(xhci, trace_xhci_dbg_quirks, > + "NEC firmware version %2x.%02x", > + NEC_FW_MAJOR(le32_to_cpu(event->status)), > + NEC_FW_MINOR(le32_to_cpu(event->status))); > +} > + > +static void handle_cmd_completion(struct xhci_hcd *xhci, > + struct xhci_event_cmd *event) > +{ > + u64 cmd_dma; > + dma_addr_t cmd_dequeue_dma; > + union xhci_trb *cmd_trb; > + u32 cmd_comp_code; > + u32 cmd_type; > > cmd_dma = le64_to_cpu(event->cmd_trb); > + cmd_trb = xhci->cmd_ring->dequeue; > cmd_dequeue_dma = xhci_trb_virt_to_dma(xhci->cmd_ring->deq_seg, > - xhci->cmd_ring->dequeue); > + cmd_trb); > /* Is the command ring deq ptr out of sync with the deq seg ptr? */ > if (cmd_dequeue_dma == 0) { > xhci->error_bitmask |= 1 << 4; > @@ -1398,142 +1535,65 @@ static void handle_cmd_completion(struct xhci_hcd *xhci, > return; > } > > - trace_xhci_cmd_completion(&xhci->cmd_ring->dequeue->generic, > - (struct xhci_generic_trb *) event); > + trace_xhci_cmd_completion(cmd_trb, (struct xhci_generic_trb *) event); > > - if ((GET_COMP_CODE(le32_to_cpu(event->status)) == COMP_CMD_ABORT) || > - (GET_COMP_CODE(le32_to_cpu(event->status)) == COMP_CMD_STOP)) { > + cmd_comp_code = GET_COMP_CODE(le32_to_cpu(event->status)); > + if (cmd_comp_code == COMP_CMD_ABORT || cmd_comp_code == COMP_CMD_STOP) { > /* If the return value is 0, we think the trb pointed by > * command ring dequeue pointer is a good trb. The good > * trb means we don't want to cancel the trb, but it have > * been stopped by host. So we should handle it normally. > * Otherwise, driver should invoke inc_deq() and return. > */ > - if (handle_stopped_cmd_ring(xhci, > - GET_COMP_CODE(le32_to_cpu(event->status)))) { > - inc_deq(xhci, xhci->cmd_ring); > - return; > - } > + if (handle_stopped_cmd_ring(xhci, cmd_comp_code)) > + goto update_ring; > } > > - switch (le32_to_cpu(xhci->cmd_ring->dequeue->generic.field[3]) > - & TRB_TYPE_BITMASK) { > - case TRB_TYPE(TRB_ENABLE_SLOT): > - if (GET_COMP_CODE(le32_to_cpu(event->status)) == COMP_SUCCESS) > - xhci->slot_id = slot_id; > - else > - xhci->slot_id = 0; > - complete(&xhci->addr_dev); > + cmd_type = TRB_FIELD_TO_TYPE(le32_to_cpu(cmd_trb->generic.field[3])); > + > + switch (cmd_type) { > + > + case TRB_ENABLE_SLOT: > + xhci_handle_cmd_enable_slot(xhci, event, cmd_comp_code); > break; > - case TRB_TYPE(TRB_DISABLE_SLOT): > - if (xhci->devs[slot_id]) { > - if (xhci->quirks & XHCI_EP_LIMIT_QUIRK) > - /* Delete default control endpoint resources */ > - xhci_free_device_endpoint_resources(xhci, > - xhci->devs[slot_id], true); > - xhci_free_virt_device(xhci, slot_id); > - } > + case TRB_DISABLE_SLOT: > + xhci_handle_cmd_disable_slot(xhci, event); > break; > - case TRB_TYPE(TRB_CONFIG_EP): > - virt_dev = xhci->devs[slot_id]; > - if (handle_cmd_in_cmd_wait_list(xhci, virt_dev, event)) > - break; > - /* > - * Configure endpoint commands can come from the USB core > - * configuration or alt setting changes, or because the HW > - * needed an extra configure endpoint command after a reset > - * endpoint command or streams were being configured. > - * If the command was for a halted endpoint, the xHCI driver > - * is not waiting on the configure endpoint command. > - */ > - ctrl_ctx = xhci_get_input_control_ctx(xhci, > - virt_dev->in_ctx); > - if (!ctrl_ctx) { > - xhci_warn(xhci, "Could not get input context, bad type.\n"); > - break; > - } > - /* Input ctx add_flags are the endpoint index plus one */ > - ep_index = xhci_last_valid_endpoint(le32_to_cpu(ctrl_ctx->add_flags)) - 1; > - /* A usb_set_interface() call directly after clearing a halted > - * condition may race on this quirky hardware. Not worth > - * worrying about, since this is prototype hardware. Not sure > - * if this will work for streams, but streams support was > - * untested on this prototype. > - */ > - if (xhci->quirks & XHCI_RESET_EP_QUIRK && > - ep_index != (unsigned int) -1 && > - le32_to_cpu(ctrl_ctx->add_flags) - SLOT_FLAG == > - le32_to_cpu(ctrl_ctx->drop_flags)) { > - ep_ring = xhci->devs[slot_id]->eps[ep_index].ring; > - ep_state = xhci->devs[slot_id]->eps[ep_index].ep_state; > - if (!(ep_state & EP_HALTED)) > - goto bandwidth_change; > - xhci_dbg_trace(xhci, trace_xhci_dbg_quirks, > - "Completed config ep cmd - " > - "last ep index = %d, state = %d", > - ep_index, ep_state); > - /* Clear internal halted state and restart ring(s) */ > - xhci->devs[slot_id]->eps[ep_index].ep_state &= > - ~EP_HALTED; > - ring_doorbell_for_active_rings(xhci, slot_id, ep_index); > - break; > - } > -bandwidth_change: > - xhci_dbg_trace(xhci, trace_xhci_dbg_context_change, > - "Completed config ep cmd"); > - xhci->devs[slot_id]->cmd_status = > - GET_COMP_CODE(le32_to_cpu(event->status)); > - complete(&xhci->devs[slot_id]->cmd_completion); > + case TRB_CONFIG_EP: > + xhci_handle_cmd_config_ep(xhci, event, cmd_comp_code); > break; > - case TRB_TYPE(TRB_EVAL_CONTEXT): > - virt_dev = xhci->devs[slot_id]; > - if (handle_cmd_in_cmd_wait_list(xhci, virt_dev, event)) > - break; > - xhci->devs[slot_id]->cmd_status = GET_COMP_CODE(le32_to_cpu(event->status)); > - complete(&xhci->devs[slot_id]->cmd_completion); > + case TRB_EVAL_CONTEXT: > + xhci_handle_cmd_eval_ctx(xhci, event, cmd_comp_code); > break; > - case TRB_TYPE(TRB_ADDR_DEV): > - xhci->devs[slot_id]->cmd_status = GET_COMP_CODE(le32_to_cpu(event->status)); > - complete(&xhci->addr_dev); > + case TRB_ADDR_DEV: > + xhci_handle_cmd_addr_dev(xhci, event, cmd_comp_code); > break; > - case TRB_TYPE(TRB_STOP_RING): > - handle_stopped_endpoint(xhci, xhci->cmd_ring->dequeue, event); > + case TRB_STOP_RING: > + xhci_handle_cmd_stop_ep(xhci, event, cmd_trb); > break; > - case TRB_TYPE(TRB_SET_DEQ): > - handle_set_deq_completion(xhci, event, xhci->cmd_ring->dequeue); > + case TRB_SET_DEQ: > + xhci_handle_cmd_set_deq(xhci, event, cmd_trb); > break; > - case TRB_TYPE(TRB_CMD_NOOP): > + case TRB_CMD_NOOP: > break; > - case TRB_TYPE(TRB_RESET_EP): > - handle_reset_ep_completion(xhci, event, xhci->cmd_ring->dequeue); > + case TRB_RESET_EP: > + xhci_handle_cmd_reset_ep(xhci, event, cmd_trb); > break; > - case TRB_TYPE(TRB_RESET_DEV): > - xhci_dbg(xhci, "Completed reset device command.\n"); > - slot_id = TRB_TO_SLOT_ID( > - le32_to_cpu(xhci->cmd_ring->dequeue->generic.field[3])); > - virt_dev = xhci->devs[slot_id]; > - if (virt_dev) > - handle_cmd_in_cmd_wait_list(xhci, virt_dev, event); > - else > - xhci_warn(xhci, "Reset device command completion " > - "for disabled slot %u\n", slot_id); > + case TRB_RESET_DEV: > + xhci_handle_cmd_reset_dev(xhci, event); > break; > - case TRB_TYPE(TRB_NEC_GET_FW): > - if (!(xhci->quirks & XHCI_NEC_HOST)) { > - xhci->error_bitmask |= 1 << 6; > - break; > - } > - xhci_dbg_trace(xhci, trace_xhci_dbg_quirks, > - "NEC firmware version %2x.%02x", > - NEC_FW_MAJOR(le32_to_cpu(event->status)), > - NEC_FW_MINOR(le32_to_cpu(event->status))); > + case TRB_NEC_GET_FW: > + xhci_handle_cmd_nec_get_fw(xhci, event); > break; > default: > /* Skip over unknown commands on the event ring */ > xhci->error_bitmask |= 1 << 6; > break; > } > + > +update_ring: > inc_deq(xhci, xhci->cmd_ring); > + return; > } > > static void handle_vendor_event(struct xhci_hcd *xhci, > -- > 1.8.3.4 > -- 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
