Skip to contentSkip to conversation

PATCH: usb-uhci: interrupt out with urb->interval 0 [linux-usb-devel]

DISCUSSION Pete Zaitcev 2 participants 5 messages Apr 29, 2003 – May 21, 2003 1 message not in the archive USB

lore
Share & tools
lore

Pete Zaitcev

author
#1 lore raw
In reply to a message not in the archive
Quoted 11 lines

From: Frode Isaksen <[email protected]>
Date: Wed, 16 Apr 2003 17:44:55 +0200

A recent change (2.4.21) in the usb-uhci driver calls "uhci_clean_iso_step2" after the completion of one-shot (urb->interval
0) interrupt out transfers. This call clears the list of descriptors. However, it crashes when trying to get the next desciptor in the "for" loop in the "process_interrupt" function,

--- drivers/usb/usb-uhci.c.orig 2003-04-16 15:39:04.000000000 +0200

Review line 2633

Quoted 6 lines
(quoted diff) +1 -0
@@ -2628,6 +2628,7 @@
 				// correct toggle after unlink
 				usb_dotoggle (urb->dev, usb_pipeendpoint 
 				(urb->pipe), usb_pipeout (urb->pipe));
 				clr_td_ioc(desc); // inactivate TD
+				break;
 			}

Pretty obvious, but I'd rather see something like the attached patch instead. The original code was obviously a case of a rash copy-paste, complete with an unused "int i".

Is there a more interesting/popular/widespread device than Lego Tower which uses these oneshots?

-- Pete

drivers/usb/host/usb-uhci.c +4 -4
diff -urN -X dontdiff linux-2.4.21-rc1/drivers/usb/host/usb-uhci.c linux-2.4.21-rc1-nip/drivers/usb/host/usb-uhci.c
@@ -2430,9 +2430,9 @@
 
 _static int process_interrupt (uhci_t *s, struct urb *urb)
 {
-	int i, ret = -EINPROGRESS;
+	int ret = -EINPROGRESS;
 	urb_priv_t *urb_priv = urb->hcpriv;
-	struct list_head *p = urb_priv->desc_list.next;
+	struct list_head *p;
 	uhci_desc_t *desc = list_entry (urb_priv->desc_list.prev, uhci_desc_t, desc_list);
 
 	int actual_length;

@@ -2440,8 +2440,8 @@
 
 	//dbg("urb contains interrupt request");
 
-	for (i = 0; p != &urb_priv->desc_list; p = p->next, i++)	// Maybe we allow more than one TD later ;-)
-	{
+	// Maybe we allow more than one TD later ;-)
+	while ((p = urb_priv->desc_list.next) != &urb_priv->desc_list) {
 		desc = list_entry (p, uhci_desc_t, desc_list);
 
 		if (is_td_active(desc)) {
#2 lore raw
Quoted 3 lines · Pete Zaitcev

Pretty obvious, but I'd rather see something like the attached patch instead. The original code was obviously a case of a rash copy-paste, complete with an unused "int i".

I will test your patch - my patch was the quickest way to fix the problem, but it was obviously not very elegant !!!!

Quoted 3 lines

Is there a more interesting/popular/widespread device than Lego Tower which uses these oneshots?

I am using it in an USB adsl adapter (unicorn - ST70137 chip from STMicro).

Hilsen,
Frode

Quoted 2 lines

-- Pete

#3 lore raw
In reply to Pete Zaitcev
Quoted 4 lines

Is there a more interesting/popular/widespread device than Lego Tower which uses these oneshots?

I have tested the patch with the Unicorn (ST70137) adsl usb chip and it is ok. Thanks... Do you think it will be in the official 2.4.21 kernel ?

Frode

author
#4 lore raw
Quoted 7 lines

Date: Wed, 30 Apr 2003 14:38:58 +0200
From: Frode Isaksen <[email protected]>

I have tested the patch with the Unicorn (ST70137) adsl usb chip and it is ok. Thanks...

Do you think it will be in the official 2.4.21 kernel ?

This is a question to Greg Kroah. He wanted to put brakes on it due to insufficient testing. Personally, I do not agree. I think we should put it into -rc2 and see if anything breaks (which is unlikely in this case). But ultimately it's his call.

-- Pete

#5 lore raw
Quoted 6 lines · Pete Zaitcev
Quoted 5 lines

Date: Wed, 30 Apr 2003 14:38:58 +0200
From: Frode Isaksen <[email protected]>

I have tested the patch with the Unicorn (ST70137) adsl usb chip and it is ok. Thanks...

Quoted 6 lines
Quoted 1 line

Do you think it will be in the official 2.4.21 kernel ?

This is a question to Greg Kroah. He wanted to put brakes on it due to insufficient testing. Personally, I do not agree. I think we should put it into -rc2 and see if anything breaks (which is unlikely in this case). But ultimately it's his call.

I just downloaded 2.4.21-rc2, and the patch is not in...
Is it any hope for it to be in the official 2.4.21 kernel at all ???
For your information, my router with an adsl usb modem has run with this patch for weeks now.

Thanks,

Frode

Quoted 4 lines

-- Pete

drivers/usb/host/usb-uhci.c +2 -2
diff -urN -X dontdiff linux-2.4.21-rc1/drivers/usb/host/usb-uhci.c linux-2.4.21-rc1-nip/drivers/usb/host/usb-uhci.c
@@ -2430,9 +2430,9 @@
 
 _static int process_interrupt (uhci_t *s, struct urb *urb)
 {
-	int i, ret = -EINPROGRESS;
+	int ret = -EINPROGRESS;
 	urb_priv_t *urb_priv = urb->hcpriv;
-	struct list_head *p = urb_priv->desc_list.next;
+	struct list_head *p;
 	uhci_desc_t *desc = list_entry (urb_priv->desc_list.prev,

uhci_desc_t, desc_list);

int actual_length;
@@ -2440,8 +2440,8 @@

//dbg("urb contains interrupt request");

- for (i = 0; p != &urb_priv->desc_list; p = p->next, i++) // Maybe we allow more than one TD later ;-)
- {
+ // Maybe we allow more than one TD later ;-)
+ while ((p = urb_priv->desc_list.next) != &urb_priv->desc_list) {
desc = list_entry (p, uhci_desc_t, desc_list);

if (is_td_active(desc)) {