[U-Boot] [PATCH v3 2/6] usb: xhci: return ERR_PTR(-ETIMEDOUT) at the end of xhci_wait_for_event()

[Marek Vasut]

On 01/28/2018 11:18 AM, Masahiro Yamada wrote:
> Bin, Marek,

Hi,

> Would you take a look at xHCI code?

I was kinda hoping Bin would, oh well ...

> Lots of xHCI code invokes BUG_ON()/BUG()
> when the hardware access fails,
> then halts the system completely.
> This is not a software bug.
> 
> 
> 2018-01-10 10:45 GMT+09:00 Masahiro Yamada <yamada.masahiro at socionext.com>:
>> xhci_wait_for_event() is supposed to return a pointer to union xhci_trb,
>> but it does not return anything at the end of the function.
>>
>> This relies on that the end of the function is unreachable due to BUG().
>>
>> We are planning to make BUG() no-op for platforms with strong image size
>> constraint.

But BUG() should hang the system because it means an unrecoverable issue
happened. Changing it to nop would cause a lot of weird misbehavior, so
that is IMO a bad idea. Just change it to hang(), that should be present
even on size-constrained systems.

>> Doing so would cause compiler warning:
>>
>> drivers/usb/host/xhci-ring.c:475:1: warning: control reaches end of non-void function [-Wreturn-type]
>>  }
>>  ^
>>
>> So, this function must return something.  From the error message just
>> above, ERR_PTR(-ETIMEDOUT) seems a good choice.

This is an imported code from Linux, so you might want to check what
they do/did there.

>> The use of BUG() looks suspicious here in the first place; no response
>> from hardware is not a bug.  It should be treated as a normal error.
>> So, this function must return an error pointer instead of BUG(), then
>> the caller must handle it properly.
>>
>> I am not fixing the code because this is not the only place that stops
>> the system.  Just one failure of xHCI halts the system, here and there.
>>
>> I left a comment block, hoping somebody will take a look.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com>
>> ---
>>
>> Changes in v3:
>>   - newly added
>>
>> Changes in v2: None
>>
>>  drivers/usb/host/xhci-ring.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index 579e670..d780367 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -471,7 +471,14 @@ union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected)
>>                 return NULL;
>>
>>         printf("XHCI timeout on event type %d... cannot recover.\n", expected);
>> +
>> +       /*
>> +        * CHECK:
>> +        * Is this software bug?  Is this a good reason to halt the system?
>> +        */
>>         BUG();
>> +
>> +       return ERR_PTR(-ETIMEDOUT);
>>  }
>>
>>  /*
>> --
>> 2.7.4
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
> 
> 
> 


-- 
Best regards,
Marek Vasut