Re: [PATCH v2] usb: ehci: refactor scan_isoc function
From: Geyslan G. Bem
Date: Fri Dec 11 2015 - 15:27:22 EST
2015-12-10 21:55 GMT-03:00 Geyslan G. Bem <geyslan@xxxxxxxxx>:
> This patch removes an infinite for loop and makes use of the already
> existing 'restart' tag, reducing one leading tab.
>
> The comments and code were corrected conforming coding style.
>
> Tested by compilation only.
> Caught by checkpatch:
> WARNING: Too many leading tabs - consider code refactoring
>
> Signed-off-by: Geyslan G. Bem <geyslan@xxxxxxxxx>
> ---
> v2: fixes coding style (removes spaces before parens and open brackets)
> ---
> drivers/usb/host/ehci-sched.c | 203 ++++++++++++++++++++++--------------------
> 1 file changed, 104 insertions(+), 99 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
> index f9a3327..86b2484 100644
> --- a/drivers/usb/host/ehci-sched.c
> +++ b/drivers/usb/host/ehci-sched.c
> @@ -2383,6 +2383,9 @@ static void scan_isoc(struct ehci_hcd *ehci)
> unsigned fmask = ehci->periodic_size - 1;
> bool modified, live;
>
> + union ehci_shadow q, *q_p;
> + __hc32 type, *hw_p;
> +
> /*
> * When running, scan from last scan point up to "now"
> * else clean up by scanning everything that's left.
> @@ -2399,119 +2402,121 @@ static void scan_isoc(struct ehci_hcd *ehci)
> ehci->now_frame = now_frame;
>
> frame = ehci->last_iso_frame;
> - for (;;) {
> - union ehci_shadow q, *q_p;
> - __hc32 type, *hw_p;
>
> restart:
> - /* scan each element in frame's queue for completions */
> - q_p = &ehci->pshadow [frame];
> - hw_p = &ehci->periodic [frame];
> - q.ptr = q_p->ptr;
> - type = Q_NEXT_TYPE(ehci, *hw_p);
> - modified = false;
> -
> - while (q.ptr != NULL) {
> - switch (hc32_to_cpu(ehci, type)) {
> - case Q_TYPE_ITD:
> - /* If this ITD is still active, leave it for
> - * later processing ... check the next entry.
> - * No need to check for activity unless the
> - * frame is current.
> - */
> - if (frame == now_frame && live) {
> - rmb();
> - for (uf = 0; uf < 8; uf++) {
> - if (q.itd->hw_transaction[uf] &
> - ITD_ACTIVE(ehci))
> - break;
> - }
> - if (uf < 8) {
> - q_p = &q.itd->itd_next;
> - hw_p = &q.itd->hw_next;
> - type = Q_NEXT_TYPE(ehci,
> - q.itd->hw_next);
> - q = *q_p;
> + /* Scan each element in frame's queue for completions */
> + q_p = &ehci->pshadow[frame];
> + hw_p = &ehci->periodic[frame];
> + q.ptr = q_p->ptr;
> + type = Q_NEXT_TYPE(ehci, *hw_p);
> + modified = false;
> +
> + while (q.ptr != NULL) {
> + switch (hc32_to_cpu(ehci, type)) {
> + case Q_TYPE_ITD:
> + /*
> + * If this ITD is still active, leave it for
> + * later processing ... check the next entry.
> + * No need to check for activity unless the
> + * frame is current.
> + */
> + if (frame == now_frame && live) {
> + rmb();
> + for (uf = 0; uf < 8; uf++) {
> + if (q.itd->hw_transaction[uf] &
> + ITD_ACTIVE(ehci))
> break;
> - }
> }
> -
> - /* Take finished ITDs out of the schedule
> - * and process them: recycle, maybe report
> - * URB completion. HC won't cache the
> - * pointer for much longer, if at all.
> - */
> - *q_p = q.itd->itd_next;
> - if (!ehci->use_dummy_qh ||
> - q.itd->hw_next != EHCI_LIST_END(ehci))
> - *hw_p = q.itd->hw_next;
> - else
> - *hw_p = cpu_to_hc32(ehci,
> - ehci->dummy->qh_dma);
> - type = Q_NEXT_TYPE(ehci, q.itd->hw_next);
> - wmb();
> - modified = itd_complete (ehci, q.itd);
> - q = *q_p;
> - break;
> - case Q_TYPE_SITD:
> - /* If this SITD is still active, leave it for
> - * later processing ... check the next entry.
> - * No need to check for activity unless the
> - * frame is current.
> - */
> - if (((frame == now_frame) ||
> - (((frame + 1) & fmask) == now_frame))
> - && live
> - && (q.sitd->hw_results &
> - SITD_ACTIVE(ehci))) {
> -
> - q_p = &q.sitd->sitd_next;
> - hw_p = &q.sitd->hw_next;
> + if (uf < 8) {
> + q_p = &q.itd->itd_next;
> + hw_p = &q.itd->hw_next;
> type = Q_NEXT_TYPE(ehci,
> - q.sitd->hw_next);
> + q.itd->hw_next);
> q = *q_p;
> break;
> }
> + }
>
> - /* Take finished SITDs out of the schedule
> - * and process them: recycle, maybe report
> - * URB completion.
> - */
> - *q_p = q.sitd->sitd_next;
> - if (!ehci->use_dummy_qh ||
> - q.sitd->hw_next != EHCI_LIST_END(ehci))
> - *hw_p = q.sitd->hw_next;
> - else
> - *hw_p = cpu_to_hc32(ehci,
> - ehci->dummy->qh_dma);
> - type = Q_NEXT_TYPE(ehci, q.sitd->hw_next);
> - wmb();
> - modified = sitd_complete (ehci, q.sitd);
> + /*
> + * Take finished ITDs out of the schedule
> + * and process them: recycle, maybe report
> + * URB completion. HC won't cache the
> + * pointer for much longer, if at all.
> + */
> + *q_p = q.itd->itd_next;
> + if (!ehci->use_dummy_qh ||
> + q.itd->hw_next != EHCI_LIST_END(ehci))
> + *hw_p = q.itd->hw_next;
> + else
> + *hw_p = cpu_to_hc32(ehci,
> + ehci->dummy->qh_dma);
> + type = Q_NEXT_TYPE(ehci, q.itd->hw_next);
> + wmb();
> + modified = itd_complete(ehci, q.itd);
> + q = *q_p;
> + break;
> + case Q_TYPE_SITD:
> + /*
> + * If this SITD is still active, leave it for
> + * later processing ... check the next entry.
> + * No need to check for activity unless the
> + * frame is current.
> + */
> + if (((frame == now_frame) ||
> + (((frame + 1) & fmask) == now_frame))
> + && live
> + && (q.sitd->hw_results &
> + SITD_ACTIVE(ehci))) {
> +
> + q_p = &q.sitd->sitd_next;
> + hw_p = &q.sitd->hw_next;
> + type = Q_NEXT_TYPE(ehci,
> + q.sitd->hw_next);
> q = *q_p;
> break;
> - default:
> - ehci_dbg(ehci, "corrupt type %d frame %d shadow %p\n",
> - type, frame, q.ptr);
> - // BUG ();
> - /* FALL THROUGH */
> - case Q_TYPE_QH:
> - case Q_TYPE_FSTN:
> - /* End of the iTDs and siTDs */
> - q.ptr = NULL;
> - break;
> }
>
> - /* assume completion callbacks modify the queue */
> - if (unlikely(modified && ehci->isoc_count > 0))
> - goto restart;
> - }
> -
> - /* Stop when we have reached the current frame */
> - if (frame == now_frame)
> + /*
> + * Take finished SITDs out of the schedule
> + * and process them: recycle, maybe report
> + * URB completion.
> + */
> + *q_p = q.sitd->sitd_next;
> + if (!ehci->use_dummy_qh ||
> + q.sitd->hw_next != EHCI_LIST_END(ehci))
> + *hw_p = q.sitd->hw_next;
> + else
> + *hw_p = cpu_to_hc32(ehci,
> + ehci->dummy->qh_dma);
> + type = Q_NEXT_TYPE(ehci, q.sitd->hw_next);
> + wmb();
> + modified = sitd_complete(ehci, q.sitd);
> + q = *q_p;
> break;
> + default:
> + ehci_dbg(ehci, "corrupt type %d frame %d shadow %p\n",
> + type, frame, q.ptr);
> + /* BUG(); */
> + /* FALL THROUGH */
> + case Q_TYPE_QH:
> + case Q_TYPE_FSTN:
> + /* End of the iTDs and siTDs */
> + q.ptr = NULL;
> + break;
> + }
>
> - /* The last frame may still have active siTDs */
> - ehci->last_iso_frame = frame;
> - frame = (frame + 1) & fmask;
> + /* Assume completion callbacks modify the queue */
> + if (unlikely(modified && ehci->isoc_count > 0))
> + goto restart;
> }
> +
> + /* Stop when we have reached the current frame */
> + if (frame == now_frame)
> + return;
> +
> + /* The last frame may still have active siTDs */
> + ehci->last_iso_frame = frame;
> + frame = (frame + 1) & fmask;
> +
> + goto restart;
> }
> --
> 2.6.3
>
I'm doing a cleanup in this file. It's better to wait the whole pack of patches.
--
Regards,
Geyslan G. Bem
hackingbits.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/