| Message ID | 1443738182-4077-2-git-send-email-srinivas.pandruvada@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 01-10-15, 15:23, Srinivas Pandruvada wrote: > When scaling_available_frequencies is read on an offlined cpu, then > either lockup or junk values are displayed. This is caused by > freed freq_table, which policy is using. > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > drivers/cpufreq/cpufreq.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index ef5ed94..25c4c15 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1436,8 +1436,10 @@ static void cpufreq_offline_finish(unsigned int cpu) > * since this is a core component, and is essential for the > * subsequent light-weight ->init() to succeed. > */ > - if (cpufreq_driver->exit) > + if (cpufreq_driver->exit) { > cpufreq_driver->exit(policy); > + policy->freq_table = NULL; > + } > } freq_table was set from the ->init() callbacks and only they should set it to NULL, isn't it?
On Wed, 2015-10-07 at 18:15 +0530, Viresh Kumar wrote: > On 01-10-15, 15:23, Srinivas Pandruvada wrote: > > When scaling_available_frequencies is read on an offlined cpu, then > > either lockup or junk values are displayed. This is caused by > > freed freq_table, which policy is using. > > > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > > --- > > drivers/cpufreq/cpufreq.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index ef5ed94..25c4c15 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -1436,8 +1436,10 @@ static void cpufreq_offline_finish(unsigned int cpu) > > * since this is a core component, and is essential for the > > * subsequent light-weight ->init() to succeed. > > */ > > - if (cpufreq_driver->exit) > > + if (cpufreq_driver->exit) { > > cpufreq_driver->exit(policy); > > + policy->freq_table = NULL; > > + } > > } > > freq_table was set from the ->init() callbacks and only they should > set it to NULL, isn't it? freq_table is allocated in init() callbacks and freed on exit() callback. But I don't see any driver changing policy->freq_table to NULL or setting value to policy->freq_table to their allocated freq_table . I see policy->freq_table is only assigned value in function cpufreq_table_validate_and_show, as part of the core API. So I think we should set to NULL after clients freed the freq_table as part of core function. Otherwise we need to modify every client cpufreq driver and assign policy->freq_table=NULL on .exit(). Since they don't assign value to this policy->freq_table in first place, they shouldn't change this. Also I don't know why we need to keep the sysfs entry cpufreq after offline. There are other values, which we can read without crash, but may not be valid. We may delete this sysfs entry. But may be some cpufreq driver cares about this even after offline. If we can delete this entry, it can be part of cleanup of cpufreq subsystem. Thanks, Srinivas > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07-10-15, 09:18, Srinivas Pandruvada wrote: > freq_table is allocated in init() callbacks and freed on exit() > callback. But I don't see any driver changing policy->freq_table to NULL > or setting value to policy->freq_table to their allocated freq_table . That's what I am asking for. > I see policy->freq_table is only assigned value in function > cpufreq_table_validate_and_show, as part of the core API. Its just an helper written to reduce code redundancy.. > So I think we > should set to NULL after clients freed the freq_table as part of core > function. Not really. If we want to do the opposite, then we should initiate policy->freq_table = NULL, right from exit(), as its done as part of init() first. > Otherwise we need to modify every client cpufreq driver and assign > policy->freq_table=NULL on .exit(). Since they don't assign value to > this policy->freq_table in first place, they shouldn't change this. They did set it in the first place, with help of a helper routine though. But I do understand the code redundancy we are about to create. So, maybe we can do this in the core as a special case. But then you missed another location where exit() was called. > Also I don't know why we need to keep the sysfs entry cpufreq after > offline. There are other values, which we can read without crash, but > may not be valid. We may delete this sysfs entry. But may be some > cpufreq driver cares about this even after offline. If we can delete > this entry, it can be part of cleanup of cpufreq subsystem. Go thought the mail threads where that support is added, it was indeed useful.
On Wed, 2015-10-07 at 22:33 +0530, Viresh Kumar wrote: > On 07-10-15, 09:18, Srinivas Pandruvada wrote: > > freq_table is allocated in init() callbacks and freed on exit() > > callback. But I don't see any driver changing policy->freq_table to NULL > > or setting value to policy->freq_table to their allocated freq_table . > > That's what I am asking for. > > > I see policy->freq_table is only assigned value in function > > cpufreq_table_validate_and_show, as part of the core API. > > Its just an helper written to reduce code redundancy.. > > > So I think we > > should set to NULL after clients freed the freq_table as part of core > > function. > > Not really. If we want to do the opposite, then we should initiate > policy->freq_table = NULL, right from exit(), as its done as part of > init() first. > > > Otherwise we need to modify every client cpufreq driver and assign > > policy->freq_table=NULL on .exit(). Since they don't assign value to > > this policy->freq_table in first place, they shouldn't change this. > > They did set it in the first place, with help of a helper routine > though. > > But I do understand the code redundancy we are about to create. So, > maybe we can do this in the core as a special case. > > But then you missed another location where exit() was called. In cpufreq_online() function? In this case whole policy is freed. Do we need to assign here also? > > > Also I don't know why we need to keep the sysfs entry cpufreq after > > offline. There are other values, which we can read without crash, but > > may not be valid. We may delete this sysfs entry. But may be some > > cpufreq driver cares about this even after offline. If we can delete > > this entry, it can be part of cleanup of cpufreq subsystem. > > Go thought the mail threads where that support is added, it was indeed > useful. > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01-10-15, 15:23, Srinivas Pandruvada wrote: > When scaling_available_frequencies is read on an offlined cpu, then > either lockup or junk values are displayed. This is caused by > freed freq_table, which policy is using. > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > --- > drivers/cpufreq/cpufreq.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index ef5ed94..25c4c15 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1436,8 +1436,10 @@ static void cpufreq_offline_finish(unsigned int cpu) > * since this is a core component, and is essential for the > * subsequent light-weight ->init() to succeed. > */ > - if (cpufreq_driver->exit) > + if (cpufreq_driver->exit) { > cpufreq_driver->exit(policy); > + policy->freq_table = NULL; > + } > } Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index ef5ed94..25c4c15 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1436,8 +1436,10 @@ static void cpufreq_offline_finish(unsigned int cpu) * since this is a core component, and is essential for the * subsequent light-weight ->init() to succeed. */ - if (cpufreq_driver->exit) + if (cpufreq_driver->exit) { cpufreq_driver->exit(policy); + policy->freq_table = NULL; + } } /**
When scaling_available_frequencies is read on an offlined cpu, then either lockup or junk values are displayed. This is caused by freed freq_table, which policy is using. Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> --- drivers/cpufreq/cpufreq.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)