acpi-cpufreq: Add a miss ifdef CONFIG_X86_ACPI_CPUFREQ_CPB

PATCH In review Pan Xinhui 3 participants 9 messages Jul 10, 2015 – Jul 20, 2015 CPU frequency scaling framework

lore

Share & tools

lore

Pan Xinhui

author

2015-07-10 05:52 UTC #0 lore raw

PATCH 1/1 acpi-cpufreq: Add a miss ifdef CONFIG_X86_ACPI_CPUFREQ_CPB

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

drivers/cpufreq/acpi-cpufreq.c +2 -0

index e7fcaa6..314a19e 100644
@@ -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,
 };

Signature · 1 line

1.9.1

Rafael J. Wysocki

maintainer

2015-07-10 20:44 UTC #1 lore raw

Hi,

Quoted 14 lines · 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 <[email protected]>
---
 drivers/cpufreq/acpi-cpufreq.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/cpufreq/acpi-cpufreq.c +0 -0
index e7fcaa6..314a19e 100644

drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_resume(struct cpufreq_policy *policy) · line 889

Quoted 6 lines

drivers/cpufreq/acpi-cpufreq.c +2 -0

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

drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_resume(struct cpufreq_policy *policy) · line 891

Quoted 2 lines

drivers/cpufreq/acpi-cpufreq.c +0 -0
        NULL,
 };

Thanks,
Rafael

Pan Xinhui

author

2015-07-13 06:35 UTC #2 lore raw

hi, Rafeal
thanks for your reply. :)

Quoted 12 lines · Rafael J. Wysocki

Hi,
Quoted 9 lines · 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 <[email protected]>
---
 drivers/cpufreq/acpi-cpufreq.c | 2 ++
 1 file changed, 2 insertions(+)

Quoted 16 lines

drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_resume(struct cpufreq_policy *policy) · line 889
Quoted 11 lines
drivers/cpufreq/acpi-cpufreq.c +2 -0
index e7fcaa6..314a19e 100644
@@ -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.

Quoted 3 lines

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

Quoted 6 lines

drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_resume(struct cpufreq_policy *policy) · line 891
Quoted 2 lines
drivers/cpufreq/acpi-cpufreq.c +0 -0
        NULL,
 };
Thanks,
Rafael

Rafael J. Wysocki

2015-07-13 22:59 UTC #3 lore raw

Quoted 32 lines · Pan Xinhui

hi, Rafeal
thanks for your reply. :)
Quoted 12 lines · Rafael J. Wysocki
Hi,
Quoted 9 lines · 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 <[email protected]>
---
 drivers/cpufreq/acpi-cpufreq.c | 2 ++
 1 file changed, 2 insertions(+)
Quoted 16 lines
drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_resume(struct cpufreq_policy *policy) · line 889
Quoted 11 lines
drivers/cpufreq/acpi-cpufreq.c +2 -0
index e7fcaa6..314a19e 100644
@@ -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.

Quoted 3 lines

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?

Quoted 6 lines

Quoted 4 lines
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.

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Pan Xinhui

author

2015-07-14 02:12 UTC #4 lore raw

hi, Rafael,
thanks for you reply :)

Quoted 33 lines · Rafael J. Wysocki

Quoted 32 lines · Pan Xinhui
hi, Rafeal
thanks for your reply. :)
Quoted 28 lines · Rafael J. Wysocki
Hi,
Quoted 14 lines · 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 <[email protected]>
---
 drivers/cpufreq/acpi-cpufreq.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/cpufreq/acpi-cpufreq.c +0 -0
index e7fcaa6..314a19e 100644
drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_resume(struct cpufreq_policy *policy) · line 889
Quoted 6 lines
drivers/cpufreq/acpi-cpufreq.c +2 -0
@@ -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.

Quoted 8 lines

Quoted 3 lines
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

Quoted 11 lines

Quoted 7 lines
Quoted 5 lines
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.

Pan Xinhui

author

2015-07-14 02:55 UTC #5 lore raw

hi, Rafael,
let me do more explanation :)

Quoted 64 lines · Pan Xinhui

hi, Rafael,
thanks for you reply :)
Quoted 33 lines · Rafael J. Wysocki
Quoted 32 lines · Pan Xinhui
hi, Rafeal
thanks for your reply. :)
Quoted 28 lines · Rafael J. Wysocki
Hi,
Quoted 14 lines · 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 <[email protected]>
---
 drivers/cpufreq/acpi-cpufreq.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/cpufreq/acpi-cpufreq.c +0 -0
index e7fcaa6..314a19e 100644
drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_resume(struct cpufreq_policy *policy) · line 889
Quoted 6 lines
drivers/cpufreq/acpi-cpufreq.c +2 -0
@@ -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.
Quoted 8 lines
Quoted 3 lines
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[].

Quoted 15 lines

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

Quoted 14 lines

thanks
xinhui
Quoted 11 lines
Quoted 7 lines
Quoted 5 lines
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.

Rafael J. Wysocki

2015-07-18 00:08 UTC #6 lore raw

Quoted 67 lines · Pan Xinhui

hi, Rafael,
let me do more explanation :)
Quoted 63 lines · Pan Xinhui
hi, Rafael,
thanks for you reply :)
Quoted 33 lines · Rafael J. Wysocki
Quoted 32 lines · Pan Xinhui
hi, Rafeal
thanks for your reply. :)
Quoted 28 lines · Rafael J. Wysocki
Hi,
Quoted 14 lines · 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 <[email protected]>
---
 drivers/cpufreq/acpi-cpufreq.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/cpufreq/acpi-cpufreq.c +0 -0
index e7fcaa6..314a19e 100644
drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_resume(struct cpufreq_policy *policy) · line 889
Quoted 6 lines
drivers/cpufreq/acpi-cpufreq.c +2 -0
@@ -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.
Quoted 8 lines
Quoted 3 lines
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.

Quoted 15 lines

Quoted 15 lines
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(-)

drivers/cpufreq/acpi-cpufreq.c +11 ~~-10~~

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

Pan Xinhui

author

2015-07-20 05:17 UTC #7 lore raw

hi, Rafael
thanks for your reply :)

Quoted 88 lines · Rafael J. Wysocki

Quoted 67 lines · Pan Xinhui
hi, Rafael,
let me do more explanation :)
Quoted 63 lines · Pan Xinhui
hi, Rafael,
thanks for you reply :)
Quoted 33 lines · Rafael J. Wysocki
Quoted 32 lines · Pan Xinhui
hi, Rafeal
thanks for your reply. :)
Quoted 28 lines · Rafael J. Wysocki
Hi,
Quoted 14 lines · 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 <[email protected]>
---
 drivers/cpufreq/acpi-cpufreq.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/cpufreq/acpi-cpufreq.c +0 -0
index e7fcaa6..314a19e 100644
drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_resume(struct cpufreq_policy *policy) · line 889
Quoted 6 lines
drivers/cpufreq/acpi-cpufreq.c +2 -0
@@ -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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 15 lines
Quoted 15 lines
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.

Quoted 4 lines

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.

Quoted 9 lines

---
 drivers/cpufreq/acpi-cpufreq.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)
Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
===================================================================

drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_resume(struct cp · line 892

Quoted 10 lines

drivers/cpufreq/acpi-cpufreq.c +3 -1

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

Quoted 27 lines

drivers/cpufreq/acpi-cpufreq.c +8 -9

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

Rafael J. Wysocki

2015-07-20 21:21 UTC #8 lore raw

Quoted 124 lines · Pan Xinhui

hi, Rafael
thanks for your reply :)
Quoted 88 lines · Rafael J. Wysocki
Quoted 67 lines · Pan Xinhui
hi, Rafael,
let me do more explanation :)
Quoted 63 lines · Pan Xinhui
hi, Rafael,
thanks for you reply :)
Quoted 33 lines · Rafael J. Wysocki
Quoted 32 lines · Pan Xinhui
hi, Rafeal
thanks for your reply. :)
Quoted 28 lines · Rafael J. Wysocki
Hi,
Quoted 14 lines · 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 <[email protected]>
---
 drivers/cpufreq/acpi-cpufreq.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/cpufreq/acpi-cpufreq.c +0 -0
index e7fcaa6..314a19e 100644
drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_resume(struct cpufreq_policy *policy) · line 889
Quoted 6 lines
drivers/cpufreq/acpi-cpufreq.c +2 -0
@@ -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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 15 lines
Quoted 15 lines
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.
Quoted 4 lines
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.
Quoted 9 lines
---
 drivers/cpufreq/acpi-cpufreq.c |   21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)
Index: linux-pm/drivers/cpufreq/acpi-cpufreq.c
===================================================================
drivers/cpufreq/acpi-cpufreq.c static int acpi_cpufreq_resume(struct cp · line 892
Quoted 10 lines
drivers/cpufreq/acpi-cpufreq.c +3 -1
@@ -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
drivers/cpufreq/acpi-cpufreq.c +4 -0
+#ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
+	&cpb,
+#endif
+#ifdef CONFIG_NEW_XXXXXX

Quoted 5 lines

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

Quoted 2 lines

it seems good to me. thanks for your patch :)

OK, thanks!

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.