Skip to contentSkip to conversation

USB: gadget: fix porting bugs

PATCH New Vitaly Kuzmichev 6 participants 52 messages Aug 12, 2010 – Aug 13, 2010 USB

lore
Share & tools
lore

Vitaly Kuzmichev

author
#0 lore raw

cover 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(-)
author
#1 lore raw

PATCH 1/8 USB-CDC: Restuct USB gadget Makefile

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 <[email protected]>
---
 drivers/usb/gadget/Makefile |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
drivers/usb/gadget/Makefile +6 -3
index 27e7f40..05aa213 100644
@@ -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)
Signature · 1 line
1.7.1.1
#2 lore raw

PATCH 1/8 USB-CDC: Restuct USB gadget Makefile

Hi,

2010/8/12 Vitaly Kuzmichev <[email protected]>:

Quoted 7 lines

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 <[email protected]>

---
?drivers/usb/gadget/Makefile | ? ?9 ++++++---
?1 files changed, 6 insertions(+), 3 deletions(-)

Applied to u-boot-usb/cdc branch.
Thanks.

Remy

author
#3 lore raw

PATCH 2/8 USB-CDC: Add lost 'qmult' definition

In reply to Vitaly Kuzmichev

Add lost 'qmult' definition for High Speed devices and make it configurable through CONFIG_USB_ETH_QMULT.

Signed-off-by Vitaly Kuzmichev <[email protected]>
---
 drivers/usb/gadget/ether.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
drivers/usb/gadget/ether.c +6 -0
index 3d871fa..a07738f 100644
@@ -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))
Signature · 1 line
1.7.1.1
#4 lore raw

PATCH 2/8 USB-CDC: Add lost 'qmult' definition

Hi,

2010/8/12 Vitaly Kuzmichev <[email protected]>:

Quoted 7 lines

Add lost 'qmult' definition for High Speed devices and make it configurable through CONFIG_USB_ETH_QMULT.

Signed-off-by Vitaly Kuzmichev <[email protected]>

---
?drivers/usb/gadget/ether.c | ? ?6 ++++++
?1 files changed, 6 insertions(+), 0 deletions(-)

Applied to u-boot-usb/cdc branch.
Thanks.

Remy

author
#5 lore raw

PATCH 3/8 USB-CDC: Linux-like debug printout

In reply to Vitaly Kuzmichev

Take debug printout macros back from linux-2.6.27 and make them more useful and more compatible.

Signed-off-by Vitaly Kuzmichev <[email protected]>
---
 drivers/usb/gadget/ether.c |   65 +++++++++++++++++++++++---------------------
 1 files changed, 34 insertions(+), 31 deletions(-)
drivers/usb/gadget/ether.c +34 -31
index a07738f..b6f5f4d 100644
@@ -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;
 }
Signature · 1 line
1.7.1.1
#6 lore raw

PATCH 3/8 USB-CDC: Linux-like debug printout

Hello.

Quoted 12 lines · Vitaly Kuzmichev

Take debug printout macros back from linux-2.6.27 and make them more useful and more compatible.

Signed-off-by Vitaly Kuzmichev <[email protected]>
---
 drivers/usb/gadget/ether.c |   65 +++++++++++++++++++++++---------------------
 1 files changed, 34 insertions(+), 31 deletions(-)
drivers/usb/gadget/ether.c +0 -0
index a07738f..b6f5f4d 100644

[...]

Quoted 10 lines
drivers/usb/gadget/ether.c +1 -1
@@ -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;
 		}

drivers/usb/gadget/ether.c set_ether_config (struct eth_dev *dev, gfp_t gfp_flags) · line 794

Quoted 5 lines
drivers/usb/gadget/ether.c +1 -1
@@ -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.

drivers/usb/gadget/ether.c static void eth_status_complete (struct usb_ep *ep, struct usb_request *req) · line 952

Quoted 11 lines
drivers/usb/gadget/ether.c +2 -2
@@ -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",

Why add a space where there was none before?

drivers/usb/gadget/ether.c fail: · line 1304

Quoted 6 lines
drivers/usb/gadget/ether.c +1 -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,

No cast here?

drivers/usb/gadget/ether.c fail: · line 1306

Quoted 3 lines
drivers/usb/gadget/ether.c +0 -0
 "%s: status %s\n", __func__, (req->status)?"failed":"ok");
 	packet_sent=1;
 }

WBR, Sergei

author
#7 lore raw

PATCH 3/8 USB-CDC: Linux-like debug printout

Hi Sergei,

Quoted 6 lines · Sergei Shtylyov

drivers/usb/gadget/ether.c set_ether_config (struct eth_dev *dev, gfp_t gfp_flags) · line 794

Quoted 2 lines
drivers/usb/gadget/ether.c +1 -1
-            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.

#8 lore raw

PATCH 3/8 USB-CDC: Linux-like debug printout

Hello.

Quoted 11 lines · Vitaly Kuzmichev
Quoted 6 lines

drivers/usb/gadget/ether.c set_ether_config (struct eth_dev *dev, gfp_t gfp_flags) · line 794

Quoted 2 lines
drivers/usb/gadget/ether.c +1 -1
-            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. ;-)

Quoted 2 lines

Best regards,
Vitaly.

WBR, Sergei

#9 lore raw

PATCH 3/8 USB-CDC: Linux-like debug printout

In reply to Vitaly Kuzmichev

Hi,

2010/8/12 Vitaly Kuzmichev <[email protected]>:

Quoted 16 lines

Take debug printout macros back from linux-2.6.27 and make them more useful and more compatible.

Signed-off-by Vitaly Kuzmichev <[email protected]>

---
?drivers/usb/gadget/ether.c | ? 65 +++++++++++++++++++++++--------------------- ?1 files changed, 34 insertions(+), 31 deletions(-)

drivers/usb/gadget/ether.c +0 -0
index a07738f..b6f5f4d 100644
@@ -37,8 +37,10 @@
 ?#define dev_err(x, stuff...) printf(stuff)

?#define dev_dbg dev_err
?#define dev_warn dev_err

drivers/usb/gadget/ether.c line 42

Quoted 5 lines
drivers/usb/gadget/ether.c +3 -2
-#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

author
#10 lore raw

PATCH 3/8 USB-CDC: Linux-like debug printout

Hi Remy,

No, it does not.
DEBUG is defined as dev_err and dev_err is defined as printf. Anyway I can change it.

Quoted 16 lines · Remy Bohmer

drivers/usb/gadget/ether.c line 42

Quoted 9 lines
drivers/usb/gadget/ether.c +3 -2
@@ -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

Signature · 2 lines
Best regards,
Vitaly Kuzmichev.
#11 lore raw

PATCH 3/8 USB-CDC: Linux-like debug printout

Hi,

2010/8/13 Vitaly Kuzmichev <[email protected]>:

Quoted 5 lines

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!

Quoted 14 lines · Remy Bohmer
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0 -0
@@ -37,8 +37,10 @@
 ?#define dev_err(x, stuff...) printf(stuff)

?#define dev_dbg dev_err
?#define dev_warn dev_err

Quoted 8 lines

drivers/usb/gadget/ether.c line 42

Quoted 5 lines
drivers/usb/gadget/ether.c +3 -2
-#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?

Quoted 1 line

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()...

Quoted 1 line

Anyway I can change it.

please do, DEBUG logging should not be on by default...

Kind regards,

Remy

#12 lore raw

PATCH 3/8 USB-CDC: Linux-like debug printout

Dear Remy Bohmer,

Quoted 10 lines · In message <[email protected]> you…
Quoted 9 lines
Quoted 7 lines

drivers/usb/gadget/ether.c line 42

Quoted 4 lines
drivers/usb/gadget/ether.c +3 -0
+#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.

Quoted 7 lines

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()...

Quoted 1 line

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

Signature · 6 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You are an excellent tactician, Captain. You let your second in  com-
mand attack while you sit and watch for weakness.
	-- Khan Noonian Singh, "Space Seed", stardate 3141.9
#13 lore raw

PATCH 3/8 USB-CDC: Linux-like debug printout

Hi,

Quoted 7 lines

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

#14 lore raw

PATCH 3/8 USB-CDC: Linux-like debug printout

In reply to Vitaly Kuzmichev
Quoted 8 lines · Vitaly Kuzmichev

Take debug printout macros back from linux-2.6.27 and make them more useful and more compatible.

Signed-off-by Vitaly Kuzmichev <[email protected]>
---
 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

Signature · 5 lines
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================
author
#15 lore raw

PATCH 4/8 USB-CDC: Correct freeing usb requests

In reply to Vitaly Kuzmichev

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 <[email protected]>
---
 drivers/usb/gadget/ether.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
drivers/usb/gadget/ether.c +7 -2
index b6f5f4d..dd3ab8c 100644
@@ -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;
 	}
Signature · 1 line
1.7.1.1
author
#16 lore raw

PATCH 5/8 USB-CDC: Replace 'strcpy' by 'strlcpy'

In reply to Vitaly Kuzmichev

Replace 'strcpy' by more safe 'strlcpy' that is implemented in ether.c

Signed-off-by Vitaly Kuzmichev <[email protected]>
---
 drivers/usb/gadget/ether.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/usb/gadget/ether.c +3 -3
index dd3ab8c..25d6243 100644
@@ -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 */
Signature · 1 line
1.7.1.1
author
#17 lore raw

PATCH 6/8 USB-CDC: Correct stat_req initialization

In reply to Vitaly Kuzmichev

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 <[email protected]>
---
 drivers/usb/gadget/ether.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)
drivers/usb/gadget/ether.c +3 -4
index 25d6243..5710ddf 100644
@@ -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
Signature · 1 line
1.7.1.1
author
#18 lore raw

PATCH 7/8 USB: gadget: ethernet error path potential oops fix

In reply to Vitaly Kuzmichev

From: David Brownell <[email protected]>

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 <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>

(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 <[email protected]>
---
 drivers/usb/gadget/ether.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/usb/gadget/ether.c +1 -1
index 5710ddf..8f0f5be 100644
@@ -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);
Signature · 1 line
1.7.1.1
#19 lore raw

PATCH 7/8 USB: gadget: ethernet error path potential oops fix

Hello.

Quoted 4 lines · Vitaly Kuzmichev

From: David Brownell <[email protected]>

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.

Quoted 7 lines

Also make the adjacent "else" block look like an "else" block instead of hiding like a bug.

Signed-off-by David Brownell <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>

(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.

Quoted 7 lines

Conflicts:

drivers/usb/gadget/ether.c
Cause: "else" block was removed while porting. Removing this part of the patch.

Signed-off-by Vitaly Kuzmichev <[email protected]>

WBR, Sergei

author
#20 lore raw

PATCH 8/8 USB: gadget: change simple_strtol to simple_strtoul

In reply to Vitaly Kuzmichev

From: Julia Lawall <[email protected]>

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 at p(...)

@@
position p != r2.p;
type T;
T e;
@@

e =
- simple_strtol at p
+ simple_strtoul
(...)
// </smpl>

Signed-off-by Julia Lawall <[email protected]>
Acked-by David Brownell <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>

(cherry picked from commit bb9496c6f7e853e5d4edd5397c9d45f1968d623c)

Signed-off-by Vitaly Kuzmichev <[email protected]>
---
 drivers/usb/gadget/epautoconf.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/usb/gadget/epautoconf.c +1 -1
index c7fad39..e11cc20 100644
@@ -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) {
Signature · 1 line
1.7.1.1
#21 lore raw

cover USB: gadget: fix porting bugs

In reply to Vitaly Kuzmichev

Hi,

2010/8/12 Vitaly Kuzmichev <[email protected]>:

Quoted 4 lines

1) Restuct Makefile to unable usage old and new stacks simultaneously
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

OK

Quoted 1 line

2) 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...

Quoted 2 lines

3) Take debug printout macros back and make them more useful
7) 2 updates backported from linux-2.6.git

Besides the already made review comments: OK

Please fix. Thanks.

Kind regards,

Remy

#22 lore raw

cover USB: gadget: fix porting bugs

Il 12/08/2010 20:37, Remy Bohmer ha scritto:

Quoted 2 lines

Hi,

Hi,

Quoted 6 lines

2010/8/12 Vitaly Kuzmichev <[email protected]>:

Quoted 4 lines

1) Restuct Makefile to unable usage old and new stacks simultaneously
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

Quoted 6 lines

OK

Quoted 1 line

2) 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

Signature · 5 lines
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================
author
#23 lore raw

cover USB: gadget: fix porting bugs

In reply to Remy Bohmer

Hi Remy,

Quoted 7 lines · Remy Bohmer

Hi,

Quoted 1 line

2) 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?

#24 lore raw

cover USB: gadget: fix porting bugs

Quoted 8 lines · Vitaly Kuzmichev

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?

Hi Vitaly,

I have already agreed about the patch. Your patch makes qmult configurable, and there is no side-effects to replace mine.

Stefano

Signature · 5 lines
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================
author
#25 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

In reply to Vitaly Kuzmichev

Replace Linux-like debug printout macros by native ones.

Signed-off-by Vitaly Kuzmichev <[email protected]>
---
 drivers/usb/gadget/ether.c |   99 ++++++++++++++++++++-----------------------
 1 files changed, 46 insertions(+), 53 deletions(-)
drivers/usb/gadget/ether.c +46 -53
index a07738f..c287ec2 100644
@@ -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;
 }
Signature · 1 line
1.7.1.1
#26 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

Quoted 12 lines · Vitaly Kuzmichev

Replace Linux-like debug printout macros by native ones.

Signed-off-by Vitaly Kuzmichev<[email protected]>
---
  drivers/usb/gadget/ether.c |   99 ++++++++++++++++++++-----------------------
  1 files changed, 46 insertions(+), 53 deletions(-)
drivers/usb/gadget/ether.c +0 -0
index a07738f..c287ec2 100644

drivers/usb/gadget/ether.c line 25

Quoted 6 lines
drivers/usb/gadget/ether.c +3 -0
@@ -20,6 +20,9 @@
   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
   */
 
+#define DEBUG
+#undef DEBUG
+

Eh?

author
#27 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

Hi,

Quoted 5 lines · Rogan Dawes

drivers/usb/gadget/ether.c line 25

Quoted 3 lines
drivers/usb/gadget/ether.c +3 -0
+#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.

author
#28 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

Quoted 1 line · Vitaly Kuzmichev

There are at least 67 files in U-Boot that use such construction.

I was wrong.
I calculated for "linux/drivers" directory.
Actually 13 for u-boot.

#29 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

In reply to Vitaly Kuzmichev
Quoted 8 lines · Vitaly Kuzmichev

Hi,

Quoted 4 lines · Rogan Dawes

drivers/usb/gadget/ether.c line 25

Quoted 3 lines
drivers/usb/gadget/ether.c +3 -0
+#define DEBUG
+#undef DEBUG
+

Eh?

Quoted 4 lines

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

Signature · 5 lines
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================
author
#30 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

Hi Stefano,

Quoted 3 lines · Stefano Babic

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

#31 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

Quoted 13 lines · Vitaly Kuzmichev

Hi Stefano,

Quoted 3 lines · Stefano Babic

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

Quoted 4 lines

2)
#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

NAK
#32 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

In reply to Vitaly Kuzmichev

Dear Vitaly Kuzmichev,

Quoted 13 lines · In message <[email protected]> you wrote:

Hi Stefano,

Quoted 3 lines · Stefano Babic

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

Quoted 4 lines

2)
#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

Signature · 5 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Behind every great man, there is a woman -- urging him on.
	-- Harry Mudd, "I, Mudd", stardate 4513.3
#33 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

In reply to Vitaly Kuzmichev
Quoted 2 lines · Vitaly Kuzmichev

Hi Stefano,

Hi Vitaly,

Quoted 10 lines
Quoted 3 lines · Stefano Babic

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

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).

Quoted 5 lines

2)
#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

Signature · 5 lines
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================
#34 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

Dear Stefano Babic,

Quoted 8 lines · In message <[email protected]> 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

Signature · 6 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Where shall I begin, please your Majesty?" he asked. "Begin  at  the
beginning,"  the  King said, gravely, "and go on till you come to the
end: then stop."    - Alice's Adventures in Wonderland, Lewis Carroll
#35 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

In reply to Vitaly Kuzmichev

Dear Vitaly Kuzmichev,

Quoted 11 lines · In message <[email protected]> you wrote:
Quoted 6 lines

drivers/usb/gadget/ether.c line 25

Quoted 4 lines
drivers/usb/gadget/ether.c +3 -0
+#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

Signature · 6 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
PLEASE NOTE: Some Quantum Physics Theories Suggest That When the Con-
sumer Is Not Directly Observing This Product, It May Cease  to  Exist
or Will Exist Only in a Vague and Undetermined State.
#36 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

In reply to Vitaly Kuzmichev

Hello.

Quoted 8 lines · Vitaly Kuzmichev
Quoted 5 lines · Rogan Dawes

drivers/usb/gadget/ether.c line 25

Quoted 3 lines
drivers/usb/gadget/ether.c +3 -0
+#define DEBUG
+#undef DEBUG
+

Eh?

Quoted 4 lines

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

#37 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

Dear Sergei Shtylyov,

Quoted 3 lines · In message <[email protected]> you wrote:

I'd suggest leaving just #undef DEBUG (which can be edited into #define
DEBUG if desired)...

Don't you listen? Remove that, completely, please!

Best regards,

Wolfgang Denk

Signature · 5 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Commitment, n.:      Commitment can be illustrated by a breakfast
of ham and eggs. The chicken was involved, the pig was committed.
#38 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

In reply to Vitaly Kuzmichev

Dear Vitaly Kuzmichev,

Quoted 12 lines · In message <[email protected]> you wrote:

Replace Linux-like debug printout macros by native ones.

Signed-off-by Vitaly Kuzmichev <[email protected]>
---
 drivers/usb/gadget/ether.c |   99 ++++++++++++++++++++-----------------------
 1 files changed, 46 insertions(+), 53 deletions(-)
drivers/usb/gadget/ether.c +0 -0
index a07738f..c287ec2 100644

drivers/usb/gadget/ether.c line 25

Quoted 6 lines
drivers/usb/gadget/ether.c +3 -0
@@ -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

Signature · 6 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Every program has at least one bug and can be shortened by  at  least
one  instruction  --  from  which,  by induction, one can deduce that
every program can be reduced to one instruction which doesn't work.
#39 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

In reply to Vitaly Kuzmichev

Hi,

2010/8/13 Vitaly Kuzmichev <[email protected]>:

Quoted 15 lines

Replace Linux-like debug printout macros by native ones.

Signed-off-by Vitaly Kuzmichev <[email protected]>

---
?drivers/usb/gadget/ether.c | ? 99 ++++++++++++++++++++----------------------- ?1 files changed, 46 insertions(+), 53 deletions(-)

drivers/usb/gadget/ether.c +0 -0
index a07738f..c287ec2 100644
@@ -20,6 +20,9 @@

?* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA ?02111-1307 ?USA ?*/

drivers/usb/gadget/ether.c line 24

Quoted 2 lines
drivers/usb/gadget/ether.c +2 -0
+#define DEBUG
+#undef DEBUG

Applied to u-boot-usb/cdc branch after removing ' #define DEBUG' as Sergei suggested.
Thanks.

Remy

#40 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

Dear Remy Bohmer,

Quoted 6 lines · In message <[email protected]> you…

drivers/usb/gadget/ether.c line 24

Quoted 3 lines
drivers/usb/gadget/ether.c +2 -0
+#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

Signature · 5 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If the odds are a million to one against something occuring,  chances
are 50-50 it will.
#41 lore raw

PATCH v2 3/8 USB-CDC: Use native debug printout macros

Hi,

2010/8/13 Wolfgang Denk <[email protected]>:

Quoted 4 lines

Dear Remy Bohmer,

Quoted 1 line · In message <[email protected]> you…
Quoted 8 lines
Quoted 5 lines

drivers/usb/gadget/ether.c line 24

Quoted 2 lines
drivers/usb/gadget/ether.c +2 -0
+#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

author
#42 lore raw

PATCH v2 4/8 USB-CDC: Correct freeing usb requests

In reply to Vitaly Kuzmichev

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 <[email protected]>
---
 drivers/usb/gadget/ether.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)
drivers/usb/gadget/ether.c +7 -2
index c287ec2..19a63de 100644
@@ -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;
 	}
Signature · 1 line
1.7.1.1
#43 lore raw

PATCH v2 4/8 USB-CDC: Correct freeing usb requests

Hi,

2010/8/13 Vitaly Kuzmichev <[email protected]>:

Quoted 7 lines

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 <[email protected]>

---
?drivers/usb/gadget/ether.c | ? ?9 +++++++--
?1 files changed, 7 insertions(+), 2 deletions(-)

Applied to u-boot-usb/cdc branch.
Thanks.

Remy

author
#44 lore raw

PATCH v2 5/8 USB-CDC: Replace 'strcpy' by 'strlcpy'

In reply to Vitaly Kuzmichev

Replace 'strcpy' by more safe 'strlcpy' that is implemented in ether.c

Signed-off-by Vitaly Kuzmichev <[email protected]>
---
 drivers/usb/gadget/ether.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/usb/gadget/ether.c +3 -3
index 19a63de..65f3ff9 100644
@@ -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 */
Signature · 1 line
1.7.1.1
#45 lore raw

PATCH v2 5/8 USB-CDC: Replace 'strcpy' by 'strlcpy'

Hi,

2010/8/13 Vitaly Kuzmichev <[email protected]>:

Quoted 7 lines

Replace 'strcpy' by more safe 'strlcpy' that is implemented in ether.c

Signed-off-by Vitaly Kuzmichev <[email protected]>

---
?drivers/usb/gadget/ether.c | ? ?6 +++---
?1 files changed, 3 insertions(+), 3 deletions(-)

Applied to u-boot-usb/cdc branch.
Thanks.

Remy

author
#46 lore raw

PATCH v2 6/8 USB-CDC: Correct stat_req initialization

In reply to Vitaly Kuzmichev

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 <[email protected]>
Signed-off-by Stefano Babic <[email protected]>
---
 drivers/usb/gadget/ether.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)
drivers/usb/gadget/ether.c +3 -4
index 65f3ff9..03d8f0b 100644
@@ -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
Signature · 1 line
1.7.1.1
#47 lore raw

PATCH v2 6/8 USB-CDC: Correct stat_req initialization

Hi,

2010/8/13 Vitaly Kuzmichev <[email protected]>:

Quoted 9 lines

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 <[email protected]>
Signed-off-by Stefano Babic <[email protected]>

---
?drivers/usb/gadget/ether.c | ? ?7 +++----
?1 files changed, 3 insertions(+), 4 deletions(-)

Applied to u-boot-usb/cdc branch.
Thanks.

Remy

author
#48 lore raw

PATCH v2 7/8 USB-CDC: ethernet error path potential oops fix

In reply to Vitaly Kuzmichev

Fix potential oops on rare error path.
The patch is based on commit e7b13ec9235b9fded90f826ceeb8c34548631351 (done by David Brownell <[email protected]>) 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 <[email protected]>
---
 drivers/usb/gadget/ether.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/usb/gadget/ether.c +1 -1
index 03d8f0b..62dd008 100644
@@ -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);
Signature · 1 line
1.7.1.1
#49 lore raw

PATCH v2 7/8 USB-CDC: ethernet error path potential oops fix

Hi,

2010/8/13 Vitaly Kuzmichev <[email protected]>:

Quoted 25 lines

Fix potential oops on rare error path.
The patch is based on commit e7b13ec9235b9fded90f826ceeb8c34548631351 (done by David Brownell <[email protected]>) 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 }

Quoted 7 lines

--

Signed-off-by Vitaly Kuzmichev <[email protected]>

---
?drivers/usb/gadget/ether.c | ? ?2 +-
?1 files changed, 1 insertions(+), 1 deletions(-)

Applied to u-boot-usb/cdc branch.
Thanks.

Remy

author
#50 lore raw

PATCH v2 8/8 USB-CDC: change simple_strtol to simple_strtoul

In reply to Vitaly Kuzmichev

The patch is based on commit bb9496c6f7e853e5d4edd5397c9d45f1968d623c (done by Julia Lawall <[email protected]>) 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 <[email protected]>
---
 drivers/usb/gadget/epautoconf.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/usb/gadget/epautoconf.c +1 -1
index c7fad39..e11cc20 100644
@@ -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) {
Signature · 1 line
1.7.1.1
#51 lore raw

PATCH v2 8/8 USB-CDC: change simple_strtol to simple_strtoul

Hi,

2010/8/13 Vitaly Kuzmichev <[email protected]>:

Quoted 9 lines

The patch is based on commit bb9496c6f7e853e5d4edd5397c9d45f1968d623c (done by Julia Lawall <[email protected]>) 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 <[email protected]>

---
?drivers/usb/gadget/epautoconf.c | ? ?2 +-

Applied to u-boot-usb/cdc branch.
Thanks.

Remy