linux-kernel - Re: [PATCH 10/14] RFC: usb/host/fotg210: Add function scan_frame

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20151002173500.GU5552@saruman.tx.rr.com>
Date:	Fri, 2 Oct 2015 12:35:00 -0500
From:	Felipe Balbi <balbi@...com>
To:	Peter Senna Tschudin <peter.senna@...il.com>
CC:	<balbi@...com>, <stern@...land.harvard.edu>,
	<sergei.shtylyov@...entembedded.com>, <standby24x7@...il.com>,
	<pmladek@...e.cz>, <linux-usb@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <gregkh@...uxfoundation.org>
Subject: Re: [PATCH 10/14] RFC: usb/host/fotg210: Add function
 scan_frame_queue()

On Mon, Sep 21, 2015 at 05:01:14PM +0200, Peter Senna Tschudin wrote:
> checkpatch complains about too many leading tabs because the if
> statement starts after 6 tabs:
> 
> scan_iosoc() -> for() -> while() -> switch() -> if() -> for() -> if()
> 
> There is also a goto statement going backwards in case of failure. This
> patch creates a new inline function named scan_frame_queue() containing
> the last 4 nesting levels, and removes the need of backwards goto,
> making the code easier to read. After the patch it becomes:
> 
> scan_iosoc() -> for() -> while() -> scan_frame_queue()
> 
> Signed-off-by: Peter Senna Tschudin <peter.senna@...il.com>
> ---
>  drivers/usb/host/fotg210-hcd.c | 142 ++++++++++++++++++++++-------------------
>  1 file changed, 76 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
> index 13cca41..e60a239 100644
> --- a/drivers/usb/host/fotg210-hcd.c
> +++ b/drivers/usb/host/fotg210-hcd.c
> @@ -4645,13 +4645,81 @@ done:
>  	return status;
>  }
>  
> -/*-------------------------------------------------------------------------*/
> +static inline int scan_frame_queue(struct fotg210_hcd *fotg210, unsigned frame,
> +				   unsigned now_frame, bool live)
> +{
> +	unsigned uf;
> +	bool modified;
> +	union fotg210_shadow q, *q_p;
> +	__hc32 type, *hw_p;
> +
> +	/* scan each element in frame's queue for completions */
> +	q_p = &fotg210->pshadow[frame];
> +	hw_p = &fotg210->periodic[frame];
> +	q.ptr = q_p->ptr;
> +	type = Q_NEXT_TYPE(fotg210, *hw_p);
> +	modified = false;
> +
> +	while (q.ptr != NULL) {

while (q.ptr) is enough

> +		switch (hc32_to_cpu(fotg210, type)) {
> +		case Q_TYPE_ITD:
> +			/* If this ITD is still active, leave it for

comment style

> +			 * 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(fotg210))
> +						break;
> +				}
> +				if (uf < 8) {
> +					q_p = &q.itd->itd_next;
> +					hw_p = &q.itd->hw_next;
> +					type = Q_NEXT_TYPE(fotg210,
> +						q.itd->hw_next);
> +					q = *q_p;
> +					break;
> +				}
> +			}
> +
> +			/* Take finished ITDs out of the schedule

comment style

> +			 * 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;
> +			*hw_p = q.itd->hw_next;
> +			type = Q_NEXT_TYPE(fotg210, q.itd->hw_next);
> +			wmb();
> +			modified = itd_complete(fotg210, q.itd);
> +			q = *q_p;
> +			break;
> +		default:
> +			fotg210_dbg(fotg210, "corrupt type %d frame %d shadow %p\n",
> +				type, frame, q.ptr);
> +			/* 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 && fotg210->isoc_count > 0))
> +			return -1;

can't you add a proper error code here ?

other than these, patch looks good.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)