[U-Boot] [PATCH 0/8] USB: gadget: fix porting bugs
1) Restuct Makefile to unable usage old and new stacks simultaneously 2) Add lost 'qmult' definition for High Speed devices 3) Take debug printout macros back and make them more useful 4) Fix in_ep, out_ep, ep0 and status_ep confusions 5) Replace 'strcpy' by 'strlcpy' 6) Fix possible oops on stat_req->buf initialization 7) 2 updates backported from linux-2.6.git
David Brownell (1): USB: gadget: ethernet error path potential oops fix
Julia Lawall (1): USB: gadget: change simple_strtol to simple_strtoul
Vitaly Kuzmichev (6): USB-CDC: Restuct USB gadget Makefile USB-CDC: Add lost 'qmult' definition USB-CDC: Linux-like debug printout USB-CDC: Correct freeing usb requests USB-CDC: Replace 'strcpy' by 'strlcpy' USB-CDC: Correct stat_req initialization
drivers/usb/gadget/Makefile | 9 +++- drivers/usb/gadget/epautoconf.c | 2 +- drivers/usb/gadget/ether.c | 95 ++++++++++++++++++++++----------------- 3 files changed, 61 insertions(+), 45 deletions(-)
Prohibit simultaneous usage of both old and new gadget stacks and allow UDC drivers to be dependent on CONFIG_USB_ETHER.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/Makefile | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile index 27e7f40..05aa213 100644 --- a/drivers/usb/gadget/Makefile +++ b/drivers/usb/gadget/Makefile @@ -25,6 +25,11 @@ include $(TOPDIR)/config.mk
LIB := $(obj)libusb_gadget.a
+# new USB gadget layer dependencies +ifdef CONFIG_USB_ETHER +COBJS-y += ether.o epautoconf.o config.o usbstring.o +COBJS-$(CONFIG_USB_GADGET_AT91) += at91_udc.o +else # Devices not related to the new gadget layer depend on CONFIG_USB_DEVICE ifdef CONFIG_USB_DEVICE COBJS-y += core.o @@ -35,9 +40,7 @@ COBJS-$(CONFIG_MPC885_FAMILY) += mpc8xx_udc.o COBJS-$(CONFIG_PXA27X) += pxa27x_udc.o COBJS-$(CONFIG_SPEARUDC) += spr_udc.o endif -# new USB gadget layer dependencies -COBJS-$(CONFIG_USB_ETHER) += ether.o epautoconf.o config.o usbstring.o -COBJS-$(CONFIG_USB_GADGET_AT91) += at91_udc.o +endif
COBJS := $(COBJS-y) SRCS := $(COBJS:.o=.c)
Hi,
2010/8/12 Vitaly Kuzmichev vkuzmichev@mvista.com:
Prohibit simultaneous usage of both old and new gadget stacks and allow UDC drivers to be dependent on CONFIG_USB_ETHER.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
drivers/usb/gadget/Makefile | 9 ++++++--- 1 files changed, 6 insertions(+), 3 deletions(-)
Applied to u-boot-usb/cdc branch. Thanks.
Remy
Add lost 'qmult' definition for High Speed devices and make it configurable through CONFIG_USB_ETH_QMULT.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/ether.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index 3d871fa..a07738f 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -140,6 +140,12 @@ static inline int is_cdc(struct eth_dev *dev) #ifdef CONFIG_USB_GADGET_DUALSPEED #define DEVSPEED USB_SPEED_HIGH
+#ifdef CONFIG_USB_ETH_QMULT +#define qmult CONFIG_USB_ETH_QMULT +#else +#define qmult 5 +#endif + /* for dual-speed hardware, use deeper queues at highspeed */ #define qlen(gadget) \ (DEFAULT_QLEN*((gadget->speed == USB_SPEED_HIGH) ? qmult : 1))
Hi,
2010/8/12 Vitaly Kuzmichev vkuzmichev@mvista.com:
Add lost 'qmult' definition for High Speed devices and make it configurable through CONFIG_USB_ETH_QMULT.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
drivers/usb/gadget/ether.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
Applied to u-boot-usb/cdc branch. Thanks.
Remy
Take debug printout macros back from linux-2.6.27 and make them more useful and more compatible.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/ether.c | 65 +++++++++++++++++++++++--------------------- 1 files changed, 34 insertions(+), 31 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index a07738f..b6f5f4d 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -37,8 +37,10 @@ #define dev_err(x, stuff...) printf(stuff) #define dev_dbg dev_err #define dev_warn dev_err -#define DEBUG dev_err -#define VDEBUG DEBUG +#define WARN INFO +#define ERROR INFO +#define DEBUG INFO +#define VDEBUG dprintf #define atomic_read extern struct platform_data brd; #define spin_lock(x) @@ -769,7 +771,7 @@ set_ether_config (struct eth_dev *dev, gfp_t gfp_flags)
result = usb_ep_enable (dev->status_ep, dev->status); if (result != 0) { - printf ("enable %s --> %d\n", + DEBUG (dev, "enable %s --> %d\n", dev->status_ep->name, result); goto done; } @@ -789,14 +791,14 @@ set_ether_config (struct eth_dev *dev, gfp_t gfp_flags) if (!cdc_active(dev)) { result = usb_ep_enable (dev->in_ep, dev->in); if (result != 0) { - printf ("enable %s --> %d\n", + DEBUG(dev, "enable %s --> %d\n", dev->in_ep->name, result); goto done; }
result = usb_ep_enable (dev->out_ep, dev->out); if (result != 0) { - printf ("enable %s --> %d\n", + DEBUG (dev, "enable %s --> %d\n", dev->out_ep->name, result); goto done; } @@ -827,6 +829,8 @@ static void eth_reset_config (struct eth_dev *dev) if (dev->config == 0) return;
+ DEBUG (dev, "%s\n", __func__); + /* disable endpoints, forcing (synchronous) completion of * pending i/o. then free the requests. */ @@ -941,17 +945,17 @@ static void eth_status_complete (struct usb_ep *ep, struct usb_request *req)
req->length = STATUS_BYTECOUNT; value = usb_ep_queue (ep, req, GFP_ATOMIC); - dprintf ("send SPEED_CHANGE --> %d\n", value); + DEBUG (dev, "send SPEED_CHANGE --> %d\n", value); if (value == 0) return; } else if (value != -ECONNRESET) { - dprintf("event %02x --> %d\n", + DEBUG (dev, "event %02x --> %d\n", event->bNotificationType, value); if (event->bNotificationType== USB_CDC_NOTIFY_SPEED_CHANGE) { l_ethdev.network_started=1; - printf("USB network up!\n"); + INFO(&l_ethdev, "USB network up!\n"); } } req->context = NULL; @@ -991,7 +995,7 @@ static void issue_start_status (struct eth_dev *dev)
value = usb_ep_queue (dev->status_ep, req, GFP_ATOMIC); if (value < 0) - printf ("status buf queue --> %d\n", value); + DEBUG (dev, "status buf queue --> %d\n", value); }
#endif @@ -1001,7 +1005,7 @@ static void issue_start_status (struct eth_dev *dev) static void eth_setup_complete (struct usb_ep *ep, struct usb_request *req) { if (req->status || req->actual != req->length) - dprintf (/*(struct eth_dev *) ep->driver_data*/ + DEBUG ((struct eth_dev *) ep->driver_data, "setup complete --> %d, %d/%d\n", req->status, req->actual, req->length); } @@ -1029,7 +1033,7 @@ eth_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) * while config change events may enable network traffic. */
- dprintf("eth_setup:...\n"); + VDEBUG(dev, "%s\n", __func__);
req->complete = eth_setup_complete; switch (ctrl->bRequest) { @@ -1179,7 +1183,7 @@ done_set_intf: || wLength != 0 || wIndex > 1) break; - printf ("packet filter %02x\n", wValue); + DEBUG (dev, "packet filter %02x\n", wValue); dev->cdc_filter = wValue; value = 0; break; @@ -1194,7 +1198,7 @@ done_set_intf: #endif /* DEV_CONFIG_CDC */
default: - printf ( + VDEBUG (dev, "unknown control req%02x.%02x v%04x i%04x l%d\n", ctrl->bRequestType, ctrl->bRequest, wValue, wIndex, wLength); @@ -1202,7 +1206,7 @@ done_set_intf:
/* respond with data transfer before status phase? */ if (value >= 0) { - dprintf("respond with data transfer before status phase\n"); + DEBUG(dev, "respond with data transfer before status phase\n"); req->length = value; req->zero = value < wLength && (value % gadget->ep0->maxpacket) == 0; @@ -1237,7 +1241,7 @@ static int rx_submit ( struct eth_dev *dev, struct usb_request *req, \ * byte off the end (to force hardware errors on overflow). */
- dprintf("%s\n", __func__); + VDEBUG(dev, "%s\n", __func__);
size = (ETHER_HDR_SIZE + dev->mtu + RX_EXTRA); size += dev->out_ep->maxpacket - 1; @@ -1255,7 +1259,7 @@ static int rx_submit ( struct eth_dev *dev, struct usb_request *req, \ retval = usb_ep_queue (dev->out_ep, req, gfp_flags);
if (retval) { - dprintf ("rx submit --> %d\n", retval); + DEBUG (dev, "rx submit --> %d\n", retval); } return retval; } @@ -1265,8 +1269,7 @@ static void rx_complete (struct usb_ep *ep, struct usb_request *req) { struct eth_dev *dev = ep->driver_data;
- dprintf("%s\n", __func__); - dprintf("rx status %d\n", req->status); + VDEBUG(dev, "%s: status %d\n", __func__, req->status);
packet_received=1;
@@ -1298,7 +1301,7 @@ fail:
static void tx_complete (struct usb_ep *ep, struct usb_request *req) { - dprintf("%s, status: %s\n", __func__,(req->status) ? "failed":"ok"); + VDEBUG(ep->driver_data, "%s: status %s\n", __func__, (req->status)?"failed":"ok"); packet_sent=1; }
@@ -1427,7 +1430,7 @@ static void eth_unbind (struct usb_gadget *gadget) { struct eth_dev *dev = get_gadget_data (gadget);
- printf("eth_unbind:...\n"); + DEBUG (dev, "%s...\n", __func__);
if (dev->stat_req) { usb_ep_free_request (dev->status_ep, dev->stat_req); @@ -1768,7 +1771,7 @@ static int usb_eth_init(struct eth_device* netdev, bd_t* bd) unsigned long timeout = USB_CONNECT_TIMEOUT;
if (!netdev) { - printf("ERROR: received NULL ptr\n"); + ERROR(dev, "ERROR: received NULL ptr\n"); goto fail; }
@@ -1790,7 +1793,7 @@ static int usb_eth_init(struct eth_device* netdev, bd_t* bd) { /* Handle control-c and timeouts */ if (ctrlc() || (get_timer(ts) > timeout)) { - printf("The remote end did not respond in time.\n"); + ERROR(dev, "The remote end did not respond in time.\n"); goto fail; } usb_gadget_handle_interrupts(); @@ -1806,9 +1809,9 @@ static int usb_eth_send(struct eth_device* netdev, volatile void* packet, int le { int retval; struct usb_request *req = NULL; + struct eth_dev *dev = &l_ethdev;
- struct eth_dev *dev = &l_ethdev; - dprintf("%s:...\n",__func__); + VDEBUG(dev, "%s:...\n", __func__);
req = dev->tx_req;
@@ -1836,7 +1839,7 @@ static int usb_eth_send(struct eth_device* netdev, volatile void* packet, int le retval = usb_ep_queue (dev->in_ep, req, GFP_ATOMIC);
if (!retval) - dprintf("%s: packet queued\n",__func__); + VDEBUG(dev, "%s: packet queued\n",__func__); while(!packet_sent) { packet_sent=0; @@ -1853,7 +1856,7 @@ static int usb_eth_recv(struct eth_device* netdev)
if (packet_received) { - dprintf("%s: packet received \n",__func__); + VDEBUG(dev, "%s: packet received \n",__func__); if (dev->rx_req) { NetReceive(NetRxPackets[0],dev->rx_req->length); @@ -1862,7 +1865,7 @@ static int usb_eth_recv(struct eth_device* netdev) if (dev->rx_req) rx_submit (dev, dev->rx_req, 0); } - else printf("dev->rx_req invalid\n"); + else WARN(dev, "dev->rx_req invalid\n"); } return 0; } @@ -1873,7 +1876,7 @@ void usb_eth_halt(struct eth_device* netdev)
if (!netdev) { - printf("ERROR: received NULL ptr\n"); + ERROR(dev, "ERROR: received NULL ptr\n"); return; }
@@ -1929,11 +1932,11 @@ int usb_eth_initialize(bd_t *bi) host_addr[sizeof(host_addr)-1] = '\0';
if (!is_eth_addr_valid(dev_addr)) { - printf("ERROR: Need valid 'usbnet_devaddr' to be set\n"); + ERROR(dev, "ERROR: Need valid 'usbnet_devaddr' to be set\n"); status = -1; } if (!is_eth_addr_valid(host_addr)) { - printf("ERROR: Need valid 'usbnet_hostaddr' to be set\n"); + ERROR(dev, "ERROR: Need valid 'usbnet_hostaddr' to be set\n"); status = -1; } if (status) @@ -1947,7 +1950,7 @@ int usb_eth_initialize(bd_t *bi) return 0;
fail: - printf("%s failed\n", __func__ ); + ERROR(dev, "%s failed. error = %d\n", __func__, status); return status; }
Hello.
Vitaly Kuzmichev wrote:
Take debug printout macros back from linux-2.6.27 and make them more useful and more compatible.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
drivers/usb/gadget/ether.c | 65 +++++++++++++++++++++++--------------------- 1 files changed, 34 insertions(+), 31 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index a07738f..b6f5f4d 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c
[...]
@@ -769,7 +771,7 @@ set_ether_config (struct eth_dev *dev, gfp_t gfp_flags)
result = usb_ep_enable (dev->status_ep, dev->status); if (result != 0) {
printf ("enable %s --> %d\n",
DEBUG (dev, "enable %s --> %d\n", dev->status_ep->name, result); goto done;@@ -789,14 +791,14 @@ set_ether_config (struct eth_dev *dev, gfp_t gfp_flags) if (!cdc_active(dev)) { result = usb_ep_enable (dev->in_ep, dev->in); if (result != 0) {
printf ("enable %s --> %d\n",
DEBUG(dev, "enable %s --> %d\n",
Well, I think the coding style shouldbe consistent -- you either leave the space before ( or remove it. And as U-Boot seems to follow the Linux coding style now, it seems better to remove the space.
@@ -941,17 +945,17 @@ static void eth_status_complete (struct usb_ep *ep, struct usb_request *req)
req->length = STATUS_BYTECOUNT; value = usb_ep_queue (ep, req, GFP_ATOMIC);
dprintf ("send SPEED_CHANGE --> %d\n", value);
DEBUG (dev, "send SPEED_CHANGE --> %d\n", value);
dprintf("event %02x --> %d\n",
DEBUG (dev, "event %02x --> %d\n",
Why add a space where there was none before?
@@ -1298,7 +1301,7 @@ fail:
static void tx_complete (struct usb_ep *ep, struct usb_request *req) {
- dprintf("%s, status: %s\n", __func__,(req->status) ? "failed":"ok");
- VDEBUG(ep->driver_data,
No cast here?
"%s: status %s\n", __func__, (req->status)?"failed":"ok"); packet_sent=1; }
WBR, Sergei
Hi Sergei,
Sergei Shtylyov wrote:
printf ("enable %s --> %d\n",
DEBUG(dev, "enable %s --> %d\n",Well, I think the coding style shouldbe consistent -- you either leave the space before ( or remove it. And as U-Boot seems to follow the Linux coding style now, it seems better to remove the space.
Well, I would say: "either leave the space before ( or remove it from EVERYWHERE". I think that the last thing should be done as a separate patch. So I would like to keep the coding style that is in this file.
Best regards, Vitaly.
Hello.
Vitaly Kuzmichev wrote:
printf ("enable %s --> %d\n",
DEBUG(dev, "enable %s --> %d\n",
Well, I think the coding style shouldbe consistent -- you either leave the space before ( or remove it. And as U-Boot seems to follow the Linux coding style now, it seems better to remove the space.
Well, I would say: "either leave the space before ( or remove it from EVERYWHERE". I think that the last thing should be done as a separate patch. So I would like to keep the coding style that is in this file.
Which you've failed to do so far. ;-)
Best regards, Vitaly.
WBR, Sergei
Hi,
2010/8/12 Vitaly Kuzmichev vkuzmichev@mvista.com:
Take debug printout macros back from linux-2.6.27 and make them more useful and more compatible.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
drivers/usb/gadget/ether.c | 65 +++++++++++++++++++++++--------------------- 1 files changed, 34 insertions(+), 31 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index a07738f..b6f5f4d 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -37,8 +37,10 @@ #define dev_err(x, stuff...) printf(stuff) #define dev_dbg dev_err #define dev_warn dev_err -#define DEBUG dev_err -#define VDEBUG DEBUG +#define WARN INFO +#define ERROR INFO +#define DEBUG INFO
This switches DEBUG logging on by default. This is not wanted. Can you please change that?
Kind regards,
Remy
Hi Remy,
No, it does not. DEBUG is defined as dev_err and dev_err is defined as printf. Anyway I can change it.
On 08/12/2010 10:33 PM, Remy Bohmer wrote:
@@ -37,8 +37,10 @@ #define dev_err(x, stuff...) printf(stuff) #define dev_dbg dev_err #define dev_warn dev_err -#define DEBUG dev_err -#define VDEBUG DEBUG +#define WARN INFO +#define ERROR INFO +#define DEBUG INFO
This switches DEBUG logging on by default. This is not wanted. Can you please change that?
Kind regards,
Remy
Hi,
2010/8/13 Vitaly Kuzmichev vkuzmichev@mvista.com:
Hi Remy,
No, it does not. DEBUG is defined as dev_err and dev_err is defined as printf. Anyway I can change it.
Please do not top-post!
On 08/12/2010 10:33 PM, Remy Bohmer wrote:
@@ -37,8 +37,10 @@ #define dev_err(x, stuff...) printf(stuff) #define dev_dbg dev_err #define dev_warn dev_err -#define DEBUG dev_err -#define VDEBUG DEBUG +#define WARN INFO +#define ERROR INFO +#define DEBUG INFO
This switches DEBUG logging on by default. This is not wanted. Can you please change that?
No, it does not.
Well, I see with this patch much more debug logging then without it... Hmm, it seems that you replaced all use of dprintf (which is trashed) by DEBUG()...
Anyway I can change it.
please do, DEBUG logging should not be on by default...
Kind regards,
Remy
Dear Remy Bohmer,
In message AANLkTi=gnFZr6wT-RJ6CLFEKuGGhhpJUDbj1VftAR9kX@mail.gmail.com you wrote:
+#define WARN INFO +#define ERROR INFO +#define DEBUG INFO
This switches DEBUG logging on by default. This is not wanted. Can you please change that?
No, it does not.
Well, I see with this patch much more debug logging then without it... Hmm, it seems that you replaced all use of dprintf (which is trashed) by DEBUG()...
Anyway I can change it.
please do, DEBUG logging should not be on by default...
DEBUG is already a well-defined name. Any different use of the same name will result in the patches being rejected.
Please fix this!
I also object against names like WARN, ERROR and INFO. They are just too dangerous.
Best regards,
Wolfgang Denk
Hi,
DEBUG is already a well-defined name. Any different use of the same name will result in the patches being rejected.
Please fix this!
I also object against names like WARN, ERROR and INFO. They are just too dangerous.
Agree.
Remy
Vitaly Kuzmichev wrote:
Take debug printout macros back from linux-2.6.27 and make them more useful and more compatible.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
drivers/usb/gadget/ether.c | 65 +++++++++++++++++++++++--------------------- 1 files changed, 34 insertions(+), 31 deletions(-)
Hi Vitaly,
a general comment. In u-boot is DEBUG generally defined to activate the output, as you see in a lot of drivers. It should not be changed as meanining in a single driver.
However, it seems more consistent to apply the comment sent by Reinhard:
http://lists.denx.de/pipermail/u-boot/2010-August/075346.html
debug() is already provided in u-boot, it makes sense not to add another slightly modification of a debug output.
Best regards, Stefano
Fix in_ep and out_ep confusion (rx_req was allocated from out_ep, not from in_ep) and add lost dev->req freeing.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/ether.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index b6f5f4d..dd3ab8c 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -845,7 +845,7 @@ static void eth_reset_config (struct eth_dev *dev) if (dev->out) { usb_ep_disable (dev->out_ep); if (dev->rx_req) { - usb_ep_free_request (dev->in_ep, dev->rx_req); + usb_ep_free_request (dev->out_ep, dev->rx_req); dev->rx_req=NULL; } } @@ -1432,6 +1432,11 @@ static void eth_unbind (struct usb_gadget *gadget)
DEBUG (dev, "%s...\n", __func__);
+ /* we've already been disconnected ... no i/o is active */ + if (dev->req) { + usb_ep_free_request (gadget->ep0, dev->req); + dev->req = NULL; + } if (dev->stat_req) { usb_ep_free_request (dev->status_ep, dev->stat_req); dev->stat_req = NULL; @@ -1443,7 +1448,7 @@ static void eth_unbind (struct usb_gadget *gadget) }
if (dev->rx_req) { - usb_ep_free_request (dev->in_ep, dev->rx_req); + usb_ep_free_request (dev->out_ep, dev->rx_req); dev->rx_req=NULL; }
Replace 'strcpy' by more safe 'strlcpy' that is implemented in ether.c
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/ether.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index dd3ab8c..25d6243 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -1600,12 +1600,12 @@ static int eth_bind(struct usb_gadget *gadget) if (bcdDevice) device_desc.bcdDevice = cpu_to_le16(bcdDevice); if (iManufacturer) - strcpy (manufacturer, iManufacturer); + strlcpy (manufacturer, iManufacturer, sizeof manufacturer); if (iProduct) - strcpy (product_desc, iProduct); + strlcpy (product_desc, iProduct, sizeof product_desc); if (iSerialNumber) { device_desc.iSerialNumber = STRING_SERIALNUMBER, - strcpy(serial_number, iSerialNumber); + strlcpy(serial_number, iSerialNumber, sizeof serial_number); }
/* all we really need is bulk IN/OUT */
Fix possible oops on stat_req->buf initialization and fix ep0 and status_ep confusion (last one is just intended for stat_req keeping).
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/ether.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index 25d6243..5710ddf 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -1739,14 +1739,13 @@ autoconf_fail: /* ... and maybe likewise for status transfer */ #if defined(DEV_CONFIG_CDC) if (dev->status_ep) { - dev->stat_req = usb_ep_alloc_request(gadget->ep0, GFP_KERNEL); - dev->stat_req->buf = status_req; + dev->stat_req = usb_ep_alloc_request(dev->status_ep, GFP_KERNEL); if (!dev->stat_req) { - dev->stat_req->buf=NULL; - usb_ep_free_request (gadget->ep0, dev->req); + usb_ep_free_request (dev->status_ep, dev->req);
goto fail; } + dev->stat_req->buf = status_req; dev->stat_req->context = NULL; } #endif
From: David Brownell david-b@pacbell.net
Fix potential (never-observed) oops on rare error path, bugzilla #9594. Fix uses the same test as used earlier.
Also make the adjacent "else" block look like an "else" block instead of hiding like a bug.
Signed-off-by: David Brownell dbrownell@users.sourceforge.net Signed-off-by: Greg Kroah-Hartman gregkh@suse.de
(cherry picked from commit e7b13ec9235b9fded90f826ceeb8c34548631351)
Conflicts:
drivers/usb/gadget/ether.c Cause: "else" block was removed while porting. Removing this part of the patch.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/ether.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index 5710ddf..8f0f5be 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -810,7 +810,7 @@ done:
/* on error, disable any endpoints */ if (result < 0) { - if (!subset_active(dev)) + if (!subset_active(dev) && dev->status_ep) (void) usb_ep_disable (dev->status_ep); dev->status = NULL; (void) usb_ep_disable (dev->in_ep);
Hello.
Vitaly Kuzmichev wrote:
From: David Brownell david-b@pacbell.net
Fix potential (never-observed) oops on rare error path, bugzilla #9594. Fix uses the same test as used earlier.
I think references to bugzilla.kernel.org bugs look out of place in the U-Boot patch's changelog.
Also make the adjacent "else" block look like an "else" block instead of hiding like a bug.
Signed-off-by: David Brownell dbrownell@users.sourceforge.net Signed-off-by: Greg Kroah-Hartman gregkh@suse.de
(cherry picked from commit e7b13ec9235b9fded90f826ceeb8c34548631351)
Hm, I'm not sure you can cherry-pick across different projects. :-) You'd better describe this patch as *based* on that commit.
Conflicts:
drivers/usb/gadget/ether.c Cause: "else" block was removed while porting. Removing this part of the patch.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
WBR, Sergei
From: Julia Lawall julia@diku.dk
Since num is unsigned, it would seem better to use simple_strtoul that simple_strtol.
A simplified version of the semantic patch that makes this change is as follows: (http://www.emn.fr/x-info/coccinelle/)
// <smpl> @r2@ long e; position p; @@
e = simple_strtol@p(...)
@@ position p != r2.p; type T; T e; @@
e = - simple_strtol@p + simple_strtoul (...) // </smpl>
Signed-off-by: Julia Lawall julia@diku.dk Acked-by: David Brownell dbrownell@users.sourceforge.net Signed-off-by: Greg Kroah-Hartman gregkh@suse.de
(cherry picked from commit bb9496c6f7e853e5d4edd5397c9d45f1968d623c)
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/epautoconf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c index c7fad39..e11cc20 100644 --- a/drivers/usb/gadget/epautoconf.c +++ b/drivers/usb/gadget/epautoconf.c @@ -156,7 +156,7 @@ ep_matches (
/* report address */ if (isdigit (ep->name [2])) { - u8 num = simple_strtol (&ep->name [2], NULL, 10); + u8 num = simple_strtoul (&ep->name [2], NULL, 10); desc->bEndpointAddress |= num; #ifdef MANY_ENDPOINTS } else if (desc->bEndpointAddress & USB_DIR_IN) {
Hi,
2010/8/12 Vitaly Kuzmichev vkuzmichev@mvista.com:
- Restuct Makefile to unable usage old and new stacks simultaneously
- Fix in_ep, out_ep, ep0 and status_ep confusions
- Replace 'strcpy' by 'strlcpy'
- Fix possible oops on stat_req->buf initialization
OK
- Add lost 'qmult' definition for High Speed devices
Stefano, which version of this fix do you prefer? It seems there are now 2 patches fixing the same thing...
- Take debug printout macros back and make them more useful
- 2 updates backported from linux-2.6.git
Besides the already made review comments: OK
Please fix. Thanks.
Kind regards,
Remy
Il 12/08/2010 20:37, Remy Bohmer ha scritto:
Hi,
Hi,
2010/8/12 Vitaly Kuzmichev vkuzmichev@mvista.com:
- Restuct Makefile to unable usage old and new stacks simultaneously
- Fix in_ep, out_ep, ep0 and status_ep confusions
- Replace 'strcpy' by 'strlcpy'
- Fix possible oops on stat_req->buf initialization
OK
- Add lost 'qmult' definition for High Speed devices
Stefano, which version of this fix do you prefer? It seems there are now 2 patches fixing the same thing...
It is not a problem for me to get Vitaly's patch, the only important thing is that the issue is fixed ! Vitaly makes additionally qmult configurable using a CONFIG_ switch, we can get his patch.
Best regards, Stefano
Hi Remy,
On 08/12/2010 10:37 PM, Remy Bohmer wrote:
Hi,
- Add lost 'qmult' definition for High Speed devices
Stefano, which version of this fix do you prefer? It seems there are now 2 patches fixing the same thing...
If I were you I would take my patch for 2 reasons: 1) more accurate comment (this is not OTG-related) 2) it is configurable as it was in the kernel
As alternative way I can add Stefano's signature in the patch. I like this idea :) What do you think, Stefano?
Vitaly Kuzmichev wrote:
If I were you I would take my patch for 2 reasons:
- more accurate comment (this is not OTG-related)
- it is configurable as it was in the kernel
As alternative way I can add Stefano's signature in the patch. I like this idea :) What do you think, Stefano?
Hi Vitaly,
I have already agreed about the patch. Your patch makes qmult configurable, and there is no side-effects to replace mine.
Stefano
Replace Linux-like debug printout macros by native ones.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/ether.c | 99 ++++++++++++++++++++----------------------- 1 files changed, 46 insertions(+), 53 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index a07738f..c287ec2 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -20,6 +20,9 @@ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
+#define DEBUG +#undef DEBUG + #include <common.h> #include <asm/errno.h> #include <linux/usb/ch9.h> @@ -31,14 +34,7 @@ #include "gadget_chips.h"
#define USB_NET_NAME "usb0" -#define dprintf(x, ...) -#undef INFO -#define INFO(x, s...) printf(s) -#define dev_err(x, stuff...) printf(stuff) -#define dev_dbg dev_err -#define dev_warn dev_err -#define DEBUG dev_err -#define VDEBUG DEBUG + #define atomic_read extern struct platform_data brd; #define spin_lock(x) @@ -769,7 +765,7 @@ set_ether_config (struct eth_dev *dev, gfp_t gfp_flags)
result = usb_ep_enable (dev->status_ep, dev->status); if (result != 0) { - printf ("enable %s --> %d\n", + debug("enable %s --> %d\n", dev->status_ep->name, result); goto done; } @@ -789,14 +785,14 @@ set_ether_config (struct eth_dev *dev, gfp_t gfp_flags) if (!cdc_active(dev)) { result = usb_ep_enable (dev->in_ep, dev->in); if (result != 0) { - printf ("enable %s --> %d\n", + debug("enable %s --> %d\n", dev->in_ep->name, result); goto done; }
result = usb_ep_enable (dev->out_ep, dev->out); if (result != 0) { - printf ("enable %s --> %d\n", + debug("enable %s --> %d\n", dev->out_ep->name, result); goto done; } @@ -827,6 +823,8 @@ static void eth_reset_config (struct eth_dev *dev) if (dev->config == 0) return;
+ debug("%s\n", __func__); + /* disable endpoints, forcing (synchronous) completion of * pending i/o. then free the requests. */ @@ -864,7 +862,7 @@ static int eth_set_config (struct eth_dev *dev, unsigned number, gfp_t gfp_flags && dev->config && dev->tx_qlen != 0) { /* tx fifo is full, but we can't clear it...*/ - INFO (dev, "can't change configurations\n"); + error("can't change configurations"); return -ESPIPE; } eth_reset_config (dev); @@ -901,7 +899,7 @@ static int eth_set_config (struct eth_dev *dev, unsigned number, gfp_t gfp_flags }
dev->config = number; - INFO (dev, "%s speed config #%d: %d mA, %s, using %s\n", + printf("%s speed config #%d: %d mA, %s, using %s\n", speed, number, power, driver_desc, (cdc_active(dev)? "CDC Ethernet" : "CDC Ethernet Subset")); @@ -941,11 +939,11 @@ static void eth_status_complete (struct usb_ep *ep, struct usb_request *req)
req->length = STATUS_BYTECOUNT; value = usb_ep_queue (ep, req, GFP_ATOMIC); - dprintf ("send SPEED_CHANGE --> %d\n", value); + debug("send SPEED_CHANGE --> %d\n", value); if (value == 0) return; } else if (value != -ECONNRESET) { - dprintf("event %02x --> %d\n", + debug("event %02x --> %d\n", event->bNotificationType, value); if (event->bNotificationType== USB_CDC_NOTIFY_SPEED_CHANGE) @@ -991,7 +989,7 @@ static void issue_start_status (struct eth_dev *dev)
value = usb_ep_queue (dev->status_ep, req, GFP_ATOMIC); if (value < 0) - printf ("status buf queue --> %d\n", value); + debug("status buf queue --> %d\n", value); }
#endif @@ -1001,8 +999,7 @@ static void issue_start_status (struct eth_dev *dev) static void eth_setup_complete (struct usb_ep *ep, struct usb_request *req) { if (req->status || req->actual != req->length) - dprintf (/*(struct eth_dev *) ep->driver_data*/ - "setup complete --> %d, %d/%d\n", + debug("setup complete --> %d, %d/%d\n", req->status, req->actual, req->length); }
@@ -1029,7 +1026,7 @@ eth_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) * while config change events may enable network traffic. */
- dprintf("eth_setup:...\n"); + debug("%s\n", __func__);
req->complete = eth_setup_complete; switch (ctrl->bRequest) { @@ -1078,9 +1075,9 @@ eth_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) if (ctrl->bRequestType != 0) break; if (gadget->a_hnp_support) - DEBUG (dev, "HNP available\n"); + debug("HNP available\n"); else if (gadget->a_alt_hnp_support) - DEBUG (dev, "HNP needs a different root port\n"); + debug("HNP needs a different root port\n"); value = eth_set_config (dev, wValue, GFP_ATOMIC); break; case USB_REQ_GET_CONFIGURATION: @@ -1145,7 +1142,7 @@ eth_setup (struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) /* FIXME this is wrong, as is the assumption that * all non-PXA hardware talks real CDC ... */ - dev_warn (&gadget->dev, "set_interface ignored!\n"); + debug("set_interface ignored!\n"); #endif /* DEV_CONFIG_CDC */
done_set_intf: @@ -1179,7 +1176,7 @@ done_set_intf: || wLength != 0 || wIndex > 1) break; - printf ("packet filter %02x\n", wValue); + debug("packet filter %02x\n", wValue); dev->cdc_filter = wValue; value = 0; break; @@ -1194,21 +1191,20 @@ done_set_intf: #endif /* DEV_CONFIG_CDC */
default: - printf ( - "unknown control req%02x.%02x v%04x i%04x l%d\n", + debug("unknown control req%02x.%02x v%04x i%04x l%d\n", ctrl->bRequestType, ctrl->bRequest, wValue, wIndex, wLength); }
/* respond with data transfer before status phase? */ if (value >= 0) { - dprintf("respond with data transfer before status phase\n"); + debug("respond with data transfer before status phase\n"); req->length = value; req->zero = value < wLength && (value % gadget->ep0->maxpacket) == 0; value = usb_ep_queue (gadget->ep0, req, GFP_ATOMIC); if (value < 0) { - DEBUG (dev, "ep_queue --> %d\n", value); + debug("ep_queue --> %d\n", value); req->status = 0; eth_setup_complete (gadget->ep0, req); } @@ -1237,7 +1233,7 @@ static int rx_submit ( struct eth_dev *dev, struct usb_request *req, \ * byte off the end (to force hardware errors on overflow). */
- dprintf("%s\n", __func__); + debug("%s\n", __func__);
size = (ETHER_HDR_SIZE + dev->mtu + RX_EXTRA); size += dev->out_ep->maxpacket - 1; @@ -1255,7 +1251,7 @@ static int rx_submit ( struct eth_dev *dev, struct usb_request *req, \ retval = usb_ep_queue (dev->out_ep, req, gfp_flags);
if (retval) { - dprintf ("rx submit --> %d\n", retval); + error("rx submit --> %d", retval); } return retval; } @@ -1265,8 +1261,7 @@ static void rx_complete (struct usb_ep *ep, struct usb_request *req) { struct eth_dev *dev = ep->driver_data;
- dprintf("%s\n", __func__); - dprintf("rx status %d\n", req->status); + debug("%s: status %d\n", __func__, req->status);
packet_received=1;
@@ -1291,14 +1286,14 @@ static int alloc_requests (struct eth_dev *dev, unsigned n, gfp_t gfp_flags) return 0;
fail: - DEBUG (dev, "can't alloc requests\n"); + error("can't alloc requests"); return -1; }
static void tx_complete (struct usb_ep *ep, struct usb_request *req) { - dprintf("%s, status: %s\n", __func__,(req->status) ? "failed":"ok"); + debug("%s: status %s\n", __func__, (req->status)?"failed":"ok"); packet_sent=1; }
@@ -1427,7 +1422,7 @@ static void eth_unbind (struct usb_gadget *gadget) { struct eth_dev *dev = get_gadget_data (gadget);
- printf("eth_unbind:...\n"); + debug("%s...\n", __func__);
if (dev->stat_req) { usb_ep_free_request (dev->status_ep, dev->stat_req); @@ -1567,8 +1562,7 @@ static int eth_bind(struct usb_gadget *gadget) * anything less functional on CDC-capable hardware, * so we fail in this case. */ - dev_err (&gadget->dev, - "controller '%s' not recognized\n", + error("controller '%s' not recognized", gadget->name); return -ENODEV; } @@ -1605,8 +1599,7 @@ static int eth_bind(struct usb_gadget *gadget) in_ep = usb_ep_autoconfig (gadget, &fs_source_desc); if (!in_ep) { autoconf_fail: - dev_err (&gadget->dev, - "can't autoconfigure on %s\n", + error("can't autoconfigure on %s\n", gadget->name); return -ENODEV; } @@ -1700,18 +1693,18 @@ autoconf_fail: dev->host_mac [2], dev->host_mac [3], dev->host_mac [4], dev->host_mac [5]);
- INFO (dev, "using %s, OUT %s IN %s%s%s\n", gadget->name, + printf("using %s, OUT %s IN %s%s%s\n", gadget->name, out_ep->name, in_ep->name, status_ep ? " STATUS " : "", status_ep ? status_ep->name : "" ); - INFO (dev, "MAC %02x:%02x:%02x:%02x:%02x:%02x\n", + printf("MAC %02x:%02x:%02x:%02x:%02x:%02x\n", dev->net->enetaddr [0], dev->net->enetaddr [1], dev->net->enetaddr [2], dev->net->enetaddr [3], dev->net->enetaddr [4], dev->net->enetaddr [5]);
if (cdc) { - INFO (dev, "HOST MAC %02x:%02x:%02x:%02x:%02x:%02x\n", + printf("HOST MAC %02x:%02x:%02x:%02x:%02x:%02x\n", dev->host_mac [0], dev->host_mac [1], dev->host_mac [2], dev->host_mac [3], dev->host_mac [4], dev->host_mac [5]); @@ -1755,7 +1748,7 @@ autoconf_fail: return 0;
fail: - dev_dbg(&gadget->dev, "register_netdev failed\n"); + error("%s failed", __func__); eth_unbind (gadget); return -ENOMEM; } @@ -1768,7 +1761,7 @@ static int usb_eth_init(struct eth_device* netdev, bd_t* bd) unsigned long timeout = USB_CONNECT_TIMEOUT;
if (!netdev) { - printf("ERROR: received NULL ptr\n"); + error("received NULL ptr"); goto fail; }
@@ -1790,7 +1783,7 @@ static int usb_eth_init(struct eth_device* netdev, bd_t* bd) { /* Handle control-c and timeouts */ if (ctrlc() || (get_timer(ts) > timeout)) { - printf("The remote end did not respond in time.\n"); + error("The remote end did not respond in time."); goto fail; } usb_gadget_handle_interrupts(); @@ -1806,9 +1799,9 @@ static int usb_eth_send(struct eth_device* netdev, volatile void* packet, int le { int retval; struct usb_request *req = NULL; + struct eth_dev *dev = &l_ethdev;
- struct eth_dev *dev = &l_ethdev; - dprintf("%s:...\n",__func__); + debug("%s:...\n", __func__);
req = dev->tx_req;
@@ -1836,7 +1829,7 @@ static int usb_eth_send(struct eth_device* netdev, volatile void* packet, int le retval = usb_ep_queue (dev->in_ep, req, GFP_ATOMIC);
if (!retval) - dprintf("%s: packet queued\n",__func__); + debug("%s: packet queued\n", __func__); while(!packet_sent) { packet_sent=0; @@ -1853,7 +1846,7 @@ static int usb_eth_recv(struct eth_device* netdev)
if (packet_received) { - dprintf("%s: packet received \n",__func__); + debug("%s: packet received \n", __func__); if (dev->rx_req) { NetReceive(NetRxPackets[0],dev->rx_req->length); @@ -1862,7 +1855,7 @@ static int usb_eth_recv(struct eth_device* netdev) if (dev->rx_req) rx_submit (dev, dev->rx_req, 0); } - else printf("dev->rx_req invalid\n"); + else error("dev->rx_req invalid"); } return 0; } @@ -1873,7 +1866,7 @@ void usb_eth_halt(struct eth_device* netdev)
if (!netdev) { - printf("ERROR: received NULL ptr\n"); + error("received NULL ptr"); return; }
@@ -1929,11 +1922,11 @@ int usb_eth_initialize(bd_t *bi) host_addr[sizeof(host_addr)-1] = '\0';
if (!is_eth_addr_valid(dev_addr)) { - printf("ERROR: Need valid 'usbnet_devaddr' to be set\n"); + error("Need valid 'usbnet_devaddr' to be set"); status = -1; } if (!is_eth_addr_valid(host_addr)) { - printf("ERROR: Need valid 'usbnet_hostaddr' to be set\n"); + error("Need valid 'usbnet_hostaddr' to be set"); status = -1; } if (status) @@ -1947,7 +1940,7 @@ int usb_eth_initialize(bd_t *bi) return 0;
fail: - printf("%s failed\n", __func__ ); + error("%s failed. error = %d", __func__, status); return status; }
On 2010/08/13 2:57 PM, Vitaly Kuzmichev wrote:
Replace Linux-like debug printout macros by native ones.
Signed-off-by: Vitaly Kuzmichevvkuzmichev@mvista.com
drivers/usb/gadget/ether.c | 99 ++++++++++++++++++++----------------------- 1 files changed, 46 insertions(+), 53 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index a07738f..c287ec2 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -20,6 +20,9 @@
- Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
+#define DEBUG +#undef DEBUG
Eh?
Hi,
Rogan Dawes wrote:
+#define DEBUG +#undef DEBUG
Eh?
Such thing is used to let someone know that this driver supports debug output through native U-Boot macros. So one need to comment #undef to compile ether.c with debug messages. There are at least 67 files in U-Boot that use such construction.
Vitaly Kuzmichev wrote:
Hi,
Rogan Dawes wrote:
+#define DEBUG +#undef DEBUG
Eh?
Such thing is used to let someone know that this driver supports debug output through native U-Boot macros. So one need to comment #undef to compile ether.c with debug messages. There are at least 67 files in U-Boot that use such construction.
Well, but probably it is better to remove both lines. In the rest of u-boot, DEBUG is neither set or unset - you see only #ifdef DEBUG or #ifndef DEBUG. You have found the examples how we should not do...
If you want to remember how to set the debug output, it should be enough to add a comments with "to enable the debugging, define DEBUG before common.h" or something like that. I vote to remove only the two lines...
Best regards, Stefano
Hi Stefano,
Stefano Babic wrote:
If you want to remember how to set the debug output, it should be enough to add a comments with "to enable the debugging, define DEBUG before common.h" or something like that. I vote to remove only the two lines...
Would not it be better to make one of the following? 1) #if 0 #define DEBUG #endif
2) #ifdef CONFIG_USB_ETH_DEBUG #define DEBUG #endif
On 2010/08/13 3:40 PM, Vitaly Kuzmichev wrote:
Hi Stefano,
Stefano Babic wrote:
If you want to remember how to set the debug output, it should be enough to add a comments with "to enable the debugging, define DEBUG before common.h" or something like that. I vote to remove only the two lines...
Would not it be better to make one of the following?
#if 0 #define DEBUG #endif
#ifdef CONFIG_USB_ETH_DEBUG #define DEBUG #endif
Personally, I like option 2, as it means that I have less need to touch general code when trying to debug my board.
I can just define that in my board config. Although, that could lead to a proliferation of *_DEBUG defines, which may or may not be documented.
Perhaps without the CONFIG_ part, and people who are trying to debug their boards need to read the code anyway to determine what debugging can be enabled?
Rogan
Dear Vitaly Kuzmichev,
In message 4C654B30.9060106@mvista.com you wrote:
Hi Stefano,
Stefano Babic wrote:
If you want to remember how to set the debug output, it should be enough to add a comments with "to enable the debugging, define DEBUG before common.h" or something like that. I vote to remove only the two lines...
Would not it be better to make one of the following?
#if 0 #define DEBUG #endif
#ifdef CONFIG_USB_ETH_DEBUG #define DEBUG #endif
NAK.
It is so trivial o #define DEBUG (either in the file or on the command line) that thhere is zero need for any "clever" wrappers around this.
Best regards,
Wolfgang Denk
Vitaly Kuzmichev wrote:
Hi Stefano,
Hi Vitaly,
Stefano Babic wrote:
If you want to remember how to set the debug output, it should be enough to add a comments with "to enable the debugging, define DEBUG before common.h" or something like that. I vote to remove only the two lines...
Would not it be better to make one of the following?
#if 0 #define DEBUG #endif
However, this is seen as dead code. As this one, my preferance is to set only a comment (I see several files in u-boot with such a comment, for example arch/arm/lib/board.c).
#ifdef CONFIG_USB_ETH_DEBUG #define DEBUG #endif
It is ok, but it generates another new CONFIG_ switch, that is unusable for the rest of u-boot. I agree that in u-boot there is a lot of different and local functions to setup, and probably a general mechanism should be better defined. However, setting "#define DEBUG" before any include files is quite usual. I do not like to set a CONFIG_ switch only for debug purpose, as in the delivered system all debug output should be turned off.
Best regards, Stefano
Dear Stefano Babic,
In message 4C6557C5.4000300@denx.de you wrote:
It is ok, but it generates another new CONFIG_ switch, that is unusable for the rest of u-boot. I agree that in u-boot there is a lot of different and local functions to setup, and probably a general mechanism should be better defined. However, setting "#define DEBUG" before any include files is quite usual. I do not like to set a CONFIG_ switch only for debug purpose, as in the delivered system all debug output should be turned off.
We already have such a general mechanism, the problem is just that people ignore it and re-invent the wheel.
We have debug(), debugX(), error(), and BUG_ON().
What exactly seems to be missing?
Note that when using constants as "level" argument in debugX() the compiler can even optimize away non-relevant code.
I remember we had a similar discussion in the past, which resulted that I accepted to have debugX() added - yet how many files use it? 2 (in words: TWO)!
So before we add even more debug utilities someone has to bring up really good arguments to convince me.
Best regards,
Wolfgang Denk
Dear Vitaly Kuzmichev,
In message 4C6544B9.3000503@mvista.com you wrote:
+#define DEBUG +#undef DEBUG
Eh?
Such thing is used to let someone know that this driver supports debug output through native U-Boot macros. So one need to comment #undef to compile ether.c with debug messages. There are at least 67 files in U-Boot that use such construction.
Patches to clean up these are welcome.
I will NAK this whenever I see it.
Best regards,
Wolfgang Denk
Hello.
Vitaly Kuzmichev wrote:
Rogan Dawes wrote:
+#define DEBUG +#undef DEBUG
Eh?
Such thing is used to let someone know that this driver supports debug output through native U-Boot macros. So one need to comment #undef to compile ether.c with debug messages. There are at least 67 files in U-Boot that use such construction.
I'd suggest leaving just #undef DEBUG (which can be edited into #define DEBUG if desired)...
WBR, Sergei
Dear Sergei Shtylyov,
In message 4C6558D4.9070308@mvista.com you wrote:
I'd suggest leaving just #undef DEBUG (which can be edited into #defineDEBUG if desired)...
Don't you listen? Remove that, completely, please!
Best regards,
Wolfgang Denk
Dear Vitaly Kuzmichev,
In message 1281704276-29115-1-git-send-email-vkuzmichev@mvista.com you wrote:
Replace Linux-like debug printout macros by native ones.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
drivers/usb/gadget/ether.c | 99 ++++++++++++++++++++----------------------- 1 files changed, 46 insertions(+), 53 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index a07738f..c287ec2 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -20,6 +20,9 @@
- Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
+#define DEBUG +#undef DEBUG
NAK. Please remove this dead code.
Best regards,
Wolfgang Denk
Hi,
2010/8/13 Vitaly Kuzmichev vkuzmichev@mvista.com:
Replace Linux-like debug printout macros by native ones.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
drivers/usb/gadget/ether.c | 99 ++++++++++++++++++++----------------------- 1 files changed, 46 insertions(+), 53 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index a07738f..c287ec2 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -20,6 +20,9 @@ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
+#define DEBUG +#undef DEBUG
Applied to u-boot-usb/cdc branch after removing ' #define DEBUG' as Sergei suggested. Thanks.
Remy
Dear Remy Bohmer,
In message AANLkTikBeeSeKKuAECPZbWM3RAmJkjcpKk=ykrNHwVPW@mail.gmail.com you wrote:
+#define DEBUG +#undef DEBUG
Applied to u-boot-usb/cdc branch after removing ' #define DEBUG' as Sergei suggested.
NAK. Please also remove the dead and completely needless #undef DEBUG as well.
Thanks.
Wolfgang Denk
Hi,
2010/8/13 Wolfgang Denk wd@denx.de:
Dear Remy Bohmer,
In message AANLkTikBeeSeKKuAECPZbWM3RAmJkjcpKk=ykrNHwVPW@mail.gmail.com you wrote:
+#define DEBUG +#undef DEBUG
Applied to u-boot-usb/cdc branch after removing ' #define DEBUG' as Sergei suggested.
NAK. Please also remove the dead and completely needless #undef DEBUG as well.
Done!
Remy
Fix in_ep and out_ep confusion (rx_req was allocated from out_ep, not from in_ep) and add lost dev->req freeing.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/ether.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index c287ec2..19a63de 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -839,7 +839,7 @@ static void eth_reset_config (struct eth_dev *dev) if (dev->out) { usb_ep_disable (dev->out_ep); if (dev->rx_req) { - usb_ep_free_request (dev->in_ep, dev->rx_req); + usb_ep_free_request (dev->out_ep, dev->rx_req); dev->rx_req=NULL; } } @@ -1424,6 +1424,11 @@ static void eth_unbind (struct usb_gadget *gadget)
debug("%s...\n", __func__);
+ /* we've already been disconnected ... no i/o is active */ + if (dev->req) { + usb_ep_free_request (gadget->ep0, dev->req); + dev->req = NULL; + } if (dev->stat_req) { usb_ep_free_request (dev->status_ep, dev->stat_req); dev->stat_req = NULL; @@ -1435,7 +1440,7 @@ static void eth_unbind (struct usb_gadget *gadget) }
if (dev->rx_req) { - usb_ep_free_request (dev->in_ep, dev->rx_req); + usb_ep_free_request (dev->out_ep, dev->rx_req); dev->rx_req=NULL; }
Hi,
2010/8/13 Vitaly Kuzmichev vkuzmichev@mvista.com:
Fix in_ep and out_ep confusion (rx_req was allocated from out_ep, not from in_ep) and add lost dev->req freeing.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
drivers/usb/gadget/ether.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)
Applied to u-boot-usb/cdc branch. Thanks.
Remy
Replace 'strcpy' by more safe 'strlcpy' that is implemented in ether.c
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/ether.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index 19a63de..65f3ff9 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -1591,12 +1591,12 @@ static int eth_bind(struct usb_gadget *gadget) if (bcdDevice) device_desc.bcdDevice = cpu_to_le16(bcdDevice); if (iManufacturer) - strcpy (manufacturer, iManufacturer); + strlcpy (manufacturer, iManufacturer, sizeof manufacturer); if (iProduct) - strcpy (product_desc, iProduct); + strlcpy (product_desc, iProduct, sizeof product_desc); if (iSerialNumber) { device_desc.iSerialNumber = STRING_SERIALNUMBER, - strcpy(serial_number, iSerialNumber); + strlcpy(serial_number, iSerialNumber, sizeof serial_number); }
/* all we really need is bulk IN/OUT */
Hi,
2010/8/13 Vitaly Kuzmichev vkuzmichev@mvista.com:
Replace 'strcpy' by more safe 'strlcpy' that is implemented in ether.c
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
drivers/usb/gadget/ether.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
Applied to u-boot-usb/cdc branch. Thanks.
Remy
Fix possible oops on stat_req->buf initialization and fix ep0 and status_ep confusion (last one is just intended for stat_req keeping).
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com Signed-off-by: Stefano Babic sbabic@denx.de --- drivers/usb/gadget/ether.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index 65f3ff9..03d8f0b 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -1729,14 +1729,13 @@ autoconf_fail: /* ... and maybe likewise for status transfer */ #if defined(DEV_CONFIG_CDC) if (dev->status_ep) { - dev->stat_req = usb_ep_alloc_request(gadget->ep0, GFP_KERNEL); - dev->stat_req->buf = status_req; + dev->stat_req = usb_ep_alloc_request(dev->status_ep, GFP_KERNEL); if (!dev->stat_req) { - dev->stat_req->buf=NULL; - usb_ep_free_request (gadget->ep0, dev->req); + usb_ep_free_request (dev->status_ep, dev->req);
goto fail; } + dev->stat_req->buf = status_req; dev->stat_req->context = NULL; } #endif
Hi,
2010/8/13 Vitaly Kuzmichev vkuzmichev@mvista.com:
Fix possible oops on stat_req->buf initialization and fix ep0 and status_ep confusion (last one is just intended for stat_req keeping).
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com Signed-off-by: Stefano Babic sbabic@denx.de
drivers/usb/gadget/ether.c | 7 +++---- 1 files changed, 3 insertions(+), 4 deletions(-)
Applied to u-boot-usb/cdc branch. Thanks.
Remy
Fix potential oops on rare error path. The patch is based on commit e7b13ec9235b9fded90f826ceeb8c34548631351 (done by David Brownell david-b@pacbell.net) from linux-2.6.git.
Description of the issue taken from linux kernel bugzilla: (https://bugzilla.kernel.org/show_bug.cgi?id=9594)
The potential error can be tracked down as follows:
(1) line 807: let the second conjunct on the "if" statment be false meaning "dev->status_ep" is null. This means the "if" evaluates to false.
follow thru the code until...
(2) line 808: usb_ep_disable(dev->status_ep) passes in a null argument, however "usb_ep_disable" cannot handle that:
(from include/linux/usb/gadget.h) 191 static inline int 192 usb_ep_disable (struct usb_ep *ep) 193 { 194 return ep->ops->disable (ep); 195 }
--
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/ether.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index 03d8f0b..62dd008 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -804,7 +804,7 @@ done:
/* on error, disable any endpoints */ if (result < 0) { - if (!subset_active(dev)) + if (!subset_active(dev) && dev->status_ep) (void) usb_ep_disable (dev->status_ep); dev->status = NULL; (void) usb_ep_disable (dev->in_ep);
Hi,
2010/8/13 Vitaly Kuzmichev vkuzmichev@mvista.com:
Fix potential oops on rare error path. The patch is based on commit e7b13ec9235b9fded90f826ceeb8c34548631351 (done by David Brownell david-b@pacbell.net) from linux-2.6.git.
Description of the issue taken from linux kernel bugzilla: (https://bugzilla.kernel.org/show_bug.cgi?id=9594)
The potential error can be tracked down as follows:
(1) line 807: let the second conjunct on the "if" statment be false meaning "dev->status_ep" is null. This means the "if" evaluates to false.
follow thru the code until...
(2) line 808: usb_ep_disable(dev->status_ep) passes in a null argument, however "usb_ep_disable" cannot handle that:
(from include/linux/usb/gadget.h) 191 static inline int 192 usb_ep_disable (struct usb_ep *ep) 193 { 194 return ep->ops->disable (ep); 195 }
--
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
drivers/usb/gadget/ether.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
Applied to u-boot-usb/cdc branch. Thanks.
Remy
The patch is based on commit bb9496c6f7e853e5d4edd5397c9d45f1968d623c (done by Julia Lawall julia@diku.dk) from linux-2.6.git.
Since num is unsigned, it would seem better to use simple_strtoul that simple_strtol.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com --- drivers/usb/gadget/epautoconf.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c index c7fad39..e11cc20 100644 --- a/drivers/usb/gadget/epautoconf.c +++ b/drivers/usb/gadget/epautoconf.c @@ -156,7 +156,7 @@ ep_matches (
/* report address */ if (isdigit (ep->name [2])) { - u8 num = simple_strtol (&ep->name [2], NULL, 10); + u8 num = simple_strtoul (&ep->name [2], NULL, 10); desc->bEndpointAddress |= num; #ifdef MANY_ENDPOINTS } else if (desc->bEndpointAddress & USB_DIR_IN) {
Hi,
2010/8/13 Vitaly Kuzmichev vkuzmichev@mvista.com:
The patch is based on commit bb9496c6f7e853e5d4edd5397c9d45f1968d623c (done by Julia Lawall julia@diku.dk) from linux-2.6.git.
Since num is unsigned, it would seem better to use simple_strtoul that simple_strtol.
Signed-off-by: Vitaly Kuzmichev vkuzmichev@mvista.com
drivers/usb/gadget/epautoconf.c | 2 +-
Applied to u-boot-usb/cdc branch. Thanks.
Remy
participants (7)
-
Remy Bohmer -
Rogan Dawes -
Sergei Shtylyov -
Stefano Babic -
stefano babic
-
Vitaly Kuzmichev -
Wolfgang Denk