>-----Original Message-----
>From: devel <driverdev-devel-bounces@xxxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
