[1/3] arm64: perf: Mark expected switch fall-through

Message ID 20190726112716.19104-1-anders.roxell@linaro.org
State New
Headers show
Series
  • [1/3] arm64: perf: Mark expected switch fall-through
Related show

Commit Message

Anders Roxell July 26, 2019, 11:27 a.m.
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(-)

-- 
2.20.1

Comments

Robin Murphy July 26, 2019, 12:38 p.m. | #1
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

>

Patch

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;
 		}