Re: [PATCH 3/3] xhci: Remove busy loop from xhci_abort_cmd_ring()
Mathias Nymanwrites: > 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()
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()
ping about [PATCH 1/3, 2/3, 3/3]? OGAWA Hirofumiwrites: > 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 Nymanwrites: >> - 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()
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