Providing a range for usleep_range() allows the hrtimer subsystem to
coalesce timers - the delay is runtime configurable so a factor 2
is taken to provide the range.
Signed-off-by: Nicholas Mc Guire <[email protected]>
---
Problem located with an experimental coccinelle script
Q: Basically usleep_range() with min == max never makes much sense notably
in non-atomic context. If the factor of 2 is tolerable or a fixed
offset of e.g. 1000 would be more suitable is not clear to me - maybe
someone familiar with that driver can clarify this.
Patch was compile tested with: u8500_defconfig (implies COMMON_CLK=y)
(with some sparse warnings about not implemented system calls)
Patch is against 5.1-rc3 (localversion-next is next=20190405)
drivers/clk/ux500/clk-sysctrl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c
index 7c0403b..a1fa3fb 100644
--- a/drivers/clk/ux500/clk-sysctrl.c
+++ b/drivers/clk/ux500/clk-sysctrl.c
@@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw)
clk->reg_bits[0]);
if (!ret && clk->enable_delay_us)
- usleep_range(clk->enable_delay_us, clk->enable_delay_us);
+ usleep_range(clk->enable_delay_us, clk->enable_delay_us*2);
return ret;
}
--
2.1.4
Quoting Nicholas Mc Guire (2019-04-06 20:13:24)
> Providing a range for usleep_range() allows the hrtimer subsystem to
> coalesce timers - the delay is runtime configurable so a factor 2
> is taken to provide the range.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> ---
I think this driver is in maintenance mode. I'll wait for Ulf to ack or
review this change before applying.
> diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c
> index 7c0403b..a1fa3fb 100644
> --- a/drivers/clk/ux500/clk-sysctrl.c
> +++ b/drivers/clk/ux500/clk-sysctrl.c
> @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw)
> clk->reg_bits[0]);
>
> if (!ret && clk->enable_delay_us)
> - usleep_range(clk->enable_delay_us, clk->enable_delay_us);
> + usleep_range(clk->enable_delay_us, clk->enable_delay_us*2);
Please add space around that multiply.
On Wed, Apr 10, 2019 at 03:53:51PM -0700, Stephen Boyd wrote:
> Quoting Nicholas Mc Guire (2019-04-06 20:13:24)
> > Providing a range for usleep_range() allows the hrtimer subsystem to
> > coalesce timers - the delay is runtime configurable so a factor 2
> > is taken to provide the range.
> >
> > Signed-off-by: Nicholas Mc Guire <[email protected]>
> > ---
>
> I think this driver is in maintenance mode. I'll wait for Ulf to ack or
> review this change before applying.
>
> > diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c
> > index 7c0403b..a1fa3fb 100644
> > --- a/drivers/clk/ux500/clk-sysctrl.c
> > +++ b/drivers/clk/ux500/clk-sysctrl.c
> > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw)
> > clk->reg_bits[0]);
> >
> > if (!ret && clk->enable_delay_us)
> > - usleep_range(clk->enable_delay_us, clk->enable_delay_us);
> > + usleep_range(clk->enable_delay_us, clk->enable_delay_us*2);
>
> Please add space around that multiply.
>
I can do that but it does not seem common and also checkpatch
did not complain about this - now a simple grep -re "\*10" on the
kernel shows that it seems more common not to use spaces around *
that to use them. Greping specifically for cases using usleep_range()
(not that many) it seems more or less evenly devided between space
and no space - so the concern is overlooking that factor 2 ?
thx!
hofrat
On Sun, 7 Apr 2019 at 05:13, Nicholas Mc Guire <[email protected]> wrote:
>
> Providing a range for usleep_range() allows the hrtimer subsystem to
> coalesce timers - the delay is runtime configurable so a factor 2
> is taken to provide the range.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> ---
>
> Problem located with an experimental coccinelle script
>
> Q: Basically usleep_range() with min == max never makes much sense notably
> in non-atomic context. If the factor of 2 is tolerable or a fixed
> offset of e.g. 1000 would be more suitable is not clear to me - maybe
> someone familiar with that driver can clarify this.
>
> Patch was compile tested with: u8500_defconfig (implies COMMON_CLK=y)
> (with some sparse warnings about not implemented system calls)
>
> Patch is against 5.1-rc3 (localversion-next is next=20190405)
>
> drivers/clk/ux500/clk-sysctrl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c
> index 7c0403b..a1fa3fb 100644
> --- a/drivers/clk/ux500/clk-sysctrl.c
> +++ b/drivers/clk/ux500/clk-sysctrl.c
> @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw)
> clk->reg_bits[0]);
>
> if (!ret && clk->enable_delay_us)
> - usleep_range(clk->enable_delay_us, clk->enable_delay_us);
> + usleep_range(clk->enable_delay_us, clk->enable_delay_us*2);
The range being used is actually in ms, so not sure we actually need
to double it for the range.
How about adding ~25% instead, along the lines of below:
usleep_range(clk->enable_delay_us, clk->enable_delay_us +
(clk->enable_delay_us >> 2));
Kind regards
Uffe
On Thu, Apr 11, 2019 at 11:36:45AM +0200, Ulf Hansson wrote:
> On Sun, 7 Apr 2019 at 05:13, Nicholas Mc Guire <[email protected]> wrote:
> >
> > Providing a range for usleep_range() allows the hrtimer subsystem to
> > coalesce timers - the delay is runtime configurable so a factor 2
> > is taken to provide the range.
> >
> > Signed-off-by: Nicholas Mc Guire <[email protected]>
> > ---
> >
> > Problem located with an experimental coccinelle script
> >
> > Q: Basically usleep_range() with min == max never makes much sense notably
> > in non-atomic context. If the factor of 2 is tolerable or a fixed
> > offset of e.g. 1000 would be more suitable is not clear to me - maybe
> > someone familiar with that driver can clarify this.
> >
> > Patch was compile tested with: u8500_defconfig (implies COMMON_CLK=y)
> > (with some sparse warnings about not implemented system calls)
> >
> > Patch is against 5.1-rc3 (localversion-next is next=20190405)
> >
> > drivers/clk/ux500/clk-sysctrl.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c
> > index 7c0403b..a1fa3fb 100644
> > --- a/drivers/clk/ux500/clk-sysctrl.c
> > +++ b/drivers/clk/ux500/clk-sysctrl.c
> > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw)
> > clk->reg_bits[0]);
> >
> > if (!ret && clk->enable_delay_us)
> > - usleep_range(clk->enable_delay_us, clk->enable_delay_us);
> > + usleep_range(clk->enable_delay_us, clk->enable_delay_us*2);
>
> The range being used is actually in ms, so not sure we actually need
> to double it for the range.
>
> How about adding ~25% instead, along the lines of below:
> usleep_range(clk->enable_delay_us, clk->enable_delay_us +
> (clk->enable_delay_us >> 2));
>
that would be perfectly sufficient for hrtimers - if the range is
in ms - so I?ll send out a V2 shortly.
thx!
hofrat
On Thu, 2019-04-11 at 04:56 +0200, Nicholas Mc Guire wrote:
> On Wed, Apr 10, 2019 at 03:53:51PM -0700, Stephen Boyd wrote:
> > Quoting Nicholas Mc Guire (2019-04-06 20:13:24)
> > > Providing a range for usleep_range() allows the hrtimer subsystem to
> > > coalesce timers - the delay is runtime configurable so a factor 2
> > > is taken to provide the range.
> > >
> > > Signed-off-by: Nicholas Mc Guire <[email protected]>
> > > ---
> >
> > I think this driver is in maintenance mode. I'll wait for Ulf to ack or
> > review this change before applying.
> >
> > > diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c
> > > index 7c0403b..a1fa3fb 100644
> > > --- a/drivers/clk/ux500/clk-sysctrl.c
> > > +++ b/drivers/clk/ux500/clk-sysctrl.c
> > > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw)
> > > clk->reg_bits[0]);
> > >
> > > if (!ret && clk->enable_delay_us)
> > > - usleep_range(clk->enable_delay_us, clk->enable_delay_us);
> > > + usleep_range(clk->enable_delay_us, clk->enable_delay_us*2);
> >
> > Please add space around that multiply.
> >
> I can do that but it does not seem common and also checkpatch
> did not complain about this - now a simple grep -re "\*10" on the
> kernel shows that it seems more common not to use spaces around *
> that to use them.
Not really
$ git grep -P '\*\s*10' | grep -oh -P '\*\s*10' | \
sort | uniq -c | sort -rn | head
11800 * 10
1705 *10
179 * 10
74 * 10
67 * 10
20 * 10
20 * 10
14 * 10
14 * 10
12 * 10
> Greping specifically for cases using usleep_range()
> (not that many) it seems more or less evenly devided between space
> and no space - so the concern is overlooking that factor 2 ?
>
> thx!
> hofrat
On Thu, Apr 11, 2019 at 04:51:25AM -0700, Joe Perches wrote:
> On Thu, 2019-04-11 at 04:56 +0200, Nicholas Mc Guire wrote:
> > On Wed, Apr 10, 2019 at 03:53:51PM -0700, Stephen Boyd wrote:
> > > Quoting Nicholas Mc Guire (2019-04-06 20:13:24)
> > > > Providing a range for usleep_range() allows the hrtimer subsystem to
> > > > coalesce timers - the delay is runtime configurable so a factor 2
> > > > is taken to provide the range.
> > > >
> > > > Signed-off-by: Nicholas Mc Guire <[email protected]>
> > > > ---
> > >
> > > I think this driver is in maintenance mode. I'll wait for Ulf to ack or
> > > review this change before applying.
> > >
> > > > diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c
> > > > index 7c0403b..a1fa3fb 100644
> > > > --- a/drivers/clk/ux500/clk-sysctrl.c
> > > > +++ b/drivers/clk/ux500/clk-sysctrl.c
> > > > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw)
> > > > clk->reg_bits[0]);
> > > >
> > > > if (!ret && clk->enable_delay_us)
> > > > - usleep_range(clk->enable_delay_us, clk->enable_delay_us);
> > > > + usleep_range(clk->enable_delay_us, clk->enable_delay_us*2);
> > >
> > > Please add space around that multiply.
> > >
> > I can do that but it does not seem common and also checkpatch
> > did not complain about this - now a simple grep -re "\*10" on the
> > kernel shows that it seems more common not to use spaces around *
> > that to use them.
>
> Not really
>
> $ git grep -P '\*\s*10' | grep -oh -P '\*\s*10' | \
> sort | uniq -c | sort -rn | head
> 11800 * 10
> 1705 *10
> 179 * 10
> 74 * 10
> 67 * 10
> 20 * 10
> 20 * 10
> 14 * 10
> 14 * 10
> 12 * 10
yup - my bad - If you restrict it to code lines - its 1:10
not quite sure how I got the first numbers - sloppy check.
hofrat@debian:~/git/linux-next$ grep -re '.*\*\s*10.*;$' * | grep -oh '\*\s*10' | sort | uniq -c | sort -nr | more
8568 * 10
860 *10
Anyway - is there a reason checkpatch will not flag this ?
thx!
hofrat
>
> > Greping specifically for cases using usleep_range()
> > (not that many) it seems more or less evenly devided between space
> > and no space - so the concern is overlooking that factor 2 ?
> >
> > thx!
> > hofrat
>
On Thu, 2019-04-11 at 14:59 +0200, Nicholas Mc Guire wrote:
> On Thu, Apr 11, 2019 at 04:51:25AM -0700, Joe Perches wrote:
> > On Thu, 2019-04-11 at 04:56 +0200, Nicholas Mc Guire wrote:
> > > On Wed, Apr 10, 2019 at 03:53:51PM -0700, Stephen Boyd wrote:
> > > > Quoting Nicholas Mc Guire (2019-04-06 20:13:24)
> > > > > Providing a range for usleep_range() allows the hrtimer subsystem to
> > > > > coalesce timers - the delay is runtime configurable so a factor 2
> > > > > is taken to provide the range.
> > > > >
> > > > > Signed-off-by: Nicholas Mc Guire <[email protected]>
> > > > > ---
> > > >
> > > > I think this driver is in maintenance mode. I'll wait for Ulf to ack or
> > > > review this change before applying.
> > > >
> > > > > diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c
> > > > > index 7c0403b..a1fa3fb 100644
> > > > > --- a/drivers/clk/ux500/clk-sysctrl.c
> > > > > +++ b/drivers/clk/ux500/clk-sysctrl.c
> > > > > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw)
> > > > > clk->reg_bits[0]);
> > > > >
> > > > > if (!ret && clk->enable_delay_us)
> > > > > - usleep_range(clk->enable_delay_us, clk->enable_delay_us);
> > > > > + usleep_range(clk->enable_delay_us, clk->enable_delay_us*2);
> > > >
> > > > Please add space around that multiply.
> > > >
> > > I can do that but it does not seem common and also checkpatch
> > > did not complain about this - now a simple grep -re "\*10" on the
> > > kernel shows that it seems more common not to use spaces around *
> > > that to use them.
> >
> > Not really
> >
> > $ git grep -P '\*\s*10' | grep -oh -P '\*\s*10' | \
> > sort | uniq -c | sort -rn | head
> > 11800 * 10
> > 1705 *10
> > 179 * 10
> > 74 * 10
> > 67 * 10
> > 20 * 10
> > 20 * 10
> > 14 * 10
> > 14 * 10
> > 12 * 10
>
> yup - my bad - If you restrict it to code lines - its 1:10
> not quite sure how I got the first numbers - sloppy check.
>
> hofrat@debian:~/git/linux-next$ grep -re '.*\*\s*10.*;$' * | grep -oh '\*\s*10' | sort | uniq -c | sort -nr | more
> 8568 * 10
> 860 *10
The ratio is about the same in any case.
> Anyway - is there a reason checkpatch will not flag this ?
Because the style is not mentioned in coding style.
checkpatch flags it only when using the --strict option
On Thu, Apr 11, 2019 at 06:12:49AM -0700, Joe Perches wrote:
> On Thu, 2019-04-11 at 14:59 +0200, Nicholas Mc Guire wrote:
> > On Thu, Apr 11, 2019 at 04:51:25AM -0700, Joe Perches wrote:
> > > On Thu, 2019-04-11 at 04:56 +0200, Nicholas Mc Guire wrote:
> > > > On Wed, Apr 10, 2019 at 03:53:51PM -0700, Stephen Boyd wrote:
> > > > > Quoting Nicholas Mc Guire (2019-04-06 20:13:24)
> > > > > > Providing a range for usleep_range() allows the hrtimer subsystem to
> > > > > > coalesce timers - the delay is runtime configurable so a factor 2
> > > > > > is taken to provide the range.
> > > > > >
> > > > > > Signed-off-by: Nicholas Mc Guire <[email protected]>
> > > > > > ---
> > > > >
> > > > > I think this driver is in maintenance mode. I'll wait for Ulf to ack or
> > > > > review this change before applying.
> > > > >
> > > > > > diff --git a/drivers/clk/ux500/clk-sysctrl.c b/drivers/clk/ux500/clk-sysctrl.c
> > > > > > index 7c0403b..a1fa3fb 100644
> > > > > > --- a/drivers/clk/ux500/clk-sysctrl.c
> > > > > > +++ b/drivers/clk/ux500/clk-sysctrl.c
> > > > > > @@ -42,7 +42,7 @@ static int clk_sysctrl_prepare(struct clk_hw *hw)
> > > > > > clk->reg_bits[0]);
> > > > > >
> > > > > > if (!ret && clk->enable_delay_us)
> > > > > > - usleep_range(clk->enable_delay_us, clk->enable_delay_us);
> > > > > > + usleep_range(clk->enable_delay_us, clk->enable_delay_us*2);
> > > > >
> > > > > Please add space around that multiply.
> > > > >
> > > > I can do that but it does not seem common and also checkpatch
> > > > did not complain about this - now a simple grep -re "\*10" on the
> > > > kernel shows that it seems more common not to use spaces around *
> > > > that to use them.
> > >
> > > Not really
> > >
> > > $ git grep -P '\*\s*10' | grep -oh -P '\*\s*10' | \
> > > sort | uniq -c | sort -rn | head
> > > 11800 * 10
> > > 1705 *10
> > > 179 * 10
> > > 74 * 10
> > > 67 * 10
> > > 20 * 10
> > > 20 * 10
> > > 14 * 10
> > > 14 * 10
> > > 12 * 10
> >
> > yup - my bad - If you restrict it to code lines - its 1:10
> > not quite sure how I got the first numbers - sloppy check.
> >
> > hofrat@debian:~/git/linux-next$ grep -re '.*\*\s*10.*;$' * | grep -oh '\*\s*10' | sort | uniq -c | sort -nr | more
> > 8568 * 10
> > 860 *10
>
> The ratio is about the same in any case.
>
> > Anyway - is there a reason checkpatch will not flag this ?
>
> Because the style is not mentioned in coding style.
> checkpatch flags it only when using the --strict option
>
hmm...It sees it is:
3.1) Spaces
***********
...
Use one space around (on each side of) most binary and ternary operators,
such as any of these::
= + - < > * / % | & ^ <= >= == != ? :
thx!
hofrat