* [PATCH 1/1] usb: xhci: clean up error_bitmask usage
@ 2016-10-19 0:53 Lu Baolu
2016-10-19 6:52 ` Mathias Nyman
0 siblings, 1 reply; 3+ messages in thread
From: Lu Baolu @ 2016-10-19 0:53 UTC (permalink / raw)
To: Mathias Nyman; +Cc: linux-usb, linux-kernel, Lu Baolu
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@linux.intel.com>
---
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 *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);
}
/* Any of the above functions may drop and re-acquire the lock, so check
* to make sure a watchdog timer didn't mark the host as non-responsive.
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index b2c1dc5..ab0393d 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1616,8 +1616,6 @@ struct xhci_hcd {
#define XHCI_STATE_DYING (1 << 0)
#define XHCI_STATE_HALTED (1 << 1)
#define XHCI_STATE_REMOVING (1 << 2)
- /* Statistics */
- int error_bitmask;
unsigned int quirks;
#define XHCI_LINK_TRB_QUIRK (1 << 0)
#define XHCI_RESET_EP_QUIRK (1 << 1)
--
2.1.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] usb: xhci: clean up error_bitmask usage
2016-10-19 0:53 [PATCH 1/1] usb: xhci: clean up error_bitmask usage Lu Baolu
@ 2016-10-19 6:52 ` Mathias Nyman
2016-10-20 0:35 ` Lu Baolu
0 siblings, 1 reply; 3+ messages in thread
From: Mathias Nyman @ 2016-10-19 6:52 UTC (permalink / raw)
To: Lu Baolu; +Cc: linux-usb, linux-kernel
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@linux.intel.com>
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] usb: xhci: clean up error_bitmask usage
2016-10-19 6:52 ` Mathias Nyman
@ 2016-10-20 0:35 ` Lu Baolu
0 siblings, 0 replies; 3+ messages in thread
From: Lu Baolu @ 2016-10-20 0:35 UTC (permalink / raw)
To: Mathias Nyman; +Cc: linux-usb, linux-kernel
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 <baolu.lu@linux.intel.com>
>
> 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):
>> 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.
Yes. How about below code?
ret = handle_tx_event(xhci, &event->trans_event);
if (ret >=0)
update_ptrs = 0;
>
>> 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
Okay, I will fix this.
>
> -Mathias
>
Best regards,
Lu Baolu
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-10-20 0:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-19 0:53 [PATCH 1/1] usb: xhci: clean up error_bitmask usage Lu Baolu
2016-10-19 6:52 ` Mathias Nyman
2016-10-20 0:35 ` Lu Baolu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).