hi, Rafael
thanks for your reply :)
On 2015年07月18日 08:34, Rafael J. Wysocki wrote:
> On Tuesday, July 14, 2015 10:52:35 AM Pan Xinhui wrote:
>> hi, Rafael,
>> let me do more explanation :)
>>
>> On 2015年07月14日 10:09, Pan Xinhui wrote:
>>> hi, Rafael,
>>> thanks for you reply :)
>>> On 2015年07月14日 07:26, Rafael J. Wysocki wrote:
>>>> On Monday, July 13, 2015 02:33:08 PM Pan Xinhui wrote:
>>>>> hi, Rafeal
>>>>> thanks for your reply. :)
>>>>>
>>>>> On 2015年07月11日 04:44, Rafael J. Wysocki wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Fri, Jul 10, 2015 at 7:50 AM, Pan Xinhui <xinhuix....@intel.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> If CONFIG_X86_ACPI_CPUFREQ_CPB has not been defined, the placeholder for
>>>>>>> cpb is not needed. Add ifdef around it.
>>>>>>>
>>>>>>> Signed-off-by: Pan Xinhui <xinhuix....@intel.com>
>>>>>>> ---
>>>>>>> drivers/cpufreq/acpi-cpufreq.c | 2 ++
>>>>>>> 1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/cpufreq/acpi-cpufreq.c
>>>>>>> b/drivers/cpufreq/acpi-cpufreq.c
>>>>>>> index e7fcaa6..314a19e 100644
>>>>>>> --- a/drivers/cpufreq/acpi-cpufreq.c
>>>>>>> +++ b/drivers/cpufreq/acpi-cpufreq.c
>>>>>>> @@ -884,7 +884,9 @@ static int acpi_cpufreq_resume(struct
>>>>>>> cpufreq_policy *policy)
>>>>>>> static struct freq_attr *acpi_cpufreq_attr[] = {
>>>>>>> &cpufreq_freq_attr_scaling_available_freqs,
>>>>>>> &freqdomain_cpus,
>>>>>>> +#ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
>>>>>>> NULL, /* this is a placeholder for cpb, do not remove */
>>>>>>> +#endif
>>>>>>
>>>>>> Adding the ifdef here doesn't change anything, because the next NULL
>>>>>> will play the role of the one you've just #ifdefed and the structure
>>>>>> will be filled with zeros from that point on anyway.
>>>>>>
>>>>> Yes, adding ifdef here does not change any binary codes. But I want to
>>>>> make the codes more readable. :)
>>>>> Patch author has noticed two *NULL* here would confuse people, especially
>>>>> who first read this acpi-cpufreq.c file
>>>>> From code style point, it would be better to have #ifdef around it.
>>>>
>>>> Not really.
>>>>
>>>> Why don't you simply drop *both* NULLs?
>>>>
>>> Just like string end with *NULL* :)
>>>
>>> 1021 static int cpufreq_add_dev_interface(struct cpufreq_policy *policy,
>>> 1022 struct device *dev)
>>> 1023 {
>>> 1024 struct freq_attr **drv_attr;
>>> 1025 int ret = 0;
>>> 1026
>>> 1027 /* set up files for this cpu device */
>>> 1028 drv_attr = cpufreq_driver->attr;
>>> 1029 while (drv_attr && *drv_attr) {
>>> 1030 ret = sysfs_create_file(&policy->kobj, &((*drv_attr)->attr));
>>> 1031 if (ret)
>>> 1032 return ret;
>>> 1033 drv_attr++;
>>> 1034 }
>>> If struct freq_attr *acpi_cpufreq_attr[] did not end with NULL, line 1033
>>> will access invalid data area.
>>> If *drv_attr(the data after struct freq_attr * array[]) happened to be not
>>> NULL. panic may hit in sysfs_create_file :(
>>> So at least one *NULL* must be in the end of freq_attr *array[].
>
> OK, so the array is NULL-terminated and one NULL is needed to mark the end of
> it.
>
>
>>>
>>> Actually in acpi-cpufreq.c, in acpi_cpufreq_init function.
>>> 957 struct freq_attr **iter;
>>> 958
>>> 959 pr_debug("adding sysfs entry for cpb\n");
>>> 960
>>> 961 for (iter = acpi_cpufreq_attr; *iter != NULL; iter++)
>>> 962 ;
>>> 963
>>> 964 /* make sure there is a terminator behind it */
>>> 965 if (iter[1] == NULL)
>>> 966 *iter = &cpb;
>>> 967 }
>>> line965, check of iter[1] is not needed. Maybe the patch author was afraid
>>> of an unexpected remove of first *NULL*.
>>> It might be a better solution to add ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
>>> around that *NULL*, and remove this !iter[1] check.
>
> Ah, so that is an exceptionally ugly piece of code.
>
> What about the patch below?
>
agree, seems a little better than two-NULLs. I just have one minor question
listed below.
> ---
> drivers/cpufreq/acpi-cpufreq.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/acpi-cpufreq.c
> +++ linux-pm/drivers/cpufreq/acpi-cpufreq.c
> @@ -884,7 +884,9 @@ static int acpi_cpufreq_resume(struct cp
> static struct freq_attr *acpi_cpufreq_attr[] = {
> &cpufreq_freq_attr_scaling_available_freqs,
> &freqdomain_cpus,
> - NULL, /* this is a placeholder for cpb, do not remove */
> +#ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
> + &cpb,
> +#endif
> NULL,
> };
>
such definition may hide a fact that it might be set to NULL if cpb is not
supported.
So if that happen, other member of this array whose index is large than cpb
might not registered.
for example
+#ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
+ &cpb,
+#endif
+#ifdef CONFIG_NEW_XXXXXX
&new_cpufreq_attrs,
#endif
NULL
}
anyway, at that time, people could work out a new solution. if they really have
to add such new cpufreq attr. :)
it seems good to me. thanks for your patch :)
thanks
xinhui
> @@ -957,17 +959,16 @@ static int __init acpi_cpufreq_init(void
> * only if configured. This is considered legacy code, which
> * will probably be removed at some point in the future.
> */
> - if (check_amd_hwpstate_cpu(0)) {
> - struct freq_attr **iter;
> -
> - pr_debug("adding sysfs entry for cpb\n");
> + if (!check_amd_hwpstate_cpu(0)) {
> + struct freq_attr **attr;
>
> - for (iter = acpi_cpufreq_attr; *iter != NULL; iter++)
> - ;
> + pr_debug("CPB unsupported, do not expose it\n");
>
> - /* make sure there is a terminator behind it */
> - if (iter[1] == NULL)
> - *iter = &cpb;
> + for (attr = acpi_cpufreq_attr; *attr; attr++)
> + if (*attr == &cpb) {
> + *attr = NULL;
> + break;
> + }
> }
> #endif
> acpi_cpufreq_boost_init();
>
--
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/
