Re: [PATCH V2 3/4] cpufreq: Add Kconfig option to enable/disable have_multiple_policies

2013-02-21 Thread Viresh Kumar 
On 22 February 2013 07:59, Rafael J. Wysocki  wrote:
> On Friday, February 22, 2013 07:44:23 AM Viresh Kumar wrote:

>> If you don't like this one then we can add another entry
>> into struct policy like: gov_sysfs_parent.
>
> I don't know.  This is going to look kind of ugly this way or another I think.
>
> Maybe I'll figure out something ...

Another simple way of doing this is, leave this patch and here is why i say so.

struct policy is allocated dynamically with kzalloc and so every field is zero
including have_multiple_policies. And only the platforms needing this feature
would make it 1 and all remaining ones would stay unchanged.

This variable would waste just "4" bytes for platforms that don't need this
feature.

About performance: This if/else is called only on policy creation or
destruction.
For platforms that doesn't have multiple policies and thus all cpus share the
same policy struct, the destruction might never happen unless we rmmod/insmod
cpufreq driver, because policy destruction would only happen when all the cpus
are removed :)

So it will execute only once at boot time when we initialize policy struct.

Is this patch worth keeping then?

--
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 3/4] cpufreq: Add Kconfig option to enable/disable have_multiple_policies

2013-02-21 Thread Rafael J. Wysocki 
On Friday, February 22, 2013 07:44:23 AM Viresh Kumar wrote:
> On 22 February 2013 05:23, Rafael J. Wysocki  wrote:
> > On Monday, February 11, 2013 01:20:02 PM Viresh Kumar wrote:
> 
> >> +config CPU_FREQ_HAVE_MULTIPLE_POLICIES
> >> + bool
> >> +
> >
> > So I suppose some architectures will select this, right?
> 
> Yes. And they have to enable have_multiple_policies too from their
> drivers init code.
> 
> > What architecture they are?
> 
> Atleast all big.LITTLE SoCs. Or any other SoC that has multiple policy
> structs alive at any time.
> 
> 
> > I'm not really sure I like this. ->
> 
> >>  static inline struct kobject *
> >>  get_governor_parent_kobj(struct cpufreq_policy *policy)
> >>  {
> >> +#ifdef CONFIG_CPU_FREQ_HAVE_MULTIPLE_POLICIES
> >>   if (policy->have_multiple_policies)
> >>   return >kobj;
> >>   else
> >> +#endif
> >>   return cpufreq_global_kobject;
> >
> > -> I wonder why don't you arrange things so that policy->kobj is always
> > returned, but it points to cpufreq_global_kobject in case there's only one
> > (i.e. make policy->kobj a pointer)?
> 
> policy->kobj is a struct instance rather than a pointer and it is widely used
> within cpufreq.c.

Yeah, policy attributes.  Never mind.

> If you don't like this one then we can add another entry
> into struct policy like: gov_sysfs_parent.

I don't know.  This is going to look kind of ugly this way or another I think.

Maybe I'll figure out something ...

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 3/4] cpufreq: Add Kconfig option to enable/disable have_multiple_policies

2013-02-21 Thread Viresh Kumar 
On 22 February 2013 05:23, Rafael J. Wysocki  wrote:
> On Monday, February 11, 2013 01:20:02 PM Viresh Kumar wrote:

>> +config CPU_FREQ_HAVE_MULTIPLE_POLICIES
>> + bool
>> +
>
> So I suppose some architectures will select this, right?

Yes. And they have to enable have_multiple_policies too from their
drivers init code.

> What architecture they are?

Atleast all big.LITTLE SoCs. Or any other SoC that has multiple policy
structs alive at any time.


> I'm not really sure I like this. ->

>>  static inline struct kobject *
>>  get_governor_parent_kobj(struct cpufreq_policy *policy)
>>  {
>> +#ifdef CONFIG_CPU_FREQ_HAVE_MULTIPLE_POLICIES
>>   if (policy->have_multiple_policies)
>>   return >kobj;
>>   else
>> +#endif
>>   return cpufreq_global_kobject;
>
> -> I wonder why don't you arrange things so that policy->kobj is always
> returned, but it points to cpufreq_global_kobject in case there's only one
> (i.e. make policy->kobj a pointer)?

policy->kobj is a struct instance rather than a pointer and it is widely used
within cpufreq.c. If you don't like this one then we can add another entry
into struct policy like: gov_sysfs_parent.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 3/4] cpufreq: Add Kconfig option to enable/disable have_multiple_policies

2013-02-21 Thread Rafael J. Wysocki 
On Monday, February 11, 2013 01:20:02 PM Viresh Kumar wrote:
> have_multiple_policies is required by platforms having multiple clock-domains
> for cpus, i.e. supporting multiple policies for cpus. This patch adds in a
> Kconfig option for enabling execution of this code.
> 
> Reported-by: Borislav Petkov 
> Signed-off-by: Viresh Kumar 
> ---
>  drivers/cpufreq/Kconfig | 3 +++
>  include/linux/cpufreq.h | 4 
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index cbcb21e..e6e6939 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -23,6 +23,9 @@ config CPU_FREQ_TABLE
>  config CPU_FREQ_GOV_COMMON
>   bool
>  
> +config CPU_FREQ_HAVE_MULTIPLE_POLICIES
> + bool
> +

So I suppose some architectures will select this, right?

What architecture they are?

>  config CPU_FREQ_STAT
>   tristate "CPU frequency translation statistics"
>   select CPU_FREQ_TABLE
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index c5ac9a5..0d84bfa 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -107,11 +107,13 @@ struct cpufreq_policy {
>   unsigned intpolicy; /* see above */
>   struct cpufreq_governor *governor; /* see below */
>   void*governor_data;
> +#ifdef CONFIG_CPU_FREQ_HAVE_MULTIPLE_POLICIES
>   /* This should be set by init() of platforms having multiple
>* clock-domains, i.e.  supporting multiple policies. With this sysfs
>* directories of governor would be created in cpu/cpu/cpufreq/
>* directory */
>   boolhave_multiple_policies;
> +#endif

I'm not really sure I like this. ->

>   struct work_struct  update; /* if update_policy() needs to be
>* called, but you're in IRQ context */
> @@ -142,9 +144,11 @@ static inline bool policy_is_shared(struct 
> cpufreq_policy *policy)
>  static inline struct kobject *
>  get_governor_parent_kobj(struct cpufreq_policy *policy)
>  {
> +#ifdef CONFIG_CPU_FREQ_HAVE_MULTIPLE_POLICIES
>   if (policy->have_multiple_policies)
>   return >kobj;
>   else
> +#endif
>   return cpufreq_global_kobject;

-> I wonder why don't you arrange things so that policy->kobj is always
returned, but it points to cpufreq_global_kobject in case there's only one
(i.e. make policy->kobj a pointer)?

>  }

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 3/4] cpufreq: Add Kconfig option to enable/disable have_multiple_policies

2013-02-21 Thread Rafael J. Wysocki 
On Monday, February 11, 2013 01:20:02 PM Viresh Kumar wrote:
 have_multiple_policies is required by platforms having multiple clock-domains
 for cpus, i.e. supporting multiple policies for cpus. This patch adds in a
 Kconfig option for enabling execution of this code.
 
 Reported-by: Borislav Petkov b...@alien8.de
 Signed-off-by: Viresh Kumar viresh.ku...@linaro.org
 ---
  drivers/cpufreq/Kconfig | 3 +++
  include/linux/cpufreq.h | 4 
  2 files changed, 7 insertions(+)
 
 diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
 index cbcb21e..e6e6939 100644
 --- a/drivers/cpufreq/Kconfig
 +++ b/drivers/cpufreq/Kconfig
 @@ -23,6 +23,9 @@ config CPU_FREQ_TABLE
  config CPU_FREQ_GOV_COMMON
   bool
  
 +config CPU_FREQ_HAVE_MULTIPLE_POLICIES
 + bool
 +

So I suppose some architectures will select this, right?

What architecture they are?

  config CPU_FREQ_STAT
   tristate CPU frequency translation statistics
   select CPU_FREQ_TABLE
 diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
 index c5ac9a5..0d84bfa 100644
 --- a/include/linux/cpufreq.h
 +++ b/include/linux/cpufreq.h
 @@ -107,11 +107,13 @@ struct cpufreq_policy {
   unsigned intpolicy; /* see above */
   struct cpufreq_governor *governor; /* see below */
   void*governor_data;
 +#ifdef CONFIG_CPU_FREQ_HAVE_MULTIPLE_POLICIES
   /* This should be set by init() of platforms having multiple
* clock-domains, i.e.  supporting multiple policies. With this sysfs
* directories of governor would be created in cpu/cpunum/cpufreq/
* directory */
   boolhave_multiple_policies;
 +#endif

I'm not really sure I like this. -

   struct work_struct  update; /* if update_policy() needs to be
* called, but you're in IRQ context */
 @@ -142,9 +144,11 @@ static inline bool policy_is_shared(struct 
 cpufreq_policy *policy)
  static inline struct kobject *
  get_governor_parent_kobj(struct cpufreq_policy *policy)
  {
 +#ifdef CONFIG_CPU_FREQ_HAVE_MULTIPLE_POLICIES
   if (policy-have_multiple_policies)
   return policy-kobj;
   else
 +#endif
   return cpufreq_global_kobject;

- I wonder why don't you arrange things so that policy-kobj is always
returned, but it points to cpufreq_global_kobject in case there's only one
(i.e. make policy-kobj a pointer)?

  }

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 3/4] cpufreq: Add Kconfig option to enable/disable have_multiple_policies

2013-02-21 Thread Viresh Kumar 
On 22 February 2013 05:23, Rafael J. Wysocki r...@sisk.pl wrote:
 On Monday, February 11, 2013 01:20:02 PM Viresh Kumar wrote:

 +config CPU_FREQ_HAVE_MULTIPLE_POLICIES
 + bool
 +

 So I suppose some architectures will select this, right?

Yes. And they have to enable have_multiple_policies too from their
drivers init code.

 What architecture they are?

Atleast all big.LITTLE SoCs. Or any other SoC that has multiple policy
structs alive at any time.


 I'm not really sure I like this. -

  static inline struct kobject *
  get_governor_parent_kobj(struct cpufreq_policy *policy)
  {
 +#ifdef CONFIG_CPU_FREQ_HAVE_MULTIPLE_POLICIES
   if (policy-have_multiple_policies)
   return policy-kobj;
   else
 +#endif
   return cpufreq_global_kobject;

 - I wonder why don't you arrange things so that policy-kobj is always
 returned, but it points to cpufreq_global_kobject in case there's only one
 (i.e. make policy-kobj a pointer)?

policy-kobj is a struct instance rather than a pointer and it is widely used
within cpufreq.c. If you don't like this one then we can add another entry
into struct policy like: gov_sysfs_parent.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 3/4] cpufreq: Add Kconfig option to enable/disable have_multiple_policies

2013-02-21 Thread Rafael J. Wysocki 
On Friday, February 22, 2013 07:44:23 AM Viresh Kumar wrote:
 On 22 February 2013 05:23, Rafael J. Wysocki r...@sisk.pl wrote:
  On Monday, February 11, 2013 01:20:02 PM Viresh Kumar wrote:
 
  +config CPU_FREQ_HAVE_MULTIPLE_POLICIES
  + bool
  +
 
  So I suppose some architectures will select this, right?
 
 Yes. And they have to enable have_multiple_policies too from their
 drivers init code.
 
  What architecture they are?
 
 Atleast all big.LITTLE SoCs. Or any other SoC that has multiple policy
 structs alive at any time.
 
 
  I'm not really sure I like this. -
 
   static inline struct kobject *
   get_governor_parent_kobj(struct cpufreq_policy *policy)
   {
  +#ifdef CONFIG_CPU_FREQ_HAVE_MULTIPLE_POLICIES
if (policy-have_multiple_policies)
return policy-kobj;
else
  +#endif
return cpufreq_global_kobject;
 
  - I wonder why don't you arrange things so that policy-kobj is always
  returned, but it points to cpufreq_global_kobject in case there's only one
  (i.e. make policy-kobj a pointer)?
 
 policy-kobj is a struct instance rather than a pointer and it is widely used
 within cpufreq.c.

Yeah, policy attributes.  Never mind.

 If you don't like this one then we can add another entry
 into struct policy like: gov_sysfs_parent.

I don't know.  This is going to look kind of ugly this way or another I think.

Maybe I'll figure out something ...

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 3/4] cpufreq: Add Kconfig option to enable/disable have_multiple_policies

2013-02-21 Thread Viresh Kumar 
On 22 February 2013 07:59, Rafael J. Wysocki r...@sisk.pl wrote:
 On Friday, February 22, 2013 07:44:23 AM Viresh Kumar wrote:

 If you don't like this one then we can add another entry
 into struct policy like: gov_sysfs_parent.

 I don't know.  This is going to look kind of ugly this way or another I think.

 Maybe I'll figure out something ...

Another simple way of doing this is, leave this patch and here is why i say so.

struct policy is allocated dynamically with kzalloc and so every field is zero
including have_multiple_policies. And only the platforms needing this feature
would make it 1 and all remaining ones would stay unchanged.

This variable would waste just 4 bytes for platforms that don't need this
feature.

About performance: This if/else is called only on policy creation or
destruction.
For platforms that doesn't have multiple policies and thus all cpus share the
same policy struct, the destruction might never happen unless we rmmod/insmod
cpufreq driver, because policy destruction would only happen when all the cpus
are removed :)

So it will execute only once at boot time when we initialize policy struct.

Is this patch worth keeping then?

--
viresh
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/