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 (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)
@@ -283,7 +336,8 @@ static void postproc_atl_queue(struct is
struct urb *urb;
struct usb_device *udev;
struct ptd *ptd;
- int short_not_ok;
+ int short_packet_received;
+ int status;
u8 cc;
for (ep = isp116x->atl_active; ep; ep = ep->active) {
@@ -293,56 +347,36 @@ static void postproc_atl_queue(struct is
udev = urb->dev;
ptd = &ep->ptd;
cc = PTD_GET_CC(ptd);
- short_not_ok = 1;
- spin_lock(&urb->lock);
+ short_packet_received = 0;
+ status = -EINPROGRESS;
- /* 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");
cc = TD_CC_NOERROR;
- short_not_ok = 0;
+ short_packet_received = 1;
} 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;
+ status = cc_to_error[TD_DATAUNDERRUN];
+ 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
|| cc == TD_DATAOVERRUN)) {
- if (urb->status == -EINPROGRESS)
- urb->status = cc_to_error[cc];
- if (ep->nextpid == USB_PID_ACK)
- ep->nextpid = 0;
- spin_unlock(&urb->lock);
- continue;
+ status = cc_to_error[cc];
+ goto done;
}
/* According to usb spec, zero-length Int transfer signals
finishing of the urb. Hey, does this apply only
for IN endpoints? */
if (usb_pipeint(urb->pipe) && !PTD_GET_LEN(ptd)) {
- if (urb->status == -EINPROGRESS)
- urb->status = 0;
- spin_unlock(&urb->lock);
- continue;
+ status = 0;
+ goto done;
}
/* Relax after previously failed, but later succeeded
@@ -367,22 +401,21 @@ static void postproc_atl_queue(struct is
if (PTD_GET_ACTIVE(ptd)
|| (cc != TD_CC_NOERROR && cc < 0x0E))
break;
- if (urb->transfer_buffer_length != urb->actual_length) {
- if (short_not_ok)
- break;
- } else {
- if (urb->transfer_flags & URB_ZERO_PACKET
- && ep->nextpid == USB_PID_OUT
- && !(PTD_GET_COUNT(ptd) % ep->maxpacket)) {
- DBG("Zero packet requested\n");
- break;
- }
+ if (urb->transfer_buffer_length != urb->actual_length
+ && !short_packet_received)
+ break; /* Still in progress */
+ if (usb_pipecontrol(urb->pipe)) {
+ ep->nextpid = USB_PID_ACK;
+ break;
+ }
+ if (urb->transfer_flags & URB_ZERO_PACKET
+ && ep->nextpid == USB_PID_OUT
+ && !(PTD_GET_COUNT(ptd) % ep->maxpacket)) {
+ DBG("Zero packet requested\n");
+ break;
}
/* All data for this URB is transferred, let's finish */
- if (usb_pipecontrol(urb->pipe))
- ep->nextpid = USB_PID_ACK;
- else if (urb->status == -EINPROGRESS)
- urb->status = 0;
+ status = 0;
break;
case USB_PID_SETUP:
if (PTD_GET_ACTIVE(ptd)
@@ -402,69 +435,22 @@ static void postproc_atl_queue(struct is
if (PTD_GET_ACTIVE(ptd)
|| (cc != TD_CC_NOERROR && cc < 0x0E))
break;
- if (urb->status == -EINPROGRESS)
- urb->status = 0;
+ status = 0;
ep->nextpid = 0;
break;
default:
BUG();
}
- spin_unlock(&urb->lock);
- }
-}
-
-/*
- 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;
+ done:
+ if (status != -EINPROGRESS) {
+ spin_lock(&urb->lock);
+ if (urb->status == -EINPROGRESS)
+ urb->status = status;
+ spin_unlock(&urb->lock);
+ }
+ if (urb->status != -EINPROGRESS)
+ finish_request(isp116x, ep, urb);
}
}
@@ -570,9 +556,6 @@ static void start_atl_transfers(struct i
*/
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 +565,6 @@ static void finish_atl_transfers(struct
atomic_inc(&isp116x->atl_finishing);
unpack_fifo(isp116x);
postproc_atl_queue(isp116x);
- for (ep = isp116x->atl_active; ep; ep = ep->active) {
- 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
- && ep->nextpid != USB_PID_ACK)
- finish_request(isp116x, ep, urb);
- }
atomic_dec(&isp116x->atl_finishing);
}
@@ -821,15 +794,12 @@ static int isp116x_urb_enqueue(struct us
}
/* in case of unlink-during-submit */
- spin_lock(&urb->lock);
if (urb->status != -EINPROGRESS) {
- spin_unlock(&urb->lock);
finish_request(isp116x, ep, urb);
ret = 0;
goto fail;
}
urb->hcpriv = hep;
- spin_unlock(&urb->lock);
start_atl_transfers(isp116x);
fail:
-------------------------------------------------------------------------
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
