[PATCH 5/9] usb: xhci: use list_for_each_entry

[PATCH 5/9] usb: xhci: use list_for_each_entry

[Geliang Tang]

Use list_for_each_entry() instead of list_for_each() to simplify
the code.

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
 drivers/usb/host/xhci-ring.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f1c21c4..4ed15c0 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -640,7 +640,6 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id,
 	unsigned int ep_index;
 	struct xhci_ring *ep_ring;
 	struct xhci_virt_ep *ep;
-	struct list_head *entry;
 	struct xhci_td *cur_td = NULL;
 	struct xhci_td *last_unlinked_td;
 
@@ -670,8 +669,7 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, int slot_id,
 	 * it.  We're also in the event handler, so we can't get re-interrupted
 	 * if another Stop Endpoint command completes
 	 */
-	list_for_each(entry, &ep->cancelled_td_list) {
-		cur_td = list_entry(entry, struct xhci_td, cancelled_td_list);
+	list_for_each_entry(cur_td, &ep->cancelled_td_list, cancelled_td_list) {
 		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
 				"Removing canceled TD starting at 0x%llx (dma).",
 				(unsigned long long)xhci_trb_virt_to_dma(
-- 
2.5.0

Re: [PATCH 5/9] usb: xhci: use list_for_each_entry

[Julia Lawall]

Geliang,

Please check whether it is acceptable that last_unlinked_td point to the
dummy entry at th beginning of the list, in the case where the
list_for_each_entry loop runs out normally.

It seems that you have sent a bunch of these patches.  Please recheck them
all to see if they really follow the semantics of list_for_each_entry
properly.  If particular, you should not normally use the index variable
after leaving the loop, unless it is guaranteed that the exit from the
loop was via a break.

julia

On Sat, 19 Dec 2015, kbuild test robot wrote:

> CC: kbuild-all@01.org
> In-Reply-To: <0fbea26fdbcb76e24188fc0800d425f927f45b6f.1450455485.git.geliangtang@163.com>
> TO: Geliang Tang <geliangtang@163.com>
> CC: Mathias Nyman <mathias.nyman@intel.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: Geliang Tang <geliangtang@163.com>, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
>
> Hi Geliang,
>
> [auto build test WARNING on usb/usb-testing]
> [also build test WARNING on v4.4-rc5 next-20151218]
>
> url:    https://github.com/0day-ci/linux/commits/Geliang-Tang/usb-host-fotg210-use-list_for_each_entry_safe/20151219-003955
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> :::::: branch date: 2 hours ago
> :::::: commit date: 2 hours ago
>
> >> drivers/usb/host/xhci-ring.c:714:20-26: ERROR: invalid reference to the index variable of the iterator on line 672
>
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 0af06cc5bab47032f7fc8e2e6a3df0fb29513b52
> vim +714 drivers/usb/host/xhci-ring.c
>
> ae6367471 Sarah Sharp      2009-04-29  666
> ae6367471 Sarah Sharp      2009-04-29  667  	/* Fix up the ep ring first, so HW stops executing cancelled TDs.
> ae6367471 Sarah Sharp      2009-04-29  668  	 * We have the xHCI lock, so nothing can modify this list until we drop
> ae6367471 Sarah Sharp      2009-04-29  669  	 * it.  We're also in the event handler, so we can't get re-interrupted
> ae6367471 Sarah Sharp      2009-04-29  670  	 * if another Stop Endpoint command completes
> ae6367471 Sarah Sharp      2009-04-29  671  	 */
> 0af06cc5b Geliang Tang     2015-12-19 @672  	list_for_each_entry(cur_td, &ep->cancelled_td_list, cancelled_td_list) {
> aa50b2906 Xenia Ragiadakou 2013-08-14  673  		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
> aa50b2906 Xenia Ragiadakou 2013-08-14  674  				"Removing canceled TD starting at 0x%llx (dma).",
> 79688acfb Sarah Sharp      2011-12-19  675  				(unsigned long long)xhci_trb_virt_to_dma(
> 79688acfb Sarah Sharp      2011-12-19  676  					cur_td->start_seg, cur_td->first_trb));
> e9df17eb1 Sarah Sharp      2010-04-02  677  		ep_ring = xhci_urb_to_transfer_ring(xhci, cur_td->urb);
> e9df17eb1 Sarah Sharp      2010-04-02  678  		if (!ep_ring) {
> e9df17eb1 Sarah Sharp      2010-04-02  679  			/* This shouldn't happen unless a driver is mucking
> e9df17eb1 Sarah Sharp      2010-04-02  680  			 * with the stream ID after submission.  This will
> e9df17eb1 Sarah Sharp      2010-04-02  681  			 * leave the TD on the hardware ring, and the hardware
> e9df17eb1 Sarah Sharp      2010-04-02  682  			 * will try to execute it, and may access a buffer
> e9df17eb1 Sarah Sharp      2010-04-02  683  			 * that has already been freed.  In the best case, the
> e9df17eb1 Sarah Sharp      2010-04-02  684  			 * hardware will execute it, and the event handler will
> e9df17eb1 Sarah Sharp      2010-04-02  685  			 * ignore the completion event for that TD, since it was
> e9df17eb1 Sarah Sharp      2010-04-02  686  			 * removed from the td_list for that endpoint.  In
> e9df17eb1 Sarah Sharp      2010-04-02  687  			 * short, don't muck with the stream ID after
> e9df17eb1 Sarah Sharp      2010-04-02  688  			 * submission.
> e9df17eb1 Sarah Sharp      2010-04-02  689  			 */
> e9df17eb1 Sarah Sharp      2010-04-02  690  			xhci_warn(xhci, "WARN Cancelled URB %p "
> e9df17eb1 Sarah Sharp      2010-04-02  691  					"has invalid stream ID %u.\n",
> e9df17eb1 Sarah Sharp      2010-04-02  692  					cur_td->urb,
> e9df17eb1 Sarah Sharp      2010-04-02  693  					cur_td->urb->stream_id);
> e9df17eb1 Sarah Sharp      2010-04-02  694  			goto remove_finished_td;
> e9df17eb1 Sarah Sharp      2010-04-02  695  		}
> ae6367471 Sarah Sharp      2009-04-29  696  		/*
> ae6367471 Sarah Sharp      2009-04-29  697  		 * If we stopped on the TD we need to cancel, then we have to
> ae6367471 Sarah Sharp      2009-04-29  698  		 * move the xHC endpoint ring dequeue pointer past this TD.
> ae6367471 Sarah Sharp      2009-04-29  699  		 */
> 63a0d9abd Sarah Sharp      2009-09-04  700  		if (cur_td == ep->stopped_td)
> e9df17eb1 Sarah Sharp      2010-04-02  701  			xhci_find_new_dequeue_state(xhci, slot_id, ep_index,
> e9df17eb1 Sarah Sharp      2010-04-02  702  					cur_td->urb->stream_id,
> e9df17eb1 Sarah Sharp      2010-04-02  703  					cur_td, &deq_state);
> ae6367471 Sarah Sharp      2009-04-29  704  		else
> 522989a27 Sarah Sharp      2011-07-29  705  			td_to_noop(xhci, ep_ring, cur_td, false);
> e9df17eb1 Sarah Sharp      2010-04-02  706  remove_finished_td:
> ae6367471 Sarah Sharp      2009-04-29  707  		/*
> ae6367471 Sarah Sharp      2009-04-29  708  		 * The event handler won't see a completion for this TD anymore,
> ae6367471 Sarah Sharp      2009-04-29  709  		 * so remove it from the endpoint ring's TD list.  Keep it in
> ae6367471 Sarah Sharp      2009-04-29  710  		 * the cancelled TD list for URB completion later.
> ae6367471 Sarah Sharp      2009-04-29  711  		 */
> 585df1d90 Sarah Sharp      2011-08-02  712  		list_del_init(&cur_td->td_list);
> ae6367471 Sarah Sharp      2009-04-29  713  	}
> ae6367471 Sarah Sharp      2009-04-29 @714  	last_unlinked_td = cur_td;
> 6f5165cf9 Sarah Sharp      2009-10-27  715  	xhci_stop_watchdog_timer_in_irq(xhci, ep);
> ae6367471 Sarah Sharp      2009-04-29  716
> ae6367471 Sarah Sharp      2009-04-29  717  	/* If necessary, queue a Set Transfer Ring Dequeue Pointer command */
>
> :::::: The code at line 714 was first introduced by commit
> :::::: ae636747146ea97efa18e04576acd3416e2514f5 USB: xhci: URB cancellation support.
>
> :::::: TO: Sarah Sharp <sarah.a.sharp@linux.intel.com>
> :::::: CC: Greg Kroah-Hartman <gregkh@suse.de>
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>

Re: [PATCH 5/9] usb: xhci: use list_for_each_entry

[Mathias Nyman]

On 18.12.2015 20:30, Julia Lawall wrote:
> Geliang,
>
> Please check whether it is acceptable that last_unlinked_td point to the
> dummy entry at th beginning of the list, in the case where the
> list_for_each_entry loop runs out normally.
>
> It seems that you have sent a bunch of these patches.  Please recheck them
> all to see if they really follow the semantics of list_for_each_entry
> properly.  If particular, you should not normally use the index variable
> after leaving the loop, unless it is guaranteed that the exit from the
> loop was via a break.
>

Julia is correct, we can't use list_for_each_entry() here as cur_td would end up
pointing to list head, and we really need it to point to the last entry in the list at that point.

old:
-	list_for_each(entry, &ep->cancelled_td_list) {
-		cur_td = list_entry(entry, struct xhci_td, cancelled_td_list);
cur_td_will point to last element in list. "entry" will point to head, but is on longer used.

new:
+	list_for_each_entry(cur_td, &ep->cancelled_td_list, cancelled_td_list) {
cur_td will point to head of list.

This is important as newly canceled TDs can be added to the tail of the list while
looping through and returning the canceled TDs.  We want to make sure we
only return and delete the TDs up to the point we have handled them on the ring.

(code continues with: last_unlinked_td = cur_td;)

Thanks Julia for spotting this

-Mathias