Re: [PATCH] eeh_pseries: Missing break?

On Sun, 2014-03-09 at 00:16 +0800, Gavin Shan wrote:

On Fri, Mar 07, 2014 at 04:31:32PM -0800, Joe Perches wrote:

quoted

Looks like this is unintentional as the
result = EEH_STATE_UNAVAILABLE is being
overwritten by EEH_STATE_NOT_SUPPORT in the
fallthrough to the default case.

Thanks, Joe. It wasn't unintentional.

Hi Gavin.

English usages of "double negatives" are different
than other languages.  "it wasn't unintentional"
means the same thing as "it was intentional".

Could you have better commit log
and subject, then repost it?

The format looks like:

---

powerpc/eeh: Fix overwritten PE state

In pseries_eeh_get_state(), we always have EEH_STATE_UNAVAILABLE
overwritten by EEH_STATE_NOT_SUPPORT because of the missed "break"
the patch fixes the issue.

Signed-off-by: Joe Perches <joe@perches.com>

From my perspective, you should write up a commit

message of your own choice (I wouldn't use "we",
but the rest seems OK) and add a Reported-by:

All I did was notice it and bring it to your
attention.

---

With the better commit log/subject, please have:

Acked-by: Gavin Shan <redacted>

quoted

---

diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 8a8f047..83da53f 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c

@@ -460,14 +460,15 @@ static int pseries_eeh_get_state(struct eeh_pe *pe, int *state)

		case 5:
			if (rets[2]) {
				if (state) *state = rets[2];
				result = EEH_STATE_UNAVAILABLE;
			} else {
				result = EEH_STATE_NOT_SUPPORT;
			}
+			break;
		default:
			result = EEH_STATE_NOT_SUPPORT;
		}
	} else {
		result = EEH_STATE_NOT_SUPPORT;
	}

Thanks,
Gavin

On Sat, Mar 08, 2014 at 08:26:43AM -0800, Joe Perches wrote:

On Sun, 2014-03-09 at 00:16 +0800, Gavin Shan wrote:

quoted

On Fri, Mar 07, 2014 at 04:31:32PM -0800, Joe Perches wrote:

.../...

English usages of "double negatives" are different
than other languages.  "it wasn't unintentional"
means the same thing as "it was intentional".

Sorry, typo :)

quoted

Could you have better commit log
and subject, then repost it?

.../...

From my perspective, you should write up a commit
message of your own choice (I wouldn't use "we",
but the rest seems OK) and add a Reported-by:

All I did was notice it and bring it to your
attention.

Ok. I will post it. Thanks!

Thanks,
Gavin

quoted

quoted

---

diff --git a/arch/powerpc/platforms/pseries/eeh_pseries.c b/arch/powerpc/platforms/pseries/eeh_pseries.c
index 8a8f047..83da53f 100644
--- a/arch/powerpc/platforms/pseries/eeh_pseries.c
+++ b/arch/powerpc/platforms/pseries/eeh_pseries.c

@@ -460,14 +460,15 @@ static int pseries_eeh_get_state(struct eeh_pe *pe, int *state)

		case 5:
			if (rets[2]) {
				if (state) *state = rets[2];
				result = EEH_STATE_UNAVAILABLE;
			} else {
				result = EEH_STATE_NOT_SUPPORT;
			}
+			break;
		default:
			result = EEH_STATE_NOT_SUPPORT;
		}
	} else {
		result = EEH_STATE_NOT_SUPPORT;
	}

Thanks,
Gavin