[PATCH] acpi-cpufreq: Add a miss ifdef CONFIG_X86_ACPI_CPUFREQ_CPB

[PATCH] acpi-cpufreq: Add a miss ifdef CONFIG_X86_ACPI_CPUFREQ_CPB

[Pan Xinhui]

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.pan@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
 	NULL,
 };
 
-- 
1.9.1

Re: [PATCH] acpi-cpufreq: Add a miss ifdef CONFIG_X86_ACPI_CPUFREQ_CPB

[Rafael J. Wysocki]

Hi,

On Fri, Jul 10, 2015 at 7:50 AM, Pan Xinhui <xinhuix.pan@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.pan@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.

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

Re: [PATCH] acpi-cpufreq: Add a miss ifdef CONFIG_X86_ACPI_CPUFREQ_CPB

[Pan Xinhui]

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.pan@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.pan@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. 

> 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
> 

Re: [PATCH] acpi-cpufreq: Add a miss ifdef CONFIG_X86_ACPI_CPUFREQ_CPB

[Rafael J. Wysocki]

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.pan@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.pan@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?


> 
> > 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.

Re: [PATCH] acpi-cpufreq: Add a miss ifdef CONFIG_X86_ACPI_CPUFREQ_CPB

[Pan Xinhui]

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.pan@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.pan@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[].

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.
> 
> 

Re: [PATCH] acpi-cpufreq: Add a miss ifdef CONFIG_X86_ACPI_CPUFREQ_CPB

[Pan Xinhui]

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.pan@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.pan@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[].
> 
> 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.
>>
>>

Re: [PATCH] acpi-cpufreq: Add a miss ifdef CONFIG_X86_ACPI_CPUFREQ_CPB

[Rafael J. Wysocki]

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.pan@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.pan@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?

---
 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();

Re: [PATCH] acpi-cpufreq: Add a miss ifdef CONFIG_X86_ACPI_CPUFREQ_CPB

[Pan Xinhui]

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.pan@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.pan@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();
> 

Re: [PATCH] acpi-cpufreq: Add a miss ifdef CONFIG_X86_ACPI_CPUFREQ_CPB

[Rafael J. Wysocki]

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 <xinhuix.pan@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.pan@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. :)

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.