Technical content of this one seems fine, but there are lots of
Documentation/CodingStyle issues level to resolve.
> patch 1: This patch slightly refactors isoch stream cleanup such that
> stream state is more persistent; it is instantiated at first transfer
> and not released until endpoint shutdown. This is to give isoch transfers
> something persistent to associate with bandwidth budget reservations
> later.
Umm, and it also reworks ehci_endpoint_disable() for all endpoint types.
Which I actually like, but it came as a big surprise given that comment.
If I were nitpicky I'd ask you to split this in two; but I won't. :)
I'm thinking what I'd rather see is a re-issue of this patch, rather than
a separate patch to fix issues here. That way, what goes upstream will
be appropriately clean/correct.
See below for comments.
- Dave
> +#define EHCI_EP_QH(x) (((x)->desc.bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
> \
> + != USB_ENDPOINT_XFER_ISOC ? (x)->hcpriv : NULL)
> +#define EHCI_EP_STREAM(x) (((x)->desc.bmAttributes & \
> + USB_ENDPOINT_XFERTYPE_MASK) == \
> + USB_ENDPOINT_XFER_ISOC ? (x)->hcpriv : NULL)
Hmm, usb_endpoint_xfer_isoc(&x->desc) would look cleaner; I'm not entirely
sure why those predicates aren't inlined. In this context I think you're
probably right to insist on an inlined version, but I would actually rather
see those as inline functions rather than macros.
> +
> +/*-------------------------------------------------------------------------*/
> +
> #ifdef CONFIG_USB_EHCI_ROOT_HUB_TT
>
> /*
> diff -rup a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> --- a/drivers/usb/host/ehci-hcd.c 2006-09-27 20:07:29.000000000 -0400
> +++ b/drivers/usb/host/ehci-hcd.c 2006-09-27 20:59:41.000000000 -0400
> @@ -835,63 +835,80 @@ ehci_endpoint_disable (struct usb_hcd *h
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> unsigned long flags;
> struct ehci_qh *qh, *tmp;
> + struct ehci_iso_stream *iso;
>
> /* ASSERT: any requests/urbs are being unlinked */
> /* ASSERT: nobody can be submitting urbs for this any more */
>
> -rescan:
> spin_lock_irqsave (&ehci->lock, flags);
> - qh = ep->hcpriv;
> - if (!qh)
> - goto done;
> + iso = EHCI_EP_STREAM(ep);
>
> - /* endpoints can be iso streams. for now, we don't
> - * accelerate iso completions ... so spin a while.
> - */
> - if (qh->hw_info1 == 0) {
> - ehci_vdbg (ehci, "iso delay\n");
> - goto idle_timeout;
> - }
> + if (iso){
Use "if (iso) {" instead ... one espace before such brackets.
> + /* for now, we don't accelerate iso completions ... so spin
> + a while. */
> +
"a while" is not as useful as "until the only reference is from udev->ep_*[]".
Comment style should be
/* just one line */
or
/* line 1
* line 2..n-1
*/
... and you should never add end-of-line whitespace.
> + while(iso->refcount>1){
Use "while () {" not "while(){" ... it's not a function call.
In fact,
while (iso->refcount > 1) {
... you're not including whitespace which you _should_ include,
and (elsewhere) are adding whitespace you should not include.
> + spin_unlock_irqrestore (&ehci->lock, flags);
> + schedule_timeout_uninterruptible(1);
> + spin_lock_irqsave (&ehci->lock, flags);
> + }
>
> - if (!HC_IS_RUNNING (hcd->state))
> - qh->qh_state = QH_STATE_IDLE;
> - switch (qh->qh_state) {
> - case QH_STATE_LINKED:
> - for (tmp = ehci->async->qh_next.qh;
> - tmp && tmp != qh;
> - tmp = tmp->qh_next.qh)
> - continue;
> - /* periodic qh self-unlinks on empty */
> - if (!tmp)
> - goto nogood;
> - unlink_async (ehci, qh);
> - /* FALL THROUGH */
> - case QH_STATE_UNLINK: /* wait for hw to finish? */
> - case QH_STATE_UNLINK_WAIT:
> -idle_timeout:
> + /* we want to be sure completions deref the stream to 1,
> + then we finally pull the plug here */
> + iso_stream_put(ehci,iso);
> + ep->hcpriv = NULL;
> spin_unlock_irqrestore (&ehci->lock, flags);
> - schedule_timeout_uninterruptible(1);
> - goto rescan;
> - case QH_STATE_IDLE: /* fully unlinked */
> - if (list_empty (&qh->qtd_list)) {
> - qh_put (qh);
> + return;
> + }
> +
> + while ( (qh = EHCI_EP_QH(ep)) ){
> +
Please don't add end-of-line whitespace ... or blank lines right
at the top of code blocks. Also
while ((qh = ...) != NULL) {
Your whitespace usage _really_ needs to obey standard kernel coding style.
> + if (!HC_IS_RUNNING (hcd->state))
> + qh->qh_state = QH_STATE_IDLE;
> + switch (qh->qh_state) {
> + case QH_STATE_LINKED:
> + for (tmp = ehci->async->qh_next.qh;
> + tmp && tmp != qh;
> + tmp = tmp->qh_next.qh)
> + continue;
> +
> + /* periodic qh self-unlinks on empty */
> + if (!tmp) goto error;
if (!tmp)
goto error;
... as it was previously ...
> + unlink_async (ehci, qh);
> + /* FALL THROUGH */
> +
> + case QH_STATE_UNLINK: /* wait for hw to finish? */
> + case QH_STATE_UNLINK_WAIT:
> +
No blank line there please
> + spin_unlock_irqrestore (&ehci->lock, flags);
> + schedule_timeout_uninterruptible(1);
> + spin_lock_irqsave (&ehci->lock, flags);
> break;
> +
> + case QH_STATE_IDLE: /* fully unlinked */
> +
No blank line there please
> + if (list_empty (&qh->qtd_list)) {
> + qh_put (qh);
> + ep->hcpriv = NULL;
> + break;
> + }
> + /* else FALL THROUGH */
> + default:
> + goto error;
> }
> - /* else FALL THROUGH */
> - default:
> -nogood:
> - /* caller was supposed to have unlinked any requests;
> - * that's not our job. just leak this memory.
> - */
> - ehci_err (ehci, "qh %p (#%02x) state %d%s\n",
> - qh, ep->desc.bEndpointAddress, qh->qh_state,
> - list_empty (&qh->qtd_list) ? "" : "(has tds)");
> - break;
> }
> - ep->hcpriv = NULL;
> -done:
> +
> spin_unlock_irqrestore (&ehci->lock, flags);
> return;
> +
> +error:
> + /* caller was supposed to have unlinked any requests;
> + * that's not our job. just leak this memory.
> + */
> + ehci_err (ehci, "qh %p (#%02x) state %d%s\n",
> + qh, ep->desc.bEndpointAddress, qh->qh_state,
> + list_empty (&qh->qtd_list) ? "" : "(has tds)");
> + ep->hcpriv = NULL;
> }
>
> static int ehci_get_frame (struct usb_hcd *hcd)
> diff -rup a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> --- a/drivers/usb/host/ehci-sched.c 2006-09-27 20:07:29.000000000 -0400
> +++ b/drivers/usb/host/ehci-sched.c 2006-09-27 20:59:41.000000000 -0400
> @@ -959,10 +959,14 @@ iso_stream_put(struct ehci_hcd *ehci, st
> {
> stream->refcount--;
>
> - /* free whenever just a dev->ep reference remains.
> - * not like a QH -- no persistent state (toggle, halt)
> - */
> - if (stream->refcount == 1) {
> + /* don't free on last descriptor; free when endpoint disable
> + finally releases last refcount. Although it is technically
> + broken for an endpoint driver to submit its streaming
> + descriptors such that a new one appears after the old one
> + ends, it is only punishing the users to insist on breaking
> + these drivers when it's not necessary to do so. This saves
> + substantial overhead in that case.*/
The comment should be corrected, both for content and style. In this
case I'd suggest
/* if refcount == 1 this is just from udev->ep_{in,out}[];
* we can at least avoid reallocating this stream's memory, and
* the previously budgeted bandwidth may still be available.
*/
> + if (stream->refcount == 0) {
> int is_in;
>
> // BUG_ON (!list_empty(&stream->td_list));
>
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
