| Message ID | 20240912033551.910337-2-xu.yang_2@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] usb: chipidea: udc: handle USB Error Interrupt if IOC not set | expand |
On Thu, Sep 12, 2024 at 11:35:50AM +0800, Xu Yang wrote: > Currently, ATDTW semaphore is used to safety link new dTD to dQH. But this > code has a bug when the endpoint is already in error before polling ATDTW > or just met error during polling ATDTW. In that cases, ATDTW will never > turn to 1 and the cpu will busy loop there. It should be bug fixes, add fixes tag and cc stable. > > When the endpoint met error, ENDPTSTAT will be cleared by HW. Therefore, > ENDPTSTAT should also be considered during this process. In case of > endpoint error, the current dTD should not be pushed to the head of dQH > since some dTDs may be still not executed. Therefore, the link logic is > also improved accordingly. > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > --- > drivers/usb/chipidea/udc.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index b9ccf62e0a50..0ab57b87b07b 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -612,10 +612,17 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq) > do { > hw_write(ci, OP_USBCMD, USBCMD_ATDTW, USBCMD_ATDTW); > tmp_stat = hw_read(ci, OP_ENDPTSTAT, BIT(n)); > - } while (!hw_read(ci, OP_USBCMD, USBCMD_ATDTW)); > + } while (!hw_read(ci, OP_USBCMD, USBCMD_ATDTW) && tmp_stat); > hw_write(ci, OP_USBCMD, USBCMD_ATDTW, 0); > if (tmp_stat) > goto done; > + > + /* In case of error, ENDPTSTAT will also turn into 0, then > + * don't push this dTD to dQH head if current dTD pointer > + * is not the last dTD in previous request. > + */ OP_ENDPTSTAT will be clear by HW when the endpoint met err. This dTD don't push to dQH if current dTD point is not the last one in previous request. > + if (hwep->qh.ptr->curr != prevlastnode->dma) > + goto done; > } > > /* QH configuration */ > -- > 2.34.1 >
On Thu, Sep 12, 2024 at 12:54:53AM -0400, Frank Li wrote: > On Thu, Sep 12, 2024 at 11:35:50AM +0800, Xu Yang wrote: > > Currently, ATDTW semaphore is used to safety link new dTD to dQH. But this > > code has a bug when the endpoint is already in error before polling ATDTW > > or just met error during polling ATDTW. In that cases, ATDTW will never > > turn to 1 and the cpu will busy loop there. > > It should be bug fixes, add fixes tag and cc stable. Okay. > > > > > When the endpoint met error, ENDPTSTAT will be cleared by HW. Therefore, > > ENDPTSTAT should also be considered during this process. In case of > > endpoint error, the current dTD should not be pushed to the head of dQH > > since some dTDs may be still not executed. Therefore, the link logic is > > also improved accordingly. > > > > Signed-off-by: Xu Yang <xu.yang_2@nxp.com> > > --- > > drivers/usb/chipidea/udc.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > index b9ccf62e0a50..0ab57b87b07b 100644 > > --- a/drivers/usb/chipidea/udc.c > > +++ b/drivers/usb/chipidea/udc.c > > @@ -612,10 +612,17 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq) > > do { > > hw_write(ci, OP_USBCMD, USBCMD_ATDTW, USBCMD_ATDTW); > > tmp_stat = hw_read(ci, OP_ENDPTSTAT, BIT(n)); > > - } while (!hw_read(ci, OP_USBCMD, USBCMD_ATDTW)); > > + } while (!hw_read(ci, OP_USBCMD, USBCMD_ATDTW) && tmp_stat); > > hw_write(ci, OP_USBCMD, USBCMD_ATDTW, 0); > > if (tmp_stat) > > goto done; > > + > > + /* In case of error, ENDPTSTAT will also turn into 0, then > > + * don't push this dTD to dQH head if current dTD pointer > > + * is not the last dTD in previous request. > > + */ > > OP_ENDPTSTAT will be clear by HW when the endpoint met err. This dTD don't > push to dQH if current dTD point is not the last one in previous request. Okay, will replace with it. Thanks, Xu Yang
Hi Xu, kernel test robot noticed the following build warnings: [auto build test WARNING on usb/usb-testing] [also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.11-rc7 next-20240912] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Xu-Yang/usb-chipidea-udc-improve-dTD-link-logic/20240912-113632 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing patch link: https://lore.kernel.org/r/20240912033551.910337-2-xu.yang_2%40nxp.com patch subject: [PATCH 2/3] usb: chipidea: udc: improve dTD link logic config: i386-randconfig-061-20240913 (https://download.01.org/0day-ci/archive/20240913/202409131638.TJCPmqQZ-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240913/202409131638.TJCPmqQZ-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409131638.TJCPmqQZ-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/usb/chipidea/udc.c:624:33: sparse: sparse: restricted __le32 degrades to integer vim +624 drivers/usb/chipidea/udc.c 542 543 /** 544 * _hardware_enqueue: configures a request at hardware level 545 * @hwep: endpoint 546 * @hwreq: request 547 * 548 * This function returns an error code 549 */ 550 static int _hardware_enqueue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq) 551 { 552 struct ci_hdrc *ci = hwep->ci; 553 int ret = 0; 554 struct td_node *firstnode, *lastnode; 555 556 /* don't queue twice */ 557 if (hwreq->req.status == -EALREADY) 558 return -EALREADY; 559 560 hwreq->req.status = -EALREADY; 561 562 ret = usb_gadget_map_request_by_dev(ci->dev->parent, 563 &hwreq->req, hwep->dir); 564 if (ret) 565 return ret; 566 567 if (hwreq->req.num_mapped_sgs) 568 ret = prepare_td_for_sg(hwep, hwreq); 569 else 570 ret = prepare_td_for_non_sg(hwep, hwreq); 571 572 if (ret) 573 return ret; 574 575 lastnode = list_entry(hwreq->tds.prev, 576 struct td_node, td); 577 578 lastnode->ptr->next = cpu_to_le32(TD_TERMINATE); 579 if (!hwreq->req.no_interrupt) 580 lastnode->ptr->token |= cpu_to_le32(TD_IOC); 581 582 list_for_each_entry_safe(firstnode, lastnode, &hwreq->tds, td) 583 trace_ci_prepare_td(hwep, hwreq, firstnode); 584 585 firstnode = list_first_entry(&hwreq->tds, struct td_node, td); 586 587 wmb(); 588 589 hwreq->req.actual = 0; 590 if (!list_empty(&hwep->qh.queue)) { 591 struct ci_hw_req *hwreqprev; 592 int n = hw_ep_bit(hwep->num, hwep->dir); 593 int tmp_stat; 594 struct td_node *prevlastnode; 595 u32 next = firstnode->dma & TD_ADDR_MASK; 596 597 hwreqprev = list_entry(hwep->qh.queue.prev, 598 struct ci_hw_req, queue); 599 prevlastnode = list_entry(hwreqprev->tds.prev, 600 struct td_node, td); 601 602 prevlastnode->ptr->next = cpu_to_le32(next); 603 wmb(); 604 605 if (ci->rev == CI_REVISION_22) { 606 if (!hw_read(ci, OP_ENDPTSTAT, BIT(n))) 607 reprime_dtd(ci, hwep, prevlastnode); 608 } 609 610 if (hw_read(ci, OP_ENDPTPRIME, BIT(n))) 611 goto done; 612 do { 613 hw_write(ci, OP_USBCMD, USBCMD_ATDTW, USBCMD_ATDTW); 614 tmp_stat = hw_read(ci, OP_ENDPTSTAT, BIT(n)); 615 } while (!hw_read(ci, OP_USBCMD, USBCMD_ATDTW) && tmp_stat); 616 hw_write(ci, OP_USBCMD, USBCMD_ATDTW, 0); 617 if (tmp_stat) 618 goto done; 619 620 /* In case of error, ENDPTSTAT will also turn into 0, then 621 * don't push this dTD to dQH head if current dTD pointer 622 * is not the last dTD in previous request. 623 */ > 624 if (hwep->qh.ptr->curr != prevlastnode->dma) 625 goto done; 626 } 627 628 /* QH configuration */ 629 hwep->qh.ptr->td.next = cpu_to_le32(firstnode->dma); 630 hwep->qh.ptr->td.token &= 631 cpu_to_le32(~(TD_STATUS_HALTED|TD_STATUS_ACTIVE)); 632 633 if (hwep->type == USB_ENDPOINT_XFER_ISOC && hwep->dir == RX) { 634 u32 mul = hwreq->req.length / hwep->ep.maxpacket; 635 636 if (hwreq->req.length == 0 637 || hwreq->req.length % hwep->ep.maxpacket) 638 mul++; 639 hwep->qh.ptr->cap |= cpu_to_le32(mul << __ffs(QH_MULT)); 640 } 641 642 ret = hw_ep_prime(ci, hwep->num, hwep->dir, 643 hwep->type == USB_ENDPOINT_XFER_CONTROL); 644 done: 645 return ret; 646 } 647
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index b9ccf62e0a50..0ab57b87b07b 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -612,10 +612,17 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq) do { hw_write(ci, OP_USBCMD, USBCMD_ATDTW, USBCMD_ATDTW); tmp_stat = hw_read(ci, OP_ENDPTSTAT, BIT(n)); - } while (!hw_read(ci, OP_USBCMD, USBCMD_ATDTW)); + } while (!hw_read(ci, OP_USBCMD, USBCMD_ATDTW) && tmp_stat); hw_write(ci, OP_USBCMD, USBCMD_ATDTW, 0); if (tmp_stat) goto done; + + /* In case of error, ENDPTSTAT will also turn into 0, then + * don't push this dTD to dQH head if current dTD pointer + * is not the last dTD in previous request. + */ + if (hwep->qh.ptr->curr != prevlastnode->dma) + goto done; } /* QH configuration */
Currently, ATDTW semaphore is used to safety link new dTD to dQH. But this code has a bug when the endpoint is already in error before polling ATDTW or just met error during polling ATDTW. In that cases, ATDTW will never turn to 1 and the cpu will busy loop there. When the endpoint met error, ENDPTSTAT will be cleared by HW. Therefore, ENDPTSTAT should also be considered during this process. In case of endpoint error, the current dTD should not be pushed to the head of dQH since some dTDs may be still not executed. Therefore, the link logic is also improved accordingly. Signed-off-by: Xu Yang <xu.yang_2@nxp.com> --- drivers/usb/chipidea/udc.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)