| Message ID | 1554552067-15421-1-git-send-email-hofrat@osadl.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [RFC] staging: wilc1000: give usleep_range a range | expand |
Hi Nicholas On 4/6/19 5:01 AM, Nicholas Mc Guire wrote: > External E-Mail > > > usleep_range() is called in non-atomic context so there is little point > in setting min==max the jitter of hrtimer is determined by interruptions > anyway. usleep_range can only perform the intended coalescence if some > room for placing the hrtimer is provided. Given the range of milliseconds > the delay will be anything from 2 to a few anyway - so make it 2-5 ms. > > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> > --- > > Problem located with an experimental coccinelle script > ./drivers/staging/wilc1000/wilc_wlan.c:411:4-16: WARNING: inefficient usleep_range with range 0 (min==max) > ./drivers/staging/wilc1000/wilc_wlan.c:426:4-16: WARNING: inefficient usleep_range with range 0 (min==max) > > Someone that knows the motivation for setting the time to 2 millisecond > might need to check if the 2 milliseconds where seen as tollerable max or > min - I'm assuming it was the min so extending. > > Patch was compile tested with: x86_64_defconfig + Staging=y, > WILC1000_SDIO=m, WILC1000_SPI=m, WILC1000=m > > Patch is against 5.1-rc3 (localversion-next is -next-20190405) > > drivers/staging/wilc1000/wilc_wlan.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) This > diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c > index c238969..42da533 100644 > --- a/drivers/staging/wilc1000/wilc_wlan.c > +++ b/drivers/staging/wilc1000/wilc_wlan.c > @@ -408,7 +408,7 @@ void chip_wakeup(struct wilc *wilc) > wilc->hif_func->hif_write_reg(wilc, 1, reg & ~BIT(1)); > > do { > - usleep_range(2 * 1000, 2 * 1000); > + usleep_range(2 * 1000, 5 * 1000); > wilc_get_chipid(wilc, true); > } while (wilc_get_chipid(wilc, true) == 0); > } while (wilc_get_chipid(wilc, true) == 0); > @@ -423,7 +423,7 @@ void chip_wakeup(struct wilc *wilc) > &clk_status_reg); > > while ((clk_status_reg & 0x1) == 0) { > - usleep_range(2 * 1000, 2 * 1000); > + usleep_range(2 * 1000, 5 * 1000); > > wilc->hif_func->hif_read_reg(wilc, 0xf1, > &clk_status_reg);
Hi Nicholas On 4/6/19 5:01 AM, Nicholas Mc Guire wrote: > External E-Mail > > > Someone that knows the motivation for setting the time to 2 millisecond > might need to check if the 2 milliseconds where seen as tollerable max or > min - I'm assuming it was the min so extending. 2 msec is the time the chip takes to wake up from sleep. Increasing the maximum to 5 msec will impact the throughput since this call is on the transmit path. > diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c > index c238969..42da533 100644 > --- a/drivers/staging/wilc1000/wilc_wlan.c > +++ b/drivers/staging/wilc1000/wilc_wlan.c > @@ -408,7 +408,7 @@ void chip_wakeup(struct wilc *wilc) > wilc->hif_func->hif_write_reg(wilc, 1, reg & ~BIT(1)); > > do { > - usleep_range(2 * 1000, 2 * 1000); > + usleep_range(2 * 1000, 5 * 1000); > wilc_get_chipid(wilc, true); > } while (wilc_get_chipid(wilc, true) == 0); > } while (wilc_get_chipid(wilc, true) == 0); > @@ -423,7 +423,7 @@ void chip_wakeup(struct wilc *wilc) > &clk_status_reg); > > while ((clk_status_reg & 0x1) == 0) { > - usleep_range(2 * 1000, 2 * 1000); > + usleep_range(2 * 1000, 5 * 1000); > > wilc->hif_func->hif_read_reg(wilc, 0xf1, > &clk_status_reg); Thanks, Adham
On Mon, Apr 08, 2019 at 09:10:00PM +0000, Adham.Abozaeid@microchip.com wrote: > Hi Nicholas > > On 4/6/19 5:01 AM, Nicholas Mc Guire wrote: > > External E-Mail > > > > > > Someone that knows the motivation for setting the time to 2 millisecond > > might need to check if the 2 milliseconds where seen as tollerable max or > > min - I'm assuming it was the min so extending. > > 2 msec is the time the chip takes to wake up from sleep. > > Increasing the maximum to 5 msec will impact the throughput since this call is on the transmit path. > ok - would it be tollerable to make it 2 - 2.5 ms ? even that would allow for the hrtimer subsystem to optimize a lot. In any case the min==max case gives you very little if you run a test-case with usleep_range(1000,1000) and a loop with usleep_range(1000,2000) and look at the distribution you will have a hard time seeing any difference. I doubt you would readily see the change from usleep_range(2000,2000) to usleep_range(2000,3000) in benchmarks - maybe (2000,5000) would be visible. My assumption (I have not analyzed it in detail) is that if you have a high re-use of existing timers that the setup of the timer is faster and thats why increasing the range > 0 can actually result in better jitter distribution. thx! hofrat > > diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c > > index c238969..42da533 100644 > > --- a/drivers/staging/wilc1000/wilc_wlan.c > > +++ b/drivers/staging/wilc1000/wilc_wlan.c > > @@ -408,7 +408,7 @@ void chip_wakeup(struct wilc *wilc) > > wilc->hif_func->hif_write_reg(wilc, 1, reg & ~BIT(1)); > > > > do { > > - usleep_range(2 * 1000, 2 * 1000); > > + usleep_range(2 * 1000, 5 * 1000); > > wilc_get_chipid(wilc, true); > > } while (wilc_get_chipid(wilc, true) == 0); > > } while (wilc_get_chipid(wilc, true) == 0); > > @@ -423,7 +423,7 @@ void chip_wakeup(struct wilc *wilc) > > &clk_status_reg); > > > > while ((clk_status_reg & 0x1) == 0) { > > - usleep_range(2 * 1000, 2 * 1000); > > + usleep_range(2 * 1000, 5 * 1000); > > > > wilc->hif_func->hif_read_reg(wilc, 0xf1, > > &clk_status_reg); > > > Thanks, > > Adham >
Hi Nicolas On 4/8/19 6:36 PM, Nicholas Mc Guire wrote: > On Mon, Apr 08, 2019 at 09:10:00PM +0000, Adham.Abozaeid@microchip.com wrote: >> Hi Nicholas >> >> On 4/6/19 5:01 AM, Nicholas Mc Guire wrote: >>> External E-Mail >>> >>> >>> Someone that knows the motivation for setting the time to 2 millisecond >>> might need to check if the 2 milliseconds where seen as tollerable max or >>> min - I'm assuming it was the min so extending. >> 2 msec is the time the chip takes to wake up from sleep. >> >> Increasing the maximum to 5 msec will impact the throughput since this call is on the transmit path. >> > ok - would it be tollerable to make it 2 - 2.5 ms ? > even that would allow for the hrtimer subsystem to optimize > a lot. In any case the min==max case gives you very little > if you run a test-case with usleep_range(1000,1000) and > a loop with usleep_range(1000,2000) and look at the distribution > you will have a hard time seeing any difference. yes, I believe 2.5 shouldn't be a problem. Thanks, Adham
On Wed, Apr 10, 2019 at 06:31:21PM +0000, Adham.Abozaeid@microchip.com wrote: > Hi Nicolas > > On 4/8/19 6:36 PM, Nicholas Mc Guire wrote: > > On Mon, Apr 08, 2019 at 09:10:00PM +0000, Adham.Abozaeid@microchip.com wrote: > >> Hi Nicholas > >> > >> On 4/6/19 5:01 AM, Nicholas Mc Guire wrote: > >>> External E-Mail > >>> > >>> > >>> Someone that knows the motivation for setting the time to 2 millisecond > >>> might need to check if the 2 milliseconds where seen as tollerable max or > >>> min - I'm assuming it was the min so extending. > >> 2 msec is the time the chip takes to wake up from sleep. > >> > >> Increasing the maximum to 5 msec will impact the throughput since this call is on the transmit path. > >> > > ok - would it be tollerable to make it 2 - 2.5 ms ? > > even that would allow for the hrtimer subsystem to optimize > > a lot. In any case the min==max case gives you very little > > if you run a test-case with usleep_range(1000,1000) and > > a loop with usleep_range(1000,2000) and look at the distribution > > you will have a hard time seeing any difference. > > yes, I believe 2.5 shouldn't be a problem. > thanks - will send out a V2 then shortly. thx! hofrat
diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c index c238969..42da533 100644 --- a/drivers/staging/wilc1000/wilc_wlan.c +++ b/drivers/staging/wilc1000/wilc_wlan.c @@ -408,7 +408,7 @@ void chip_wakeup(struct wilc *wilc) wilc->hif_func->hif_write_reg(wilc, 1, reg & ~BIT(1)); do { - usleep_range(2 * 1000, 2 * 1000); + usleep_range(2 * 1000, 5 * 1000); wilc_get_chipid(wilc, true); } while (wilc_get_chipid(wilc, true) == 0); } while (wilc_get_chipid(wilc, true) == 0); @@ -423,7 +423,7 @@ void chip_wakeup(struct wilc *wilc) &clk_status_reg); while ((clk_status_reg & 0x1) == 0) { - usleep_range(2 * 1000, 2 * 1000); + usleep_range(2 * 1000, 5 * 1000); wilc->hif_func->hif_read_reg(wilc, 0xf1, &clk_status_reg);
usleep_range() is called in non-atomic context so there is little point in setting min==max the jitter of hrtimer is determined by interruptions anyway. usleep_range can only perform the intended coalescence if some room for placing the hrtimer is provided. Given the range of milliseconds the delay will be anything from 2 to a few anyway - so make it 2-5 ms. Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org> --- Problem located with an experimental coccinelle script ./drivers/staging/wilc1000/wilc_wlan.c:411:4-16: WARNING: inefficient usleep_range with range 0 (min==max) ./drivers/staging/wilc1000/wilc_wlan.c:426:4-16: WARNING: inefficient usleep_range with range 0 (min==max) Someone that knows the motivation for setting the time to 2 millisecond might need to check if the 2 milliseconds where seen as tollerable max or min - I'm assuming it was the min so extending. Patch was compile tested with: x86_64_defconfig + Staging=y, WILC1000_SDIO=m, WILC1000_SPI=m, WILC1000=m Patch is against 5.1-rc3 (localversion-next is -next-20190405) drivers/staging/wilc1000/wilc_wlan.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)