Skip to contentSkip to conversation

USB: musb_udc: Bugfix musb_peri_rx_ep

PATCH New Tom Rini 3 participants 5 messages Sep 13, 2012 – Sep 14, 2012

Subsystems (2)
lore
Share & tools
lore

Tom Rini

author
#0 lore raw

PATCH 1/1 USB: musb_udc: Bugfix musb_peri_rx_ep

From: Pankaj Bharadiya <[email protected]>

The endpoint rx count register value will be zero if it is read before receive packet ready bit (PERI_RXCSR:RXPKTRDY) is set.

Check for the receive packet ready bit (PERI_RXCSR:RXPKTRDY) before reading endpoint rx count register. Proceed with rx count read and FIFO read only if RXPKTRDY bit is set.

As this makes the function fit less-well within 80 columns, use __func__ in some debug statements rather than __PRETTY_FUNCTION__ as they are identical for C programs.

---
 drivers/usb/musb/musb_udc.c |   97 +++++++++++++++++++++++--------------------
 1 file changed, 52 insertions(+), 45 deletions(-)
drivers/usb/musb/musb_udc.c +52 -45
index 09cdec3..7180de8 100644
@@ -640,58 +640,65 @@ static void musb_peri_ep0(void)
 
 static void musb_peri_rx_ep(unsigned int ep)
 {
-	u16 peri_rxcount = readw(&musbr->ep[ep].epN.rxcount);
-
-	if (peri_rxcount) {
-		struct usb_endpoint_instance *endpoint;
-		u32 length;
-		u8 *data;
-
-		endpoint = GET_ENDPOINT(udc_device, ep);
-		if (endpoint && endpoint->rcv_urb) {
-			struct urb *urb = endpoint->rcv_urb;
-			unsigned int remaining_space = urb->buffer_length -
-				urb->actual_length;
-
-			if (remaining_space) {
-				int urb_bad = 0; /* urb is good */
-
-				if (peri_rxcount > remaining_space)
-					length = remaining_space;
-				else
-					length = peri_rxcount;
-
-				data = (u8 *) urb->buffer_data;
-				data += urb->actual_length;
-
-				/* The common musb fifo reader */
-				read_fifo(ep, length, data);
-
-				musb_peri_rx_ack(ep);
-
-				/*
-				 * urb's actual_length is updated in
-				 * usbd_rcv_complete
-				 */
-				usbd_rcv_complete(endpoint, length, urb_bad);
+	u8 peri_rxcsr = readw(&musbr->ep[ep].epN.rxcsr);
+	if ((peri_rxcsr & MUSB_RXCSR_RXPKTRDY)) {
+		u16 peri_rxcount = readw(&musbr->ep[ep].epN.rxcount);
+		if (peri_rxcount) {
+			struct usb_endpoint_instance *endpoint;
+			u32 length;
+			u8 *data;
 
+			endpoint = GET_ENDPOINT(udc_device, ep);
+			if (endpoint && endpoint->rcv_urb) {
+				struct urb *urb = endpoint->rcv_urb;
+				unsigned int remaining_space =
+					urb->buffer_length -
+					urb->actual_length;
+
+				if (remaining_space) {
+					int urb_bad = 0; /* urb is good */
+
+					if (peri_rxcount > remaining_space)
+						length = remaining_space;
+					else
+						length = peri_rxcount;
+
+					data = (u8 *) urb->buffer_data;
+					data += urb->actual_length;
+
+					/* The common musb fifo reader */
+					read_fifo(ep, length, data);
+
+					musb_peri_rx_ack(ep);
+
+					/*
+					 * urb's actual_length is updated in
+					 * usbd_rcv_complete
+					 */
+					usbd_rcv_complete(endpoint, length,
+							urb_bad);
+
+				} else {
+					if (debug_level > 0)
+						serial_printf("ERROR : %s %d"
+								" no space "
+								"in rcv "
+								"buffer\n",
+								__func__,
+								ep);
+				}
 			} else {
 				if (debug_level > 0)
-					serial_printf("ERROR : %s %d no space "
-						      "in rcv buffer\n",
-						      __PRETTY_FUNCTION__, ep);
+					serial_printf("ERROR : %s %d problem "
+							"with endpoint\n",
+							__func__, ep);
+
 			}
 		} else {
 			if (debug_level > 0)
-				serial_printf("ERROR : %s %d problem with "
-					      "endpoint\n",
-					      __PRETTY_FUNCTION__, ep);
+				serial_printf("ERROR : %s %d with nothing "
+					"to do\n", __func__, ep);
 		}
-
-	} else {
-		if (debug_level > 0)
-			serial_printf("ERROR : %s %d with nothing to do\n",
-				      __PRETTY_FUNCTION__, ep);
 	}
 }
 
Signature · 1 line
1.7.9.5
#1 lore raw

PATCH 1/1 USB: musb_udc: Bugfix musb_peri_rx_ep

drivers/usb/musb/musb_udc.c static void musb_peri_ep0(void) · line 689

Quoted 9 lines · On Thu, 13 Sep 2012 10:26:31 -0700 Tom Rini <[email protected]> wrote:
drivers/usb/musb/musb_udc.c +9 -0
+				} else {
+					if (debug_level > 0)
+						serial_printf("ERROR : %s %d"
+								" no space "
+								"in rcv "
+								"buffer\n",
+								__func__,
+								ep);
+				}

that's pretty bad.

strings are allowed to exceed the 80 column limit, for grep purposes.

also, if the conditional logic is reversed, one can regain some of that horizontal space, i.e.:

if (!(peri_rxcsr & MUSB_RXCSR_RXPKTRDY)) {
	if (debug_level > 0)
		serial_printf("ERROR : %s %d with nothing to do\n",
			      __PRETTY_FUNCTION__, ep);
	return;
}
peri_rxcount = readw(&musbr->ep[ep].epN.rxcount);
if (!peri_rxcount)
	if (debug_level > 0) {
		serial_printf("ERROR : %s %d problem with endpoint\n",
			      __PRETTY_FUNCTION__, ep);
		serial_printf("ERROR : %s %d with nothing to do\n",
			      __func__, ep);
	return;
}

...and so on.

Kim

author
#2 lore raw

PATCH 1/1 USB: musb_udc: Bugfix musb_peri_rx_ep

Quoted 14 lines · Kim Phillips

drivers/usb/musb/musb_udc.c static void musb_peri_ep0(void) · line 689

Quoted 9 lines · On Thu, 13 Sep 2012 10:26:31 -0700 Tom Rini <[email protected]> wrote:
drivers/usb/musb/musb_udc.c +9 -0
+				} else {
+					if (debug_level > 0)
+						serial_printf("ERROR : %s %d"
+								" no space "
+								"in rcv "
+								"buffer\n",
+								__func__,
+								ep);
+				}

that's pretty bad.

Yes, it is.

Quoted 1 line

strings are allowed to exceed the 80 column limit, for grep purposes.

Good point, just following existing style to the extreme in this case.

Quoted 2 lines

also, if the conditional logic is reversed, one can regain some of that horizontal space, i.e.:

An execellent point. I'll post v2 which will debug print and return if the bit is not set.

Signature · 1 line
Tom
#3 lore raw

PATCH 1/1 USB: musb_udc: Bugfix musb_peri_rx_ep

In reply to Tom Rini

Dear Tom Rini,

Quoted 5 lines

From: Pankaj Bharadiya <[email protected]>

The endpoint rx count register value will be zero if it is read before receive packet ready bit (PERI_RXCSR:RXPKTRDY) is set.

Quoted 7 lines

Check for the receive packet ready bit (PERI_RXCSR:RXPKTRDY) before reading endpoint rx count register. Proceed with rx count read and FIFO read only if RXPKTRDY bit is set.

As this makes the function fit less-well within 80 columns, use __func__ in some debug statements rather than __PRETTY_FUNCTION__ as they are identical for C programs.

I'd say this is TI stuff, so let's push it through TI tree (both the musb patches please).

Best regards,
Marek Vasut

#4 lore raw

PATCH 1/1 USB: musb_udc: Bugfix musb_peri_rx_ep

In reply to Tom Rini

Dear Tom Rini,

Quoted 54 lines

From: Pankaj Bharadiya <[email protected]>

The endpoint rx count register value will be zero if it is read before receive packet ready bit (PERI_RXCSR:RXPKTRDY) is set.

Check for the receive packet ready bit (PERI_RXCSR:RXPKTRDY) before reading endpoint rx count register. Proceed with rx count read and FIFO read only if RXPKTRDY bit is set.

As this makes the function fit less-well within 80 columns, use __func__ in some debug statements rather than __PRETTY_FUNCTION__ as they are identical for C programs.

---
 drivers/usb/musb/musb_udc.c |   97

+++++++++++++++++++++++-------------------- 1 file changed, 52 insertions(+), 45 deletions(-)

drivers/usb/musb/musb_udc.c +0 -26
index 09cdec3..7180de8 100644
@@ -640,58 +640,65 @@ static void musb_peri_ep0(void)
 
 static void musb_peri_rx_ep(unsigned int ep)
 {
-	u16 peri_rxcount = readw(&musbr->ep[ep].epN.rxcount);
-
-	if (peri_rxcount) {
-		struct usb_endpoint_instance *endpoint;
-		u32 length;
-		u8 *data;
-
-		endpoint = GET_ENDPOINT(udc_device, ep);
-		if (endpoint && endpoint->rcv_urb) {
-			struct urb *urb = endpoint->rcv_urb;
-			unsigned int remaining_space = urb->buffer_length -
-				urb->actual_length;
-
-			if (remaining_space) {
-				int urb_bad = 0; /* urb is good */
-
-				if (peri_rxcount > remaining_space)
-					length = remaining_space;
-				else
-					length = peri_rxcount;
-
-				data = (u8 *) urb->buffer_data;
-				data += urb->actual_length;
-
-				/* The common musb fifo reader */
-				read_fifo(ep, length, data);

drivers/usb/musb/musb_udc.c static void musb_peri_ep0(void) · line 644

Quoted 10 lines
drivers/usb/musb/musb_udc.c +2 -8
-
-				musb_peri_rx_ack(ep);
-
-				/*
-				 * urb's actual_length is updated in
-				 * usbd_rcv_complete
-				 */
-				usbd_rcv_complete(endpoint, length, urb_bad);
+	u8 peri_rxcsr = readw(&musbr->ep[ep].epN.rxcsr);
+	if ((peri_rxcsr & MUSB_RXCSR_RXPKTRDY)) {

if (!cond)
return;

This will cut down one indent below.

Quoted 34 lines
drivers/usb/musb/musb_udc.c +33 -0
+		u16 peri_rxcount = readw(&musbr->ep[ep].epN.rxcount);
+		if (peri_rxcount) {
+			struct usb_endpoint_instance *endpoint;
+			u32 length;
+			u8 *data;
 
+			endpoint = GET_ENDPOINT(udc_device, ep);
+			if (endpoint && endpoint->rcv_urb) {
+				struct urb *urb = endpoint->rcv_urb;
+				unsigned int remaining_space =
+					urb->buffer_length -
+					urb->actual_length;
+
+				if (remaining_space) {
+					int urb_bad = 0; /* urb is good */
+
+					if (peri_rxcount > remaining_space)
+						length = remaining_space;
+					else
+						length = peri_rxcount;
+
+					data = (u8 *) urb->buffer_data;
+					data += urb->actual_length;
+
+					/* The common musb fifo reader */
+					read_fifo(ep, length, data);
+
+					musb_peri_rx_ack(ep);
+
+					/*
+					 * urb's actual_length is updated in
+					 * usbd_rcv_complete
+					 */
+					usbd_rcv_complete(endpoint, length,

drivers/usb/musb/musb_udc.c static void musb_peri_ep0(void) · line 688

Quoted 10 lines
drivers/usb/musb/musb_udc.c +10 -0
+							urb_bad);
+
+				} else {
+					if (debug_level > 0)
+						serial_printf("ERROR : %s %d"
+								" no space "
+								"in rcv "
+								"buffer\n",
+								__func__,
+								ep);

So ... if (error) print error and die.

if (!remaining_space && debug_level)
print and die.

{ rest of the function, as above }

One more indent level down.

Apply common sense to the other else {} and it's work out.

drivers/usb/musb/musb_udc.c static void musb_peri_ep0(void) · line 692

Quoted 7 lines
drivers/usb/musb/musb_udc.c +2 -3
+				}
 			} else {
 				if (debug_level > 0)
-					serial_printf("ERROR : %s %d no space "
-						      "in rcv buffer\n",
-						      __PRETTY_FUNCTION__, ep);
+					serial_printf("ERROR : %s %d problem "

use printf(), serial_printf is flat out forbidden and this is a NACK /!\

Quoted 9 lines
drivers/usb/musb/musb_udc.c +3 -3
+							"with endpoint\n",
+							__func__, ep);
+
 			}
 		} else {
 			if (debug_level > 0)
-				serial_printf("ERROR : %s %d problem with "
-					      "endpoint\n",
-					      __PRETTY_FUNCTION__, ep);

drivers/usb/musb/musb_udc.c static void musb_peri_ep0(void) · line 703

Quoted 10 lines
drivers/usb/musb/musb_udc.c +2 -5
+				serial_printf("ERROR : %s %d with nothing "
+					"to do\n", __func__, ep);
 		}
-
-	} else {
-		if (debug_level > 0)
-			serial_printf("ERROR : %s %d with nothing to do\n",
-				      __PRETTY_FUNCTION__, ep);
 	}
 }

Best regards,
Marek Vasut