Re: [PATCH 01/10] usb: host: ehci-sched: refactor scan_isoc function

[Alan Stern]

On Fri, 11 Dec 2015, Geyslan G. Bem wrote:

> This patch removes an infinite 'for' loop and makes use of the already
> existing 'restart' tag instead, 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>
> ---
>  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;

These new declarations should line up with the ones above.  And there 
shouldn't be a blank line between.

> +	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))

The style used in this source file does not align continuation lines 
with open parens on the line above.  Instead, the continuation lines 
are indented by 2 extra tabs (or maybe 1 extra if 2 would pushd them 
beyond 80 characters).

> +				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);

Ditto.

> +			/*
> +			 * 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))

Ditto.

> +				*hw_p = q.itd->hw_next;
> +			else
> +				*hw_p = cpu_to_hc32(ehci,
> +						    ehci->dummy->qh_dma);

Ditto.

> +		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))) {

Although these lines didn't follow the style used in the rest of the 
file, they should do so now.

> +
> +				q_p = &q.sitd->sitd_next;
> +				hw_p = &q.sitd->hw_next;
> +				type = Q_NEXT_TYPE(ehci,
> +						   q.sitd->hw_next);

Ditto

> +			 * 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))

Ditto.

> +				*hw_p = q.sitd->hw_next;
> +			else
> +				*hw_p = cpu_to_hc32(ehci,
> +						    ehci->dummy->qh_dma);

Ditto.

> +			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);

Ditto.

Alan Stern

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