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 <[email protected]>
---
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
NULL,
};
--
1.9.1
Hi,
On Fri, Jul 10, 2015 at 7:50 AM, Pan Xinhui <[email protected]> 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 <[email protected]>
> ---
> 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.
You'd need to #ifdef it in the struct freq_attr definition, but I'm
not sure it's worth the effort.
> NULL,
> };
Thanks,
Rafael
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 <[email protected]> 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 <[email protected]>
>> ---
>> 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.
> You'd need to #ifdef it in the struct freq_attr definition, but I'm
> not sure it's worth the effort.
>
struct freq_attr *cpb* is defined in #ifdef section. :)
thanks
xinhui
>> NULL,
>> };
>
> Thanks,
> Rafael
>
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 <[email protected]> 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 <[email protected]>
> >> ---
> >> 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?
>
> > You'd need to #ifdef it in the struct freq_attr definition, but I'm
> > not sure it's worth the effort.
> >
>
> struct freq_attr *cpb* is defined in #ifdef section. :)
Ah, OK.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
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 <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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[].
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.
thanks
xinhui
>
>>
>>> You'd need to #ifdef it in the struct freq_attr definition, but I'm
>>> not sure it's worth the effort.
>>>
>>
>> struct freq_attr *cpb* is defined in #ifdef section. :)
>
> Ah, OK.
>
>
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 <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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[].
>
> 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.
>
According to line 961, the for loop, patch author assume the first *NULL* may be *at any index* of struct freq_attr *acpi_cpufreq_attr[].
But according to line 965, and the comment also says *a terminator behind it*. patch author assume the first *NULL* is *just right before* the second *NULL* which acts as a terminator.
The logic is incorrect. :(
Why not just
+ /* there must be two NULL in the end of acpi_cpufreq_attr[] */
+ struct freq_attr **iter = &acpi_cpufreq_attr[sizeof(acpi_cpufreq_attr) - 2];
+ if (!WARN_ON(iter[0] || iter[1]))
I don't like that codes assume the index of first *NULL*, so maybe removing line 965, the check if iter[1] == NULL, is more correct and beautiful :)
thanks
xinhui
> thanks
> xinhui
>
>>
>>>
>>>> You'd need to #ifdef it in the struct freq_attr definition, but I'm
>>>> not sure it's worth the effort.
>>>>
>>>
>>> struct freq_attr *cpb* is defined in #ifdef section. :)
>>
>> Ah, OK.
>>
>>
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 <[email protected]> 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 <[email protected]>
> >>>>> ---
> >>>>> 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?
---
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,
};
@@ -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();
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 <[email protected]> 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 <[email protected]>
>>>>>>> ---
>>>>>>> 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();
>
On Monday, July 20, 2015 01:14:41 PM Pan Xinhui wrote:
> 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 <[email protected]> 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 <[email protected]>
> >>>>>>> ---
> >>>>>>> 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. :)
Well, they just need to put their new stuff above the CPB attribute.
>
> it seems good to me. thanks for your patch :)
OK, thanks!
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.