2015-12-18 16:35:59

by Geliang Tang

[permalink] [raw]
Subject: [PATCH 1/9] usb: host: fotg210: use list_for_each_entry_safe

Use list_for_each_entry_safe() instead of list_for_each_safe() to
simplify the code.

Signed-off-by: Geliang Tang <[email protected]>
---
drivers/usb/host/fotg210-hcd.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
index 2341af4..360a5e9 100644
--- a/drivers/usb/host/fotg210-hcd.c
+++ b/drivers/usb/host/fotg210-hcd.c
@@ -2267,7 +2267,7 @@ static unsigned qh_completions(struct fotg210_hcd *fotg210,
struct fotg210_qh *qh)
{
struct fotg210_qtd *last, *end = qh->dummy;
- struct list_head *entry, *tmp;
+ struct fotg210_qtd *qtd, *tmp;
int last_status;
int stopped;
unsigned count = 0;
@@ -2301,12 +2301,10 @@ rescan:
* then let the queue advance.
* if queue is stopped, handles unlinks.
*/
- list_for_each_safe(entry, tmp, &qh->qtd_list) {
- struct fotg210_qtd *qtd;
+ list_for_each_entry_safe(qtd, tmp, &qh->qtd_list, qtd_list) {
struct urb *urb;
u32 token = 0;

- qtd = list_entry(entry, struct fotg210_qtd, qtd_list);
urb = qtd->urb;

/* clean up any state from previous QTD ...*/
@@ -2544,14 +2542,11 @@ retry_xacterr:
* used for cleanup after errors, before HC sees an URB's TDs.
*/
static void qtd_list_free(struct fotg210_hcd *fotg210, struct urb *urb,
- struct list_head *qtd_list)
+ struct list_head *head)
{
- struct list_head *entry, *temp;
-
- list_for_each_safe(entry, temp, qtd_list) {
- struct fotg210_qtd *qtd;
+ struct fotg210_qtd *qtd, *temp;

- qtd = list_entry(entry, struct fotg210_qtd, qtd_list);
+ list_for_each_entry_safe(qtd, temp, head, qtd_list) {
list_del(&qtd->qtd_list);
fotg210_qtd_free(fotg210, qtd);
}
--
2.5.0


2015-12-18 16:35:47

by Geliang Tang

[permalink] [raw]
Subject: [PATCH 2/9] usb: host: max3421-hcd: use list_for_each_entry*

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

Signed-off-by: Geliang Tang <[email protected]>
---
drivers/usb/host/max3421-hcd.c | 25 +++++++++----------------
1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index bd98706..7257962 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -665,7 +665,6 @@ max3421_select_and_start_urb(struct usb_hcd *hcd)
struct max3421_ep *max3421_ep;
int epnum, force_toggles = 0;
struct usb_host_endpoint *ep;
- struct list_head *pos;
unsigned long flags;

spin_lock_irqsave(&max3421_hcd->lock, flags);
@@ -673,10 +672,9 @@ max3421_select_and_start_urb(struct usb_hcd *hcd)
for (;
max3421_hcd->sched_pass < SCHED_PASS_DONE;
++max3421_hcd->sched_pass)
- list_for_each(pos, &max3421_hcd->ep_list) {
+ list_for_each_entry(max3421_ep, &max3421_hcd->ep_list,
+ ep_list) {
urb = NULL;
- max3421_ep = container_of(pos, struct max3421_ep,
- ep_list);
ep = max3421_ep->ep;

switch (usb_endpoint_type(&ep->desc)) {
@@ -748,7 +746,8 @@ max3421_select_and_start_urb(struct usb_hcd *hcd)
}

/* move current ep to tail: */
- list_move_tail(pos, &max3421_hcd->ep_list);
+ list_move_tail(&max3421_ep->ep_list,
+ &max3421_hcd->ep_list);
curr_urb = urb;
goto done;
}
@@ -797,19 +796,16 @@ max3421_check_unlink(struct usb_hcd *hcd)
{
struct spi_device *spi = to_spi_device(hcd->self.controller);
struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd);
- struct list_head *pos, *upos, *next_upos;
struct max3421_ep *max3421_ep;
struct usb_host_endpoint *ep;
- struct urb *urb;
+ struct urb *urb, *next;
unsigned long flags;
int retval = 0;

spin_lock_irqsave(&max3421_hcd->lock, flags);
- list_for_each(pos, &max3421_hcd->ep_list) {
- max3421_ep = container_of(pos, struct max3421_ep, ep_list);
+ list_for_each_entry(max3421_ep, &max3421_hcd->ep_list, ep_list) {
ep = max3421_ep->ep;
- list_for_each_safe(upos, next_upos, &ep->urb_list) {
- urb = container_of(upos, struct urb, urb_list);
+ list_for_each_entry_safe(urb, next, &ep->urb_list, urb_list) {
if (urb->unlinked) {
retval = 1;
dev_dbg(&spi->dev, "%s: URB %p unlinked=%d",
@@ -1184,22 +1180,19 @@ dump_eps(struct usb_hcd *hcd)
struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd);
struct max3421_ep *max3421_ep;
struct usb_host_endpoint *ep;
- struct list_head *pos, *upos;
char ubuf[512], *dp, *end;
unsigned long flags;
struct urb *urb;
int epnum, ret;

spin_lock_irqsave(&max3421_hcd->lock, flags);
- list_for_each(pos, &max3421_hcd->ep_list) {
- max3421_ep = container_of(pos, struct max3421_ep, ep_list);
+ list_for_each_entry(max3421_ep, &max3421_hcd->ep_list, ep_list) {
ep = max3421_ep->ep;

dp = ubuf;
end = dp + sizeof(ubuf);
*dp = '\0';
- list_for_each(upos, &ep->urb_list) {
- urb = container_of(upos, struct urb, urb_list);
+ list_for_each_entry(urb, &ep->urb_list, urb_list) {
ret = snprintf(dp, end - dp, " %p(%d.%s %d/%d)", urb,
usb_pipetype(urb->pipe),
usb_urb_dir_in(urb) ? "IN" : "OUT",
--
2.5.0

2015-12-18 16:35:44

by Geliang Tang

[permalink] [raw]
Subject: [PATCH 3/9] usb: host: oxu210hp-hcd: use list_for_each_entry_safe

Use list_for_each_entry_safe() instead of list_for_each_safe() to
simplify the code.

Signed-off-by: Geliang Tang <[email protected]>
---
drivers/usb/host/oxu210hp-hcd.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/host/oxu210hp-hcd.c b/drivers/usb/host/oxu210hp-hcd.c
index bc74aca..4e4d601 100644
--- a/drivers/usb/host/oxu210hp-hcd.c
+++ b/drivers/usb/host/oxu210hp-hcd.c
@@ -981,7 +981,7 @@ static int qh_schedule(struct oxu_hcd *oxu, struct ehci_qh *qh);
static unsigned qh_completions(struct oxu_hcd *oxu, struct ehci_qh *qh)
{
struct ehci_qtd *last = NULL, *end = qh->dummy;
- struct list_head *entry, *tmp;
+ struct ehci_qtd *qtd, *tmp;
int stopped;
unsigned count = 0;
int do_status = 0;
@@ -1006,12 +1006,10 @@ static unsigned qh_completions(struct oxu_hcd *oxu, struct ehci_qh *qh)
* then let the queue advance.
* if queue is stopped, handles unlinks.
*/
- list_for_each_safe(entry, tmp, &qh->qtd_list) {
- struct ehci_qtd *qtd;
+ list_for_each_entry_safe(qtd, tmp, &qh->qtd_list, qtd_list) {
struct urb *urb;
u32 token = 0;

- qtd = list_entry(entry, struct ehci_qtd, qtd_list);
urb = qtd->urb;

/* Clean up any state from previous QTD ...*/
@@ -1174,14 +1172,11 @@ halt:
* used for cleanup after errors, before HC sees an URB's TDs.
*/
static void qtd_list_free(struct oxu_hcd *oxu,
- struct urb *urb, struct list_head *qtd_list)
+ struct urb *urb, struct list_head *head)
{
- struct list_head *entry, *temp;
-
- list_for_each_safe(entry, temp, qtd_list) {
- struct ehci_qtd *qtd;
+ struct ehci_qtd *qtd, *temp;

- qtd = list_entry(entry, struct ehci_qtd, qtd_list);
+ list_for_each_entry_safe(qtd, temp, head, qtd_list) {
list_del(&qtd->qtd_list);
oxu_qtd_free(oxu, qtd);
}
--
2.5.0

2015-12-18 16:37:57

by Geliang Tang

[permalink] [raw]
Subject: [PATCH 4/9] usb: host: u132-hcd: use list_for_each_entry

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

Signed-off-by: Geliang Tang <[email protected]>
---
drivers/usb/host/u132-hcd.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/host/u132-hcd.c b/drivers/usb/host/u132-hcd.c
index 05c85c7..43d5293 100644
--- a/drivers/usb/host/u132-hcd.c
+++ b/drivers/usb/host/u132-hcd.c
@@ -1309,13 +1309,9 @@ static void u132_hcd_ring_work_scheduler(struct work_struct *work)
u132_ring_put_kref(u132, ring);
return;
} else if (ring->curr_endp) {
- struct u132_endp *last_endp = ring->curr_endp;
- struct list_head *scan;
- struct list_head *head = &last_endp->endp_ring;
+ struct u132_endp *endp, *last_endp = ring->curr_endp;
unsigned long wakeup = 0;
- list_for_each(scan, head) {
- struct u132_endp *endp = list_entry(scan,
- struct u132_endp, endp_ring);
+ list_for_each_entry(endp, &last_endp->endp_ring, endp_ring) {
if (endp->queue_next == endp->queue_last) {
} else if ((endp->delayed == 0)
|| time_after_eq(jiffies, endp->jiffies)) {
@@ -2393,14 +2389,12 @@ static int u132_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
static int dequeue_from_overflow_chain(struct u132 *u132,
struct u132_endp *endp, struct urb *urb)
{
- struct list_head *scan;
- struct list_head *head = &endp->urb_more;
- list_for_each(scan, head) {
- struct u132_urbq *urbq = list_entry(scan, struct u132_urbq,
- urb_more);
+ struct u132_urbq *urbq;
+
+ list_for_each_entry(urbq, &endp->urb_more, urb_more) {
if (urbq->urb == urb) {
struct usb_hcd *hcd = u132_to_hcd(u132);
- list_del(scan);
+ list_del(&urbq->urb_more);
endp->queue_size -= 1;
urb->error_count = 0;
usb_hcd_giveback_urb(hcd, urb, 0);
--
2.5.0

2015-12-18 16:37:56

by Geliang Tang

[permalink] [raw]
Subject: [PATCH 5/9] usb: xhci: use list_for_each_entry

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

Signed-off-by: Geliang Tang <[email protected]>
---
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

2015-12-18 16:35:56

by Geliang Tang

[permalink] [raw]
Subject: [PATCH 6/9] usb: chipidea: debug: use list_for_each_entry

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

Signed-off-by: Geliang Tang <[email protected]>
---
drivers/usb/chipidea/debug.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
index a4f7db2..de5c509 100644
--- a/drivers/usb/chipidea/debug.c
+++ b/drivers/usb/chipidea/debug.c
@@ -172,7 +172,6 @@ static int ci_requests_show(struct seq_file *s, void *data)
{
struct ci_hdrc *ci = s->private;
unsigned long flags;
- struct list_head *ptr = NULL;
struct ci_hw_req *req = NULL;
struct td_node *node, *tmpnode;
unsigned i, j, qsize = sizeof(struct ci_hw_td)/sizeof(u32);
@@ -184,9 +183,7 @@ static int ci_requests_show(struct seq_file *s, void *data)

spin_lock_irqsave(&ci->lock, flags);
for (i = 0; i < ci->hw_ep_max; i++)
- list_for_each(ptr, &ci->ci_hw_ep[i].qh.queue) {
- req = list_entry(ptr, struct ci_hw_req, queue);
-
+ list_for_each_entry(req, &ci->ci_hw_ep[i].qh.queue, queue) {
list_for_each_entry_safe(node, tmpnode, &req->tds, td) {
seq_printf(s, "EP=%02i: TD=%08X %s\n",
i % (ci->hw_ep_max / 2),
--
2.5.0

2015-12-18 16:37:04

by Geliang Tang

[permalink] [raw]
Subject: [PATCH 7/9] usb: dwc2: host: use list_for_each_entry_safe

Use list_for_each_entry_safe() instead of list_for_each_safe() to
simplify the code.

Signed-off-by: Geliang Tang <[email protected]>
---
drivers/usb/dwc2/hcd_ddma.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc2/hcd_ddma.c b/drivers/usb/dwc2/hcd_ddma.c
index 36606fc..14e004d 100644
--- a/drivers/usb/dwc2/hcd_ddma.c
+++ b/drivers/usb/dwc2/hcd_ddma.c
@@ -1222,9 +1222,8 @@ static void dwc2_complete_non_isoc_xfer_ddma(struct dwc2_hsotg *hsotg,
int chnum,
enum dwc2_halt_status halt_status)
{
- struct list_head *qtd_item, *qtd_tmp;
struct dwc2_qh *qh = chan->qh;
- struct dwc2_qtd *qtd = NULL;
+ struct dwc2_qtd *qtd = NULL, *tmp;
int xfer_done;
int desc_num = 0;

@@ -1234,10 +1233,9 @@ static void dwc2_complete_non_isoc_xfer_ddma(struct dwc2_hsotg *hsotg,
return;
}

- list_for_each_safe(qtd_item, qtd_tmp, &qh->qtd_list) {
+ list_for_each_entry_safe(qtd, tmp, &qh->qtd_list, qtd_list_entry) {
int i;

- qtd = list_entry(qtd_item, struct dwc2_qtd, qtd_list_entry);
xfer_done = 0;

for (i = 0; i < qtd->n_desc; i++) {
--
2.5.0

2015-12-18 16:35:46

by Geliang Tang

[permalink] [raw]
Subject: [PATCH 8/9] usb: gadget: rndis: use list_for_each_entry_safe

Use list_for_each_entry_safe() instead of list_for_each_safe() to
simplify the code.

Signed-off-by: Geliang Tang <[email protected]>
---
drivers/usb/gadget/function/rndis.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
index 70d3917..34a76db 100644
--- a/drivers/usb/gadget/function/rndis.c
+++ b/drivers/usb/gadget/function/rndis.c
@@ -914,7 +914,7 @@ struct rndis_params *rndis_register(void (*resp_avail)(void *v), void *v)
params->media_state = RNDIS_MEDIA_STATE_DISCONNECTED;
params->resp_avail = resp_avail;
params->v = v;
- INIT_LIST_HEAD(&(params->resp_queue));
+ INIT_LIST_HEAD(&params->resp_queue);
pr_debug("%s: configNr = %d\n", __func__, i);

return params;
@@ -1006,12 +1006,9 @@ EXPORT_SYMBOL_GPL(rndis_add_hdr);

void rndis_free_response(struct rndis_params *params, u8 *buf)
{
- rndis_resp_t *r;
- struct list_head *act, *tmp;
+ rndis_resp_t *r, *n;

- list_for_each_safe(act, tmp, &(params->resp_queue))
- {
- r = list_entry(act, rndis_resp_t, list);
+ list_for_each_entry_safe(r, n, &params->resp_queue, list) {
if (r && r->buf == buf) {
list_del(&r->list);
kfree(r);
@@ -1022,14 +1019,11 @@ EXPORT_SYMBOL_GPL(rndis_free_response);

u8 *rndis_get_next_response(struct rndis_params *params, u32 *length)
{
- rndis_resp_t *r;
- struct list_head *act, *tmp;
+ rndis_resp_t *r, *n;

if (!length) return NULL;

- list_for_each_safe(act, tmp, &(params->resp_queue))
- {
- r = list_entry(act, rndis_resp_t, list);
+ list_for_each_entry_safe(r, n, &params->resp_queue, list) {
if (!r->send) {
r->send = 1;
*length = r->length;
@@ -1053,7 +1047,7 @@ static rndis_resp_t *rndis_add_response(struct rndis_params *params, u32 length)
r->length = length;
r->send = 0;

- list_add_tail(&r->list, &(params->resp_queue));
+ list_add_tail(&r->list, &params->resp_queue);
return r;
}

--
2.5.0

2015-12-18 16:37:58

by Geliang Tang

[permalink] [raw]
Subject: [PATCH 9/9] usb: gadget: bcm63xx_udc: use list_for_each_entry_safe

Use list_for_each_entry_safe() instead of list_for_each_safe() to
simplify the code.

Signed-off-by: Geliang Tang <[email protected]>
---
drivers/usb/gadget/udc/bcm63xx_udc.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/udc/bcm63xx_udc.c b/drivers/usb/gadget/udc/bcm63xx_udc.c
index 8cbb003..f5fccb3 100644
--- a/drivers/usb/gadget/udc/bcm63xx_udc.c
+++ b/drivers/usb/gadget/udc/bcm63xx_udc.c
@@ -1083,7 +1083,7 @@ static int bcm63xx_ep_disable(struct usb_ep *ep)
struct bcm63xx_ep *bep = our_ep(ep);
struct bcm63xx_udc *udc = bep->udc;
struct iudma_ch *iudma = bep->iudma;
- struct list_head *pos, *n;
+ struct bcm63xx_req *breq, *n;
unsigned long flags;

if (!ep || !ep->desc)
@@ -1099,10 +1099,7 @@ static int bcm63xx_ep_disable(struct usb_ep *ep)
iudma_reset_channel(udc, iudma);

if (!list_empty(&bep->queue)) {
- list_for_each_safe(pos, n, &bep->queue) {
- struct bcm63xx_req *breq =
- list_entry(pos, struct bcm63xx_req, queue);
-
+ list_for_each_entry_safe(breq, n, &bep->queue, queue) {
usb_gadget_unmap_request(&udc->gadget, &breq->req,
iudma->is_tx);
list_del(&breq->queue);
--
2.5.0

2015-12-18 17:53:29

by Julia Lawall

[permalink] [raw]
Subject: Re: [PATCH 2/9] usb: host: max3421-hcd: use list_for_each_entry*

Geliang,

Please check whether line 762 can be reached in the case where the
list_for_each_entry reaches the end of the list. If that can happen,
max3421_ep should not be dereferenced.

julia

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

> CC: [email protected]
> In-Reply-To: <45e8397e370ed99ceb8aa1719c2b56a7ce741eb3.1450455485.git.geliangtang@163.com>
> TO: Geliang Tang <[email protected]>
> CC: Greg Kroah-Hartman <[email protected]>, Sergei Shtylyov <[email protected]>
> CC: Geliang Tang <[email protected]>, [email protected], [email protected]
>
> 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: 66 minutes ago
> :::::: commit date: 66 minutes ago
>
> >> drivers/usb/host/max3421-hcd.c:762:5-15: ERROR: invalid reference to the index variable of the iterator on line 675
>
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 610e92adf2a45ef78039582494d8023f385d12ec
> vim +762 drivers/usb/host/max3421-hcd.c
>
> 2d53139f David Mosberger 2014-04-28 669
> 2d53139f David Mosberger 2014-04-28 670 spin_lock_irqsave(&max3421_hcd->lock, flags);
> 2d53139f David Mosberger 2014-04-28 671
> 2d53139f David Mosberger 2014-04-28 672 for (;
> 2d53139f David Mosberger 2014-04-28 673 max3421_hcd->sched_pass < SCHED_PASS_DONE;
> 2d53139f David Mosberger 2014-04-28 674 ++max3421_hcd->sched_pass)
> 610e92ad Geliang Tang 2015-12-19 @675 list_for_each_entry(max3421_ep, &max3421_hcd->ep_list,
> 610e92ad Geliang Tang 2015-12-19 676 ep_list) {
> 2d53139f David Mosberger 2014-04-28 677 urb = NULL;
> 2d53139f David Mosberger 2014-04-28 678 ep = max3421_ep->ep;
> 2d53139f David Mosberger 2014-04-28 679
> 2d53139f David Mosberger 2014-04-28 680 switch (usb_endpoint_type(&ep->desc)) {
> 2d53139f David Mosberger 2014-04-28 681 case USB_ENDPOINT_XFER_ISOC:
> 2d53139f David Mosberger 2014-04-28 682 case USB_ENDPOINT_XFER_INT:
> 2d53139f David Mosberger 2014-04-28 683 if (max3421_hcd->sched_pass !=
> 2d53139f David Mosberger 2014-04-28 684 SCHED_PASS_PERIODIC)
> 2d53139f David Mosberger 2014-04-28 685 continue;
> 2d53139f David Mosberger 2014-04-28 686 break;
> 2d53139f David Mosberger 2014-04-28 687
> 2d53139f David Mosberger 2014-04-28 688 case USB_ENDPOINT_XFER_CONTROL:
> 2d53139f David Mosberger 2014-04-28 689 case USB_ENDPOINT_XFER_BULK:
> 2d53139f David Mosberger 2014-04-28 690 if (max3421_hcd->sched_pass !=
> 2d53139f David Mosberger 2014-04-28 691 SCHED_PASS_NON_PERIODIC)
> 2d53139f David Mosberger 2014-04-28 692 continue;
> 2d53139f David Mosberger 2014-04-28 693 break;
> 2d53139f David Mosberger 2014-04-28 694 }
> 2d53139f David Mosberger 2014-04-28 695
> 2d53139f David Mosberger 2014-04-28 696 if (list_empty(&ep->urb_list))
> 2d53139f David Mosberger 2014-04-28 697 continue; /* nothing to do */
> 2d53139f David Mosberger 2014-04-28 698 urb = list_first_entry(&ep->urb_list, struct urb,
> 2d53139f David Mosberger 2014-04-28 699 urb_list);
> 2d53139f David Mosberger 2014-04-28 700 if (urb->unlinked) {
> 2d53139f David Mosberger 2014-04-28 701 dev_dbg(&spi->dev, "%s: URB %p unlinked=%d",
> 2d53139f David Mosberger 2014-04-28 702 __func__, urb, urb->unlinked);
> 2d53139f David Mosberger 2014-04-28 703 max3421_hcd->curr_urb = urb;
> 2d53139f David Mosberger 2014-04-28 704 max3421_hcd->urb_done = 1;
> 2d53139f David Mosberger 2014-04-28 705 spin_unlock_irqrestore(&max3421_hcd->lock,
> 2d53139f David Mosberger 2014-04-28 706 flags);
> 2d53139f David Mosberger 2014-04-28 707 return 1;
> 2d53139f David Mosberger 2014-04-28 708 }
> 2d53139f David Mosberger 2014-04-28 709
> 2d53139f David Mosberger 2014-04-28 710 switch (usb_endpoint_type(&ep->desc)) {
> 2d53139f David Mosberger 2014-04-28 711 case USB_ENDPOINT_XFER_CONTROL:
> 2d53139f David Mosberger 2014-04-28 712 /*
> 2d53139f David Mosberger 2014-04-28 713 * Allow one control transaction per
> 2d53139f David Mosberger 2014-04-28 714 * frame per endpoint:
> 2d53139f David Mosberger 2014-04-28 715 */
> 2d53139f David Mosberger 2014-04-28 716 if (frame_diff(max3421_ep->last_active,
> 2d53139f David Mosberger 2014-04-28 717 max3421_hcd->frame_number) == 0)
> 2d53139f David Mosberger 2014-04-28 718 continue;
> 2d53139f David Mosberger 2014-04-28 719 break;
> 2d53139f David Mosberger 2014-04-28 720
> 2d53139f David Mosberger 2014-04-28 721 case USB_ENDPOINT_XFER_BULK:
> 2d53139f David Mosberger 2014-04-28 722 if (max3421_ep->retransmit
> 2d53139f David Mosberger 2014-04-28 723 && (frame_diff(max3421_ep->last_active,
> 2d53139f David Mosberger 2014-04-28 724 max3421_hcd->frame_number)
> 2d53139f David Mosberger 2014-04-28 725 == 0))
> 2d53139f David Mosberger 2014-04-28 726 /*
> 2d53139f David Mosberger 2014-04-28 727 * We already tried this EP
> 2d53139f David Mosberger 2014-04-28 728 * during this frame and got a
> 2d53139f David Mosberger 2014-04-28 729 * NAK or error; wait for next frame
> 2d53139f David Mosberger 2014-04-28 730 */
> 2d53139f David Mosberger 2014-04-28 731 continue;
> 2d53139f David Mosberger 2014-04-28 732 break;
> 2d53139f David Mosberger 2014-04-28 733
> 2d53139f David Mosberger 2014-04-28 734 case USB_ENDPOINT_XFER_ISOC:
> 2d53139f David Mosberger 2014-04-28 735 case USB_ENDPOINT_XFER_INT:
> 2d53139f David Mosberger 2014-04-28 736 if (frame_diff(max3421_hcd->frame_number,
> 2d53139f David Mosberger 2014-04-28 737 max3421_ep->last_active)
> 2d53139f David Mosberger 2014-04-28 738 < urb->interval)
> 2d53139f David Mosberger 2014-04-28 739 /*
> 2d53139f David Mosberger 2014-04-28 740 * We already processed this
> 2d53139f David Mosberger 2014-04-28 741 * end-point in the current
> 2d53139f David Mosberger 2014-04-28 742 * frame
> 2d53139f David Mosberger 2014-04-28 743 */
> 2d53139f David Mosberger 2014-04-28 744 continue;
> 2d53139f David Mosberger 2014-04-28 745 break;
> 2d53139f David Mosberger 2014-04-28 746 }
> 2d53139f David Mosberger 2014-04-28 747
> 2d53139f David Mosberger 2014-04-28 748 /* move current ep to tail: */
> 610e92ad Geliang Tang 2015-12-19 749 list_move_tail(&max3421_ep->ep_list,
> 610e92ad Geliang Tang 2015-12-19 750 &max3421_hcd->ep_list);
> 2d53139f David Mosberger 2014-04-28 751 curr_urb = urb;
> 2d53139f David Mosberger 2014-04-28 752 goto done;
> 2d53139f David Mosberger 2014-04-28 753 }
> 2d53139f David Mosberger 2014-04-28 754 done:
> 2d53139f David Mosberger 2014-04-28 755 if (!curr_urb) {
> 2d53139f David Mosberger 2014-04-28 756 spin_unlock_irqrestore(&max3421_hcd->lock, flags);
> 2d53139f David Mosberger 2014-04-28 757 return 0;
> 2d53139f David Mosberger 2014-04-28 758 }
> 2d53139f David Mosberger 2014-04-28 759
> 2d53139f David Mosberger 2014-04-28 760 urb = max3421_hcd->curr_urb = curr_urb;
> 2d53139f David Mosberger 2014-04-28 761 epnum = usb_endpoint_num(&urb->ep->desc);
> 2d53139f David Mosberger 2014-04-28 @762 if (max3421_ep->retransmit)
> 2d53139f David Mosberger 2014-04-28 763 /* restart (part of) a USB transaction: */
> 2d53139f David Mosberger 2014-04-28 764 max3421_ep->retransmit = 0;
> 2d53139f David Mosberger 2014-04-28 765 else {
>
> :::::: The code at line 762 was first introduced by commit
> :::::: 2d53139f31626bad6f8983d8e519ddde2cbba921 Add support for using a MAX3421E chip as a host driver.
>
> :::::: TO: David Mosberger <[email protected]>
> :::::: CC: Greg Kroah-Hartman <[email protected]>
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>

2015-12-18 18:30:26

by Julia Lawall

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

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: [email protected]
> In-Reply-To: <0fbea26fdbcb76e24188fc0800d425f927f45b6f.1450455485.git.geliangtang@163.com>
> TO: Geliang Tang <[email protected]>
> CC: Mathias Nyman <[email protected]>, Greg Kroah-Hartman <[email protected]>
> CC: Geliang Tang <[email protected]>, [email protected], [email protected]
>
> 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 <[email protected]>
> :::::: CC: Greg Kroah-Hartman <[email protected]>
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>

2015-12-18 20:10:44

by Julia Lawall

[permalink] [raw]
Subject: Re: [PATCH 7/9] usb: dwc2: host: use list_for_each_entry_safe

The call on line 1120 looks suspicious, because qtd could be non-null but
also not a valid element, if the loop has exited normally.

julia

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

> CC: [email protected]
> In-Reply-To: <c123b5df7c51a27f33f0d423bf4752a221559fd1.1450455486.git.geliangtang@163.com>
> TO: Geliang Tang <[email protected]>
> CC: John Youn <[email protected]>, Greg Kroah-Hartman <[email protected]>
> CC: Geliang Tang <[email protected]>, [email protected], [email protected]
>
> 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/dwc2/hcd_ddma.c:1119:11-14: ERROR: invalid reference to the index variable of the iterator on line 1096
>
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 61366173f3eb9bee0d5cd22e6a7c5407b14087cb
> vim +1119 drivers/usb/dwc2/hcd_ddma.c
>
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1090 if (chan->halt_status == DWC2_HC_XFER_URB_DEQUEUE) {
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1091 list_for_each_entry(qtd, &qh->qtd_list, qtd_list_entry)
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1092 qtd->in_process = 0;
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1093 return;
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1094 }
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1095
> 61366173 drivers/usb/dwc2/hcd_ddma.c Geliang Tang 2015-12-19 @1096 list_for_each_entry_safe(qtd, tmp, &qh->qtd_list, qtd_list_entry) {
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1097 int i;
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1098
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1099 xfer_done = 0;
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1100
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1101 for (i = 0; i < qtd->n_desc; i++) {
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1102 if (dwc2_process_non_isoc_desc(hsotg, chan, chnum, qtd,
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1103 desc_num, halt_status,
> fbd1cd20 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-11-22 1104 &xfer_done)) {
> fbd1cd20 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-11-22 1105 qtd = NULL;
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1106 break;
> fbd1cd20 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-11-22 1107 }
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1108 desc_num++;
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1109 }
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1110 }
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1111
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1112 if (qh->ep_type != USB_ENDPOINT_XFER_CONTROL) {
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1113 /*
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1114 * Resetting the data toggle for bulk and interrupt endpoints
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1115 * in case of stall. See handle_hc_stall_intr().
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1116 */
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1117 if (halt_status == DWC2_HC_XFER_STALL)
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1118 qh->data_toggle = DWC2_HC_PID_DATA0;
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 @1119 else if (qtd)
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1120 dwc2_hcd_save_data_toggle(hsotg, chan, chnum, qtd);
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1121 }
> dc4c76e7 drivers/staging/dwc2/hcd_ddma.c Paul Zimmerman 2013-03-11 1122
>
> :::::: The code at line 1119 was first introduced by commit
> :::::: dc4c76e7b22cdcc1ba71ff87edee55f464e01658 staging: HCD descriptor DMA support for the DWC2 driver
>
> :::::: TO: Paul Zimmerman <[email protected]>
> :::::: CC: Greg Kroah-Hartman <[email protected]>
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>

2015-12-19 16:12:54

by Geliang Tang

[permalink] [raw]
Subject: [PATCH 2/9 v2] usb: host: max3421-hcd: use list_for_each_entry*

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

Signed-off-by: Geliang Tang <[email protected]>
---
Changes in v2:
- drop changes in max3421_select_and_start_urb().
---
drivers/usb/host/max3421-hcd.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index bd98706..c369c29 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -797,19 +797,16 @@ max3421_check_unlink(struct usb_hcd *hcd)
{
struct spi_device *spi = to_spi_device(hcd->self.controller);
struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd);
- struct list_head *pos, *upos, *next_upos;
struct max3421_ep *max3421_ep;
struct usb_host_endpoint *ep;
- struct urb *urb;
+ struct urb *urb, *next;
unsigned long flags;
int retval = 0;

spin_lock_irqsave(&max3421_hcd->lock, flags);
- list_for_each(pos, &max3421_hcd->ep_list) {
- max3421_ep = container_of(pos, struct max3421_ep, ep_list);
+ list_for_each_entry(max3421_ep, &max3421_hcd->ep_list, ep_list) {
ep = max3421_ep->ep;
- list_for_each_safe(upos, next_upos, &ep->urb_list) {
- urb = container_of(upos, struct urb, urb_list);
+ list_for_each_entry_safe(urb, next, &ep->urb_list, urb_list) {
if (urb->unlinked) {
retval = 1;
dev_dbg(&spi->dev, "%s: URB %p unlinked=%d",
@@ -1184,22 +1181,19 @@ dump_eps(struct usb_hcd *hcd)
struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd);
struct max3421_ep *max3421_ep;
struct usb_host_endpoint *ep;
- struct list_head *pos, *upos;
char ubuf[512], *dp, *end;
unsigned long flags;
struct urb *urb;
int epnum, ret;

spin_lock_irqsave(&max3421_hcd->lock, flags);
- list_for_each(pos, &max3421_hcd->ep_list) {
- max3421_ep = container_of(pos, struct max3421_ep, ep_list);
+ list_for_each_entry(max3421_ep, &max3421_hcd->ep_list, ep_list) {
ep = max3421_ep->ep;

dp = ubuf;
end = dp + sizeof(ubuf);
*dp = '\0';
- list_for_each(upos, &ep->urb_list) {
- urb = container_of(upos, struct urb, urb_list);
+ list_for_each_entry(urb, &ep->urb_list, urb_list) {
ret = snprintf(dp, end - dp, " %p(%d.%s %d/%d)", urb,
usb_pipetype(urb->pipe),
usb_urb_dir_in(urb) ? "IN" : "OUT",
--
2.5.0

2015-12-21 13:04:02

by Mathias Nyman

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

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




2015-12-22 18:07:54

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH 8/9] usb: gadget: rndis: use list_for_each_entry_safe


Hi,

Geliang Tang <[email protected]> writes:
> Use list_for_each_entry_safe() instead of list_for_each_safe() to
> simplify the code.
>
> Signed-off-by: Geliang Tang <[email protected]>

there are other cleanups in this patch which shouldn't be here. Please
split and I'll apply for v4.6 merge window.

--
balbi


Attachments:
signature.asc (818.00 B)

2015-12-23 13:53:22

by Geliang Tang

[permalink] [raw]
Subject: [PATCH 8/9 v2] usb: gadget: rndis: use list_for_each_entry_safe

Use list_for_each_entry_safe() instead of list_for_each_safe() to
simplify the code.

Signed-off-by: Geliang Tang <[email protected]>
---
drivers/usb/gadget/function/rndis.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
index 70d3917..13280f5 100644
--- a/drivers/usb/gadget/function/rndis.c
+++ b/drivers/usb/gadget/function/rndis.c
@@ -1006,12 +1006,9 @@ EXPORT_SYMBOL_GPL(rndis_add_hdr);

void rndis_free_response(struct rndis_params *params, u8 *buf)
{
- rndis_resp_t *r;
- struct list_head *act, *tmp;
+ rndis_resp_t *r, *n;

- list_for_each_safe(act, tmp, &(params->resp_queue))
- {
- r = list_entry(act, rndis_resp_t, list);
+ list_for_each_entry_safe(r, n, &params->resp_queue, list) {
if (r && r->buf == buf) {
list_del(&r->list);
kfree(r);
@@ -1022,14 +1019,11 @@ EXPORT_SYMBOL_GPL(rndis_free_response);

u8 *rndis_get_next_response(struct rndis_params *params, u32 *length)
{
- rndis_resp_t *r;
- struct list_head *act, *tmp;
+ rndis_resp_t *r, *n;

if (!length) return NULL;

- list_for_each_safe(act, tmp, &(params->resp_queue))
- {
- r = list_entry(act, rndis_resp_t, list);
+ list_for_each_entry_safe(r, n, &params->resp_queue, list) {
if (!r->send) {
r->send = 1;
*length = r->length;
--
2.5.0

2015-12-25 07:46:19

by Peter Chen

[permalink] [raw]
Subject: Re: [PATCH 6/9] usb: chipidea: debug: use list_for_each_entry

On Sat, Dec 19, 2015 at 12:34:31AM +0800, Geliang Tang wrote:
> Use list_for_each_entry() instead of list_for_each() to simplify
> the code.
>
> Signed-off-by: Geliang Tang <[email protected]>
> ---
> drivers/usb/chipidea/debug.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
> index a4f7db2..de5c509 100644
> --- a/drivers/usb/chipidea/debug.c
> +++ b/drivers/usb/chipidea/debug.c
> @@ -172,7 +172,6 @@ static int ci_requests_show(struct seq_file *s, void *data)
> {
> struct ci_hdrc *ci = s->private;
> unsigned long flags;
> - struct list_head *ptr = NULL;
> struct ci_hw_req *req = NULL;
> struct td_node *node, *tmpnode;
> unsigned i, j, qsize = sizeof(struct ci_hw_td)/sizeof(u32);
> @@ -184,9 +183,7 @@ static int ci_requests_show(struct seq_file *s, void *data)
>
> spin_lock_irqsave(&ci->lock, flags);
> for (i = 0; i < ci->hw_ep_max; i++)
> - list_for_each(ptr, &ci->ci_hw_ep[i].qh.queue) {
> - req = list_entry(ptr, struct ci_hw_req, queue);
> -
> + list_for_each_entry(req, &ci->ci_hw_ep[i].qh.queue, queue) {
> list_for_each_entry_safe(node, tmpnode, &req->tds, td) {
> seq_printf(s, "EP=%02i: TD=%08X %s\n",
> i % (ci->hw_ep_max / 2),
> --
> 2.5.0
>

It is ok for chipidea part, I will queue it, you don't need to
re-send it again

--

Best Regards,
Peter Chen