Re: [linux-usb-devel] Possible bug in isp116x-hcd

2007-06-26 Thread Alan Stern 
On Tue, 26 Jun 2007, ok wrote:

  I took out the references to urb-lock in preproc_atl_queue and 
  isp116x_urb_enqueue.  They didn't seem to serve any purpose; is that 
  correct?
 
 Seems to be correct. urb-hcpriv is already protected by isp116x-lock.
 Is urb-lock entirely to guard only urb-status? If so, is the urb-lock 
 unnecessary also in preproc_atl_queue()?

That's right.  The only part of an URB that usbcore will change while
the URB is active is the status, so that's the only part protected by
urb-lock.  (Actually usbcore will also change urb-reject, but that
field isn't used by HCDs.)

 Sure this preproc_atl_queue() has suffered from my attitude better not 
 touch it if it works. Good that it can be simplified. Thanks for your 
 review!
 
 Right now I don't have the hardware available. But I am working on it. 
 These changes are big and have to be tested, no doubt. I will let you 
 know.

Maybe the whole thing could be simplified even more.  It's hard to say 
exactly without being familiar with the hardware.  For instance, is it 
possible the multiple maxpacket-sized transactions succeeded and then 
one failed?

I think the overall outline of the logic should look like this:

Find out how much has been successfully transferred and
accumulate the amount.  If the ptd is still active then
exit immediately.

Find out if there was a low-level error (CRC, bit stuffing,
data toggle, stall, no response, invalid PID, buffer 
over/underflow).  Increment the error counter and retry the 
last transaction if there are  3 errors, otherwise fail the 
transfer.

Find out if there was a high-level error (unexpected PID,
data overflow, data underflow + URB_SHORT_NOT_OK).  These
cause the transfer to fail immediately.

If no short packet was received and the requested amount of
data has not yet been transferred, proceed to transfer the
remaining data.

Send a zero-length packet if needed.  You might need an extra
flag in the endpoint structure to indicate whether the ZLP
has already been sent.

Do the status transaction for control transfers.

What I ended up with looked a lot like this, but not exactly the same.  
It probably still needs some cleaning up.

Alan Stern


-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] Possible bug in isp116x-hcd

2007-06-25 Thread ok 
On Sat, Jun 23, 2007 at 08:25:55PM -0400, Alan Stern wrote:
 Below is a patch based on your work.  There's a number of differences, 
 plus some things I left as-is but which look suspicious.

Thanks for the review. 

You touched the part of the code that took the lognest time to 
stabilize. It took me time to understand it again.

 I took out the references to urb-lock in preproc_atl_queue and 
 isp116x_urb_enqueue.  They didn't seem to serve any purpose; is that 
 correct?

Seems to be correct. urb-hcpriv is already protected by isp116x-lock.
Is urb-lock entirely to guard only urb-status? If so, is the urb-lock 
unnecessary also in preproc_atl_queue()?

 All the other updates are in postproc_atl_queue:
 
 In the revised code, short_not_ok doesn't serve a clear purpose.  I 
 changed it to short_packet_received; now it works somewhat differently 
 and it should be easier to understand.
 
 There's a new local variable called status which indicates whether 
 the URB is completed and holds the final status value.  The code at the 
 end of the big loop uses the variable to set urb-status.

I anticipated that :)

 There's a comment with associated code:
 
   /* According to usb spec, zero-length Int transfer signals
  finishing of the urb. Hey, does this apply only
  for IN endpoints? */
 
 This doesn't make any sense to me.  Firstly, zero-length packets signal
 the end of any Control-, Bulk-, or Int-IN transfer; there's nothing
 special about Int.  Secondly, with OUT transfers the only way you can
 get a zero-length packet is if it is the last packet of the transfer
 anyway; all the packets before the last must be of maximum length.  I
 left that stuff in but it probably should be removed.

I remember the purpose of explicitly handling Int transfers at that 
point was to keep the following code simpler. Maybe it did some time in 
the past, but now it seems to be unnecessary indeed.

The question in the comment was about Int-OUT transfers.

 In this section:
 
   /* Relax after previously failed, but later succeeded
  or correctly NAK'ed retransmission attempt */
   if (ep-error_count
(cc == TD_CC_NOERROR || cc == TD_NOTACCESSED))
   ep-error_count = 0;
 
 there's no need to test ep-error_count; it's easier just to set it to
 0.

OK

 In the big switch (ep-nextpid) block, I think it would be better to 
 factor this test:
 
   if (PTD_GET_ACTIVE(ptd)
   || (cc != TD_CC_NOERROR  cc  0x0E))
   break;
 
 out of the individual cases.  On the other hand, I'm not entirely sure
 what the test is intended for.  Does it have something to do with 
 retrying a failed transaction?

PTD_GET_ACTIVE(ptd) == 1 means that the transfer got only NAKed during 
the frame. The rest of the check is about retrying on soft errors.

 The USB_PID_IN/USB_PID OUT case got changed considerably.  I think the 
 logic is pretty much the same as it was before, but it makes more sense 
 to me like this.
 
 This test
 
   if (urb-transfer_flags  URB_ZERO_PACKET
ep-nextpid == USB_PID_OUT
!(PTD_GET_COUNT(ptd) % ep-maxpacket)) {
   DBG(Zero packet requested\n);
 
 seems wrong.  Wouldn't it get stuck in an infinite loop?  I think the 
 last part of the test ought to be more like:
 
PTD_GET_COUNT(ptd) == ep-maxpacket
 
 except that wouldn't be quite right when the zero-length packet has to 
 be retried.

Good question. It did not hang in my tests. But I don't understand, why. 
Tests will show.

PTD_GET_COUNT(ptd) can be multiple of ep-maxpacket.

 In the USB_PID_SETUP case, this test
 
   if (urb-transfer_buffer_length == urb-actual_length)
   ep-nextpid = USB_PID_ACK;
 
 might be a little clearer if it was changed to
 
   if (urb-transfer_buffer_length == 0)
   ep-nextpid = USB_PID_ACK;
 
 Overall, I'm not sure that the toggle setting is correct in all the
 possible pathways.  There were so many changes, it's hard to track
 them.  In particular, I don't see where the toggle gets set to 1 for
 the status stage of a control transfer.

Toggle for status stage is set in preproc_atl_queue().

 Anyway, I can't check this as carefully as you can.  Let me know what 
 you think.

Sure this preproc_atl_queue() has suffered from my attitude better not 
touch it if it works. Good that it can be simplified. Thanks for your 
review!

Right now I don't have the hardware available. But I am working on it. 
These changes are big and have to be tested, no doubt. I will let you 
know.

Olav


-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of

Re: [linux-usb-devel] Possible bug in isp116x-hcd

2007-06-23 Thread Alan Stern 
On Sat, 23 Jun 2007, ok wrote:

  If you like, I'll try revising your patch.  You'll have to test it 
 
 Yes, thanks.
 
  though; I don't have the hardware.

Olav:

Below is a patch based on your work.  There's a number of differences, 
plus some things I left as-is but which look suspicious.

I took out the references to urb-lock in preproc_atl_queue and 
isp116x_urb_enqueue.  They didn't seem to serve any purpose; is that 
correct?

All the other updates are in postproc_atl_queue:

In the revised code, short_not_ok doesn't serve a clear purpose.  I 
changed it to short_packet_received; now it works somewhat differently 
and it should be easier to understand.

There's a new local variable called status which indicates whether 
the URB is completed and holds the final status value.  The code at the 
end of the big loop uses the variable to set urb-status.

There's a comment with associated code:

/* According to usb spec, zero-length Int transfer signals
   finishing of the urb. Hey, does this apply only
   for IN endpoints? */

This doesn't make any sense to me.  Firstly, zero-length packets signal
the end of any Control-, Bulk-, or Int-IN transfer; there's nothing
special about Int.  Secondly, with OUT transfers the only way you can
get a zero-length packet is if it is the last packet of the transfer
anyway; all the packets before the last must be of maximum length.  I
left that stuff in but it probably should be removed.

In this section:

/* Relax after previously failed, but later succeeded
   or correctly NAK'ed retransmission attempt */
if (ep-error_count
 (cc == TD_CC_NOERROR || cc == TD_NOTACCESSED))
ep-error_count = 0;

there's no need to test ep-error_count; it's easier just to set it to
0.

In the big switch (ep-nextpid) block, I think it would be better to 
factor this test:

if (PTD_GET_ACTIVE(ptd)
|| (cc != TD_CC_NOERROR  cc  0x0E))
break;

out of the individual cases.  On the other hand, I'm not entirely sure
what the test is intended for.  Does it have something to do with 
retrying a failed transaction?

The USB_PID_IN/USB_PID OUT case got changed considerably.  I think the 
logic is pretty much the same as it was before, but it makes more sense 
to me like this.

This test

if (urb-transfer_flags  URB_ZERO_PACKET
 ep-nextpid == USB_PID_OUT
 !(PTD_GET_COUNT(ptd) % ep-maxpacket)) {
DBG(Zero packet requested\n);

seems wrong.  Wouldn't it get stuck in an infinite loop?  I think the 
last part of the test ought to be more like:

 PTD_GET_COUNT(ptd) == ep-maxpacket

except that wouldn't be quite right when the zero-length packet has to 
be retried.

In the USB_PID_SETUP case, this test

if (urb-transfer_buffer_length == urb-actual_length)
ep-nextpid = USB_PID_ACK;

might be a little clearer if it was changed to

if (urb-transfer_buffer_length == 0)
ep-nextpid = USB_PID_ACK;

Overall, I'm not sure that the toggle setting is correct in all the
possible pathways.  There were so many changes, it's hard to track
them.  In particular, I don't see where the toggle gets set to 1 for
the status stage of a control transfer.

Anyway, I can't check this as carefully as you can.  Let me know what 
you think.

Alan Stern



Index: usb-2.6/drivers/usb/host/isp116x-hcd.c
===
--- usb-2.6.orig/drivers/usb/host/isp116x-hcd.c
+++ usb-2.6/drivers/usb/host/isp116x-hcd.c
@@ -228,7 +228,6 @@ static void preproc_atl_queue(struct isp
   struct urb, urb_list);
ptd = ep-ptd;
len = ep-length;
-   spin_lock(urb-lock);
ep-data = (unsigned char *)urb-transfer_buffer
+ urb-actual_length;
 
@@ -264,7 +263,6 @@ static void preproc_atl_queue(struct isp
| PTD_EP(ep-epnum);
ptd-len = PTD_LEN(len) | PTD_DIR(dir);
ptd-faddr = PTD_FA(usb_pipedevice(urb-pipe));
-   spin_unlock(urb-lock);
if (!ep-active) {
ptd-mps |= PTD_LAST_MSK;
isp116x-atl_last_dir = dir;
@@ -275,6 +273,61 @@ static void preproc_atl_queue(struct isp
 }
 
 /*
+  Take done or failed requests out of schedule. Give back
+  processed urbs.
+*/
+static void finish_request(struct isp116x *isp116x, struct isp116x_ep *ep,
+  struct urb *urb)
+__releases(isp116x-lock) __acquires(isp116x-lock)
+{
+   unsigned i;
+
+   urb-hcpriv = NULL;
+   ep-error_count = 0;
+
+   if

Re: [linux-usb-devel] Possible bug in isp116x-hcd

2007-06-22 Thread ok 
On Thu, Jun 21, 2007 at 05:17:42PM -0400, Alan Stern wrote:
 On Thu, 21 Jun 2007, ok wrote:
  Do you mean something like that below? I haven't tested it.
 
 Yes, something very much like this.  It could be simplified a little;  
 in a couple of places where you have
 
   finish_request();
   spin_unlock();
   continue;
 
 you ought to be able to use a goto or break instead.  Also there
 are a few minor problems, like calling finish_request before it is
 declared and calling it before releasing urb-lock, but those can 
 easily be fixed.

Yes, I made a mistake with urb-lock. OK, moved finish_request 
unmodified up in the file, got fatter patch. The one below.

 If you like, I'll try revising your patch.  You'll have to test it 

Yes, thanks.

 though; I don't have the hardware.

Sure.

Olav


--- linux-2.6.22-rc5-or/drivers/usb/host/isp116x-hcd.c  2007-06-17 
05:09:12.0 +0300
+++ linux-2.6.22-rc5-isp/drivers/usb/host/isp116x-hcd.c 2007-06-23 
00:22:01.0 +0300
@@ -275,6 +275,61 @@
 }
 
 /*
+  Take done or failed requests out of schedule. Give back
+  processed urbs.
+*/
+static void finish_request(struct isp116x *isp116x, struct isp116x_ep *ep,
+  struct urb *urb)
+__releases(isp116x-lock) __acquires(isp116x-lock)
+{
+   unsigned i;
+
+   urb-hcpriv = NULL;
+   ep-error_count = 0;
+
+   if (usb_pipecontrol(urb-pipe))
+   ep-nextpid = USB_PID_SETUP;
+
+   urb_dbg(urb, Finish);
+
+   spin_unlock(isp116x-lock);
+   usb_hcd_giveback_urb(isp116x_to_hcd(isp116x), urb);
+   spin_lock(isp116x-lock);
+
+   /* take idle endpoints out of the schedule */
+   if (!list_empty(ep-hep-urb_list))
+   return;
+
+   /* async deschedule */
+   if (!list_empty(ep-schedule)) {
+   list_del_init(ep-schedule);
+   return;
+   }
+
+   /* periodic deschedule */
+   DBG(deschedule qh%d/%p branch %d\n, ep-period, ep, ep-branch);
+   for (i = ep-branch; i  PERIODIC_SIZE; i += ep-period) {
+   struct isp116x_ep *temp;
+   struct isp116x_ep **prev = isp116x-periodic[i];
+
+   while (*prev  ((temp = *prev) != ep))
+   prev = temp-next;
+   if (*prev)
+   *prev = ep-next;
+   isp116x-load[i] -= ep-load;
+   }
+   ep-branch = PERIODIC_SIZE;
+   isp116x_to_hcd(isp116x)-self.bandwidth_allocated -=
+   ep-load / ep-period;
+
+   /* switch irq type? */
+   if (!--isp116x-periodic_count) {
+   isp116x-irqenb = ~HCuPINT_SOF;
+   isp116x-irqenb |= HCuPINT_ATL;
+   }
+}
+
+/*
   Analyze transfer results, handle partial transfers and errors
 */
 static void postproc_atl_queue(struct isp116x *isp116x)
@@ -296,11 +351,6 @@
short_not_ok = 1;
spin_lock(urb-lock);
 
-   /* Data underrun is special. For allowed underrun
-  we clear the error and continue as normal. For
-  forbidden underrun we finish the DATA stage
-  immediately while for control transfer,
-  we do a STATUS stage. */
if (cc == TD_DATAUNDERRUN) {
if (!(urb-transfer_flags  URB_SHORT_NOT_OK)) {
DBG(Allowed data underrun\n);
@@ -308,22 +358,14 @@
short_not_ok = 0;
} else {
ep-error_count = 1;
-   if (usb_pipecontrol(urb-pipe))
-   ep-nextpid = USB_PID_ACK;
-   else
-   usb_settoggle(udev, ep-epnum,
- ep-nextpid ==
- USB_PID_OUT,
- PTD_GET_TOGGLE(ptd));
+   usb_settoggle(udev, ep-epnum,
+ ep-nextpid == USB_PID_OUT,
+ PTD_GET_TOGGLE(ptd));
urb-actual_length += PTD_GET_COUNT(ptd);
urb-status = cc_to_error[TD_DATAUNDERRUN];
-   spin_unlock(urb-lock);
-   continue;
+   goto done;
}
}
-   /* Keep underrun error through the STATUS stage */
-   if (urb-status == cc_to_error[TD_DATAUNDERRUN])
-   cc = TD_DATAUNDERRUN;
 
if (cc != TD_CC_NOERROR  cc != TD_NOTACCESSED
 (++ep-error_count = 3 || cc == TD_CC_STALL
@@ -332,8 +374,7 @@
urb-status = cc_to_error[cc];
if (ep-nextpid == USB_PID_ACK)

Re: [linux-usb-devel] Possible bug in isp116x-hcd

2007-06-21 Thread Alan Stern 
On Thu, 21 Jun 2007, ok wrote:

 Hi Alan,
 
 Do you mean something like that below? I haven't tested it.

Yes, something very much like this.  It could be simplified a little;  
in a couple of places where you have

finish_request();
spin_unlock();
continue;

you ought to be able to use a goto or break instead.  Also there
are a few minor problems, like calling finish_request before it is
declared and calling it before releasing urb-lock, but those can 
easily be fixed.

If you like, I'll try revising your patch.  You'll have to test it 
though; I don't have the hardware.

Alan Stern


-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] Possible bug in isp116x-hcd

2007-06-20 Thread ok 
On Mon, Jun 18, 2007 at 11:00:10AM -0400, Alan Stern wrote:
 Olav,
 
 What would be involved in refactoring isp116x-hcd so that the driver
 called usb_hcd_giveback_urb() as soon as the URB was completed?  If I
 understand the current code correctly, postproc_atl_queue() goes
 through all the active endpoints and stores the status for the
 completed URBs, and then later finish_atl_transfers() gives them back.
 
 It looks like this shouldn't present any difficulty except for one 
 thing: When you get a short Control-IN transfer with URB_SHORT_NOT_OK 
 set, the error status gets stored while the URB's status stage 
 executes.  That would have to be changed so that the status stage was 
 skipped entirely.  Doing this shouldn't cause any problems in practice; 
 the other HCDs skip the status stage when this sort of error occurs.
 
 The point of making this change is that it would allow the driver to 
 avoid saving values in urb-status, leading eventually to the 
 possibility of removing the status field from struct urb.

Hi Alan,

Do you mean something like that below? I haven't tested it.

Olav


--- linux-2.6.22-rc5-or/drivers/usb/host/isp116x-hcd.c  2007-06-17 
05:09:12.0 +0300
+++ linux-2.6.22-rc5-isp/drivers/usb/host/isp116x-hcd.c 2007-06-20 
22:49:27.0 +0300
@@ -210,6 +210,9 @@
 
 /*---*/
 
+static void finish_request(struct isp116x *isp116x, struct isp116x_ep *ep,
+  struct urb *urb);
+
 /*
   Set up PTD's.
 */
@@ -296,11 +299,6 @@
short_not_ok = 1;
spin_lock(urb-lock);
 
-   /* Data underrun is special. For allowed underrun
-  we clear the error and continue as normal. For
-  forbidden underrun we finish the DATA stage
-  immediately while for control transfer,
-  we do a STATUS stage. */
if (cc == TD_DATAUNDERRUN) {
if (!(urb-transfer_flags  URB_SHORT_NOT_OK)) {
DBG(Allowed data underrun\n);
@@ -308,22 +306,16 @@
short_not_ok = 0;
} else {
ep-error_count = 1;
-   if (usb_pipecontrol(urb-pipe))
-   ep-nextpid = USB_PID_ACK;
-   else
-   usb_settoggle(udev, ep-epnum,
- ep-nextpid ==
- USB_PID_OUT,
- PTD_GET_TOGGLE(ptd));
+   usb_settoggle(udev, ep-epnum,
+ ep-nextpid == USB_PID_OUT,
+ PTD_GET_TOGGLE(ptd));
urb-actual_length += PTD_GET_COUNT(ptd);
urb-status = cc_to_error[TD_DATAUNDERRUN];
+   finish_request(isp116x, ep, urb);
spin_unlock(urb-lock);
continue;
}
}
-   /* Keep underrun error through the STATUS stage */
-   if (urb-status == cc_to_error[TD_DATAUNDERRUN])
-   cc = TD_DATAUNDERRUN;
 
if (cc != TD_CC_NOERROR  cc != TD_NOTACCESSED
 (++ep-error_count = 3 || cc == TD_CC_STALL
@@ -332,6 +324,7 @@
urb-status = cc_to_error[cc];
if (ep-nextpid == USB_PID_ACK)
ep-nextpid = 0;
+   finish_request(isp116x, ep, urb);
spin_unlock(urb-lock);
continue;
}
@@ -341,6 +334,7 @@
if (usb_pipeint(urb-pipe)  !PTD_GET_LEN(ptd)) {
if (urb-status == -EINPROGRESS)
urb-status = 0;
+   finish_request(isp116x, ep, urb);
spin_unlock(urb-lock);
continue;
}
@@ -409,6 +403,8 @@
default:
BUG();
}
+   if (urb-status != -EINPROGRESS)
+   finish_request(isp116x, ep, urb);
spin_unlock(urb-lock);
}
 }
@@ -570,9 +566,6 @@
 */
 static void finish_atl_transfers(struct isp116x *isp116x)
 {
-   struct isp116x_ep *ep;
-   struct urb *urb;
-
if (!isp116x-atl_active)
return;
/* Fifo not ready? */
@@ -582,16 +575,6 @@
atomic_inc(isp116x-atl_finishing);
unpack_fifo(isp116x);
postproc_atl_queue(isp116x);
-   for (ep = isp116x-atl_active; ep; ep = ep-active) {
-   urb =
-

Re: [linux-usb-devel] Possible bug in isp116x-hcd

2007-06-18 Thread Alan Stern 
On Thu, 14 Jun 2007, Olav Kongas wrote:

 Hi Alan,
 
  Index: usb-2.6/drivers/usb/host/isp116x-hcd.c
  ===
  --- usb-2.6.orig/drivers/usb/host/isp116x-hcd.c
  +++ usb-2.6/drivers/usb/host/isp116x-hcd.c
  @@ -583,12 +583,15 @@ static void 
  finish_atl_transfers(struct
 unpack_fifo(isp116x);
 postproc_atl_queue(isp116x);
 
 I think it is guaranteed that at this point all the ep's in 
 active ATL queue have their urb_list non-empty. There's a 
 relevant BUG_ON() in postproc_atl_queue(). The list_empty() 
 check below would be unnecessary.

Olav,

What would be involved in refactoring isp116x-hcd so that the driver
called usb_hcd_giveback_urb() as soon as the URB was completed?  If I
understand the current code correctly, postproc_atl_queue() goes
through all the active endpoints and stores the status for the
completed URBs, and then later finish_atl_transfers() gives them back.

It looks like this shouldn't present any difficulty except for one 
thing: When you get a short Control-IN transfer with URB_SHORT_NOT_OK 
set, the error status gets stored while the URB's status stage 
executes.  That would have to be changed so that the status stage was 
skipped entirely.  Doing this shouldn't cause any problems in practice; 
the other HCDs skip the status stage when this sort of error occurs.

The point of making this change is that it would allow the driver to 
avoid saving values in urb-status, leading eventually to the 
possibility of removing the status field from struct urb.

Alan Stern


-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] Possible bug in isp116x-hcd

2007-06-14 Thread Olav Kongas 
Hi Alan,

 Index: usb-2.6/drivers/usb/host/isp116x-hcd.c
 ===
 --- usb-2.6.orig/drivers/usb/host/isp116x-hcd.c
 +++ usb-2.6/drivers/usb/host/isp116x-hcd.c
 @@ -583,12 +583,15 @@ static void 
 finish_atl_transfers(struct
unpack_fifo(isp116x);
postproc_atl_queue(isp116x);

I think it is guaranteed that at this point all the ep's in 
active ATL queue have their urb_list non-empty. There's a 
relevant BUG_ON() in postproc_atl_queue(). The list_empty() 
check below would be unnecessary.

for (ep = isp116x-atl_active; ep; ep = ep-active) 
 {
 +   if (list_empty(ep-hep-urb_list))
 +   continue;
urb =
container_of(ep-hep-urb_list.next, 
 struct urb, urb_list);
 +
 /* USB_PID_ACK check here avoids finishing of
control transfers, for which TD_DATAUNDERRUN
occured, while URB_SHORT_NOT_OK was set */
 -   if (urb  urb-status != -EINPROGRESS
   ^^^
Yes, this is wrong. Thanks. As you are already at it, please 
remove it.

 +   if (urb-status != -EINPROGRESS
  ep-nextpid != USB_PID_ACK)
 finish_request(isp116x, ep, urb);
 }

Olav

-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Re: [linux-usb-devel] Possible bug in isp116x-hcd

2007-06-14 Thread Alan Stern 
On Thu, 14 Jun 2007, Olav Kongas wrote:

 Hi Alan,
 
  Index: usb-2.6/drivers/usb/host/isp116x-hcd.c
  ===
  --- usb-2.6.orig/drivers/usb/host/isp116x-hcd.c
  +++ usb-2.6/drivers/usb/host/isp116x-hcd.c
  @@ -583,12 +583,15 @@ static void 
  finish_atl_transfers(struct
 unpack_fifo(isp116x);
 postproc_atl_queue(isp116x);
 
 I think it is guaranteed that at this point all the ep's in 
 active ATL queue have their urb_list non-empty. There's a 
 relevant BUG_ON() in postproc_atl_queue(). The list_empty() 
 check below would be unnecessary.

I'll remove the list_empty() test.

 for (ep = isp116x-atl_active; ep; ep = ep-active) 
  {
  +   if (list_empty(ep-hep-urb_list))
  +   continue;
 urb =
 container_of(ep-hep-urb_list.next, 
  struct urb, urb_list);
  +
  /* USB_PID_ACK check here avoids finishing of
 control transfers, for which TD_DATAUNDERRUN
 occured, while URB_SHORT_NOT_OK was set */
  -   if (urb  urb-status != -EINPROGRESS
^^^
 Yes, this is wrong. Thanks. As you are already at it, please 
 remove it.

Will do.  Thanks for the feedback.

Alan Stern


-
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
___
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel