This patch set removes any use of the list iterator variable past
the list body. This will allow defining the list iterator variable
within the list_for_each_entry_*() macros to avoid any (invalid)
use after the loop. If no break/goto was hit during list traversal
the list iterator variable would otherwise be a bogus pointer
since it is computed on something that is not actually an element
of the list.
I've basically followed what we discussed in:
https://lore.kernel.org/all/[email protected]/
There are some cases where it might be possible to 'ditch' the tmp
variable and refactor the code past the loop into the loop body.
For the sake of keeping the *_dequeue() functions more similar, I've
decided against doing it for some and leaving it in others.
In general there are four use cases after the loop body here:
1) using &req->req in a comparision after the loop
2) using the iterator as a pointer in a comparision after the loop
3) use the &iterator->list to compare with the head to see if the
loop exits early
4) using the iterator past the loop but using the rc variable to see if the
loop exits early
Change since v1:
- renamed list iterator variable from 'tmp' to 'iter' (Linus)
- inverted the conditions within the loop (Linus)
- reverted the check on 'rc' after the loop (Greg & Krzysztof)
Jakob Koschel (26):
usb: gadget: fsl: remove usage of list iterator past the loop body
usb: gadget: bdc: remove usage of list iterator past the loop body
usb: gadget: udc: atmel: remove usage of list iterator past the loop
body
usb: gadget: udc: pxa25x: remove usage of list iterator past the loop
body
usb: gadget: udc: at91: remove usage of list iterator past the loop
body
usb: gadget: goku_udc: remove usage of list iterator past the loop
body
usb: gadget: udc: gr_udc: remove usage of list iterator past the loop
body
usb: gadget: lpc32xx_udc: remove usage of list iterator past the loop
body
usb: gadget: mv_u3d: remove usage of list iterator past the loop body
usb: gadget: udc: mv_udc_core: remove usage of list iterator past the
loop body
usb: gadget: net2272: remove usage of list iterator past the loop body
usb: gadget: udc: net2280: remove usage of list iterator past the loop
body
usb: gadget: omap_udc: remove usage of list iterator past the loop
body
usb: gadget: s3c-hsudc: remove usage of list iterator past the loop
body
usb: gadget: udc-xilinx: remove usage of list iterator past the loop
body
usb: gadget: aspeed: remove usage of list iterator past the loop body
usb: gadget: configfs: remove using list iterator after loop body as a
ptr
usb: gadget: legacy: remove using list iterator after loop body as a
ptr
usb: gadget: udc: max3420_udc: remove using list iterator after loop
body as a ptr
usb: gadget: tegra-xudc: remove using list iterator after loop body as
a ptr
usb: gadget: composite: remove check of list iterator against head
past the loop body
usb: gadget: pxa27x_udc: replace usage of rc to check if a list
element was found
usb: gadget: composite: remove usage of list iterator past the loop
body
usb: gadget: udc: core: remove usage of list iterator past the loop
body
usb: gadget: dummy_hcd: remove usage of list iterator past the loop
body
usb: gadget: udc: s3c2410: remove usage of list iterator past the loop
body
drivers/usb/gadget/composite.c | 36 +++++++++++++-----------
drivers/usb/gadget/configfs.c | 24 +++++++++-------
drivers/usb/gadget/legacy/hid.c | 23 +++++++--------
drivers/usb/gadget/udc/aspeed-vhub/epn.c | 12 ++++----
drivers/usb/gadget/udc/at91_udc.c | 12 ++++----
drivers/usb/gadget/udc/atmel_usba_udc.c | 13 +++++----
drivers/usb/gadget/udc/bdc/bdc_ep.c | 13 ++++++---
drivers/usb/gadget/udc/core.c | 20 +++++++------
drivers/usb/gadget/udc/dummy_hcd.c | 17 +++++------
drivers/usb/gadget/udc/fsl_qe_udc.c | 13 +++++----
drivers/usb/gadget/udc/fsl_udc_core.c | 13 +++++----
drivers/usb/gadget/udc/goku_udc.c | 12 ++++----
drivers/usb/gadget/udc/gr_udc.c | 12 ++++----
drivers/usb/gadget/udc/lpc32xx_udc.c | 12 ++++----
drivers/usb/gadget/udc/max3420_udc.c | 18 +++++++-----
drivers/usb/gadget/udc/mv_u3d_core.c | 12 ++++----
drivers/usb/gadget/udc/mv_udc_core.c | 12 ++++----
drivers/usb/gadget/udc/net2272.c | 13 +++++----
drivers/usb/gadget/udc/net2280.c | 13 +++++----
drivers/usb/gadget/udc/omap_udc.c | 12 ++++----
drivers/usb/gadget/udc/pxa25x_udc.c | 13 +++++----
drivers/usb/gadget/udc/pxa27x_udc.c | 13 +++++----
drivers/usb/gadget/udc/s3c-hsudc.c | 12 ++++----
drivers/usb/gadget/udc/s3c2410_udc.c | 17 +++++------
drivers/usb/gadget/udc/tegra-xudc.c | 12 ++++----
drivers/usb/gadget/udc/udc-xilinx.c | 13 +++++----
26 files changed, 227 insertions(+), 165 deletions(-)
base-commit: 719fce7539cd3e186598e2aed36325fe892150cf
--
2.25.1
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/mv_u3d_core.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/mv_u3d_core.c b/drivers/usb/gadget/udc/mv_u3d_core.c
index a1057ddfbda3..598654a3cb41 100644
--- a/drivers/usb/gadget/udc/mv_u3d_core.c
+++ b/drivers/usb/gadget/udc/mv_u3d_core.c
@@ -844,7 +844,7 @@ mv_u3d_ep_queue(struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
static int mv_u3d_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct mv_u3d_ep *ep;
- struct mv_u3d_req *req;
+ struct mv_u3d_req *req = NULL, *iter;
struct mv_u3d *u3d;
struct mv_u3d_ep_context *ep_context;
struct mv_u3d_req *next_req;
@@ -861,11 +861,13 @@ static int mv_u3d_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
spin_lock_irqsave(&ep->u3d->lock, flags);
/* make sure it's actually queued on this endpoint */
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->req == _req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ req = iter;
+ break;
}
- if (&req->req != _req) {
+ if (!req) {
ret = -EINVAL;
goto out;
}
--
2.25.1
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/s3c-hsudc.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/s3c-hsudc.c b/drivers/usb/gadget/udc/s3c-hsudc.c
index 89f1f8c9f02e..bf803e013458 100644
--- a/drivers/usb/gadget/udc/s3c-hsudc.c
+++ b/drivers/usb/gadget/udc/s3c-hsudc.c
@@ -877,7 +877,7 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct s3c_hsudc_ep *hsep = our_ep(_ep);
struct s3c_hsudc *hsudc = hsep->dev;
- struct s3c_hsudc_req *hsreq;
+ struct s3c_hsudc_req *hsreq = NULL, *iter;
unsigned long flags;
hsep = our_ep(_ep);
@@ -886,11 +886,13 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req)
spin_lock_irqsave(&hsudc->lock, flags);
- list_for_each_entry(hsreq, &hsep->queue, queue) {
- if (&hsreq->req == _req)
- break;
+ list_for_each_entry(iter, &hsep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ hsreq = iter;
+ break;
}
- if (&hsreq->req != _req) {
+ if (!hsreq) {
spin_unlock_irqrestore(&hsudc->lock, flags);
return -EINVAL;
}
--
2.25.1
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/bdc/bdc_ep.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/udc/bdc/bdc_ep.c b/drivers/usb/gadget/udc/bdc/bdc_ep.c
index 8e2f20b12519..fa88f210ecd5 100644
--- a/drivers/usb/gadget/udc/bdc/bdc_ep.c
+++ b/drivers/usb/gadget/udc/bdc/bdc_ep.c
@@ -1757,6 +1757,7 @@ static int bdc_gadget_ep_dequeue(struct usb_ep *_ep,
struct usb_request *_req)
{
struct bdc_req *req;
+ struct bdc_req *iter;
unsigned long flags;
struct bdc_ep *ep;
struct bdc *bdc;
@@ -1771,12 +1772,16 @@ static int bdc_gadget_ep_dequeue(struct usb_ep *_ep,
dev_dbg(bdc->dev, "%s ep:%s req:%p\n", __func__, ep->name, req);
bdc_dbg_bd_list(bdc, ep);
spin_lock_irqsave(&bdc->lock, flags);
+
+ req = NULL;
/* make sure it's still queued on this endpoint */
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->usb_req == _req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->usb_req != _req)
+ continue;
+ req = iter;
+ break;
}
- if (&req->usb_req != _req) {
+ if (!req) {
spin_unlock_irqrestore(&bdc->lock, flags);
dev_err(bdc->dev, "usb_req !=req n");
return -EINVAL;
--
2.25.1
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/core.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 568534a0d17c..02735b463bb4 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1528,7 +1528,7 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
{
- struct usb_udc *udc = NULL;
+ struct usb_udc *udc = NULL, *iter;
int ret = -ENODEV;
if (!driver || !driver->bind || !driver->setup)
@@ -1536,10 +1536,12 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
mutex_lock(&udc_lock);
if (driver->udc_name) {
- list_for_each_entry(udc, &udc_list, list) {
- ret = strcmp(driver->udc_name, dev_name(&udc->dev));
- if (!ret)
- break;
+ list_for_each_entry(iter, &udc_list, list) {
+ ret = strcmp(driver->udc_name, dev_name(&iter->dev));
+ if (ret)
+ continue;
+ udc = iter;
+ break;
}
if (ret)
ret = -ENODEV;
@@ -1548,10 +1550,12 @@ int usb_gadget_probe_driver(struct usb_gadget_driver *driver)
else
goto found;
} else {
- list_for_each_entry(udc, &udc_list, list) {
+ list_for_each_entry(iter, &udc_list, list) {
/* For now we take the first one */
- if (!udc->driver)
- goto found;
+ if (iter->driver)
+ continue;
+ udc = iter;
+ goto found;
}
}
--
2.25.1
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/goku_udc.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/goku_udc.c b/drivers/usb/gadget/udc/goku_udc.c
index 3757a772a55e..bdc56b24b5c9 100644
--- a/drivers/usb/gadget/udc/goku_udc.c
+++ b/drivers/usb/gadget/udc/goku_udc.c
@@ -809,7 +809,7 @@ static void nuke(struct goku_ep *ep, int status)
/* dequeue JUST ONE request */
static int goku_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
- struct goku_request *req;
+ struct goku_request *req = NULL, *iter;
struct goku_ep *ep;
struct goku_udc *dev;
unsigned long flags;
@@ -833,11 +833,13 @@ static int goku_dequeue(struct usb_ep *_ep, struct usb_request *_req)
spin_lock_irqsave(&dev->lock, flags);
/* make sure it's actually queued on this endpoint */
- list_for_each_entry (req, &ep->queue, queue) {
- if (&req->req == _req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ req = iter;
+ break;
}
- if (&req->req != _req) {
+ if (!req) {
spin_unlock_irqrestore (&dev->lock, flags);
return -EINVAL;
}
--
2.25.1
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/at91_udc.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/at91_udc.c b/drivers/usb/gadget/udc/at91_udc.c
index 9040a0561466..728987280373 100644
--- a/drivers/usb/gadget/udc/at91_udc.c
+++ b/drivers/usb/gadget/udc/at91_udc.c
@@ -704,7 +704,7 @@ static int at91_ep_queue(struct usb_ep *_ep,
static int at91_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct at91_ep *ep;
- struct at91_request *req;
+ struct at91_request *req = NULL, *iter;
unsigned long flags;
struct at91_udc *udc;
@@ -717,11 +717,13 @@ static int at91_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
spin_lock_irqsave(&udc->lock, flags);
/* make sure it's actually queued on this endpoint */
- list_for_each_entry (req, &ep->queue, queue) {
- if (&req->req == _req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ req = iter;
+ break;
}
- if (&req->req != _req) {
+ if (!req) {
spin_unlock_irqrestore(&udc->lock, flags);
return -EINVAL;
}
--
2.25.1
When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will be a bogus pointer
computed based on the head element.
While it is safe to use the pointer to determine if it was computed
based on the head element, either with list_entry_is_head() or
&pos->member == head, using the iterator variable after the loop should
be avoided.
In preparation to limiting the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/composite.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 9315313108c9..4f7e789c3e07 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1690,6 +1690,7 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
u16 w_value = le16_to_cpu(ctrl->wValue);
u16 w_length = le16_to_cpu(ctrl->wLength);
struct usb_function *f = NULL;
+ struct usb_function *iter;
u8 endp;
if (w_length > USB_COMP_EP0_BUFSIZ) {
@@ -2046,12 +2047,12 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
if (!cdev->config)
break;
endp = ((w_index & 0x80) >> 3) | (w_index & 0x0f);
- list_for_each_entry(f, &cdev->config->functions, list) {
- if (test_bit(endp, f->endpoints))
+ list_for_each_entry(iter, &cdev->config->functions, list) {
+ if (test_bit(endp, iter->endpoints)) {
+ f = iter;
break;
+ }
}
- if (&f->list == &cdev->config->functions)
- f = NULL;
break;
}
try_fun_setup:
--
2.25.1
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/fsl_qe_udc.c | 13 ++++++++-----
drivers/usb/gadget/udc/fsl_udc_core.c | 13 ++++++++-----
2 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c b/drivers/usb/gadget/udc/fsl_qe_udc.c
index 15db7a3868fe..d80a7fe5ff62 100644
--- a/drivers/usb/gadget/udc/fsl_qe_udc.c
+++ b/drivers/usb/gadget/udc/fsl_qe_udc.c
@@ -1776,7 +1776,8 @@ static int qe_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
static int qe_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct qe_ep *ep = container_of(_ep, struct qe_ep, ep);
- struct qe_req *req;
+ struct qe_req *req = NULL;
+ struct qe_req *iter;
unsigned long flags;
if (!_ep || !_req)
@@ -1785,12 +1786,14 @@ static int qe_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
spin_lock_irqsave(&ep->udc->lock, flags);
/* make sure it's actually queued on this endpoint */
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->req == _req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue
+ req = iter;
+ break;
}
- if (&req->req != _req) {
+ if (!req) {
spin_unlock_irqrestore(&ep->udc->lock, flags);
return -EINVAL;
}
diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c b/drivers/usb/gadget/udc/fsl_udc_core.c
index 29fcb9b461d7..50435e804118 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -918,7 +918,8 @@ fsl_ep_queue(struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
static int fsl_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct fsl_ep *ep = container_of(_ep, struct fsl_ep, ep);
- struct fsl_req *req;
+ struct fsl_req *req = NULL;
+ struct fsl_req *iter;
unsigned long flags;
int ep_num, stopped, ret = 0;
u32 epctrl;
@@ -940,11 +941,13 @@ static int fsl_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
fsl_writel(epctrl, &dr_regs->endptctrl[ep_num]);
/* make sure it's actually queued on this endpoint */
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->req == _req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ req = iter;
+ break;
}
- if (&req->req != _req) {
+ if (!req) {
ret = -EINVAL;
goto out;
}
--
2.25.1
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/dummy_hcd.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
index a2d956af42a2..35aec8e7fc73 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -751,7 +751,7 @@ static int dummy_dequeue(struct usb_ep *_ep, struct usb_request *_req)
struct dummy *dum;
int retval = -EINVAL;
unsigned long flags;
- struct dummy_request *req = NULL;
+ struct dummy_request *req = NULL, *iter;
if (!_ep || !_req)
return retval;
@@ -763,13 +763,14 @@ static int dummy_dequeue(struct usb_ep *_ep, struct usb_request *_req)
local_irq_save(flags);
spin_lock(&dum->lock);
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->req == _req) {
- list_del_init(&req->queue);
- _req->status = -ECONNRESET;
- retval = 0;
- break;
- }
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ list_del_init(&iter->queue);
+ _req->status = -ECONNRESET;
+ req = iter;
+ retval = 0;
+ break;
}
spin_unlock(&dum->lock);
--
2.25.1
If the list does not contain the expected element, the value of
list_for_each_entry() iterator will not point to a valid structure.
To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.
In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found element [1].
Determining if an element was found is then simply checking if
the pointer is != NULL instead of using the potentially bogus pointer.
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/max3420_udc.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/udc/max3420_udc.c b/drivers/usb/gadget/udc/max3420_udc.c
index d2a2b20cc1ad..ac8a46bc1057 100644
--- a/drivers/usb/gadget/udc/max3420_udc.c
+++ b/drivers/usb/gadget/udc/max3420_udc.c
@@ -1044,22 +1044,26 @@ static int max3420_ep_queue(struct usb_ep *_ep, struct usb_request *_req,
static int max3420_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
- struct max3420_req *t, *req = to_max3420_req(_req);
+ struct max3420_req *t = NULL;
+ struct max3420_req *req = to_max3420_req(_req);
+ struct max3420_req *iter;
struct max3420_ep *ep = to_max3420_ep(_ep);
unsigned long flags;
spin_lock_irqsave(&ep->lock, flags);
/* Pluck the descriptor from queue */
- list_for_each_entry(t, &ep->queue, queue)
- if (t == req) {
- list_del_init(&req->queue);
- break;
- }
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (iter != req)
+ continue;
+ list_del_init(&req->queue);
+ t = iter;
+ break;
+ }
spin_unlock_irqrestore(&ep->lock, flags);
- if (t == req)
+ if (t)
max3420_req_done(req, -ECONNRESET);
return 0;
--
2.25.1
On Tue, Mar 8, 2022 at 9:19 AM Jakob Koschel <[email protected]> wrote:
>
> This patch set removes any use of the list iterator variable past
> the list body [..]
This looks good to me. It also looks like now all of those cna
trivially be written as that "list_traverse()" thing, but that can be
done separately.
Linus
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/gr_udc.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/gr_udc.c b/drivers/usb/gadget/udc/gr_udc.c
index 4b35739d3695..22096f8505de 100644
--- a/drivers/usb/gadget/udc/gr_udc.c
+++ b/drivers/usb/gadget/udc/gr_udc.c
@@ -1690,7 +1690,7 @@ static int gr_queue_ext(struct usb_ep *_ep, struct usb_request *_req,
/* Dequeue JUST ONE request */
static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
- struct gr_request *req;
+ struct gr_request *req = NULL, *iter;
struct gr_ep *ep;
struct gr_udc *dev;
int ret = 0;
@@ -1710,11 +1710,13 @@ static int gr_dequeue(struct usb_ep *_ep, struct usb_request *_req)
spin_lock_irqsave(&dev->lock, flags);
/* Make sure it's actually queued on this endpoint */
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->req == _req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ req = iter;
+ break;
}
- if (&req->req != _req) {
+ if (!req) {
ret = -EINVAL;
goto out;
}
--
2.25.1
If the list does not contain the expected element, the value of
list_for_each_entry() iterator will not point to a valid structure.
To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.
In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found element [1].
Determining if an element was found is then simply checking if
the pointer is != NULL instead of using the potentially bogus pointer.
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/tegra-xudc.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/udc/tegra-xudc.c
index 43f1b0d461c1..1ad616d81c96 100644
--- a/drivers/usb/gadget/udc/tegra-xudc.c
+++ b/drivers/usb/gadget/udc/tegra-xudc.c
@@ -1413,18 +1413,20 @@ __tegra_xudc_ep_dequeue(struct tegra_xudc_ep *ep,
struct tegra_xudc_request *req)
{
struct tegra_xudc *xudc = ep->xudc;
- struct tegra_xudc_request *r;
+ struct tegra_xudc_request *r = NULL, *iter;
struct tegra_xudc_trb *deq_trb;
bool busy, kick_queue = false;
int ret = 0;
/* Make sure the request is actually queued to this endpoint. */
- list_for_each_entry(r, &ep->queue, list) {
- if (r == req)
- break;
+ list_for_each_entry(iter, &ep->queue, list) {
+ if (iter != req)
+ continue;
+ r = iter;
+ break;
}
- if (r != req)
+ if (!r)
return -EINVAL;
/* Request hasn't been queued in the transfer ring yet. */
--
2.25.1
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/lpc32xx_udc.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c b/drivers/usb/gadget/udc/lpc32xx_udc.c
index a25d01c89564..6117ae8e7242 100644
--- a/drivers/usb/gadget/udc/lpc32xx_udc.c
+++ b/drivers/usb/gadget/udc/lpc32xx_udc.c
@@ -1830,7 +1830,7 @@ static int lpc32xx_ep_queue(struct usb_ep *_ep,
static int lpc32xx_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct lpc32xx_ep *ep;
- struct lpc32xx_request *req;
+ struct lpc32xx_request *req = NULL, *iter;
unsigned long flags;
ep = container_of(_ep, struct lpc32xx_ep, ep);
@@ -1840,11 +1840,13 @@ static int lpc32xx_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
spin_lock_irqsave(&ep->udc->lock, flags);
/* make sure it's actually queued on this endpoint */
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->req == _req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ req = iter;
+ break;
}
- if (&req->req != _req) {
+ if (!req) {
spin_unlock_irqrestore(&ep->udc->lock, flags);
return -EINVAL;
}
--
2.25.1
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/aspeed-vhub/epn.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
index 917892ca8753..b5252880b389 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
@@ -466,19 +466,21 @@ static int ast_vhub_epn_dequeue(struct usb_ep* u_ep, struct usb_request *u_req)
{
struct ast_vhub_ep *ep = to_ast_ep(u_ep);
struct ast_vhub *vhub = ep->vhub;
- struct ast_vhub_req *req;
+ struct ast_vhub_req *req = NULL, *iter;
unsigned long flags;
int rc = -EINVAL;
spin_lock_irqsave(&vhub->lock, flags);
/* Make sure it's actually queued on this endpoint */
- list_for_each_entry (req, &ep->queue, queue) {
- if (&req->req == u_req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != u_req)
+ continue;
+ req = iter;
+ break;
}
- if (&req->req == u_req) {
+ if (req) {
EPVDBG(ep, "dequeue req @%p active=%d\n",
req, req->active);
if (req->active)
--
2.25.1
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/omap_udc.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/omap_udc.c b/drivers/usb/gadget/udc/omap_udc.c
index 494da00398d7..2d9815dad2ff 100644
--- a/drivers/usb/gadget/udc/omap_udc.c
+++ b/drivers/usb/gadget/udc/omap_udc.c
@@ -1003,7 +1003,7 @@ omap_ep_queue(struct usb_ep *_ep, struct usb_request *_req, gfp_t gfp_flags)
static int omap_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct omap_ep *ep = container_of(_ep, struct omap_ep, ep);
- struct omap_req *req;
+ struct omap_req *req = NULL, *iter;
unsigned long flags;
if (!_ep || !_req)
@@ -1012,11 +1012,13 @@ static int omap_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
spin_lock_irqsave(&ep->udc->lock, flags);
/* make sure it's actually queued on this endpoint */
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->req == _req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ req = iter;
+ break;
}
- if (&req->req != _req) {
+ if (!req) {
spin_unlock_irqrestore(&ep->udc->lock, flags);
return -EINVAL;
}
--
2.25.1
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/atmel_usba_udc.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 2b893bceea45..ae2bfbac603e 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -860,7 +860,8 @@ static int usba_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct usba_ep *ep = to_usba_ep(_ep);
struct usba_udc *udc = ep->udc;
- struct usba_request *req;
+ struct usba_request *req = NULL;
+ struct usba_request *iter;
unsigned long flags;
u32 status;
@@ -869,12 +870,14 @@ static int usba_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
spin_lock_irqsave(&udc->lock, flags);
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->req == _req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ req = iter;
+ break;
}
- if (&req->req != _req) {
+ if (!req) {
spin_unlock_irqrestore(&udc->lock, flags);
return -EINVAL;
}
--
2.25.1
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/composite.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 4f7e789c3e07..2eaeaae96759 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -863,24 +863,25 @@ static int set_config(struct usb_composite_dev *cdev,
const struct usb_ctrlrequest *ctrl, unsigned number)
{
struct usb_gadget *gadget = cdev->gadget;
- struct usb_configuration *c = NULL;
+ struct usb_configuration *c = NULL, *iter;
int result = -EINVAL;
unsigned power = gadget_is_otg(gadget) ? 8 : 100;
int tmp;
if (number) {
- list_for_each_entry(c, &cdev->configs, list) {
- if (c->bConfigurationValue == number) {
- /*
- * We disable the FDs of the previous
- * configuration only if the new configuration
- * is a valid one
- */
- if (cdev->config)
- reset_config(cdev);
- result = 0;
- break;
- }
+ list_for_each_entry(iter, &cdev->configs, list) {
+ if (iter->bConfigurationValue != number)
+ continue;
+ /*
+ * We disable the FDs of the previous
+ * configuration only if the new configuration
+ * is a valid one
+ */
+ if (cdev->config)
+ reset_config(cdev);
+ c = iter;
+ result = 0;
+ break;
}
if (result < 0)
goto done;
--
2.25.1
If the list does not contain the expected element, the value of
list_for_each_entry() iterator will not point to a valid structure.
To avoid type confusion in such case, the list iterator
scope will be limited to list_for_each_entry() loop.
In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found element [1].
Determining if an element was found is then simply checking if
the pointer is != NULL instead of using the potentially bogus pointer.
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/legacy/hid.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/gadget/legacy/hid.c b/drivers/usb/gadget/legacy/hid.c
index 3912cc805f3a..1187ee4f316a 100644
--- a/drivers/usb/gadget/legacy/hid.c
+++ b/drivers/usb/gadget/legacy/hid.c
@@ -134,7 +134,7 @@ static int hid_bind(struct usb_composite_dev *cdev)
{
struct usb_gadget *gadget = cdev->gadget;
struct list_head *tmp;
- struct hidg_func_node *n, *m;
+ struct hidg_func_node *n = NULL, *m, *iter_n;
struct f_hid_opts *hid_opts;
int status, funcs = 0;
@@ -144,18 +144,19 @@ static int hid_bind(struct usb_composite_dev *cdev)
if (!funcs)
return -ENODEV;
- list_for_each_entry(n, &hidg_func_list, node) {
- n->fi = usb_get_function_instance("hid");
- if (IS_ERR(n->fi)) {
- status = PTR_ERR(n->fi);
+ list_for_each_entry(iter_n, &hidg_func_list, node) {
+ iter_n->fi = usb_get_function_instance("hid");
+ if (IS_ERR(iter_n->fi)) {
+ status = PTR_ERR(iter_n->fi);
+ n = iter_n;
goto put;
}
- hid_opts = container_of(n->fi, struct f_hid_opts, func_inst);
- hid_opts->subclass = n->func->subclass;
- hid_opts->protocol = n->func->protocol;
- hid_opts->report_length = n->func->report_length;
- hid_opts->report_desc_length = n->func->report_desc_length;
- hid_opts->report_desc = n->func->report_desc;
+ hid_opts = container_of(iter_n->fi, struct f_hid_opts, func_inst);
+ hid_opts->subclass = iter_n->func->subclass;
+ hid_opts->protocol = iter_n->func->protocol;
+ hid_opts->report_length = iter_n->func->report_length;
+ hid_opts->report_desc_length = iter_n->func->report_desc_length;
+ hid_opts->report_desc = iter_n->func->report_desc;
}
--
2.25.1
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/pxa25x_udc.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/pxa25x_udc.c b/drivers/usb/gadget/udc/pxa25x_udc.c
index b38747fd3bb0..6c414c99d01c 100644
--- a/drivers/usb/gadget/udc/pxa25x_udc.c
+++ b/drivers/usb/gadget/udc/pxa25x_udc.c
@@ -966,7 +966,8 @@ static void nuke(struct pxa25x_ep *ep, int status)
static int pxa25x_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct pxa25x_ep *ep;
- struct pxa25x_request *req;
+ struct pxa25x_request *req = NULL;
+ struct pxa25x_request *iter;
unsigned long flags;
ep = container_of(_ep, struct pxa25x_ep, ep);
@@ -976,11 +977,13 @@ static int pxa25x_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
local_irq_save(flags);
/* make sure it's actually queued on this endpoint */
- list_for_each_entry (req, &ep->queue, queue) {
- if (&req->req == _req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ req = iter;
+ break;
}
- if (&req->req != _req) {
+ if (!req) {
local_irq_restore(flags);
return -EINVAL;
}
--
2.25.1
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/pxa27x_udc.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c b/drivers/usb/gadget/udc/pxa27x_udc.c
index f4b7a2a3e711..ac980d6a4740 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -1159,7 +1159,7 @@ static int pxa_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct pxa_ep *ep;
struct udc_usb_ep *udc_usb_ep;
- struct pxa27x_request *req;
+ struct pxa27x_request *req = NULL, *iter;
unsigned long flags;
int rc = -EINVAL;
@@ -1173,11 +1173,12 @@ static int pxa_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
spin_lock_irqsave(&ep->lock, flags);
/* make sure it's actually queued on this endpoint */
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->req == _req) {
- rc = 0;
- break;
- }
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ req = iter;
+ rc = 0;
+ break;
}
spin_unlock_irqrestore(&ep->lock, flags);
--
2.25.1
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/net2280.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index 16e7d2db6411..051d024b369e 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -1240,7 +1240,8 @@ static void nuke(struct net2280_ep *ep)
static int net2280_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct net2280_ep *ep;
- struct net2280_request *req;
+ struct net2280_request *req = NULL;
+ struct net2280_request *iter;
unsigned long flags;
u32 dmactl;
int stopped;
@@ -1266,11 +1267,13 @@ static int net2280_dequeue(struct usb_ep *_ep, struct usb_request *_req)
}
/* make sure it's still queued on this endpoint */
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->req == _req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ req = iter;
+ break;
}
- if (&req->req != _req) {
+ if (!req) {
ep->stopped = stopped;
spin_unlock_irqrestore(&ep->dev->lock, flags);
ep_dbg(ep->dev, "%s: Request mismatch\n", __func__);
--
2.25.1
If the list representing the request queue does not contain the expected
request, the value of the list_for_each_entry() iterator will not point
to a valid structure. To avoid type confusion in such case, the list
iterator scope will be limited to the list_for_each_entry() loop.
In preparation to limiting scope of the list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object [1].
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Jakob Koschel <[email protected]>
---
drivers/usb/gadget/udc/mv_udc_core.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/udc/mv_udc_core.c b/drivers/usb/gadget/udc/mv_udc_core.c
index b6d34dda028b..fdb17d86cd65 100644
--- a/drivers/usb/gadget/udc/mv_udc_core.c
+++ b/drivers/usb/gadget/udc/mv_udc_core.c
@@ -771,7 +771,7 @@ static void mv_prime_ep(struct mv_ep *ep, struct mv_req *req)
static int mv_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
{
struct mv_ep *ep = container_of(_ep, struct mv_ep, ep);
- struct mv_req *req;
+ struct mv_req *req = NULL, *iter;
struct mv_udc *udc = ep->udc;
unsigned long flags;
int stopped, ret = 0;
@@ -793,11 +793,13 @@ static int mv_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
writel(epctrlx, &udc->op_regs->epctrlx[ep->ep_num]);
/* make sure it's actually queued on this endpoint */
- list_for_each_entry(req, &ep->queue, queue) {
- if (&req->req == _req)
- break;
+ list_for_each_entry(iter, &ep->queue, queue) {
+ if (&iter->req != _req)
+ continue;
+ req = iter;
+ break;
}
- if (&req->req != _req) {
+ if (!req) {
ret = -EINVAL;
goto out;
}
--
2.25.1
On 09/03/2022 18:36, Jakob Koschel wrote:
>
>> On 9. Mar 2022, at 18:25, Krzysztof Kozlowski <[email protected]> wrote:
>>
>> On 08/03/2022 18:18, Jakob Koschel wrote:
>>> If the list representing the request queue does not contain the expected
>>> request, the value of the list_for_each_entry() iterator will not point
>>> to a valid structure. To avoid type confusion in such case, the list
>>> iterator scope will be limited to the list_for_each_entry() loop.
>>>
>>> In preparation to limiting scope of the list iterator to the list traversal
>>> loop, use a dedicated pointer to point to the found request object [1].
>>>
>>> Link: https://lore.kernel.org/all/[email protected]/
>>> Signed-off-by: Jakob Koschel <[email protected]>
>>> ---
>>> drivers/usb/gadget/udc/s3c-hsudc.c | 12 +++++++-----
>>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/udc/s3c-hsudc.c b/drivers/usb/gadget/udc/s3c-hsudc.c
>>> index 89f1f8c9f02e..bf803e013458 100644
>>> --- a/drivers/usb/gadget/udc/s3c-hsudc.c
>>> +++ b/drivers/usb/gadget/udc/s3c-hsudc.c
>>> @@ -877,7 +877,7 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>>> {
>>> struct s3c_hsudc_ep *hsep = our_ep(_ep);
>>> struct s3c_hsudc *hsudc = hsep->dev;
>>> - struct s3c_hsudc_req *hsreq;
>>> + struct s3c_hsudc_req *hsreq = NULL, *iter;
>>> unsigned long flags;
>>>
>>> hsep = our_ep(_ep);
>>> @@ -886,11 +886,13 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>>>
>>> spin_lock_irqsave(&hsudc->lock, flags);
>>>
>>> - list_for_each_entry(hsreq, &hsep->queue, queue) {
>>> - if (&hsreq->req == _req)
>>> - break;
>>> + list_for_each_entry(iter, &hsep->queue, queue) {
>>> + if (&iter->req != _req)
>>> + continue;
>>> + hsreq = iter;
>>> + break;
>>
>> You have in the loop both continue and break, looks a bit complicated.
>> Why not simpler:
>>
>> if (&iter->req == _req) {
>> hsreq = iter;
>> break;
>> }
>
> Ultimately I changed this based on the feedback from Linus
> (Link: https://lore.kernel.org/linux-kernel/CAHk-=wheru6rEfzC2wuO9k03PRF6s3nhxryCAnwR5bzKwPV2ww@mail.gmail.com/).
Not cleaner to me at all, but it's all personal opinion. :)
>>
>> ?
>> Less code, typical (expected) code flow when looking for something in
>> the list. Is your approach related to some speculative execution?
>
> no relation to speculative execution.
>
> I have no preference for one over the other, so I'll just change it to
> however it is desired.
>
> It would just be great to have a (somewhat) consistent way so I can prepare
> the rest of the patch sets accordingly.
>
Yeah, I understand. The code itself looks good, so:
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 08/03/2022 18:18, Jakob Koschel wrote:
> If the list representing the request queue does not contain the expected
> request, the value of the list_for_each_entry() iterator will not point
> to a valid structure. To avoid type confusion in such case, the list
> iterator scope will be limited to the list_for_each_entry() loop.
>
> In preparation to limiting scope of the list iterator to the list traversal
> loop, use a dedicated pointer to point to the found request object [1].
>
> Link: https://lore.kernel.org/all/[email protected]/
> Signed-off-by: Jakob Koschel <[email protected]>
> ---
> drivers/usb/gadget/udc/s3c-hsudc.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/s3c-hsudc.c b/drivers/usb/gadget/udc/s3c-hsudc.c
> index 89f1f8c9f02e..bf803e013458 100644
> --- a/drivers/usb/gadget/udc/s3c-hsudc.c
> +++ b/drivers/usb/gadget/udc/s3c-hsudc.c
> @@ -877,7 +877,7 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req)
> {
> struct s3c_hsudc_ep *hsep = our_ep(_ep);
> struct s3c_hsudc *hsudc = hsep->dev;
> - struct s3c_hsudc_req *hsreq;
> + struct s3c_hsudc_req *hsreq = NULL, *iter;
> unsigned long flags;
>
> hsep = our_ep(_ep);
> @@ -886,11 +886,13 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>
> spin_lock_irqsave(&hsudc->lock, flags);
>
> - list_for_each_entry(hsreq, &hsep->queue, queue) {
> - if (&hsreq->req == _req)
> - break;
> + list_for_each_entry(iter, &hsep->queue, queue) {
> + if (&iter->req != _req)
> + continue;
> + hsreq = iter;
> + break;
You have in the loop both continue and break, looks a bit complicated.
Why not simpler:
if (&iter->req == _req) {
hsreq = iter;
break;
}
?
Less code, typical (expected) code flow when looking for something in
the list. Is your approach related to some speculative execution?
> }
> - if (&hsreq->req != _req) {
> + if (!hsreq) {
> spin_unlock_irqrestore(&hsudc->lock, flags);
> return -EINVAL;
> }
Best regards,
Krzysztof
> On 9. Mar 2022, at 18:25, Krzysztof Kozlowski <[email protected]> wrote:
>
> On 08/03/2022 18:18, Jakob Koschel wrote:
>> If the list representing the request queue does not contain the expected
>> request, the value of the list_for_each_entry() iterator will not point
>> to a valid structure. To avoid type confusion in such case, the list
>> iterator scope will be limited to the list_for_each_entry() loop.
>>
>> In preparation to limiting scope of the list iterator to the list traversal
>> loop, use a dedicated pointer to point to the found request object [1].
>>
>> Link: https://lore.kernel.org/all/[email protected]/
>> Signed-off-by: Jakob Koschel <[email protected]>
>> ---
>> drivers/usb/gadget/udc/s3c-hsudc.c | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/udc/s3c-hsudc.c b/drivers/usb/gadget/udc/s3c-hsudc.c
>> index 89f1f8c9f02e..bf803e013458 100644
>> --- a/drivers/usb/gadget/udc/s3c-hsudc.c
>> +++ b/drivers/usb/gadget/udc/s3c-hsudc.c
>> @@ -877,7 +877,7 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>> {
>> struct s3c_hsudc_ep *hsep = our_ep(_ep);
>> struct s3c_hsudc *hsudc = hsep->dev;
>> - struct s3c_hsudc_req *hsreq;
>> + struct s3c_hsudc_req *hsreq = NULL, *iter;
>> unsigned long flags;
>>
>> hsep = our_ep(_ep);
>> @@ -886,11 +886,13 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>>
>> spin_lock_irqsave(&hsudc->lock, flags);
>>
>> - list_for_each_entry(hsreq, &hsep->queue, queue) {
>> - if (&hsreq->req == _req)
>> - break;
>> + list_for_each_entry(iter, &hsep->queue, queue) {
>> + if (&iter->req != _req)
>> + continue;
>> + hsreq = iter;
>> + break;
>
> You have in the loop both continue and break, looks a bit complicated.
> Why not simpler:
>
> if (&iter->req == _req) {
> hsreq = iter;
> break;
> }
Ultimately I changed this based on the feedback from Linus
(Link: https://lore.kernel.org/linux-kernel/CAHk-=wheru6rEfzC2wuO9k03PRF6s3nhxryCAnwR5bzKwPV2ww@mail.gmail.com/).
>
> ?
> Less code, typical (expected) code flow when looking for something in
> the list. Is your approach related to some speculative execution?
no relation to speculative execution.
I have no preference for one over the other, so I'll just change it to
however it is desired.
It would just be great to have a (somewhat) consistent way so I can prepare
the rest of the patch sets accordingly.
>
>> }
>> - if (&hsreq->req != _req) {
>> + if (!hsreq) {
>> spin_unlock_irqrestore(&hsudc->lock, flags);
>> return -EINVAL;
>> }
>
>
> Best regards,
> Krzysztof
Jakob