On Fri, Aug 23, 2013 at 11:15:15AM +0300, Xenia Ragiadakou wrote:
> Since the Slot ID field in the command completion event matches the Slot ID
> field in the associated command TRB for the Stop Endpoint, Set Dequeue Pointer
> and Reset Endpoint commands, this patch adds in the handlers of their
> completion events a 'slot_id' argument and removes the slot id calculation
> in each of them.
Hmm, again, this is relying on the Slot ID that the host controller
generates, instead of the one the xHCI driver has placed on the command
ring.
Can you add a WARN_ON() in handle_cmd_completion() if the slot ID from
the command completion event doesn't match the one on the command ring
dequeue pointer? If you do that, then this patch is fine.
Sarah Sharp
> Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
> ---
> drivers/usb/host/xhci-ring.c | 19 ++++++-------------
> 1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 7a58095..405db44 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -755,10 +755,9 @@ 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 xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci,
> +static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id,
> union xhci_trb *trb, struct xhci_event_cmd *event)
> {
> - unsigned int slot_id;
> unsigned int ep_index;
> struct xhci_virt_device *virt_dev;
> struct xhci_ring *ep_ring;
> @@ -770,7 +769,6 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci,
> struct xhci_dequeue_state deq_state;
>
> if (unlikely(TRB_TO_SUSPEND_PORT(le32_to_cpu(trb->generic.field[3])))) {
> - slot_id = TRB_TO_SLOT_ID(le32_to_cpu(trb->generic.field[3]));
> virt_dev = xhci->devs[slot_id];
> if (virt_dev)
> handle_cmd_in_cmd_wait_list(xhci, virt_dev,
> @@ -783,7 +781,6 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci,
> }
>
> memset(&deq_state, 0, sizeof(deq_state));
> - slot_id = TRB_TO_SLOT_ID(le32_to_cpu(trb->generic.field[3]));
> ep_index = TRB_TO_EP_INDEX(le32_to_cpu(trb->generic.field[3]));
> ep = &xhci->devs[slot_id]->eps[ep_index];
>
> @@ -1061,10 +1058,9 @@ 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 xhci_handle_cmd_set_deq(struct xhci_hcd *xhci,
> +static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci, int slot_id,
> struct xhci_event_cmd *event, union xhci_trb *trb)
> {
> - unsigned int slot_id;
> unsigned int ep_index;
> unsigned int stream_id;
> struct xhci_ring *ep_ring;
> @@ -1072,7 +1068,6 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci,
> struct xhci_ep_ctx *ep_ctx;
> struct xhci_slot_ctx *slot_ctx;
>
> - slot_id = TRB_TO_SLOT_ID(le32_to_cpu(trb->generic.field[3]));
> ep_index = TRB_TO_EP_INDEX(le32_to_cpu(trb->generic.field[3]));
> stream_id = TRB_TO_STREAM_ID(le32_to_cpu(trb->generic.field[2]));
> dev = xhci->devs[slot_id];
> @@ -1154,13 +1149,11 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd *xhci,
> ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
> }
>
> -static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci,
> +static void xhci_handle_cmd_reset_ep(struct xhci_hcd *xhci, int slot_id,
> struct xhci_event_cmd *event, union xhci_trb *trb)
> {
> - int slot_id;
> unsigned int ep_index;
>
> - slot_id = TRB_TO_SLOT_ID(le32_to_cpu(trb->generic.field[3]));
> ep_index = TRB_TO_EP_INDEX(le32_to_cpu(trb->generic.field[3]));
> /* This command will only fail if the endpoint wasn't halted,
> * but we don't care.
> @@ -1554,15 +1547,15 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
> xhci_handle_cmd_addr_dev(xhci, slot_id, cmd_comp_code);
> break;
> case TRB_STOP_RING:
> - xhci_handle_cmd_stop_ep(xhci, cmd_trb, event);
> + xhci_handle_cmd_stop_ep(xhci, slot_id, cmd_trb, event);
> break;
> case TRB_SET_DEQ:
> - xhci_handle_cmd_set_deq(xhci, event, cmd_trb);
> + xhci_handle_cmd_set_deq(xhci, slot_id, event, cmd_trb);
> break;
> case TRB_CMD_NOOP:
> break;
> case TRB_RESET_EP:
> - xhci_handle_cmd_reset_ep(xhci, event, cmd_trb);
> + xhci_handle_cmd_reset_ep(xhci, slot_id, event, cmd_trb);
> break;
> case TRB_RESET_DEV:
> xhci_handle_cmd_reset_dev(xhci, slot_id, event);
> --
> 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
