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

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

[Pete Zaitcev]

> From: Frode Isaksen <fisaksen@bewan.com>
> 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
> @@ -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

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
--- linux-2.4.21-rc1/drivers/usb/host/usb-uhci.c	2003-04-24 10:52:56.000000000 -0700
+++ linux-2.4.21-rc1-nip/drivers/usb/host/usb-uhci.c	2003-04-29 12:43:28.000000000 -0700
@@ -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)) {

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

[Frode Isaksen]

Le mardi, 29 avr 2003, à 21:58 Europe/Paris, Pete Zaitcev a écrit :

> 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  !!!!

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

>
> -- Pete

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

[Frode Isaksen]

> 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

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

[Pete Zaitcev]

> Date: Wed, 30 Apr 2003 14:38:58 +0200
> From: Frode Isaksen <fisaksen@bewan.com>

> 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

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

[Frode Isaksen]

Le jeudi, 1 mai 2003, à 00:00 Europe/Paris, Pete Zaitcev a écrit :

>> Date: Wed, 30 Apr 2003 14:38:58 +0200
>> From: Frode Isaksen <fisaksen@bewan.com>
>
>> 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.

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

>
> -- Pete
>
>


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
--- linux-2.4.21-rc1/drivers/usb/host/usb-uhci.c	2003-04-24 
10:52:56.000000000 -0700
+++ linux-2.4.21-rc1-nip/drivers/usb/host/usb-uhci.c	2003-04-29 
12:43:28.000000000 -0700
@@ -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)) {