From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753541AbcJSGwq (ORCPT ); Wed, 19 Oct 2016 02:52:46 -0400 Received: from mga06.intel.com ([134.134.136.31]:50292 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751105AbcJSGwg (ORCPT ); Wed, 19 Oct 2016 02:52:36 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,513,1473145200"; d="scan'208";a="181334606" Subject: Re: [PATCH 1/1] usb: xhci: clean up error_bitmask usage To: Lu Baolu References: <1476838427-4834-1-git-send-email-baolu.lu@linux.intel.com> Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org From: Mathias Nyman Message-ID: <58071844.20903@linux.intel.com> Date: Wed, 19 Oct 2016 09:52:52 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <1476838427-4834-1-git-send-email-baolu.lu@linux.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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