When fall-through warnings was enabled by default, commit d93512ef0f0e ("Makefile: Globally enable fall-through warning"), the following warnings was starting to show up:
../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’: ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1) ^ ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here case 2: ^~~~ ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2) ^ ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here default: ^~~~~~~
Rework so that the compiler doesn't warn about fall-through. Rework so the code looks like the arm code. Since the comment in the function indicates taht this is supposed to behave the same way as arm32 because it handles 32-bit tasks also.
Cc: stable@vger.kernel.org # v3.16+ Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code") Signed-off-by: Anders Roxell anders.roxell@linaro.org --- arch/arm64/kernel/hw_breakpoint.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c index dceb84520948..ea616adf1cf1 100644 --- a/arch/arm64/kernel/hw_breakpoint.c +++ b/arch/arm64/kernel/hw_breakpoint.c @@ -535,14 +535,17 @@ int hw_breakpoint_arch_parse(struct perf_event *bp, case 0: /* Aligned */ break; - case 1: - /* Allow single byte watchpoint. */ - if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1) - break; case 2: /* Allow halfword watchpoints and breakpoints. */ if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2) break; + /* Fall through */ + case 1: + case 3: + /* Allow single byte watchpoint. */ + if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1) + break; + /* Fall through */ default: return -EINVAL; }
On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
When fall-through warnings was enabled by default, commit d93512ef0f0e ("Makefile: Globally enable fall-through warning"), the following warnings was starting to show up:
../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’: ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1) ^ ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here case 2: ^~~~ ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2) ^ ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here default: ^~~~~~~
Rework so that the compiler doesn't warn about fall-through. Rework so the code looks like the arm code. Since the comment in the function indicates taht this is supposed to behave the same way as arm32 because
Typo: s/taht/that/
it handles 32-bit tasks also.
Cc: stable@vger.kernel.org # v3.16+ Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code") Signed-off-by: Anders Roxell anders.roxell@linaro.org
The patch itself looks fine, but I don't think this needs a CC to stable, nor does it require that fixes tag, as there's no functional problem.
If anything, it fixes:
d93512ef0f0e (" Makefile: Globally enable fall-through warning")
... given the commit message for that patch states:
Now that all the fall-through warnings have been addressed in the kernel, enable the fall-through warning globally.
... and the existence of this patch implies otherwise.
IIUC that patch isn't even in mainline yet, but given this is simple I imagine that Will and Catalin might be happy to pick this up for the next rc.
Thanks, Mark.
arch/arm64/kernel/hw_breakpoint.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c index dceb84520948..ea616adf1cf1 100644 --- a/arch/arm64/kernel/hw_breakpoint.c +++ b/arch/arm64/kernel/hw_breakpoint.c @@ -535,14 +535,17 @@ int hw_breakpoint_arch_parse(struct perf_event *bp, case 0: /* Aligned */ break;
case 1:/* Allow single byte watchpoint. */if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)break;
/* Fall through */case 1:case 3:/* Allow single byte watchpoint. */if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)break;/* Fall through */-- 2.20.1
On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
When fall-through warnings was enabled by default, commit d93512ef0f0e ("Makefile: Globally enable fall-through warning"), the following warnings was starting to show up:
../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’: ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1) ^ ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here case 2: ^~~~ ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2) ^ ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here default: ^~~~~~~
Rework so that the compiler doesn't warn about fall-through. Rework so the code looks like the arm code. Since the comment in the function indicates taht this is supposed to behave the same way as arm32 because
Typo: s/taht/that/
it handles 32-bit tasks also.
Cc: stable@vger.kernel.org # v3.16+ Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code") Signed-off-by: Anders Roxell anders.roxell@linaro.org
The patch itself looks fine, but I don't think this needs a CC to stable, nor does it require that fixes tag, as there's no functional problem.
Hmm... I now see I spoke too soon, and this is making the 1-byte breakpoint work at a 3-byte offset.
Given that:
Acked-by: Mark Rutland mark.rutland@arm.com
... and the fixes and stable tags are appropriate for that portion of the patch.
Sorry for the noise.
Thanks, Mark.
arch/arm64/kernel/hw_breakpoint.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c index dceb84520948..ea616adf1cf1 100644 --- a/arch/arm64/kernel/hw_breakpoint.c +++ b/arch/arm64/kernel/hw_breakpoint.c @@ -535,14 +535,17 @@ int hw_breakpoint_arch_parse(struct perf_event *bp, case 0: /* Aligned */ break;
case 1:/* Allow single byte watchpoint. */if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)break;
/* Fall through */case 1:case 3:/* Allow single byte watchpoint. */if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1)break;/* Fall through */-- 2.20.1
On Fri, Jul 26, 2019 at 01:13:54PM +0100, Mark Rutland wrote:
On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
When fall-through warnings was enabled by default, commit d93512ef0f0e ("Makefile: Globally enable fall-through warning"), the following warnings was starting to show up:
../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’: ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1) ^ ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here case 2: ^~~~ ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2) ^ ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here default: ^~~~~~~
Rework so that the compiler doesn't warn about fall-through. Rework so the code looks like the arm code. Since the comment in the function indicates taht this is supposed to behave the same way as arm32 because
Typo: s/taht/that/
it handles 32-bit tasks also.
Cc: stable@vger.kernel.org # v3.16+ Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code") Signed-off-by: Anders Roxell anders.roxell@linaro.org
The patch itself looks fine, but I don't think this needs a CC to stable, nor does it require that fixes tag, as there's no functional problem.
Hmm... I now see I spoke too soon, and this is making the 1-byte breakpoint work at a 3-byte offset.
I still don't think it's quite right though, since it forbids a 2-byte watchpoint on a byte-aligned address.
I think the arm64 code matches what we had on 32-bit prior to d968d2b801d8 ("ARM: 7497/1: hw_breakpoint: allow single-byte watchpoints on all addresses"), so we should have one patch bringing us up to speed with that change, and then another annotating the fallthroughs.
Will
On 26/07/2019 13:27, Will Deacon wrote:
On Fri, Jul 26, 2019 at 01:13:54PM +0100, Mark Rutland wrote:
On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
When fall-through warnings was enabled by default, commit d93512ef0f0e ("Makefile: Globally enable fall-through warning"), the following warnings was starting to show up:
../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’: ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1) ^ ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here case 2: ^~~~ ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2) ^ ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here default: ^~~~~~~
Rework so that the compiler doesn't warn about fall-through. Rework so the code looks like the arm code. Since the comment in the function indicates taht this is supposed to behave the same way as arm32 because
Typo: s/taht/that/
it handles 32-bit tasks also.
Cc: stable@vger.kernel.org # v3.16+ Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code") Signed-off-by: Anders Roxell anders.roxell@linaro.org
The patch itself looks fine, but I don't think this needs a CC to stable, nor does it require that fixes tag, as there's no functional problem.
Hmm... I now see I spoke too soon, and this is making the 1-byte breakpoint work at a 3-byte offset.
I still don't think it's quite right though, since it forbids a 2-byte watchpoint on a byte-aligned address.
Plus, AFAICS, a 1-byte watchpoint on a 2-byte-aligned address.
Not that I know anything about this code, but it does start to look like it might want rewriting without the offending switch statement anyway. At a glance, it looks like the intended semantic might boil down to:
if (hw->ctrl.len > offset) return -EINVAL;
Robin.
I think the arm64 code matches what we had on 32-bit prior to d968d2b801d8 ("ARM: 7497/1: hw_breakpoint: allow single-byte watchpoints on all addresses"), so we should have one patch bringing us up to speed with that change, and then another annotating the fallthroughs.
Will
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Jul 26, 2019 at 01:38:24PM +0100, Robin Murphy wrote:
On 26/07/2019 13:27, Will Deacon wrote:
On Fri, Jul 26, 2019 at 01:13:54PM +0100, Mark Rutland wrote:
On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
When fall-through warnings was enabled by default, commit d93512ef0f0e ("Makefile: Globally enable fall-through warning"), the following warnings was starting to show up:
../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’: ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1) ^ ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here case 2: ^~~~ ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2) ^ ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here default: ^~~~~~~
Rework so that the compiler doesn't warn about fall-through. Rework so the code looks like the arm code. Since the comment in the function indicates taht this is supposed to behave the same way as arm32 because
Typo: s/taht/that/
it handles 32-bit tasks also.
Cc: stable@vger.kernel.org # v3.16+ Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code") Signed-off-by: Anders Roxell anders.roxell@linaro.org
The patch itself looks fine, but I don't think this needs a CC to stable, nor does it require that fixes tag, as there's no functional problem.
Hmm... I now see I spoke too soon, and this is making the 1-byte breakpoint work at a 3-byte offset.
I still don't think it's quite right though, since it forbids a 2-byte watchpoint on a byte-aligned address.
Plus, AFAICS, a 1-byte watchpoint on a 2-byte-aligned address.
Not that I know anything about this code, but it does start to look like it might want rewriting without the offending switch statement anyway. At a glance, it looks like the intended semantic might boil down to:
if (hw->ctrl.len > offset) return -EINVAL;
Given that it's compat code, I think it's worth staying as close to the arch/arm/ implementation as we can. Also, beware that the ARM_BREAKPOINT_LEN_* definitions are masks because of the BAS fields in the debug architecture.
Will
On 26/07/2019 14:05, Will Deacon wrote:
On Fri, Jul 26, 2019 at 01:38:24PM +0100, Robin Murphy wrote:
On 26/07/2019 13:27, Will Deacon wrote:
On Fri, Jul 26, 2019 at 01:13:54PM +0100, Mark Rutland wrote:
On Fri, Jul 26, 2019 at 01:10:57PM +0100, Mark Rutland wrote:
On Fri, Jul 26, 2019 at 01:27:16PM +0200, Anders Roxell wrote:
When fall-through warnings was enabled by default, commit d93512ef0f0e ("Makefile: Globally enable fall-through warning"), the following warnings was starting to show up:
../arch/arm64/kernel/hw_breakpoint.c: In function ‘hw_breakpoint_arch_parse’: ../arch/arm64/kernel/hw_breakpoint.c:540:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_1) ^ ../arch/arm64/kernel/hw_breakpoint.c:542:3: note: here case 2: ^~~~ ../arch/arm64/kernel/hw_breakpoint.c:544:7: warning: this statement may fall through [-Wimplicit-fallthrough=] if (hw->ctrl.len == ARM_BREAKPOINT_LEN_2) ^ ../arch/arm64/kernel/hw_breakpoint.c:546:3: note: here default: ^~~~~~~
Rework so that the compiler doesn't warn about fall-through. Rework so the code looks like the arm code. Since the comment in the function indicates taht this is supposed to behave the same way as arm32 because
Typo: s/taht/that/
it handles 32-bit tasks also.
Cc: stable@vger.kernel.org # v3.16+ Fixes: 6ee33c2712fc ("ARM: hw_breakpoint: correct and simplify alignment fixup code") Signed-off-by: Anders Roxell anders.roxell@linaro.org
The patch itself looks fine, but I don't think this needs a CC to stable, nor does it require that fixes tag, as there's no functional problem.
Hmm... I now see I spoke too soon, and this is making the 1-byte breakpoint work at a 3-byte offset.
I still don't think it's quite right though, since it forbids a 2-byte watchpoint on a byte-aligned address.
Plus, AFAICS, a 1-byte watchpoint on a 2-byte-aligned address.
[and of course, what I missed was that that's the case the fallthrough serves... yuck indeed]
Not that I know anything about this code, but it does start to look like it might want rewriting without the offending switch statement anyway. At a glance, it looks like the intended semantic might boil down to:
if (hw->ctrl.len > offset) return -EINVAL;
Given that it's compat code, I think it's worth staying as close to the arch/arm/ implementation as we can.
Right, I also misread the accompanying arch/arm/ patch and got the impression that 32-bit also had a problem such that any fix would happen in parallel - on closer inspection the current arch/arm/ code does actually seem to make sense, even if it is horribly subtle.
Also, beware that the ARM_BREAKPOINT_LEN_* definitions are masks because of the BAS fields in the debug architecture.
Fun... Clearly it's a bit too Friday for me to be useful here, so apologies for the confusion :)
Robin.
linux-stable-mirror@lists.linaro.org
-
Anders Roxell -
Mark Rutland -
Robin Murphy -
Will Deacon