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

[Masahiro Yamada]

Hi Marek,

2018-01-28 21:43 GMT+09:00 Marek Vasut <marex at denx.de>:
> 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.


In my opinion, BUG_ON(!x) and assert(x) are equivalent.

Both check software bugs that should never happen
(but useful for debugging).


When releasing software, we can turn them into no-op.

Actually, assert() works like that in user-space programs.
(assert() is no-op if NDEBUG is defined)



>>> 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.
>


I grepped xhci_wait_for_event in the kernel source,
but did not get any hit.




-- 
Best Regards
Masahiro Yamada