[PATCH 0/4] staging: wilc1000: modify functions by making use of GENMASK macro

[PATCH 0/4] staging: wilc1000: modify functions by making use of GENMASK macro

[Ajay Singh]

This patch series contains changes to refactor functions by making use of
GENMASK macro and also removed unnecessary variable from
wilc_spi_read_int().

Ajay Singh (4):
  staging: wilc1000: remove use of 'happened' variable in
    wilc_spi_read_int()
  staging: wilc1000: modified wilc_spi_read_int() by using GENMASK macro
  staging: wilc1000: refactor wilc_spi_clear_int_ext() by using GENMASK
    macro
  staging: wilc1000: refactor sdio_clear_int_ext() by using GENMASK
    macro

 drivers/staging/wilc1000/wilc_sdio.c | 74 +++++++++++++++---------------------
 drivers/staging/wilc1000/wilc_spi.c  | 57 +++++++++++----------------
 2 files changed, 53 insertions(+), 78 deletions(-)

-- 
2.7.4

[PATCH 1/4] staging: wilc1000: remove use of 'happened' variable in wilc_spi_read_int()

[Ajay Singh]

Modified wilc_spi_read_int() by removing unnecessary use of "happened"
variable.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_spi.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
index 6b392c9..131d2b7 100644
--- a/drivers/staging/wilc1000/wilc_spi.c
+++ b/drivers/staging/wilc1000/wilc_spi.c
@@ -936,7 +936,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
 	int ret;
 	u32 tmp;
 	u32 byte_cnt;
-	int happened, j;
+	int j;
 	u32 unknown_mask;
 	u32 irq_flags;
 	int k = IRG_FLAGS_OFFSET + 5;
@@ -956,8 +956,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
 
 	j = 0;
 	do {
-		happened = 0;
-
 		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
 		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
 
@@ -972,11 +970,11 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
 			dev_err(&spi->dev,
 				"Unexpected interrupt(2):j=%d,tmp=%x,mask=%x\n",
 				j, tmp, unknown_mask);
-				happened = 1;
+				break;
 		}
 
 		j++;
-	} while (happened);
+	} while (1);
 
 	*int_status = tmp;
 
-- 
2.7.4

[PATCH 2/4] staging: wilc1000: modified wilc_spi_read_int() by using GENMASK macro

[Ajay Singh]

Use existing macro GENMASK to get the bitmask value. Moved the code to
get the bitmask value outside the loop, as its only required one time.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_spi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
index 131d2b7..c63f534 100644
--- a/drivers/staging/wilc1000/wilc_spi.c
+++ b/drivers/staging/wilc1000/wilc_spi.c
@@ -955,6 +955,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
 	tmp = (byte_cnt >> 2) & IRQ_DMA_WD_CNT_MASK;
 
 	j = 0;
+	unknown_mask = GENMASK(g_spi.nint - 1, 0);
 	do {
 		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
 		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
@@ -964,8 +965,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
 			tmp |= (((irq_flags >> 0) & 0x7) << k);
 		}
 
-		unknown_mask = ~((1ul << g_spi.nint) - 1);
-
 		if ((tmp >> IRG_FLAGS_OFFSET) & unknown_mask) {
 			dev_err(&spi->dev,
 				"Unexpected interrupt(2):j=%d,tmp=%x,mask=%x\n",
-- 
2.7.4

[PATCH 3/4] staging: wilc1000: refactor wilc_spi_clear_int_ext() by using GENMASK macro

[Ajay Singh]

Use available macro GENMASK to get the bitmask value of specific value.
Simplified the logic by adding expected_irqs & unexpected_irqs to check
the interrupt bits.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_spi.c | 46 +++++++++++++++----------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
index c63f534..12827c2 100644
--- a/drivers/staging/wilc1000/wilc_spi.c
+++ b/drivers/staging/wilc1000/wilc_spi.c
@@ -985,8 +985,9 @@ static int wilc_spi_clear_int_ext(struct wilc *wilc, u32 val)
 {
 	struct spi_device *spi = to_spi_device(wilc->dev);
 	int ret;
-	u32 flags;
 	u32 tbl_ctl;
+	u32 expected_irqs, unexpected_irqs;
+	int i;
 
 	if (g_spi.has_thrpt_enh) {
 		ret = spi_internal_write(wilc, 0xe844 - WILC_SPI_REG_BASE,
@@ -994,37 +995,26 @@ static int wilc_spi_clear_int_ext(struct wilc *wilc, u32 val)
 		return ret;
 	}
 
-	flags = val & (BIT(MAX_NUM_INT) - 1);
-	if (flags) {
-		int i;
+	expected_irqs = val & GENMASK(g_spi.nint - 1, 0);
+	unexpected_irqs = val & GENMASK(MAX_NUM_INT - 1, g_spi.nint);
 
-		ret = 1;
-		for (i = 0; i < g_spi.nint; i++) {
-			/*
-			 * No matter what you write 1 or 0,
-			 * it will clear interrupt.
-			 */
-			if (flags & 1)
-				ret = wilc_spi_write_reg(wilc,
-							 0x10c8 + i * 4, 1);
-			if (!ret)
-				break;
-			flags >>= 1;
-		}
-		if (!ret) {
-			dev_err(&spi->dev,
-				"Failed wilc_spi_write_reg, set reg %x ...\n",
-				0x10c8 + i * 4);
-			goto _fail_;
-		}
-		for (i = g_spi.nint; i < MAX_NUM_INT; i++) {
-			if (flags & 1)
+	for (i = 0; i < g_spi.nint && expected_irqs; i++) {
+		/* No matter what you write 1 or 0, it will clear interrupt. */
+		if (expected_irqs & BIT(i)) {
+			ret = wilc_spi_write_reg(wilc, 0x10c8 + i * 4, 1);
+			if (!ret) {
 				dev_err(&spi->dev,
-					"Unexpected interrupt cleared %d...\n",
-					i);
-			flags >>= 1;
+					"Failed wilc_spi_write_reg, set reg %x ...\n",
+					0x10c8 + i * 4);
+				goto _fail_;
+			}
 		}
 	}
+	for (i = g_spi.nint; i < MAX_NUM_INT && unexpected_irqs; i++) {
+		if (unexpected_irqs & BIT(i))
+			dev_err(&spi->dev,
+				"Unexpected interrupt cleared %d...\n", i);
+	}
 
 	tbl_ctl = 0;
 	/* select VMM table 0 */
-- 
2.7.4

[PATCH 4/4] staging: wilc1000: refactor sdio_clear_int_ext() by using GENMASK macro

[Ajay Singh]

Use GENMASK macro in sdio_clear_int_ext function to get the required
bitmask value. Simplified the logic by making use of GENMASK
macro.

Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
---
 drivers/staging/wilc1000/wilc_sdio.c | 74 +++++++++++++++---------------------
 1 file changed, 31 insertions(+), 43 deletions(-)

diff --git a/drivers/staging/wilc1000/wilc_sdio.c b/drivers/staging/wilc1000/wilc_sdio.c
index a088999..8218085 100644
--- a/drivers/staging/wilc1000/wilc_sdio.c
+++ b/drivers/staging/wilc1000/wilc_sdio.c
@@ -876,14 +876,9 @@ static int sdio_clear_int_ext(struct wilc *wilc, u32 val)
 	if (g_sdio.has_thrpt_enh3) {
 		u32 reg;
 
-		if (g_sdio.irq_gpio) {
-			u32 flags;
+		reg = g_sdio.irq_gpio ?
+			(val & GENMASK(MAX_NUN_INT_THRPT_ENH2 - 1, 0)) : 0;
 
-			flags = val & (BIT(MAX_NUN_INT_THRPT_ENH2) - 1);
-			reg = flags;
-		} else {
-			reg = 0;
-		}
 		/* select VMM table 0 */
 		if ((val & SEL_VMM_TBL0) == SEL_VMM_TBL0)
 			reg |= BIT(5);
@@ -918,45 +913,38 @@ static int sdio_clear_int_ext(struct wilc *wilc, u32 val)
 		 * Cannot clear multiple interrupts.
 		 * Must clear each interrupt individually.
 		 */
-		u32 flags;
-
-		flags = val & (BIT(MAX_NUM_INT) - 1);
-		if (flags) {
-			int i;
-
-			ret = 1;
-			for (i = 0; i < g_sdio.nint; i++) {
-				if (flags & 1) {
-					struct sdio_cmd52 cmd;
-
-					cmd.read_write = 1;
-					cmd.function = 0;
-					cmd.raw = 0;
-					cmd.address = 0xf8;
-					cmd.data = BIT(i);
-
-					ret = wilc_sdio_cmd52(wilc, &cmd);
-					if (ret) {
-						dev_err(&func->dev,
-							"Failed cmd52, set 0xf8 data (%d) ...\n",
-							__LINE__);
-						goto _fail_;
-					}
-				}
-				if (!ret)
-					break;
-				flags >>= 1;
-			}
-			if (!ret)
-				goto _fail_;
-			for (i = g_sdio.nint; i < MAX_NUM_INT; i++) {
-				if (flags & 1)
+		int i;
+		u32 expected_irqs, unexpected_irqs;
+
+		expected_irqs = val & GENMASK(g_sdio.nint - 1, 0);
+		unexpected_irqs = val & GENMASK(MAX_NUM_INT - 1, g_sdio.nint);
+
+		for (i = 0; i < g_sdio.nint && expected_irqs; i++) {
+			if (expected_irqs & BIT(i)) {
+				struct sdio_cmd52 cmd;
+
+				cmd.read_write = 1;
+				cmd.function = 0;
+				cmd.raw = 0;
+				cmd.address = 0xf8;
+				cmd.data = BIT(i);
+
+				ret = wilc_sdio_cmd52(wilc, &cmd);
+				if (ret) {
 					dev_err(&func->dev,
-						"Unexpected interrupt cleared %d...\n",
-						i);
-				flags >>= 1;
+						"Failed cmd52, set 0xf8 data (%d) ...\n",
+						__LINE__);
+					goto _fail_;
+				}
 			}
 		}
+
+		for (i = g_sdio.nint; i < MAX_NUM_INT && unexpected_irqs; i++) {
+			if (unexpected_irqs & BIT(i))
+				dev_err(&func->dev,
+					"Unexpected interrupt cleared %d...\n",
+					i);
+		}
 	}
 
 	vmm_ctl = 0;
-- 
2.7.4

Re: [PATCH 1/4] staging: wilc1000: remove use of 'happened' variable in wilc_spi_read_int()

[Dan Carpenter]

On Wed, Feb 21, 2018 at 09:42:09PM +0530, Ajay Singh wrote:
> Modified wilc_spi_read_int() by removing unnecessary use of "happened"
> variable.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/wilc_spi.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
> index 6b392c9..131d2b7 100644
> --- a/drivers/staging/wilc1000/wilc_spi.c
> +++ b/drivers/staging/wilc1000/wilc_spi.c
> @@ -936,7 +936,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>  	int ret;
>  	u32 tmp;
>  	u32 byte_cnt;
> -	int happened, j;
> +	int j;
>  	u32 unknown_mask;
>  	u32 irq_flags;
>  	int k = IRG_FLAGS_OFFSET + 5;
> @@ -956,8 +956,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>  
>  	j = 0;
>  	do {
> -		happened = 0;
> -
>  		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
>  		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
>  
> @@ -972,11 +970,11 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>  			dev_err(&spi->dev,
>  				"Unexpected interrupt(2):j=%d,tmp=%x,mask=%x\n",
>  				j, tmp, unknown_mask);
> -				happened = 1;
> +				break;

This is flipped around.  happened means don't break, but you've changed
it to be the opposite.

regards,
dan carpenter

>  		}
>  
>  		j++;
> -	} while (happened);
> +	} while (1);
>  

Re: [PATCH 2/4] staging: wilc1000: modified wilc_spi_read_int() by using GENMASK macro

[Dan Carpenter]

On Wed, Feb 21, 2018 at 09:42:10PM +0530, Ajay Singh wrote:
> Use existing macro GENMASK to get the bitmask value. Moved the code to
> get the bitmask value outside the loop, as its only required one time.
> 
> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> ---
>  drivers/staging/wilc1000/wilc_spi.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
> index 131d2b7..c63f534 100644
> --- a/drivers/staging/wilc1000/wilc_spi.c
> +++ b/drivers/staging/wilc1000/wilc_spi.c
> @@ -955,6 +955,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>  	tmp = (byte_cnt >> 2) & IRQ_DMA_WD_CNT_MASK;
>  
>  	j = 0;
> +	unknown_mask = GENMASK(g_spi.nint - 1, 0);
>  	do {
>  		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
>  		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
> @@ -964,8 +965,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>  			tmp |= (((irq_flags >> 0) & 0x7) << k);
>  		}
>  
> -		unknown_mask = ~((1ul << g_spi.nint) - 1);
> -

This isn't right at all...  Say g_spi.nint is zero, then we're doing
GENMASK(-1, 0) which seems like it should be undefined.  If g_spi.nint
is 1 then "GENMASK(1 - 1, 0)" is 0x1 but "~((1 < 1) - 1)" is ~0x1.

I'm done reviewing this patch series...  You need to be more careful.
Create a small test program to test your patches.  As a reviewer,
creating test programs is how I review your patches.

#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <limits.h>
#include <string.h>
#include "/home/dcarpenter/progs/smatch/devel/check_debug.h"
#include "kernel.h"
#include <assert.h>
#include <fcntl.h>
#include <sys/time.h>
#include <sys/ioctl.h>
#include <linux/blktrace_api.h>
#include <linux/fs.h>

#define BITS_PER_LONG 64
#define GENMASK(h, l) \
        (((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h))))

int main(void)
{
        int i;
        u32 mask1, mask2;

        for (i = 0; i < 32; i++) {
                mask1 = ~((1ul << i) - 1);
                mask2 = GENMASK(i - 1, 0);
                if (mask1 == mask2)
                        continue;
                printf("ONE 0x%x %d\n", mask1, i);
                printf("TWO 0x%x\n", mask2);
        }
        return 0;
}

regards,
dan carpenter

Re: [PATCH 3/4] staging: wilc1000: refactor wilc_spi_clear_int_ext() by using GENMASK macro

[Dan Carpenter]

Please check all these again, right?  I've glanced at this and it seems
wrong, but I'm too stupid to sure immediately and too lazy to be
thourough.

regards,
dan caprenter

Re: [PATCH 1/4] staging: wilc1000: remove use of 'happened' variable in wilc_spi_read_int()

[Ajay Singh]

Hi Dan,

On Thu, 22 Feb 2018 10:20:58 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Wed, Feb 21, 2018 at 09:42:09PM +0530, Ajay Singh wrote:
> > Modified wilc_spi_read_int() by removing unnecessary use of "happened"
> > variable.
> > 
> > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > ---
> >  drivers/staging/wilc1000/wilc_spi.c | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
> > index 6b392c9..131d2b7 100644
> > --- a/drivers/staging/wilc1000/wilc_spi.c
> > +++ b/drivers/staging/wilc1000/wilc_spi.c
> > @@ -936,7 +936,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
> >  	int ret;
> >  	u32 tmp;
> >  	u32 byte_cnt;
> > -	int happened, j;
> > +	int j;
> >  	u32 unknown_mask;
> >  	u32 irq_flags;
> >  	int k = IRG_FLAGS_OFFSET + 5;
> > @@ -956,8 +956,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
> >  
> >  	j = 0;
> >  	do {
> > -		happened = 0;
> > -
> >  		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
> >  		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
> >  
> > @@ -972,11 +970,11 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
> >  			dev_err(&spi->dev,
> >  				"Unexpected interrupt(2):j=%d,tmp=%x,mask=%x\n",
> >  				j, tmp, unknown_mask);
> > -				happened = 1;
> > +				break;  
> 
> This is flipped around.  happened means don't break, but you've changed
> it to be the opposite.

You are right. Thanks for pointing it out. It's was a mistake. I will
change 'break' to 'continue' and while(1) to while(0) and resubmit the
patch.


Regards,
Ajay

> 
> regards,
> dan carpenter
> 
> >  		}
> >  
> >  		j++;
> > -	} while (happened);
> > +	} while (1);
> >    
> 

Re: [PATCH 1/4] staging: wilc1000: remove use of 'happened' variable in wilc_spi_read_int()

[Dan Carpenter]

On Thu, Feb 22, 2018 at 02:09:01PM +0530, Ajay Singh wrote:
> Hi Dan,
> 
> On Thu, 22 Feb 2018 10:20:58 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > On Wed, Feb 21, 2018 at 09:42:09PM +0530, Ajay Singh wrote:
> > > Modified wilc_spi_read_int() by removing unnecessary use of "happened"
> > > variable.
> > > 
> > > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
> > > ---
> > >  drivers/staging/wilc1000/wilc_spi.c | 8 +++-----
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
> > > index 6b392c9..131d2b7 100644
> > > --- a/drivers/staging/wilc1000/wilc_spi.c
> > > +++ b/drivers/staging/wilc1000/wilc_spi.c
> > > @@ -936,7 +936,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
> > >  	int ret;
> > >  	u32 tmp;
> > >  	u32 byte_cnt;
> > > -	int happened, j;
> > > +	int j;
> > >  	u32 unknown_mask;
> > >  	u32 irq_flags;
> > >  	int k = IRG_FLAGS_OFFSET + 5;
> > > @@ -956,8 +956,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
> > >  
> > >  	j = 0;
> > >  	do {
> > > -		happened = 0;
> > > -
> > >  		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
> > >  		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
> > >  
> > > @@ -972,11 +970,11 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
> > >  			dev_err(&spi->dev,
> > >  				"Unexpected interrupt(2):j=%d,tmp=%x,mask=%x\n",
> > >  				j, tmp, unknown_mask);
> > > -				happened = 1;
> > > +				break;  
> > 
> > This is flipped around.  happened means don't break, but you've changed
> > it to be the opposite.
> 
> You are right. Thanks for pointing it out. It's was a mistake. I will
> change 'break' to 'continue' and while(1) to while(0) and resubmit the
> patch.
> 

Don't be in a hurry to resend.  I always wait over night before
resending so that I'm not stressed when I review it.  What you are
proposing still sounds wrong because the j++ is essential.  Anyway, I
can't really review your v2 patch until you send it.

regards,
dan carpenter

Re: [PATCH 2/4] staging: wilc1000: modified wilc_spi_read_int() by using GENMASK macro

[Claudiu Beznea]

Hi Dan,

On 22.02.2018 09:37, Dan Carpenter wrote:
> On Wed, Feb 21, 2018 at 09:42:10PM +0530, Ajay Singh wrote:
>> Use existing macro GENMASK to get the bitmask value. Moved the code to
>> get the bitmask value outside the loop, as its only required one time.
>>
>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>> ---
>>  drivers/staging/wilc1000/wilc_spi.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
>> index 131d2b7..c63f534 100644
>> --- a/drivers/staging/wilc1000/wilc_spi.c
>> +++ b/drivers/staging/wilc1000/wilc_spi.c
>> @@ -955,6 +955,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>>  	tmp = (byte_cnt >> 2) & IRQ_DMA_WD_CNT_MASK;
>>  
>>  	j = 0;
>> +	unknown_mask = GENMASK(g_spi.nint - 1, 0);
>>  	do {
>>  		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
>>  		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
>> @@ -964,8 +965,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>>  			tmp |= (((irq_flags >> 0) & 0x7) << k);
>>  		}
>>  
>> -		unknown_mask = ~((1ul << g_spi.nint) - 1);
>> -
> 
> This isn't right at all...  Say g_spi.nint is zero, then we're doing
> GENMASK(-1, 0) which seems like it should be undefined.  If g_spi.nint
> is 1 then "GENMASK(1 - 1, 0)" is 0x1 but "~((1 < 1) - 1)" is ~0x1.
> 

Even in this driver g_spi.nint is always constant and equal with NUM_INT_EXT
(which is 3), it seems that enabling interrupts before initializing g_spi.nint
may lead to the state where g_spi.nint = 0, as Dan pointed.

int wilc1000_wlan_init(struct net_device *dev, struct wilc_vif *vif)
{
		// ...
                if (wl->gpio >= 0 && init_irq(dev)) {
                        ret = -EIO;
                        goto _fail_locks_;
                }

		// ...
                ret = linux_wlan_start_firmware(dev); -> calls
linux_wlan_start_firmware which in turn calls wilc->hif_func->hif_sync_ext(wilc,
NUM_INT_EXT);

	
}

Thank you,
Claudiu Beznea

> I'm done reviewing this patch series...  You need to be more careful.
> Create a small test program to test your patches.  As a reviewer,
> creating test programs is how I review your patches.
> 
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <limits.h>
> #include <string.h>
> #include "/home/dcarpenter/progs/smatch/devel/check_debug.h"
> #include "kernel.h"
> #include <assert.h>
> #include <fcntl.h>
> #include <sys/time.h>
> #include <sys/ioctl.h>
> #include <linux/blktrace_api.h>
> #include <linux/fs.h>
> 
> #define BITS_PER_LONG 64
> #define GENMASK(h, l) \
>         (((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
> 
> int main(void)
> {
>         int i;
>         u32 mask1, mask2;
> 
>         for (i = 0; i < 32; i++) {
>                 mask1 = ~((1ul << i) - 1);
>                 mask2 = GENMASK(i - 1, 0);
>                 if (mask1 == mask2)
>                         continue;
>                 printf("ONE 0x%x %d\n", mask1, i);
>                 printf("TWO 0x%x\n", mask2);
>         }
>         return 0;
> }
> 
> regards,
> dan carpenter
> 
> 
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
> 

Re: [PATCH 2/4] staging: wilc1000: modified wilc_spi_read_int() by using GENMASK macro

[Claudiu Beznea]

On 22.02.2018 11:23, Claudiu Beznea wrote:
> Hi Dan,
Sorry, I intended to be address this to Ajay,
> 
> On 22.02.2018 09:37, Dan Carpenter wrote:
>> On Wed, Feb 21, 2018 at 09:42:10PM +0530, Ajay Singh wrote:
>>> Use existing macro GENMASK to get the bitmask value. Moved the code to
>>> get the bitmask value outside the loop, as its only required one time.
>>>
>>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com>
>>> ---
>>>  drivers/staging/wilc1000/wilc_spi.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/staging/wilc1000/wilc_spi.c b/drivers/staging/wilc1000/wilc_spi.c
>>> index 131d2b7..c63f534 100644
>>> --- a/drivers/staging/wilc1000/wilc_spi.c
>>> +++ b/drivers/staging/wilc1000/wilc_spi.c
>>> @@ -955,6 +955,7 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>>>  	tmp = (byte_cnt >> 2) & IRQ_DMA_WD_CNT_MASK;
>>>  
>>>  	j = 0;
>>> +	unknown_mask = GENMASK(g_spi.nint - 1, 0);
>>>  	do {
>>>  		wilc_spi_read_reg(wilc, 0x1a90, &irq_flags);
>>>  		tmp |= ((irq_flags >> 27) << IRG_FLAGS_OFFSET);
>>> @@ -964,8 +965,6 @@ static int wilc_spi_read_int(struct wilc *wilc, u32 *int_status)
>>>  			tmp |= (((irq_flags >> 0) & 0x7) << k);
>>>  		}
>>>  
>>> -		unknown_mask = ~((1ul << g_spi.nint) - 1);
>>> -
>>
>> This isn't right at all...  Say g_spi.nint is zero, then we're doing
>> GENMASK(-1, 0) which seems like it should be undefined.  If g_spi.nint
>> is 1 then "GENMASK(1 - 1, 0)" is 0x1 but "~((1 < 1) - 1)" is ~0x1.
>>
> 
> Even in this driver g_spi.nint is always constant and equal with NUM_INT_EXT
> (which is 3), it seems that enabling interrupts before initializing g_spi.nint
> may lead to the state where g_spi.nint = 0, as Dan pointed.
> 
> int wilc1000_wlan_init(struct net_device *dev, struct wilc_vif *vif)
> {
> 		// ...
>                 if (wl->gpio >= 0 && init_irq(dev)) {
>                         ret = -EIO;
>                         goto _fail_locks_;
>                 }
> 
> 		// ...
>                 ret = linux_wlan_start_firmware(dev); -> calls
> linux_wlan_start_firmware which in turn calls wilc->hif_func->hif_sync_ext(wilc,
> NUM_INT_EXT);
> 
> 	
> }
> 
> Thank you,
> Claudiu Beznea
> 
>> I'm done reviewing this patch series...  You need to be more careful.
>> Create a small test program to test your patches.  As a reviewer,
>> creating test programs is how I review your patches.
>>
>> #include <sys/types.h>
>> #include <sys/stat.h>
>> #include <fcntl.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> #include <limits.h>
>> #include <string.h>
>> #include "/home/dcarpenter/progs/smatch/devel/check_debug.h"
>> #include "kernel.h"
>> #include <assert.h>
>> #include <fcntl.h>
>> #include <sys/time.h>
>> #include <sys/ioctl.h>
>> #include <linux/blktrace_api.h>
>> #include <linux/fs.h>
>>
>> #define BITS_PER_LONG 64
>> #define GENMASK(h, l) \
>>         (((~0UL) - (1UL << (l)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (h))))
>>
>> int main(void)
>> {
>>         int i;
>>         u32 mask1, mask2;
>>
>>         for (i = 0; i < 32; i++) {
>>                 mask1 = ~((1ul << i) - 1);
>>                 mask2 = GENMASK(i - 1, 0);
>>                 if (mask1 == mask2)
>>                         continue;
>>                 printf("ONE 0x%x %d\n", mask1, i);
>>                 printf("TWO 0x%x\n", mask2);
>>         }
>>         return 0;
>> }
>>
>> regards,
>> dan carpenter
>>
>>
>>
>> _______________________________________________
>> devel mailing list
>> devel@linuxdriverproject.org
>> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
>>
> 

Re: [PATCH 0/4] staging: wilc1000: modify functions by making use of GENMASK macro

[Ajay Singh]

Please ignore this patch series, I will work on review comments and submit
another version later.
 
Regards,
Ajay