linux-kernel - Re: [PATCH RFC] comedi: rtd520: if condition with no effect

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:	Mon, 19 Jan 2015 10:51:38 +0000
From:	Ian Abbott <abbotti@....co.uk>
To:	Nicholas Mc Guire <der.herr@...r.at>
CC:	H Hartley Sweeten <hsweeten@...ionengravers.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] comedi: rtd520: if condition with no effect - if
 identical to else

On 18/01/15 11:53, Nicholas Mc Guire wrote:
> The if and the else branch code are identical - so the condition has no
> effect on the effective code - this patch removes the condition and the
> duplicated code.
>
> Signed-off-by: Nicholas Mc Guire <der.herr@...r.at>
> ---
>
> Patch is against 3.19.0-rc4 -next-20150116
>
> This patch was only compile tested with x86_64_defconfig + CONFIG_STAGING=y
> CONFIG_COMEDI=m, CONFIG_COMEDI_PCI_DRIVERS=m, CONFIG_COMEDI_RTD520=m
>
> commit c5930d66 (staging: comedi: rtd520: use the comedi_device 'mmio'
> member) introduced the current code, the probably bug was though
> present before this commit:
>
>          if (devpriv->xfer_count > 0)    /* transfer every N samples */
>                  writew(IRQM_ADC_ABOUT_CNT, dev->mmio + LAS0_IT);
>          else                            /* 1/2 FIFO transfers */
>                  writew(IRQM_ADC_ABOUT_CNT, dev->mmio + LAS0_IT);
>
> as the if and else branch are identical but the comments indicate that
> they are serving different purposes this looks like a bug. The code has
> though been in use in this form for more than 2 years - so maybe the
> condition is simply unnecessary. In any case this needs a review
> by someone who knows the details of the driver - the below patch
> is only cleaning up the current situation and may thus be wrong.
>
> ...and how did this pass multiple modifications and reviews ?...

The two halves used to output different kernel debug messages which were 
removed in commit d62bc468.  Prior to that, in commit 60aeb080, a whole 
bunch of unused #ifdef code was removed, which made the 'else' branch 
the same as the 'if' branch, apart from the debug messages, as can be 
seen here around line 1319 on the left side:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/staging/comedi/drivers/?id=60aeb08024dcd8a392917ab72a107ad451a59030&context=5&ignorews=0&dt=1

>
>   drivers/staging/comedi/drivers/rtd520.c |    6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/rtd520.c b/drivers/staging/comedi/drivers/rtd520.c
> index 06ae436..3ccdade 100644
> --- a/drivers/staging/comedi/drivers/rtd520.c
> +++ b/drivers/staging/comedi/drivers/rtd520.c
> @@ -1012,10 +1012,8 @@ static int rtd_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
>   	readw(dev->mmio + LAS0_CLEAR);
>
>   	/* TODO: allow multiple interrupt sources */
> -	if (devpriv->xfer_count > 0)	/* transfer every N samples */
> -		writew(IRQM_ADC_ABOUT_CNT, dev->mmio + LAS0_IT);
> -	else				/* 1/2 FIFO transfers */
> -		writew(IRQM_ADC_ABOUT_CNT, dev->mmio + LAS0_IT);
> +	/* transfer every N samples */
> +	writew(IRQM_ADC_ABOUT_CNT, dev->mmio + LAS0_IT);
>
>   	/* BUG: start_src is ASSUMED to be TRIG_NOW */
>   	/* BUG? it seems like things are running before the "start" */
>

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

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@....co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-
--
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/