| Message ID | 1519229532-11265-3-git-send-email-ajay.kathat@microchip.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
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
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 >
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 >> >
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",
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(-)