Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   David Laight <David.Laight@ACULAB.COM>
To:     'Andy Shevchenko' <andriy.shevchenko@linux.intel.com>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        "linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>,
        "Dennis Dalessandro" <dennis.dalessandro@cornelisnetworks.com>
Subject: RE: [PATCH v1 1/1] IB/hfi1: Don't cast parameter in bit operations
Thread-Topic: [PATCH v1 1/1] IB/hfi1: Don't cast parameter in bit operations
Thread-Index: AQHYKOavPScLVMZqEkOT//Q3noWC2qyhqG0w
Date:   Wed, 23 Feb 2022 21:44:32 +0000
Message-ID: <e39730af26cc4a4d944fa3205fa17b3c@AcuMS.aculab.com>
References: <20220223185353.51370-1-andriy.shevchenko@linux.intel.com>
In-Reply-To: <20220223185353.51370-1-andriy.shevchenko@linux.intel.com>
Accept-Language: en-GB, en-US
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Precedence: bulk

From: Andy Shevchenko
> Sent: 23 February 2022 18:54
> 
> While in this particular case it would not be a (critical) issue,
> the pattern itself is bad and error prone in case somebody blindly
> copies to their code.

It is horribly wrong on BE systems.

> Don't cast parameter to unsigned long pointer in the bit operations.
> Instead copy to a local variable on stack of a proper type and use.
> 
> Fixes: 7724105686e7 ("IB/hfi1: add driver files")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/infiniband/hw/hfi1/chip.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/chip.c b/drivers/infiniband/hw/hfi1/chip.c
> index f1245c94ae26..100274b926d3 100644
> --- a/drivers/infiniband/hw/hfi1/chip.c
> +++ b/drivers/infiniband/hw/hfi1/chip.c
> @@ -8286,34 +8286,33 @@ static void is_interrupt(struct hfi1_devdata *dd, unsigned int source)
>  irqreturn_t general_interrupt(int irq, void *data)
>  {
>  	struct hfi1_devdata *dd = data;
> -	u64 regs[CCE_NUM_INT_CSRS];
> +	DECLARE_BITMAP(pending, CCE_NUM_INT_CSRS * 64);
> +	u64 value;
>  	u32 bit;
>  	int i;
> -	irqreturn_t handled = IRQ_NONE;
> 
>  	this_cpu_inc(*dd->int_counter);
> 
>  	/* phase 1: scan and clear all handled interrupts */
>  	for (i = 0; i < CCE_NUM_INT_CSRS; i++) {
> -		if (dd->gi_mask[i] == 0) {
> -			regs[i] = 0;	/* used later */
> -			continue;
> -		}
> -		regs[i] = read_csr(dd, CCE_INT_STATUS + (8 * i)) &
> -				dd->gi_mask[i];
> +		if (dd->gi_mask[i] == 0)
> +			value = 0;	/* used later */
> +		else
> +			value = read_csr(dd, CCE_INT_STATUS + (8 * i)) & dd->gi_mask[i];
> +
> +		/* save for further use */
> +		bitmap_from_u64(&pending[BITS_TO_LONGS(i * 64)], value);
> +
>  		/* only clear if anything is set */
> -		if (regs[i])
> -			write_csr(dd, CCE_INT_CLEAR + (8 * i), regs[i]);
> +		if (value)
> +			write_csr(dd, CCE_INT_CLEAR + (8 * i), value);
>  	}

I think I'd leave all that alone.

>  	/* phase 2: call the appropriate handler */
> -	for_each_set_bit(bit, (unsigned long *)&regs[0],
> -			 CCE_NUM_INT_CSRS * 64) {
> +	for_each_set_bit(bit, pending, CCE_NUM_INT_CSRS * 64)

And do something else for that loop instead.

>  		is_interrupt(dd, bit);
> -		handled = IRQ_HANDLED;
> -	}
> 
> -	return handled;
> +	return IRQ_RETVAL(!bitmap_empty(pending, CCE_NUM_INT_CSRS * 64));

You really don't want to scan the bitmap again.

Actually, of the face of it, you could merge the two loops.
Provided you clear the status bit before calling the relevant
handler I expect it will all work.

	David

>  }
> 
>  irqreturn_t sdma_interrupt(int irq, void *data)
> --
> 2.34.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)