[U-Boot] [PATCH 0/2] Fix tx/rx_req operations

From: Vitaly Kuzmichev vkuzmichev@mvista.com
Fix bugs in tx/rx_req operations.
Vitaly Kuzmichev (2): USB-CDC: Fix tx/rx_req memory leaks USB-CDC: Prevent rx_req being enqueued twice
drivers/usb/gadget/ether.c | 21 ++++++++------------- 1 files changed, 8 insertions(+), 13 deletions(-)

From: Vitaly Kuzmichev vkuzmichev@mvista.com
Remove and fix needless and destructive operations with tx/rx_req.
1) 'req' in rx_complete is always not NULL and always equals to rx_req 2) Free allocated tx_req if rx_req allocation has failed 3) Do not zero out tx/rx_req in usb_eth_init, leave this for eth_reset_config which will be called at the next use of usb0 4) Additional check in usb_eth_recv is not required
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/ether.c | 20 +++++++------------- 1 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index bc6480c..8f5eec1 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -1276,9 +1276,6 @@ static void rx_complete(struct usb_ep *ep, struct usb_request *req) debug("%s: status %d\n", __func__, req->status);
packet_received = 1; - - if (req) - dev->rx_req = req; }
static int alloc_requests(struct eth_dev *dev, unsigned n, gfp_t gfp_flags) @@ -1287,16 +1284,18 @@ static int alloc_requests(struct eth_dev *dev, unsigned n, gfp_t gfp_flags) dev->tx_req = usb_ep_alloc_request(dev->in_ep, 0);
if (!dev->tx_req) - goto fail; + goto fail1;
dev->rx_req = usb_ep_alloc_request(dev->out_ep, 0);
if (!dev->rx_req) - goto fail; + goto fail2;
return 0;
-fail: +fail2: + usb_ep_free_request(dev->in_ep, dev->tx_req); +fail1: error("can't alloc requests"); return -1; } @@ -1791,8 +1790,6 @@ static int usb_eth_init(struct eth_device *netdev, bd_t *bd) }
dev->network_started = 0; - dev->tx_req = NULL; - dev->rx_req = NULL;
packet_received = 0; packet_sent = 0; @@ -1823,15 +1820,13 @@ static int usb_eth_send(struct eth_device *netdev, volatile void *packet, int length) { int retval; - struct usb_request *req = NULL; struct eth_dev *dev = &l_ethdev; + struct usb_request *req = dev->tx_req; unsigned long ts; unsigned long timeout = USB_CONNECT_TIMEOUT;
debug("%s:...\n", __func__);
- req = dev->tx_req; - req->buf = (void *)packet; req->context = NULL; req->complete = tx_complete; @@ -1883,8 +1878,7 @@ static int usb_eth_recv(struct eth_device *netdev) NetReceive(NetRxPackets[0], dev->rx_req->length); packet_received = 0;
- if (dev->rx_req) - rx_submit(dev, dev->rx_req, 0); + rx_submit(dev, dev->rx_req, 0); } else error("dev->rx_req invalid"); }

Hi,
2010/9/22 vkuzmichev@mvista.com:
From: Vitaly Kuzmichev vkuzmichev@mvista.com
Remove and fix needless and destructive operations with tx/rx_req.
- 'req' in rx_complete is always not NULL and always equals to rx_req
- Free allocated tx_req if rx_req allocation has failed
- Do not zero out tx/rx_req in usb_eth_init, leave this for
eth_reset_config which will be called at the next use of usb0 4) Additional check in usb_eth_recv is not required
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
Applied to u-boot-usb. Thanks.
Remy

From: Vitaly Kuzmichev vkuzmichev@mvista.com
After gadget reinitializaton (after tftp has been done once) packet_received may become equal to 1 due to nuking OUT_EP while disabling it in eth_reset_config.
rx_submit called from usb_eth_init queues rx_req first time. But the first call of usb_eth_recv from NetLoop queues rx_req again due to packet_received = 1.
The following flow shows the path of functions calls when this happens:
net/net.c:NetLoop | +-net/eth.c:eth_init | ether.c:usb_eth_init | | | +-udc_driver:usb_gadget_handle_interrupts | | udc_driver:... | | ether.c:eth_setup | | ether.c:eth_set_config | | ether.c:eth_reset_config | | udc_driver:usb_ep_disable | | udc_driver:nuke | | ether.c:rx_complete | | ether.c: packet_received = 1; | | | +-ether.c:rx_submit | udc_driver:usb_ep_queue --- The first time when rx_req is queued | +-net/eth.c:eth_rx ether.c:usb_eth_recv | +-udc_driver:usb_gadget_handle_interrupts | udc_driver:... --- no interrupts, returning +-ether.c: if (packet_received) { ... ether.c:rx_submit udc_driver:usb_ep_queue --- The second time!
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/ether.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index 8f5eec1..b22ca90 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -1810,6 +1810,7 @@ static int usb_eth_init(struct eth_device *netdev, bd_t *bd) usb_gadget_handle_interrupts(); }
+ packet_received = 0; rx_submit(dev, dev->rx_req, 0); return 0; fail:

Hi,
2010/9/22 vkuzmichev@mvista.com:
From: Vitaly Kuzmichev vkuzmichev@mvista.com
After gadget reinitializaton (after tftp has been done once) packet_received may become equal to 1 due to nuking OUT_EP while disabling it in eth_reset_config.
rx_submit called from usb_eth_init queues rx_req first time. But the first call of usb_eth_recv from NetLoop queues rx_req again due to packet_received = 1.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
drivers/usb/gadget/ether.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
Applied to u-boot-usb. Thanks.
Remy
participants (2)
-
Remy Bohmer
-
vkuzmichev@mvista.com