Re: [PATCH 17/17] usb: host: ehci-dbg: refactor fill_periodic_buffer function

From: Alan Stern
Date: Mon Jan 04 2016 - 16:01:44 EST

On Mon, 4 Jan 2016, Geyslan G. Bem wrote:

> This patch fixes a coding style issue reported by checkpatch related to
> many leading tabs, removing a 'do while' loop and making use of goto tag instead.

This is highly questionable. It's a big amount of code churn, nearly
impossible to verify visually, just to remove one level of indentation.
It also introduces an unnecessary backwards "goto", which seems like a
bad idea.

Alan Stern

> Others changes in this patch are:
> - Some multiline statements are reduced (718, 729, 780, 786, 790).
> - A constant is moved to right on line 770.
>
> Signed-off-by: Geyslan G. Bem <geyslan@xxxxxxxxx>
> ---
>
> Notes:
> Tested by compilation only.
>
> drivers/usb/host/ehci-dbg.c | 180 ++++++++++++++++++++++----------------------
> 1 file changed, 88 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-dbg.c b/drivers/usb/host/ehci-dbg.c
> index 2268756..278333d 100644
> --- a/drivers/usb/host/ehci-dbg.c
> +++ b/drivers/usb/host/ehci-dbg.c
> @@ -698,6 +698,8 @@ static ssize_t fill_periodic_buffer(struct debug_buffer *buf)
> */
> spin_lock_irqsave(&ehci->lock, flags);
> for (i = 0; i < ehci->periodic_size; i++) {
> + struct ehci_qh_hw *hw;
> +
> p = ehci->pshadow[i];
> if (likely(!p.ptr))
> continue;
> @@ -707,104 +709,98 @@ static ssize_t fill_periodic_buffer(struct debug_buffer *buf)
> size -= temp;
> next += temp;
>
> - do {
> - struct ehci_qh_hw *hw;
> -
> - switch (hc32_to_cpu(ehci, tag)) {
> - case Q_TYPE_QH:
> - hw = p.qh->hw;
> - temp = scnprintf(next, size, " qh%d-%04x/%p",
> - p.qh->ps.period,
> - hc32_to_cpup(ehci,
> - &hw->hw_info2)
> - /* uframe masks */
> - & (QH_CMASK | QH_SMASK),
> - p.qh);
> - size -= temp;
> - next += temp;
> - /* don't repeat what follows this qh */
> - for (temp = 0; temp < seen_count; temp++) {
> - if (seen[temp].ptr != p.ptr)
> +do_loop:
> + switch (hc32_to_cpu(ehci, tag)) {
> + case Q_TYPE_QH:
> + hw = p.qh->hw;
> + temp = scnprintf(next, size, " qh%d-%04x/%p",
> + p.qh->ps.period,
> + hc32_to_cpup(ehci, &hw->hw_info2)
> + /* uframe masks */
> + & (QH_CMASK | QH_SMASK),
> + p.qh);
> + size -= temp;
> + next += temp;
> + /* don't repeat what follows this qh */
> + for (temp = 0; temp < seen_count; temp++) {
> + if (seen[temp].ptr != p.ptr)
> + continue;
> + if (p.qh->qh_next.ptr) {
> + temp = scnprintf(next, size, " ...");
> + size -= temp;
> + next += temp;
> + }
> + break;
> + }
> + /* show more info the first time around */
> + if (temp == seen_count) {
> + u32 scratch = hc32_to_cpup(ehci,
> + &hw->hw_info1);
> + struct ehci_qtd *qtd;
> + char *type = "";
> +
> + /* count tds, get ep direction */
> + temp = 0;
> + list_for_each_entry(qtd,
> + &p.qh->qtd_list,
> + qtd_list) {
> + temp++;
> + switch ((hc32_to_cpu(ehci,
> + qtd->hw_token) >> 8)
> + & 0x03) {
> + case 0:
> + type = "out";
> + continue;
> + case 1:
> + type = "in";
> continue;
> - if (p.qh->qh_next.ptr) {
> - temp = scnprintf(next, size,
> - " ...");
> - size -= temp;
> - next += temp;
> }
> - break;
> }
> - /* show more info the first time around */
> - if (temp == seen_count) {
> - u32 scratch = hc32_to_cpup(ehci,
> - &hw->hw_info1);
> - struct ehci_qtd *qtd;
> - char *type = "";
> -
> - /* count tds, get ep direction */
> - temp = 0;
> - list_for_each_entry(qtd,
> - &p.qh->qtd_list,
> - qtd_list) {
> - temp++;
> - switch ((hc32_to_cpu(ehci,
> - qtd->hw_token) >> 8)
> - & 0x03) {
> - case 0:
> - type = "out";
> - continue;
> - case 1:
> - type = "in";
> - continue;
> - }
> - }
>
> - temp = scnprintf(next, size,
> - " (%c%d ep%d%s "
> - "[%d/%d] q%d p%d)",
> - speed_char (scratch),
> - scratch & 0x007f,
> - (scratch >> 8) & 0x000f, type,
> - p.qh->ps.usecs,
> - p.qh->ps.c_usecs,
> - temp,
> - 0x7ff & (scratch >> 16));
> -
> - if (seen_count < DBG_SCHED_LIMIT)
> - seen[seen_count++].qh = p.qh;
> - } else {
> - temp = 0;
> - }
> - tag = Q_NEXT_TYPE(ehci, hw->hw_next);
> - p = p.qh->qh_next;
> - break;
> - case Q_TYPE_FSTN:
> - temp = scnprintf(next, size,
> - " fstn-%8x/%p", p.fstn->hw_prev,
> - p.fstn);
> - tag = Q_NEXT_TYPE(ehci, p.fstn->hw_next);
> - p = p.fstn->fstn_next;
> - break;
> - case Q_TYPE_ITD:
> - temp = scnprintf(next, size,
> - " itd/%p", p.itd);
> - tag = Q_NEXT_TYPE(ehci, p.itd->hw_next);
> - p = p.itd->itd_next;
> - break;
> - case Q_TYPE_SITD:
> temp = scnprintf(next, size,
> - " sitd%d-%04x/%p",
> - p.sitd->stream->ps.period,
> - hc32_to_cpup(ehci, &p.sitd->hw_uframe)
> - & 0x0000ffff,
> - p.sitd);
> - tag = Q_NEXT_TYPE(ehci, p.sitd->hw_next);
> - p = p.sitd->sitd_next;
> - break;
> + " (%c%d ep%d%s "
> + "[%d/%d] q%d p%d)",
> + speed_char (scratch),
> + scratch & 0x007f,
> + (scratch >> 8) & 0x000f, type,
> + p.qh->ps.usecs,
> + p.qh->ps.c_usecs,
> + temp,
> + (scratch >> 16) & 0x7ff);
> +
> + if (seen_count < DBG_SCHED_LIMIT)
> + seen[seen_count++].qh = p.qh;
> + } else {
> + temp = 0;
> }
> - size -= temp;
> - next += temp;
> - } while (p.ptr);
> + tag = Q_NEXT_TYPE(ehci, hw->hw_next);
> + p = p.qh->qh_next;
> + break;
> + case Q_TYPE_FSTN:
> + temp = scnprintf(next, size, " fstn-%8x/%p",
> + p.fstn->hw_prev, p.fstn);
> + tag = Q_NEXT_TYPE(ehci, p.fstn->hw_next);
> + p = p.fstn->fstn_next;
> + break;
> + case Q_TYPE_ITD:
> + temp = scnprintf(next, size, " itd/%p", p.itd);
> + tag = Q_NEXT_TYPE(ehci, p.itd->hw_next);
> + p = p.itd->itd_next;
> + break;
> + case Q_TYPE_SITD:
> + temp = scnprintf(next, size, " sitd%d-%04x/%p",
> + p.sitd->stream->ps.period,
> + hc32_to_cpup(ehci, &p.sitd->hw_uframe)
> + & 0x0000ffff,
> + p.sitd);
> + tag = Q_NEXT_TYPE(ehci, p.sitd->hw_next);
> + p = p.sitd->sitd_next;
> + break;
> + }
> + size -= temp;
> + next += temp;
> + if (p.ptr)
> + goto do_loop;
>
> temp = scnprintf(next, size, "\n");
> size -= temp;
>

--
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/