linux-kernel - Re: [PATCH v8] Staging: comedi: convert while loop to timeout in ni_mio

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] [day] [month] [year] [list]

Date:	Fri, 17 Jan 2014 13:12:45 +0000
From:	Ian Abbott <abbotti@....co.uk>
To:	Chase Southwood <chase.southwood@...oo.com>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:	Ian Abbott <ian.abbott@....co.uk>,
	"hsweeten@...ionengravers.com" <hsweeten@...ionengravers.com>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8] Staging: comedi: convert while loop to timeout in
 ni_mio_common.c

On 2014-01-16 18:27, Chase Southwood wrote:
> This patch for ni_mio_common.c changes out a while loop for a timeout,
> which is preferred.
>
> Signed-off-by: Chase Southwood <chase.southwood@...oo.com>
> ---
>
> Okay, back to v2, basically.  I fixed the checkpatch warning from v2,
> and added the error checking that was from v3, but otherwise it is the
> same.  Of note, I have used a udelay of 1 here (Greg had suggested to
> use 10, but Ian prefers 1 so I have gone with that, and I assume that
> cpu_relax is no longer an option.).
>
> 2: Changed from simple clean-up to swapping a timeout in for a while loop.
>
> 3: Removed extra counter variable, and added error checking.
>
> 4: No longer using counter variable, using jiffies instead.
>
> 5: udelay for 10u, instead of 1u.
>
> 6: Scrap udelay entirely, in favor of cpu_relax. Include asm/processor.h
> in order to use cpu_relax.
>
> 7: Fix typo (msec vs msecs).
>
> 8: Revert back to v2, with some small changes (see above).
>
>   drivers/staging/comedi/drivers/ni_mio_common.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index 457b884..10c27cb 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -687,12 +687,22 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
>   {
>   	const struct ni_board_struct *board = comedi_board(dev);
>   	struct ni_private *devpriv = dev->private;
> +	static const int timeout = 10000;
> +	int i;
>
>   	if (board->reg_type == ni_reg_6143) {
>   		/*  Flush the 6143 data FIFO */
>   		ni_writel(0x10, AIFIFO_Control_6143);	/*  Flush fifo */
>   		ni_writel(0x00, AIFIFO_Control_6143);	/*  Flush fifo */
> -		while (ni_readl(AIFIFO_Status_6143) & 0x10) ;	/*  Wait for complete */
> +		/*  Wait for complete */
> +		for (i = 0; i < timeout; i++) {
> +			if (!(ni_readl(AIFIFO_Status_6143) & 0x10))
> +				break;
> +			udelay(1);
> +		}
> +		if (i == timeout) {
> +			comedi_error(dev, "FIFO flush timeout.");
> +		}
>   	} else {
>   		devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
>   		if (board->reg_type == ni_reg_625x) {
>

Personally, I'm happy with it.  The upper bound on the iterations is 
quite high for something that could be called on an interrupt, but it's 
better than an infinite loop, and it only reach the upper bound if the 
hardware is broken or there's some other bug.

Reviewed-by: Ian Abbott <abbotti@....co.uk>

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@....co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/