Re: [PATCH 1/1] usb: xhci: clean up error_bitmask usage
Hi, On 10/19/2016 02:52 PM, Mathias Nyman wrote: > 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 Thanks for your comments. > >> --- >> 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 Ah! yes. I though we don't need to handle a unsuccessful command. Thanks for pointing this out. I will fix it in the v2 patch. > > >> } >> 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): >>
Re: [PATCH 1/1] usb: xhci: clean up error_bitmask usage
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 = handl
[PATCH 1/1] usb: xhci: clean up error_bitmask usage
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 --- 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; } 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; break; case TRB_TYPE(TRB_DEV_NOTE): @@ -2686,7 +2680,9 @@ static int xhci_handle_event(struct xhci_hcd *xh