ping about [PATCH 1/3, 2/3, 3/3]?
OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx> writes:
> 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
