kernel/padata.c: always check the return value of __padata_remove_cpu() and __padata_add_cpu()

PATCH In review Chen Gang 3 participants 13 messages Aug 20, 2013 – Aug 29, 2013

Subsystems (2)

Crypto API Padata parallel execution mechanism

lore

Share & tools

lore

Chen Gang

author

2013-08-20 03:44 UTC #0 lore raw

PATCH 1/1 kernel/padata.c: always check the return value of __padata_remove_cpu() and __padata_add_cpu()

When failure occures, __padata_add_cpu() and __padata_remove_cpu() will return -ENOMEM, which need be noticed in any cases (even in cleaning up cases).

Signed-off-by Chen Gang <[email protected]>

---
 kernel/padata.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

kernel/padata.c +6 -2

index 072f4ee..6a124cd 100644
@@ -871,16 +871,20 @@ static int padata_cpu_callback(struct notifier_block *nfb,
 		if (!pinst_has_cpu(pinst, cpu))
 			break;
 		mutex_lock(&pinst->lock);
-		__padata_remove_cpu(pinst, cpu);
+		err = __padata_remove_cpu(pinst, cpu);
 		mutex_unlock(&pinst->lock);
+		if (err)
+			return notifier_from_errno(err);
 
 	case CPU_DOWN_FAILED:
 	case CPU_DOWN_FAILED_FROZEN:
 		if (!pinst_has_cpu(pinst, cpu))
 			break;
 		mutex_lock(&pinst->lock);
-		__padata_add_cpu(pinst, cpu);
+		err = __padata_add_cpu(pinst, cpu);
 		mutex_unlock(&pinst->lock);
+		if (err)
+			return notifier_from_errno(err);
 	}
 
 	return NOTIFY_OK;

Signature · 1 line

1.7.7.6

Chen Gang

author

2013-08-20 03:45 UTC #1 lore raw

If this patch is correct, better to let CPU_ONLINE and CPU_DOWN_FAILED share the same code.

And do we need a comment "/* fall through */" between CPU_UP_CANCELED and CPU_DOWN_FAILED (or it is another bug, need a 'break' statement) ?

At last, also better to let CPU_DOWN_PREPARE and CPU_UP_CANCELED share the same code (if need a 'break'), or share the most of code (if "fall through").

Thanks.

Quoted 37 lines · Chen Gang

When failure occures, __padata_add_cpu() and __padata_remove_cpu() will return -ENOMEM, which need be noticed in any cases (even in cleaning up cases).

Signed-off-by Chen Gang <[email protected]>

---
 kernel/padata.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

kernel/padata.c +6 -2

index 072f4ee..6a124cd 100644
@@ -871,16 +871,20 @@ static int padata_cpu_callback(struct notifier_block *nfb,
 		if (!pinst_has_cpu(pinst, cpu))
 			break;
 		mutex_lock(&pinst->lock);
-		__padata_remove_cpu(pinst, cpu);
+		err = __padata_remove_cpu(pinst, cpu);
 		mutex_unlock(&pinst->lock);
+		if (err)
+			return notifier_from_errno(err);
 
 	case CPU_DOWN_FAILED:
 	case CPU_DOWN_FAILED_FROZEN:
 		if (!pinst_has_cpu(pinst, cpu))
 			break;
 		mutex_lock(&pinst->lock);
-		__padata_add_cpu(pinst, cpu);
+		err = __padata_add_cpu(pinst, cpu);
 		mutex_unlock(&pinst->lock);
+		if (err)
+			return notifier_from_errno(err);
 	}
 
 	return NOTIFY_OK;

Signature · 1 line

Chen Gang

Steffen Klassert

maintainer

2013-08-22 05:11 UTC #2 lore raw

Quoted 11 lines · Chen Gang

If this patch is correct, better to let CPU_ONLINE and CPU_DOWN_FAILED share the same code.
And do we need a comment "/* fall through */" between CPU_UP_CANCELED and CPU_DOWN_FAILED (or it is another bug, need a 'break' statement) ?
At last, also better to let CPU_DOWN_PREPARE and CPU_UP_CANCELED share the same code (if need a 'break'), or share the most of code (if "fall through").

CPU_ONLINE and CPU_DOWN_FAILED can share the code. Same is true for CPU_DOWN_PREPARE and CPU_UP_CANCELED.

Thanks!

Chen Gang

author

2013-08-22 05:28 UTC #3 lore raw

Quoted 13 lines · Steffen Klassert

Quoted 11 lines · Chen Gang
If this patch is correct, better to let CPU_ONLINE and CPU_DOWN_FAILED share the same code.
And do we need a comment "/* fall through */" between CPU_UP_CANCELED and CPU_DOWN_FAILED (or it is another bug, need a 'break' statement) ?
At last, also better to let CPU_DOWN_PREPARE and CPU_UP_CANCELED share the same code (if need a 'break'), or share the most of code (if "fall through").

Quoted 6 lines

CPU_ONLINE and CPU_DOWN_FAILED can share the code. Same is true for CPU_DOWN_PREPARE and CPU_UP_CANCELED.
Thanks!

Thank you too.

And need I send another patch for it ?

Or just make by yourself (and better to mark me as Reported-by). :-)

Thanks.

Signature · 1 line

Chen Gang

Steffen Klassert

maintainer

2013-08-22 06:05 UTC #4 lore raw

Quoted 21 lines · Chen Gang

Quoted 13 lines · Steffen Klassert
Quoted 11 lines · Chen Gang
If this patch is correct, better to let CPU_ONLINE and CPU_DOWN_FAILED share the same code.
And do we need a comment "/* fall through */" between CPU_UP_CANCELED and CPU_DOWN_FAILED (or it is another bug, need a 'break' statement) ?
At last, also better to let CPU_DOWN_PREPARE and CPU_UP_CANCELED share the same code (if need a 'break'), or share the most of code (if "fall through").
Quoted 6 lines
CPU_ONLINE and CPU_DOWN_FAILED can share the code. Same is true for CPU_DOWN_PREPARE and CPU_UP_CANCELED.
Thanks!

Quoted 6 lines

Thank you too.
And need I send another patch for it ?
Or just make by yourself (and better to mark me as Reported-by). :-)

You found the problem, feel free to send a patch.

Chen Gang

author

2013-08-22 06:43 UTC #5 lore raw

PATCH v2 1/1 kernel/padata.c: share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED

Share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED.

It will fix 2 bugs:

"not check the return value of __padata_remove_cpu() and __padata_add_cpu()". "need add 'break' between CPU_UP_CANCELED and CPU_DOWN_FAILED".

Signed-off-by Chen Gang <[email protected]>

---
 kernel/padata.c |   20 ++++----------------
 1 files changed, 4 insertions(+), 16 deletions(-)

kernel/padata.c +4 ~~-16~~

index 072f4ee..2f0037a 100644
@@ -846,6 +846,8 @@ static int padata_cpu_callback(struct notifier_block *nfb,
 	switch (action) {
 	case CPU_ONLINE:
 	case CPU_ONLINE_FROZEN:
+	case CPU_DOWN_FAILED:
+	case CPU_DOWN_FAILED_FROZEN:
 		if (!pinst_has_cpu(pinst, cpu))
 			break;
 		mutex_lock(&pinst->lock);

@@ -857,6 +859,8 @@ static int padata_cpu_callback(struct notifier_block *nfb,
 
 	case CPU_DOWN_PREPARE:
 	case CPU_DOWN_PREPARE_FROZEN:
+	case CPU_UP_CANCELED:
+	case CPU_UP_CANCELED_FROZEN:
 		if (!pinst_has_cpu(pinst, cpu))
 			break;
 		mutex_lock(&pinst->lock);

@@ -865,22 +869,6 @@ static int padata_cpu_callback(struct notifier_block *nfb,
 		if (err)
 			return notifier_from_errno(err);
 		break;
-
-	case CPU_UP_CANCELED:
-	case CPU_UP_CANCELED_FROZEN:
-		if (!pinst_has_cpu(pinst, cpu))
-			break;
-		mutex_lock(&pinst->lock);
-		__padata_remove_cpu(pinst, cpu);
-		mutex_unlock(&pinst->lock);
-
-	case CPU_DOWN_FAILED:
-	case CPU_DOWN_FAILED_FROZEN:
-		if (!pinst_has_cpu(pinst, cpu))
-			break;
-		mutex_lock(&pinst->lock);
-		__padata_add_cpu(pinst, cpu);
-		mutex_unlock(&pinst->lock);
 	}
 
 	return NOTIFY_OK;

Signature · 1 line

1.7.7.6

Chen Gang

author

2013-08-23 03:48 UTC #6 lore raw

Re: [PATCH v2] kernel/padata.c: share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED

Quoted 8 lines · Chen Gang

Share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED.
It will fix 2 bugs:
"not check the return value of __padata_remove_cpu() and __padata_add_cpu()". "need add 'break' between CPU_UP_CANCELED and CPU_DOWN_FAILED".

Do we need more details descriptions ?

If so, could Steffen give more expert details information ?

Thanks.

Quoted 52 lines

Signed-off-by Chen Gang <[email protected]>

---
 kernel/padata.c |   20 ++++----------------
 1 files changed, 4 insertions(+), 16 deletions(-)

kernel/padata.c +4 ~~-16~~

index 072f4ee..2f0037a 100644
@@ -846,6 +846,8 @@ static int padata_cpu_callback(struct notifier_block *nfb,
 	switch (action) {
 	case CPU_ONLINE:
 	case CPU_ONLINE_FROZEN:
+	case CPU_DOWN_FAILED:
+	case CPU_DOWN_FAILED_FROZEN:
 		if (!pinst_has_cpu(pinst, cpu))
 			break;
 		mutex_lock(&pinst->lock);

@@ -857,6 +859,8 @@ static int padata_cpu_callback(struct notifier_block *nfb,
 
 	case CPU_DOWN_PREPARE:
 	case CPU_DOWN_PREPARE_FROZEN:
+	case CPU_UP_CANCELED:
+	case CPU_UP_CANCELED_FROZEN:
 		if (!pinst_has_cpu(pinst, cpu))
 			break;
 		mutex_lock(&pinst->lock);

@@ -865,22 +869,6 @@ static int padata_cpu_callback(struct notifier_block *nfb,
 		if (err)
 			return notifier_from_errno(err);
 		break;
-
-	case CPU_UP_CANCELED:
-	case CPU_UP_CANCELED_FROZEN:
-		if (!pinst_has_cpu(pinst, cpu))
-			break;
-		mutex_lock(&pinst->lock);
-		__padata_remove_cpu(pinst, cpu);
-		mutex_unlock(&pinst->lock);
-
-	case CPU_DOWN_FAILED:
-	case CPU_DOWN_FAILED_FROZEN:
-		if (!pinst_has_cpu(pinst, cpu))
-			break;
-		mutex_lock(&pinst->lock);
-		__padata_add_cpu(pinst, cpu);
-		mutex_unlock(&pinst->lock);
 	}
 
 	return NOTIFY_OK;

Signature · 1 line

Chen Gang

Steffen Klassert

maintainer Acked-by on v2 1/1

2013-08-23 10:44 UTC #7 lore raw

Re: [PATCH v2] kernel/padata.c: share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED

In reply to Chen Gang

Quoted 10 lines · Chen Gang

Share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED.
It will fix 2 bugs:
"not check the return value of __padata_remove_cpu() and __padata_add_cpu()". "need add 'break' between CPU_UP_CANCELED and CPU_DOWN_FAILED".
Signed-off-by Chen Gang <[email protected]>

This looks ok to me, Herbert can you take this one?

Acked-by Steffen Klassert <[email protected]>

Herbert Xu

2013-08-23 10:47 UTC #8 lore raw

Re: [PATCH v2] kernel/padata.c: share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED

Quoted 6 lines · Steffen Klassert

Quoted 5 lines · Chen Gang
Share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED.
It will fix 2 bugs:

Quoted 7 lines

Quoted 5 lines
"not check the return value of __padata_remove_cpu() and __padata_add_cpu()". "need add 'break' between CPU_UP_CANCELED and CPU_DOWN_FAILED".
Signed-off-by Chen Gang <[email protected]>
This looks ok to me, Herbert can you take this one?

Sure.

Thanks,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Chen Gang

author

2013-08-26 01:12 UTC #9 lore raw

Re: [PATCH v2] kernel/padata.c: share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED

Quoted 11 lines · Herbert Xu

Quoted 10 lines · Steffen Klassert
Quoted 9 lines · Chen Gang
Share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED.
It will fix 2 bugs:
"not check the return value of __padata_remove_cpu() and __padata_add_cpu()". "need add 'break' between CPU_UP_CANCELED and CPU_DOWN_FAILED".

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Signed-off-by Chen Gang <[email protected]>
This looks ok to me, Herbert can you take this one?
Sure.

Thank you all.

Quoted 2 lines

Thanks,

Thanks.

Signature · 1 line

Chen Gang

Herbert Xu

2013-08-29 04:43 UTC #10 lore raw

Re: [PATCH v2] kernel/padata.c: share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED

In reply to Steffen Klassert

Quoted 10 lines · Steffen Klassert

Quoted 9 lines · Chen Gang
Share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED.
It will fix 2 bugs:
"not check the return value of __padata_remove_cpu() and __padata_add_cpu()". "need add 'break' between CPU_UP_CANCELED and CPU_DOWN_FAILED".

Quoted 5 lines

Quoted 1 line
Signed-off-by Chen Gang <[email protected]>
This looks ok to me, Herbert can you take this one?
Acked-by Steffen Klassert <[email protected]>

Patch applied. Thanks!

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Chen Gang

author

2013-08-29 05:23 UTC #11 lore raw

Re: [PATCH v2] kernel/padata.c: share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED

Quoted 11 lines · Herbert Xu

Quoted 10 lines · Steffen Klassert
Quoted 9 lines · Chen Gang
Share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED.
It will fix 2 bugs:
"not check the return value of __padata_remove_cpu() and __padata_add_cpu()". "need add 'break' between CPU_UP_CANCELED and CPU_DOWN_FAILED".

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Signed-off-by Chen Gang <[email protected]>
This looks ok to me, Herbert can you take this one?
Acked-by Steffen Klassert <[email protected]>
Patch applied. Thanks!

Thank you too.

Signature · 1 line

Chen Gang

Chen Gang

author

2013-08-22 07:18 UTC #12 lore raw

In reply to Steffen Klassert

Quoted 22 lines · Steffen Klassert

Quoted 21 lines · Chen Gang
Quoted 19 lines · Steffen Klassert
Quoted 11 lines · Chen Gang
If this patch is correct, better to let CPU_ONLINE and CPU_DOWN_FAILED share the same code.
And do we need a comment "/* fall through */" between CPU_UP_CANCELED and CPU_DOWN_FAILED (or it is another bug, need a 'break' statement) ?
At last, also better to let CPU_DOWN_PREPARE and CPU_UP_CANCELED share the same code (if need a 'break'), or share the most of code (if "fall through").
CPU_ONLINE and CPU_DOWN_FAILED can share the code. Same is true for CPU_DOWN_PREPARE and CPU_UP_CANCELED.
Thanks!

Quoted 10 lines

Quoted 6 lines
Thank you too.
And need I send another patch for it ?
Or just make by yourself (and better to mark me as Reported-by). :-)
You found the problem, feel free to send a patch.

Thanks, I will send patch v2 for it.

Signature · 1 line

Chen Gang