Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx> writes:
>> - Add xhci_handshake_sleep(), and use it.
>
> This seems a but overkill, I'd rather don't have xhci_handshake(),
> xhci_handshake_sleep() and __xhci_handshake() to maintain.
I agree about it. However, on other hand, I thought about the
possibility/effort to decreasing use-cases of xhci_handshake()
busyloop. (there are several places to use more than e.g. 500ms
timeout.) Because long busyloop (especially with interrupt-off) has
really bad effect to whole system of non-usb.
> As we now can sleep in xhci_abort_cmd_ring() I would rather just first
> wait for the completion and then maybe handshake check the register.
> At that point it shouldn't need to busyloop anymore, one read should
> do it.
I worried about in the case of real internal confusing, and consider
about chip that doesn't have no stop/abort event.
To handle both case, timeout choice would not be straight-forward.
> But this is all just optimizing an error path, I'm actually fine with
> taking just patch 1/3 and 2/3 v3, and skipping 3/3. the udelay(1000)
> to msleep(1) I can add myself
Right. The main point of patchset is 1/3 and 2/3.
>> As related change, current xhci_handshake() is strange behavior,
>> E.g. xhci_handshake(ptr, mask, done, 1) does
>>
>> result = readl(ptr);
>> /* check result */
>> udelay(1); <= meaningless delay
>> return -ETIMEDOUT;
>
> Only in the timeout case, so if we after 3 or 5 million register reads
> + udelays(1) still don't get the value we want then there is an
> unnecessary udelay(1).
>
> Not worth putting much effort on it now.
True. But actual effort for it was very small.
@@ -65,16 +65,18 @@ int xhci_handshake(void __iomem *ptr, u3
{
u32 result;
- do {
+ while (1) {
result = readl(ptr);
if (result == ~(u32)0) /* card removed */
return -ENODEV;
result &= mask;
if (result == done)
return 0;
+ if (usec <= 0)
+ break;
udelay(1);
usec--;
- } while (usec > 0);
+ }
return -ETIMEDOUT;
}
Thanks.
--
OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
