Re: [PATCH 1/1] usb: xhci: clean up error_bitmask usage

[Mathias Nyman]

Hi

On 19.10.2016 03:53, Lu Baolu wrote:
> In xhci_handle_event(), when errors are detected, driver always sets
> a bit in error_bitmask (one member of the xhci private driver data).
> That means users have to retrieve and decode the value of error_bitmask
> in xhci private driver data if they want to know whether those erros
> ever happened in xhci_handle_event(). Otherwise, those errors are just
> ignored silently.
>
> This patch cleans up this by replacing the setting of error_bitmask
> with the kernel print functions, so that users can easily check and
> report the errors happened in xhci_handle_event().
>
> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>

Nice to get this cleaned out. I think the error_bitmask was something
used during driver development but has no function anymore.

A few minor comments below

> ---
>   drivers/usb/host/xhci-ring.c | 42 +++++++++++++++++++-----------------------
>   drivers/usb/host/xhci.h      |  2 --
>   2 files changed, 19 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 797137e..a460423 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1183,7 +1183,7 @@ 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;
> +		xhci_warn(xhci, "WARN NEC_GET_FW command on non-NEC host\n");
>   		return;
>   	}
>   	xhci_dbg_trace(xhci, trace_xhci_dbg_quirks,
> @@ -1325,14 +1325,13 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
>   	cmd_trb = xhci->cmd_ring->dequeue;
>   	cmd_dequeue_dma = xhci_trb_virt_to_dma(xhci->cmd_ring->deq_seg,
>   			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;
> -		return;
> -	}
> -	/* Does the DMA address match our internal dequeue pointer address? */
> -	if (cmd_dma != (u64) cmd_dequeue_dma) {
> -		xhci->error_bitmask |= 1 << 5;
> +	/*
> +	 * Check whether the completion event is for our internal kept
> +	 * command.
> +	 */
> +	if (!cmd_dequeue_dma || cmd_dma != (u64)cmd_dequeue_dma) {
> +		xhci_warn(xhci,
> +			  "ERROR mismatched command completion event\n");
>   		return;
>   	}
>
> @@ -1418,7 +1417,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
>   		break;
>   	default:
>   		/* Skip over unknown commands on the event ring */
> -		xhci->error_bitmask |= 1 << 6;
> +		xhci_info(xhci, "INFO unknown command type %d\n", cmd_type);
>   		break;
>   	}
>
> @@ -1521,7 +1520,7 @@ static void handle_port_status(struct xhci_hcd *xhci,
>   	/* Port status change events always have a successful completion code */
>   	if (GET_COMP_CODE(le32_to_cpu(event->generic.field[2])) != COMP_SUCCESS) {
>   		xhci_warn(xhci, "WARN: xHC returned failed port status event\n");
> -		xhci->error_bitmask |= 1 << 8;
> +		return;

I don't think we should return here, it will mess up the event ring dequeue pointer.
And a non-expected completion code here should not prevent us from working normally

>   	}
>   	port_id = GET_PORT_ID(le32_to_cpu(event->generic.field[0]));
>   	xhci_dbg(xhci, "Port Status Change Event for port %d\n", port_id);
> @@ -2642,20 +2641,18 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
>   {
>   	union xhci_trb *event;
>   	int update_ptrs = 1;
> -	int ret;
>
> +	/* Event ring hasn't been allocated yet. */
>   	if (!xhci->event_ring || !xhci->event_ring->dequeue) {
> -		xhci->error_bitmask |= 1 << 1;
> -		return 0;
> +		xhci_err(xhci, "ERROR event ring not ready\n");
> +		return -ENOMEM;
>   	}
>
>   	event = xhci->event_ring->dequeue;
>   	/* Does the HC or OS own the TRB? */
>   	if ((le32_to_cpu(event->event_cmd.flags) & TRB_CYCLE) !=
> -	    xhci->event_ring->cycle_state) {
> -		xhci->error_bitmask |= 1 << 2;
> +	    xhci->event_ring->cycle_state)
>   		return 0;
> -	}
>
>   	/*
>   	 * Barrier between reading the TRB_CYCLE (valid) flag above and any
> @@ -2663,7 +2660,7 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
>   	 */
>   	rmb();
>   	/* FIXME: Handle more event types. */
> -	switch ((le32_to_cpu(event->event_cmd.flags) & TRB_TYPE_BITMASK)) {
> +	switch (le32_to_cpu(event->event_cmd.flags) & TRB_TYPE_BITMASK) {
>   	case TRB_TYPE(TRB_COMPLETION):
>   		handle_cmd_completion(xhci, &event->event_cmd);
>   		break;
> @@ -2672,10 +2669,7 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
>   		update_ptrs = 0;
>   		break;
>   	case TRB_TYPE(TRB_TRANSFER):
> -		ret = handle_tx_event(xhci, &event->trans_event);
> -		if (ret < 0)
> -			xhci->error_bitmask |= 1 << 9;
> -		else
> +		if (!handle_tx_event(xhci, &event->trans_event))
>   			update_ptrs = 0;

had to check this, it works because handle_tx_event() doesn't return positive values.

>   		break;
>   	case TRB_TYPE(TRB_DEV_NOTE):
> @@ -2686,7 +2680,9 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
>   		    TRB_TYPE(48))
>   			handle_vendor_event(xhci, event);
>   		else
> -			xhci->error_bitmask |= 1 << 3;
> +			xhci_warn(xhci, "ERROR unknown event type %d\n",
> +				  le32_to_cpu(event->event_cmd.flags) &
> +				  TRB_TYPE_BITMASK);

This trb type is still shifted 10 bits when printing it out.
There are not yet any proper helpers or macros for this it seems

-Mathias