acpi: Fix unprotected smp_processor_id() in acpi_processor_cst_has_changed()

RFC PATCH In review Paul E. McKenney 4 participants 6 messages Feb 23, 2012 – Feb 28, 2012 ACPI

Share & tools

Paul E. McKenney

author

2012-02-23 20:58 UTC #0 lore raw

RFC PATCH 1/1 acpi: Fix unprotected smp_processor_id() in acpi_processor_cst_has_changed()

The acpi_processor_cst_has_changed() function is invoked from a CPU_ONLINE or CPU_DEAD function, which might well execute on CPU 0 even though the CPU being hotplugged is some other CPU. In addition, acpi_processor_cst_has_changed() invokes smp_processor_id() without protection, resulting in splats when onlining CPUs.

This commit therefore changes the smp_processor_id() to pr->id, as is used elsewhere in the code, for example, in acpi_processor_add().

This works for me, but I don't claim to understand ACPI.

Signed-off-by Paul E. McKenney <[email protected]>

drivers/acpi/processor_idle.c +1 -2

index 0e8e2de..9e57b06 100644
@@ -1159,8 +1159,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
 	 * to make the code that updates C-States be called once.
 	 */
 
-	if (smp_processor_id() == 0 &&
-			cpuidle_get_driver() == &acpi_idle_driver) {
+	if (pr->id == 0 && cpuidle_get_driver() == &acpi_idle_driver) {
 
 		cpuidle_pause_and_lock();
 		/* Protect against cpu-hotplug */

Srivatsa S. Bhat

Reviewed-by on v1 1/1

2012-02-28 06:13 UTC #1 lore raw

Quoted 6 lines · Paul E. McKenney

The acpi_processor_cst_has_changed() function is invoked from a CPU_ONLINE or CPU_DEAD function, which might well execute on CPU 0 even though the CPU being hotplugged is some other CPU. In addition, acpi_processor_cst_has_changed() invokes smp_processor_id() without protection, resulting in splats when onlining CPUs.

Quoted 7 lines

This commit therefore changes the smp_processor_id() to pr->id, as is used elsewhere in the code, for example, in acpi_processor_add().
This works for me, but I don't claim to understand ACPI.
Signed-off-by Paul E. McKenney <[email protected]>

This looks like the right fix to me, observing the disparity between what the comment says (and hence what was intended) vs what the code originally did.

Reviewed-by Srivatsa S. Bhat <[email protected]>

Quoted 6 lines

drivers/acpi/processor_idle.c +0 -0
index 0e8e2de..9e57b06 100644

drivers/acpi/processor_idle.c int acpi_processor_cst_has_changed(struct acpi_processor *pr) · line 1165

Quoted 9 lines

drivers/acpi/processor_idle.c +1 -2

@@ -1159,8 +1159,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
 	 * to make the code that updates C-States be called once.
 	 */
 
-	if (smp_processor_id() == 0 &&
-			cpuidle_get_driver() == &acpi_idle_driver) {
+	if (pr->id == 0 && cpuidle_get_driver() == &acpi_idle_driver) {
 
 		cpuidle_pause_and_lock();
 		/* Protect against cpu-hotplug */

Regards,
Srivatsa S. Bhat

Yong Zhang

Tested-by on v1 1/1

2012-02-28 07:19 UTC #2 lore raw

In reply to Paul E. McKenney

Quoted 6 lines · Paul E. McKenney

The acpi_processor_cst_has_changed() function is invoked from a CPU_ONLINE or CPU_DEAD function, which might well execute on CPU 0 even though the CPU being hotplugged is some other CPU. In addition, acpi_processor_cst_has_changed() invokes smp_processor_id() without protection, resulting in splats when onlining CPUs.

Quoted 6 lines

This commit therefore changes the smp_processor_id() to pr->id, as is used elsewhere in the code, for example, in acpi_processor_add().
This works for me, but I don't claim to understand ACPI.
Signed-off-by Paul E. McKenney <[email protected]>

Got the same warning.
And the patch works on my side.

Tested-by Yong Zhang <[email protected]>

Quoted 21 lines

drivers/acpi/processor_idle.c +1 -3
index 0e8e2de..9e57b06 100644
@@ -1159,8 +1159,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
 	 * to make the code that updates C-States be called once.
 	 */
 
-	if (smp_processor_id() == 0 &&
-			cpuidle_get_driver() == &acpi_idle_driver) {
+	if (pr->id == 0 && cpuidle_get_driver() == &acpi_idle_driver) {
 
 		cpuidle_pause_and_lock();
 		/* Protect against cpu-hotplug */
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 1 line

Only stand for myself

Paul E. McKenney

author

2012-02-28 13:30 UTC #3 lore raw

Quoted 10 lines · Yong Zhang

Quoted 9 lines · Paul E. McKenney
The acpi_processor_cst_has_changed() function is invoked from a CPU_ONLINE or CPU_DEAD function, which might well execute on CPU 0 even though the CPU being hotplugged is some other CPU. In addition, acpi_processor_cst_has_changed() invokes smp_processor_id() without protection, resulting in splats when onlining CPUs.
This commit therefore changes the smp_processor_id() to pr->id, as is used elsewhere in the code, for example, in acpi_processor_add().

Quoted 8 lines

Quoted 3 lines
This works for me, but I don't claim to understand ACPI.
Signed-off-by Paul E. McKenney <[email protected]>
Got the same warning.
And the patch works on my side.
Tested-by Yong Zhang <[email protected]>

Thank you, I have added your Tested-by (and Srivatsa's Reviewed-by, for that matter).

Any takers for this, or should I just push it up the -tip tree?

Thanx, Paul

Quoted 24 lines

Quoted 20 lines
drivers/acpi/processor_idle.c +1 -3
index 0e8e2de..9e57b06 100644
@@ -1159,8 +1159,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
 	 * to make the code that updates C-States be called once.
 	 */
 
-	if (smp_processor_id() == 0 &&
-			cpuidle_get_driver() == &acpi_idle_driver) {
+	if (pr->id == 0 && cpuidle_get_driver() == &acpi_idle_driver) {
 
 		cpuidle_pause_and_lock();
 		/* Protect against cpu-hotplug */
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Signature · 1 line
Only stand for myself

Rafael J. Wysocki

Acked-by

2012-02-28 20:53 UTC #4 lore raw

Quoted 18 lines · Paul E. McKenney

Quoted 17 lines · Yong Zhang
Quoted 6 lines · Paul E. McKenney
The acpi_processor_cst_has_changed() function is invoked from a CPU_ONLINE or CPU_DEAD function, which might well execute on CPU 0 even though the CPU being hotplugged is some other CPU. In addition, acpi_processor_cst_has_changed() invokes smp_processor_id() without protection, resulting in splats when onlining CPUs.
Quoted 6 lines
This commit therefore changes the smp_processor_id() to pr->id, as is used elsewhere in the code, for example, in acpi_processor_add().
This works for me, but I don't claim to understand ACPI.
Signed-off-by Paul E. McKenney <[email protected]>
Got the same warning.
And the patch works on my side.

Quoted 6 lines

Quoted 1 line
Tested-by Yong Zhang <[email protected]>
Thank you, I have added your Tested-by (and Srivatsa's Reviewed-by, for that matter).
Any takers for this, or should I just push it up the -tip tree?

I think this patch is for Len, either through ACPI, or through the idle tree. Len?

Anyway:

Acked-by Rafael J. Wysocki <[email protected]>

Thanks,
Rafael

Quoted 29 lines

Quoted 12 lines
Quoted 12 lines
drivers/acpi/processor_idle.c +1 -2
index 0e8e2de..9e57b06 100644
@@ -1159,8 +1159,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
 	 * to make the code that updates C-States be called once.
 	 */
 
-	if (smp_processor_id() == 0 &&
-			cpuidle_get_driver() == &acpi_idle_driver) {
+	if (pr->id == 0 && cpuidle_get_driver() == &acpi_idle_driver) {
 
Quoted 9 lines
Quoted 8 lines
drivers/acpi/processor_idle.c +0 -1
 		cpuidle_pause_and_lock();
 		/* Protect against cpu-hotplug */
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Paul E. McKenney

author Reviewed-by Srivatsa S. Bhat Tested-by Yong Zhang Acked-by Rafael J. Wysocki Applied

2012-02-28 21:27 UTC #5 lore raw

Quoted 21 lines · Rafael J. Wysocki

Quoted 20 lines · Paul E. McKenney
Quoted 18 lines · Yong Zhang
Quoted 6 lines · Paul E. McKenney
The acpi_processor_cst_has_changed() function is invoked from a CPU_ONLINE or CPU_DEAD function, which might well execute on CPU 0 even though the CPU being hotplugged is some other CPU. In addition, acpi_processor_cst_has_changed() invokes smp_processor_id() without protection, resulting in splats when onlining CPUs.
Quoted 6 lines
This commit therefore changes the smp_processor_id() to pr->id, as is used elsewhere in the code, for example, in acpi_processor_add().
This works for me, but I don't claim to understand ACPI.
Signed-off-by Paul E. McKenney <[email protected]>
Got the same warning.
And the patch works on my side.
Tested-by Yong Zhang <[email protected]>

Quoted 7 lines

Quoted 4 lines
Thank you, I have added your Tested-by (and Srivatsa's Reviewed-by, for that matter).
Any takers for this, or should I just push it up the -tip tree?
I think this patch is for Len, either through ACPI, or through the idle tree. Len?

Whichever way it is supposed to go, it reminds me of the need for it every time I forget to merge it in before testing. ;-)

Quoted 3 lines

Anyway:
Acked-by Rafael J. Wysocki <[email protected]>

Thank you, applied! The patch appears below with the recent review, testing, and ack.

Thanx, Paul

------------------------------------------------------------------------

acpi: Fix unprotected smp_processor_id() in acpi_processor_cst_has_changed()

This commit therefore changes the smp_processor_id() to pr->id, as is used elsewhere in the code, for example, in acpi_processor_add().

Signed-off-by Paul E. McKenney <[email protected]>

Reviewed-by Srivatsa S. Bhat <[email protected]>

Tested-by Yong Zhang <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

drivers/acpi/processor_idle.c +1 -2

index 0e8e2de..9e57b06 100644
@@ -1159,8 +1159,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
 	 * to make the code that updates C-States be called once.
 	 */
 
-	if (smp_processor_id() == 0 &&
-			cpuidle_get_driver() == &acpi_idle_driver) {
+	if (pr->id == 0 && cpuidle_get_driver() == &acpi_idle_driver) {
 
 		cpuidle_pause_and_lock();
 		/* Protect against cpu-hotplug */