lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Thu, 20 Oct 2016 08:35:46 +0800
From:   Lu Baolu <baolu.lu@...ux.intel.com>
To:     Mathias Nyman <mathias.nyman@...ux.intel.com>
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: 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 <baolu.lu@...ux.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

Powered by blists - more mailing lists