| Message ID | YFS5gx/gi70zlIaO@mwanda (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2f51061edab942988b1a3c057d21228e938603db |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wilc1000: fix a loop timeout condition | expand |
On 19/03/21 8:17 pm, Dan Carpenter wrote: > > If the loop fails, the "while(trials--) {" loop will exit with "trials" > set to -1. The test for that expects it to end with "trials" set to 0 > so the warning message will not be printed. > > Fix this by changing from a post-op to a pre-op. This does mean that > we only make 99 attempts instead of 100 but that's okay. > > Fixes: f135a1571a05 ("wilc1000: Support chip sleep over SPI") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Thanks Dan. Acked-by: Ajay Singh <ajay.kathat@microchip.com> > --- > drivers/net/wireless/microchip/wilc1000/wlan.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c > index d4a90c490084..2030fc7f53ca 100644 > --- a/drivers/net/wireless/microchip/wilc1000/wlan.c > +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c > @@ -575,7 +575,7 @@ void chip_allow_sleep(struct wilc *wilc) > to_host_from_fw_bit = WILC_SPI_FW_TO_HOST_BIT; > } > > - while (trials--) { > + while (--trials) { > ret = hif_func->hif_read_reg(wilc, to_host_from_fw_reg, ®); > if (ret) > return; > -- > 2.30.2 >
On Fri, 2021-03-19 at 16:09 +0000, Ajay.Kathat@microchip.com wrote: > On 19/03/21 8:17 pm, Dan Carpenter wrote: > > If the loop fails, the "while(trials--) {" loop will exit with "trials" > > set to -1. The test for that expects it to end with "trials" set to 0 > > so the warning message will not be printed. > > > > Fix this by changing from a post-op to a pre-op. This does mean that > > we only make 99 attempts instead of 100 but that's okay. > > > > Fixes: f135a1571a05 ("wilc1000: Support chip sleep over SPI") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Thanks Dan. Good catch, but wouldn't it be better to fix the time-out check condition instead? Something a long the lines of: --- drivers/net/wireless/microchip/wilc1000/wlan.c~ 2021-03-29 12:44:52.066039259 -0600 +++ drivers/net/wireless/microchip/wilc1000/wlan.c 2021-03-29 12:40:29.176365116 -0600 @@ -457,7 +457,7 @@ u32 wakeup_reg, wakeup_bit; u32 to_host_from_fw_reg, to_host_from_fw_bit; u32 from_host_to_fw_reg, from_host_to_fw_bit; - u32 trials = 100; + int trials = 100; int ret; if (wilc->io_type == WILC_HIF_SDIO) { @@ -483,7 +483,7 @@ if ((reg & to_host_from_fw_bit) == 0) break; } - if (!trials) + if (trials < 0) pr_warn("FW not responding\n"); /* Clear bit 1 */ This way, the loop could actually get executed the number of times indicated by the initialization of "trial" before issuing a warning message. --david > Acked-by: Ajay Singh <ajay.kathat@microchip.com> > > > --- > > drivers/net/wireless/microchip/wilc1000/wlan.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c > > index d4a90c490084..2030fc7f53ca 100644 > > --- a/drivers/net/wireless/microchip/wilc1000/wlan.c > > +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c > > @@ -575,7 +575,7 @@ void chip_allow_sleep(struct wilc *wilc) > > to_host_from_fw_bit = WILC_SPI_FW_TO_HOST_BIT; > > } > > > > - while (trials--) { > > + while (--trials) { > > ret = hif_func->hif_read_reg(wilc, to_host_from_fw_reg, ®); > > if (ret) > > return; > > -- > > 2.30.2 > >
On Mon, Mar 29, 2021 at 12:47:15PM -0600, David Mosberger-Tang wrote: > On Fri, 2021-03-19 at 16:09 +0000, Ajay.Kathat@microchip.com wrote: > > On 19/03/21 8:17 pm, Dan Carpenter wrote: > > > If the loop fails, the "while(trials--) {" loop will exit with "trials" > > > set to -1. The test for that expects it to end with "trials" set to 0 > > > so the warning message will not be printed. > > > > > > Fix this by changing from a post-op to a pre-op. This does mean that > > > we only make 99 attempts instead of 100 but that's okay. > > > > > > Fixes: f135a1571a05 ("wilc1000: Support chip sleep over SPI") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Thanks Dan. > > Good catch, but wouldn't it be better to fix the time-out check > condition instead? Something a long the lines of: > > --- drivers/net/wireless/microchip/wilc1000/wlan.c~ 2021-03-29 12:44:52.066039259 -0600 > +++ drivers/net/wireless/microchip/wilc1000/wlan.c 2021-03-29 12:40:29.176365116 -0600 > @@ -457,7 +457,7 @@ > u32 wakeup_reg, wakeup_bit; > u32 to_host_from_fw_reg, to_host_from_fw_bit; > u32 from_host_to_fw_reg, from_host_to_fw_bit; > - u32 trials = 100; > + int trials = 100; > int ret; > > if (wilc->io_type == WILC_HIF_SDIO) { > @@ -483,7 +483,7 @@ > if ((reg & to_host_from_fw_bit) == 0) > break; > } > - if (!trials) > + if (trials < 0) > pr_warn("FW not responding\n"); > > /* Clear bit 1 */ > > > This way, the loop could actually get executed the number of times > indicated by the initialization of "trial" before issuing a warning > message. Those numbers are just made up... It doesn't matter either way. regards, dan carpenter
Dan Carpenter <dan.carpenter@oracle.com> wrote: > If the loop fails, the "while(trials--) {" loop will exit with "trials" > set to -1. The test for that expects it to end with "trials" set to 0 > so the warning message will not be printed. > > Fix this by changing from a post-op to a pre-op. This does mean that > we only make 99 attempts instead of 100 but that's okay. > > Fixes: f135a1571a05 ("wilc1000: Support chip sleep over SPI") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > Acked-by: Ajay Singh <ajay.kathat@microchip.com> Patch applied to wireless-drivers-next.git, thanks. 2f51061edab9 wilc1000: fix a loop timeout condition
diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c index d4a90c490084..2030fc7f53ca 100644 --- a/drivers/net/wireless/microchip/wilc1000/wlan.c +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c @@ -575,7 +575,7 @@ void chip_allow_sleep(struct wilc *wilc) to_host_from_fw_bit = WILC_SPI_FW_TO_HOST_BIT; } - while (trials--) { + while (--trials) { ret = hif_func->hif_read_reg(wilc, to_host_from_fw_reg, ®); if (ret) return;
If the loop fails, the "while(trials--) {" loop will exit with "trials" set to -1. The test for that expects it to end with "trials" set to 0 so the warning message will not be printed. Fix this by changing from a post-op to a pre-op. This does mean that we only make 99 attempts instead of 100 but that's okay. Fixes: f135a1571a05 ("wilc1000: Support chip sleep over SPI") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/net/wireless/microchip/wilc1000/wlan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)