Re: [PATCH] staging: kpc2000: simplify nested conditionals that just return a boolean.

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, May 24, 2019 at 01:19:26PM +0100, Jeremy Sowden wrote:
> kp2000_check_uio_irq contained a pair of nested ifs which each evaluated
> a bitwise operation.  If both operations yielded true, the function
> returned 1; otherwise it returned 0.
> 
> Replaced the whole thing with one return statement that evaluates the
> combination of both bitwise operations.

Does this really do the same thing?

> 
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
> This applies to staging-testing.
> 
> I was in two minds whether to send this patch or something less terse:
> 
> +	return (interrupt_active & irq_check_mask) &&
> +	       (interrupt_mask_inv & irq_check_mask);
> 
>  drivers/staging/kpc2000/kpc2000/cell_probe.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> index f731a97c6cac..d10f695b6673 100644
> --- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
> +++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> @@ -240,12 +240,10 @@ int  kp2000_check_uio_irq(struct kp2000_device *pcard, u32 irq_num)
>  	u64 interrupt_mask_inv = ~readq(pcard->sysinfo_regs_base + REG_INTERRUPT_MASK);
>  	u64 irq_check_mask = (1 << irq_num);
>  
> -	if (interrupt_active & irq_check_mask) { // if it's active (interrupt pending)
> -		if (interrupt_mask_inv & irq_check_mask) {    // and if it's not masked off
> -			return 1;
> -		}
> -	}
> -	return 0;
> +	/*
> +	 * Is the IRQ active (interrupt pending) and not masked off?
> +	 */
> +	return (interrupt_active & interrupt_mask_inv & irq_check_mask) != 0;

I have no idea what these bits really are, but try the above with the
following values:
	interrupt_active = BIT(0);
	interrupt_mask_inv = BIT(1);
	irq_check_mask = (BIT(1) | BIT(0));

So in your new code you have:
	BIT(0) & BIT(1) & (BIT(1) | BIT(0))
which reduces to:
	0 & (BIT(1) | BIT(0))
which then reduces to:
	0

The original if statements will return 1.
Your new one will return 0.

You can't AND interrupt_active with interrupt_mask_inv like you did
here, right?

Or am I reading this all wrong?

And what's wrong with the original code here?  What is complaining about
it (other than the crazy comment style...)?

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel