Re: [PATCH 3/3] xhci: Remove busy loop from xhci_abort_cmd_ring()

[OGAWA Hirofumi]

Mathias Nyman  writes:

> On 14.12.2016 01:40, OGAWA Hirofumi wrote:
>> ping about [PATCH 1/3, 2/3, 3/3]?
>>
>
> 1/3 and 2/3 will be sent to 4.10 usb-linus after rc1,
> 3/3 maybe to usb-next after that

Thanks.
-- 
OGAWA Hirofumi 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] xhci: Remove busy loop from xhci_abort_cmd_ring()

[Mathias Nyman]

On 14.12.2016 01:40, OGAWA Hirofumi wrote:

ping about [PATCH 1/3, 2/3, 3/3]?



1/3 and 2/3 will be sent to 4.10 usb-linus after rc1,
3/3 maybe to usb-next after that

-Mathias

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] xhci: Remove busy loop from xhci_abort_cmd_ring()

[OGAWA Hirofumi]

ping about [PATCH 1/3, 2/3, 3/3]?

OGAWA Hirofumi  writes:

> Mathias Nyman  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 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] xhci: Remove busy loop from xhci_abort_cmd_ring()

[OGAWA Hirofumi]

Mathias Nyman  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 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] xhci: Remove busy loop from xhci_abort_cmd_ring()

[Mathias Nyman]

On 16.11.2016 07:01, OGAWA Hirofumi wrote:

Now, xhci_abort_cmd_ring() is sleepable. So no reason to use busy loop
anymore.

- Convert udelay(1000) => msleep(1).


Sounds good.


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

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.

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
 


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.

Sorry about the review delay, I got my hands quite full at the moment

-Mathias
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html