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

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

[Jeremy Sowden]

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.

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;
 }
 
 static
-- 
2.20.1

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

[Greg KH]

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

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

[Matt Sickler]

>-----Original Message-----
>From: devel <driverdev-devel-bounces@linuxdriverproject.org> On Behalf Of Greg KH
>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 indicates which IRQs are active/pending
0 = not pending
1 = pending

irq_check_mask has only a single bit set which is which IRQ we're testing for
Each one is "associated" with one of the UIO "cores" (see core_probe.c for how that association is discovered).

interrupt_mask_inv is a bitwise inversion of the HW register.  The HW register tells the HW which interrupts to ignore.
In the HW register:
0 = pass this IRQ up to the host.
1 = mask the IRQ
In interrupt_mask_inv:
0 = ignore this IRQ
1 = care about this IRQ

This code is checking 3 things:
- Is there an interrupt for this UIO core
- Is that interrupt signaled
- Is the interrupt not masked

Just in case it's not obvious yet: the mask/pending bits allow the HW to catch an interrupt but not signal the host until the interrupt is unmasked.  This allows interrupts that happen while the IRQ is masked to still be handled once the IRQ is un-masked. 

>        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...)?

I would agree with Greg, if there's nothing complaining about the way the original code was written it should probably be left that way.  This new way seems overly tricky, even if it was proven to do the same thing.

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

[Dan Carpenter]

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

Yeah.  I would prefer this.

regards,
dan carpenter


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

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

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 5011 bytes --]

On 2019-05-24, at 16:59:45 +0000, Matt Sickler wrote:
> From: devel <driverdev-devel-bounces@linuxdriverproject.org> On Behalf Of Greg KH
> > 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 indicates which IRQs are active/pending
> 0 = not pending
> 1 = pending
>
> irq_check_mask has only a single bit set which is which IRQ we're
> testing for Each one is "associated" with one of the UIO "cores" (see
> core_probe.c for how that association is discovered).
>
> interrupt_mask_inv is a bitwise inversion of the HW register.  The HW
> register tells the HW which interrupts to ignore.  In the HW register:
> 0 = pass this IRQ up to the host.
> 1 = mask the IRQ
> In interrupt_mask_inv:
> 0 = ignore this IRQ
> 1 = care about this IRQ
>
> This code is checking 3 things:
> - Is there an interrupt for this UIO core
> - Is that interrupt signaled
> - Is the interrupt not masked
>
> Just in case it's not obvious yet: the mask/pending bits allow the HW
> to catch an interrupt but not signal the host until the interrupt is
> unmasked.  This allows interrupts that happen while the IRQ is masked
> to still be handled once the IRQ is un-masked.
>
> >        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?

As Matt explained above, irq_check_mask only ever has one bit set:

  u64 irq_check_mask = (1 << irq_num);

So:

  interrupt_mask_inv & irq_check_mask

yields irq_check_mask or 0, and:

  interrupt_active & irq_check_mask

yields irq_check_mask or 0, and:

  interrupt_mask_inv & interrupt_active & irq_check_mask

yields irq_check_mask or 0.

> > And what's wrong with the original code here?  What is complaining about
> > it (other than the crazy comment style...)?
>
> I would agree with Greg, if there's nothing complaining about the way
> the original code was written it should probably be left that way.
> This new way seems overly tricky, even if it was proven to do the same
> thing.

This patch was originally part of a larger series of white-space and
formatting changes to cell_probe.c that I put to one side.  It started
out as a change to the formatting of the comments, IIRC.  I was reminded
of it while looking over the recent series by Simon Sandström (which
have fixed most of the issues that my series would have addressed).
Conditional blocks that contain nothing but statements returing boolean
constants are a (minor) pet peeve of mine, so I sent the patch by
itself.  Clearly it's not a peeve shared by many people. :)

Happy to drop the patch.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]