diff mbox series

[2/3] usb: chipidea: udc: improve dTD link logic

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

Commit Message

Xu Yang Sept. 12, 2024, 3:35 a.m. UTC
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(-)

Comments

Frank Li Sept. 12, 2024, 4:54 a.m. UTC | #1
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
>
Xu Yang Sept. 13, 2024, 1:52 a.m. UTC | #2
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
kernel test robot Sept. 13, 2024, 8:51 a.m. UTC | #3
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 mbox series

Patch

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 */