On Wednesday 04 February 2009, Sergei Shtylyov wrote:
> The driver prevents using the same numbered Rx/Tx endpoints simultaneously for
> the periodic transfers -- which would actually be correct unless they share the
> same FIFO. Use 'in_qh' and 'out_qh' fields of the 'struct musb_hw_ep' to check
> the endpoint's business and get rid of now completely useless 'periodic' array
> in the 'struct musb'. While at it, optimize the loop induction variable in the
> endpoint lookup code and remove duplicate/unneeded code elsewhere...
>
> Signed-off-by: Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx>
Your patch comments need improvement. This would better be
titled "rewrite endpoint allocation", and the description
should highlight "make better use of resources". Not using
something that's available is sub-optimal, but rarely a bug.
This doesn't seem to fix anything hurtful; so, it's not
for 2.6.29 merge.
What I'm going to do is send a replacement with a better
description and my signoff.
>
> drivers/usb/musb/musb_core.h | 1 -
> drivers/usb/musb/musb_host.c | 27 ++++++++++-----------------
> 2 files changed, 10 insertions(+), 18 deletions(-)
>
> Index: linux-2.6/drivers/usb/musb/musb_core.h
> ===================================================================
> --- linux-2.6.orig/drivers/usb/musb/musb_core.h
> +++ linux-2.6/drivers/usb/musb/musb_core.h
> @@ -331,7 +331,6 @@ struct musb {
> struct list_head control; /* of musb_qh */
> struct list_head in_bulk; /* of musb_qh */
> struct list_head out_bulk; /* of musb_qh */
> - struct musb_qh *periodic[32]; /* tree of interrupt+iso */
> #endif
>
> /* called with IRQs blocked; ON/nonzero implies starting a session,
> Index: linux-2.6/drivers/usb/musb/musb_host.c
> ===================================================================
> --- linux-2.6.orig/drivers/usb/musb/musb_host.c
> +++ linux-2.6/drivers/usb/musb/musb_host.c
> @@ -400,7 +400,6 @@ musb_giveback(struct musb_qh *qh, struct
> * de-allocated if it's tracked and allocated;
> * and where we'd update the schedule tree...
> */
> - musb->periodic[ep->epnum] = NULL;
> kfree(qh);
> qh = NULL;
> break;
> @@ -1716,31 +1715,26 @@ static int musb_schedule(
>
> /* else, periodic transfers get muxed to other endpoints */
>
> - /* FIXME this doesn't consider direction, so it can only
> - * work for one half of the endpoint hardware, and assumes
> - * the previous cases handled all non-shared endpoints...
> - */
> -
> - /* we know this qh hasn't been scheduled, so all we need to do
> + /*
> + * We know this qh hasn't been scheduled, so all we need to do
> * is choose which hardware endpoint to put it on ...
> *
> * REVISIT what we really want here is a regular schedule tree
> - * like e.g. OHCI uses, but for now musb->periodic is just an
> - * array of the _single_ logical endpoint associated with a
> - * given physical one (identity mapping logical->physical).
> - *
> - * that simplistic approach makes TT scheduling a lot simpler;
> - * there is none, and thus none of its complexity...
> + * like e.g. OHCI uses.
> */
> best_diff = 4096;
> best_end = -1;
>
> - for (epnum = 1; epnum < musb->nr_endpoints; epnum++) {
> + for (epnum = 1, hw_ep = musb->endpoints + 1; epnum < musb->nr_endpoints;
> + epnum++, hw_ep++) {
> int diff;
>
> - if (musb->periodic[epnum])
> + if (is_in || hw_ep->is_shared_fifo) {
> + if (hw_ep->in_qh != NULL)
> + continue;
> + } else if (hw_ep->out_qh != NULL)
> continue;
> - hw_ep = &musb->endpoints[epnum];
> +
> if (hw_ep == musb->bulk_ep)
> continue;
>
> @@ -1769,7 +1763,6 @@ static int musb_schedule(
> idle = 1;
> qh->mux = 0;
> hw_ep = musb->endpoints + best_end;
> - musb->periodic[best_end] = qh;
> DBG(4, "qh %p periodic slot %d\n", qh, best_end);
> success:
> if (head) {
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
