[PATCH 1/2] cpufreq: intel_pstate: Move limits->max_perf to correct position

[PATCH 1/2] cpufreq: intel_pstate: Move limits->max_perf to correct position

[Srinivas Pandruvada]

The limits->max_perf is rounded_up but immediately overwritten by
another assignment to limits->max_perf. Move it to correct
position.
While here also added a pr_debug call in set_policy to aid in
debugging.

Fixes: 785ee2788141 "cpufreq: intel_pstate: Fix limits->max_perf rounding error"
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/cpufreq/intel_pstate.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 0d159b5..724b905 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1460,6 +1460,9 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
 
 	intel_pstate_clear_update_util_hook(policy->cpu);
 
+	pr_debug("set_policy cpuinfo.max %u policy->max %u\n",
+		 policy->cpuinfo.max_freq, policy->max);
+
 	cpu = all_cpu_data[0];
 	if (cpu->pstate.max_pstate_physical > cpu->pstate.max_pstate &&
 	    policy->max < policy->cpuinfo.max_freq &&
@@ -1495,13 +1498,13 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
 				   limits->max_sysfs_pct);
 	limits->max_perf_pct = max(limits->min_policy_pct,
 				   limits->max_perf_pct);
-	limits->max_perf = round_up(limits->max_perf, FRAC_BITS);
 
 	/* Make sure min_perf_pct <= max_perf_pct */
 	limits->min_perf_pct = min(limits->max_perf_pct, limits->min_perf_pct);
 
 	limits->min_perf = div_fp(limits->min_perf_pct, 100);
 	limits->max_perf = div_fp(limits->max_perf_pct, 100);
+	limits->max_perf = round_up(limits->max_perf, FRAC_BITS);
 
  out:
 	intel_pstate_set_update_util_hook(policy->cpu);
-- 
2.5.0

[PATCH 2/2] cpufreq: intel_pstate: Fix set_policy interface for no_turbo

[Srinivas Pandruvada]

When turbo is disabled, the set_policy interface is broken.
For example, when turbo is disabled and
cpuinfo.max = 2900000 (full max turbo frequency)
Setting the limits results in frequency less than settings:
Set 1000000 KHz results in 0700000 KHz
Set 1500000 KHz results in 1100000 KHz
Set 2000000 KHz results in  1500000 KHz

This is because limits->max_perf fraction is calculated using max
turbo frequency as the reference, but when the max P-State is
capped in the function intel_pstate_get_min_max, the reference
is not the max turbo P-State. This results in reducing max
P-State.

One option is to always use max turbo as reference for calculating
limits. But this will not be correct. By definition the intel_pstate
sysfs limits, shows percentage of available performance. So when
BIOS has disabled turbo, the available performance is max non turbo.
So the max_perf_pct should still show 100%.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/cpufreq/intel_pstate.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 724b905..2116666 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1561,8 +1561,14 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
 
 	/* cpuinfo and default policy values */
 	policy->cpuinfo.min_freq = cpu->pstate.min_pstate * cpu->pstate.scaling;
-	policy->cpuinfo.max_freq =
-		cpu->pstate.turbo_pstate * cpu->pstate.scaling;
+	update_turbo_state();
+	if (limits->turbo_disabled)
+		policy->cpuinfo.max_freq =
+			cpu->pstate.max_pstate * cpu->pstate.scaling;
+	else
+		policy->cpuinfo.max_freq =
+			cpu->pstate.turbo_pstate * cpu->pstate.scaling;
+
 	intel_pstate_init_acpi_perf_limits(policy);
 	policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
 	cpumask_set_cpu(policy->cpu, policy->cpus);
-- 
2.5.0

Re: [PATCH 2/2] cpufreq: intel_pstate: Fix set_policy interface for no_turbo

[Rafael J. Wysocki]

On Wed, Jun 8, 2016 at 2:38 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> When turbo is disabled, the set_policy interface is broken.
> For example, when turbo is disabled and
> cpuinfo.max = 2900000 (full max turbo frequency)
> Setting the limits results in frequency less than settings:
> Set 1000000 KHz results in 0700000 KHz
> Set 1500000 KHz results in 1100000 KHz
> Set 2000000 KHz results in  1500000 KHz
>
> This is because limits->max_perf fraction is calculated using max
> turbo frequency as the reference, but when the max P-State is
> capped in the function intel_pstate_get_min_max, the reference
> is not the max turbo P-State. This results in reducing max
> P-State.
>
> One option is to always use max turbo as reference for calculating
> limits. But this will not be correct. By definition the intel_pstate
> sysfs limits, shows percentage of available performance. So when
> BIOS has disabled turbo, the available performance is max non turbo.
> So the max_perf_pct should still show 100%.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

I guess we need this in -stable?

If so, all of them, or is there a specific starting point?

> ---
>  drivers/cpufreq/intel_pstate.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 724b905..2116666 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1561,8 +1561,14 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
>
>         /* cpuinfo and default policy values */
>         policy->cpuinfo.min_freq = cpu->pstate.min_pstate * cpu->pstate.scaling;
> -       policy->cpuinfo.max_freq =
> -               cpu->pstate.turbo_pstate * cpu->pstate.scaling;
> +       update_turbo_state();
> +       if (limits->turbo_disabled)
> +               policy->cpuinfo.max_freq =
> +                       cpu->pstate.max_pstate * cpu->pstate.scaling;
> +       else
> +               policy->cpuinfo.max_freq =
> +                       cpu->pstate.turbo_pstate * cpu->pstate.scaling;
> +
>         intel_pstate_init_acpi_perf_limits(policy);
>         policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
>         cpumask_set_cpu(policy->cpu, policy->cpus);
> --

Re: [PATCH 2/2] cpufreq: intel_pstate: Fix set_policy interface for no_turbo

[Srinivas Pandruvada]

On Wed, 2016-06-08 at 02:42 +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 8, 2016 at 2:38 AM, Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > When turbo is disabled, the set_policy interface is broken.
> > For example, when turbo is disabled and
> > cpuinfo.max = 2900000 (full max turbo frequency)
> > Setting the limits results in frequency less than settings:
> > Set 1000000 KHz results in 0700000 KHz
> > Set 1500000 KHz results in 1100000 KHz
> > Set 2000000 KHz results in  1500000 KHz
> > 
> > This is because limits->max_perf fraction is calculated using max
> > turbo frequency as the reference, but when the max P-State is
> > capped in the function intel_pstate_get_min_max, the reference
> > is not the max turbo P-State. This results in reducing max
> > P-State.
> > 
> > One option is to always use max turbo as reference for calculating
> > limits. But this will not be correct. By definition the
> > intel_pstate
> > sysfs limits, shows percentage of available performance. So when
> > BIOS has disabled turbo, the available performance is max non
> > turbo.
> > So the max_perf_pct should still show 100%.
> > 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel
> > .com>
> I guess we need this in -stable?
Yes.
> 
> If so, all of them, or is there a specific starting point?
I think this is from (even in 3.10, it should have the same behavior
looking at the code).

Thanks,
Srinivas

> 
> > 
> > ---
> >  drivers/cpufreq/intel_pstate.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/intel_pstate.c
> > b/drivers/cpufreq/intel_pstate.c
> > index 724b905..2116666 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -1561,8 +1561,14 @@ static int intel_pstate_cpu_init(struct
> > cpufreq_policy *policy)
> > 
> >         /* cpuinfo and default policy values */
> >         policy->cpuinfo.min_freq = cpu->pstate.min_pstate * cpu-
> > >pstate.scaling;
> > -       policy->cpuinfo.max_freq =
> > -               cpu->pstate.turbo_pstate * cpu->pstate.scaling;
> > +       update_turbo_state();
> > +       if (limits->turbo_disabled)
> > +               policy->cpuinfo.max_freq =
> > +                       cpu->pstate.max_pstate * cpu-
> > >pstate.scaling;
> > +       else
> > +               policy->cpuinfo.max_freq =
> > +                       cpu->pstate.turbo_pstate * cpu-
> > >pstate.scaling;
> > +
> >         intel_pstate_init_acpi_perf_limits(policy);
> >         policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
> >         cpumask_set_cpu(policy->cpu, policy->cpus);
> > --

Re: [PATCH 2/2] cpufreq: intel_pstate: Fix set_policy interface for no_turbo

[Rafael J. Wysocki]

On Wed, Jun 8, 2016 at 2:48 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> On Wed, 2016-06-08 at 02:42 +0200, Rafael J. Wysocki wrote:
>> On Wed, Jun 8, 2016 at 2:38 AM, Srinivas Pandruvada
>> <srinivas.pandruvada@linux.intel.com> wrote:
>> >
>> > When turbo is disabled, the set_policy interface is broken.
>> > For example, when turbo is disabled and
>> > cpuinfo.max = 2900000 (full max turbo frequency)
>> > Setting the limits results in frequency less than settings:
>> > Set 1000000 KHz results in 0700000 KHz
>> > Set 1500000 KHz results in 1100000 KHz
>> > Set 2000000 KHz results in  1500000 KHz
>> >
>> > This is because limits->max_perf fraction is calculated using max
>> > turbo frequency as the reference, but when the max P-State is
>> > capped in the function intel_pstate_get_min_max, the reference
>> > is not the max turbo P-State. This results in reducing max
>> > P-State.
>> >
>> > One option is to always use max turbo as reference for calculating
>> > limits. But this will not be correct. By definition the
>> > intel_pstate
>> > sysfs limits, shows percentage of available performance. So when
>> > BIOS has disabled turbo, the available performance is max non
>> > turbo.
>> > So the max_perf_pct should still show 100%.
>> >
>> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel
>> > .com>
>> I guess we need this in -stable?
> Yes.
>>
>> If so, all of them, or is there a specific starting point?
> I think this is from (even in 3.10, it should have the same behavior
> looking at the code).

OK

All applicable, then?

Re: [PATCH 2/2] cpufreq: intel_pstate: Fix set_policy interface for no_turbo

[Srinivas Pandruvada]

On Wed, 2016-06-08 at 02:50 +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 8, 2016 at 2:48 AM, Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > On Wed, 2016-06-08 at 02:42 +0200, Rafael J. Wysocki wrote:
> > > 
> > > On Wed, Jun 8, 2016 at 2:38 AM, Srinivas Pandruvada
> > > <srinivas.pandruvada@linux.intel.com> wrote:
> > > > 
> > > > 
> > > > When turbo is disabled, the set_policy interface is broken.
> > > > For example, when turbo is disabled and
> > > > cpuinfo.max = 2900000 (full max turbo frequency)
> > > > Setting the limits results in frequency less than settings:
> > > > Set 1000000 KHz results in 0700000 KHz
> > > > Set 1500000 KHz results in 1100000 KHz
> > > > Set 2000000 KHz results in  1500000 KHz
> > > > 
> > > > This is because limits->max_perf fraction is calculated using
> > > > max
> > > > turbo frequency as the reference, but when the max P-State is
> > > > capped in the function intel_pstate_get_min_max, the reference
> > > > is not the max turbo P-State. This results in reducing max
> > > > P-State.
> > > > 
> > > > One option is to always use max turbo as reference for
> > > > calculating
> > > > limits. But this will not be correct. By definition the
> > > > intel_pstate
> > > > sysfs limits, shows percentage of available performance. So
> > > > when
> > > > BIOS has disabled turbo, the available performance is max non
> > > > turbo.
> > > > So the max_perf_pct should still show 100%.
> > > > 
> > > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.i
> > > > ntel
> > > > .com>
> > > I guess we need this in -stable?
> > Yes.
> > > 
> > > 
> > > If so, all of them, or is there a specific starting point?
> > I think this is from (even in 3.10, it should have the same
> > behavior
> > looking at the code).
> OK
> 
> All applicable, then?
Yes. But for some of the trees we need rebase, as the patch may not
apply cleanly.

Thanks,
Srinivas

Re: [PATCH 2/2] cpufreq: intel_pstate: Fix set_policy interface for no_turbo

[Rafael J. Wysocki]

On Wed, Jun 8, 2016 at 2:38 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> When turbo is disabled, the set_policy interface is broken.
> For example, when turbo is disabled and
> cpuinfo.max = 2900000 (full max turbo frequency)
> Setting the limits results in frequency less than settings:
> Set 1000000 KHz results in 0700000 KHz
> Set 1500000 KHz results in 1100000 KHz
> Set 2000000 KHz results in  1500000 KHz
>
> This is because limits->max_perf fraction is calculated using max
> turbo frequency as the reference, but when the max P-State is
> capped in the function intel_pstate_get_min_max, the reference
> is not the max turbo P-State. This results in reducing max
> P-State.
>
> One option is to always use max turbo as reference for calculating
> limits. But this will not be correct. By definition the intel_pstate
> sysfs limits, shows percentage of available performance. So when
> BIOS has disabled turbo, the available performance is max non turbo.
> So the max_perf_pct should still show 100%.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/cpufreq/intel_pstate.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 724b905..2116666 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1561,8 +1561,14 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
>
>         /* cpuinfo and default policy values */
>         policy->cpuinfo.min_freq = cpu->pstate.min_pstate * cpu->pstate.scaling;
> -       policy->cpuinfo.max_freq =
> -               cpu->pstate.turbo_pstate * cpu->pstate.scaling;
> +       update_turbo_state();
> +       if (limits->turbo_disabled)
> +               policy->cpuinfo.max_freq =
> +                       cpu->pstate.max_pstate * cpu->pstate.scaling;
> +       else
> +               policy->cpuinfo.max_freq =
> +                       cpu->pstate.turbo_pstate * cpu->pstate.scaling;
> +
>         intel_pstate_init_acpi_perf_limits(policy);
>         policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
>         cpumask_set_cpu(policy->cpu, policy->cpus);
> --

BTW, I would write this slightly differently.  What about:

policy->cpuinfo.max_freq = limits->turbo_disabled ?
cpu->pstate.max_pstate : cpu->pstate.turbo_pstate;
policy->cpuinfo.max_freq *= cpu->pstate.scaling;

Re: [PATCH 2/2] cpufreq: intel_pstate: Fix set_policy interface for no_turbo

[Rafael J. Wysocki]

On Wed, Jun 8, 2016 at 3:06 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Jun 8, 2016 at 2:38 AM, Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
>> When turbo is disabled, the set_policy interface is broken.
>> For example, when turbo is disabled and
>> cpuinfo.max = 2900000 (full max turbo frequency)
>> Setting the limits results in frequency less than settings:
>> Set 1000000 KHz results in 0700000 KHz
>> Set 1500000 KHz results in 1100000 KHz
>> Set 2000000 KHz results in  1500000 KHz
>>
>> This is because limits->max_perf fraction is calculated using max
>> turbo frequency as the reference, but when the max P-State is
>> capped in the function intel_pstate_get_min_max, the reference
>> is not the max turbo P-State. This results in reducing max
>> P-State.
>>
>> One option is to always use max turbo as reference for calculating
>> limits. But this will not be correct. By definition the intel_pstate
>> sysfs limits, shows percentage of available performance. So when
>> BIOS has disabled turbo, the available performance is max non turbo.
>> So the max_perf_pct should still show 100%.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> ---
>>  drivers/cpufreq/intel_pstate.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>> index 724b905..2116666 100644
>> --- a/drivers/cpufreq/intel_pstate.c
>> +++ b/drivers/cpufreq/intel_pstate.c
>> @@ -1561,8 +1561,14 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
>>
>>         /* cpuinfo and default policy values */
>>         policy->cpuinfo.min_freq = cpu->pstate.min_pstate * cpu->pstate.scaling;
>> -       policy->cpuinfo.max_freq =
>> -               cpu->pstate.turbo_pstate * cpu->pstate.scaling;
>> +       update_turbo_state();
>> +       if (limits->turbo_disabled)
>> +               policy->cpuinfo.max_freq =
>> +                       cpu->pstate.max_pstate * cpu->pstate.scaling;
>> +       else
>> +               policy->cpuinfo.max_freq =
>> +                       cpu->pstate.turbo_pstate * cpu->pstate.scaling;
>> +
>>         intel_pstate_init_acpi_perf_limits(policy);
>>         policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
>>         cpumask_set_cpu(policy->cpu, policy->cpus);
>> --
>
> BTW, I would write this slightly differently.  What about:
>
> policy->cpuinfo.max_freq = limits->turbo_disabled ?
> cpu->pstate.max_pstate : cpu->pstate.turbo_pstate;
> policy->cpuinfo.max_freq *= cpu->pstate.scaling;

But of course without GMail-induced whitespace breakage.

Re: [PATCH 2/2] cpufreq: intel_pstate: Fix set_policy interface for no_turbo

[Rafael J. Wysocki]

On Wed, Jun 8, 2016 at 3:10 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Jun 8, 2016 at 3:06 AM, Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Wed, Jun 8, 2016 at 2:38 AM, Srinivas Pandruvada
>> <srinivas.pandruvada@linux.intel.com> wrote:
>>> When turbo is disabled, the set_policy interface is broken.
>>> For example, when turbo is disabled and
>>> cpuinfo.max = 2900000 (full max turbo frequency)
>>> Setting the limits results in frequency less than settings:
>>> Set 1000000 KHz results in 0700000 KHz
>>> Set 1500000 KHz results in 1100000 KHz
>>> Set 2000000 KHz results in  1500000 KHz
>>>
>>> This is because limits->max_perf fraction is calculated using max
>>> turbo frequency as the reference, but when the max P-State is
>>> capped in the function intel_pstate_get_min_max, the reference
>>> is not the max turbo P-State. This results in reducing max
>>> P-State.
>>>
>>> One option is to always use max turbo as reference for calculating
>>> limits. But this will not be correct. By definition the intel_pstate
>>> sysfs limits, shows percentage of available performance. So when
>>> BIOS has disabled turbo, the available performance is max non turbo.
>>> So the max_perf_pct should still show 100%.
>>>
>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>> ---
>>>  drivers/cpufreq/intel_pstate.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
>>> index 724b905..2116666 100644
>>> --- a/drivers/cpufreq/intel_pstate.c
>>> +++ b/drivers/cpufreq/intel_pstate.c
>>> @@ -1561,8 +1561,14 @@ static int intel_pstate_cpu_init(struct cpufreq_policy *policy)
>>>
>>>         /* cpuinfo and default policy values */
>>>         policy->cpuinfo.min_freq = cpu->pstate.min_pstate * cpu->pstate.scaling;
>>> -       policy->cpuinfo.max_freq =
>>> -               cpu->pstate.turbo_pstate * cpu->pstate.scaling;
>>> +       update_turbo_state();
>>> +       if (limits->turbo_disabled)
>>> +               policy->cpuinfo.max_freq =
>>> +                       cpu->pstate.max_pstate * cpu->pstate.scaling;
>>> +       else
>>> +               policy->cpuinfo.max_freq =
>>> +                       cpu->pstate.turbo_pstate * cpu->pstate.scaling;
>>> +
>>>         intel_pstate_init_acpi_perf_limits(policy);
>>>         policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL;
>>>         cpumask_set_cpu(policy->cpu, policy->cpus);
>>> --
>>
>> BTW, I would write this slightly differently.  What about:
>>
>> policy->cpuinfo.max_freq = limits->turbo_disabled ?
>> cpu->pstate.max_pstate : cpu->pstate.turbo_pstate;
>> policy->cpuinfo.max_freq *= cpu->pstate.scaling;
>
> But of course without GMail-induced whitespace breakage.

So I went on and changed it this way before applying.  Please check
the result in bleeding-edge.

Re: [PATCH 2/2] cpufreq: intel_pstate: Fix set_policy interface for no_turbo

[Srinivas Pandruvada]

On Wed, 2016-06-08 at 03:24 +0200, Rafael J. Wysocki wrote:
> > 

[...]
> On Wed, Jun 8, 2016 at 3:10 AM, Rafael J. Wysocki <rafael@kernel.org>
> wrote:
> > > policy->cpuinfo.max_freq = limits->turbo_disabled ?
> > > cpu->pstate.max_pstate : cpu->pstate.turbo_pstate;
> > > policy->cpuinfo.max_freq *= cpu->pstate.scaling;
> > But of course without GMail-induced whitespace breakage.
> So I went on and changed it this way before applying.  Please check
> the result in bleeding-edge.
Checked. Looks good.

Thanks,
Srinivas