Re: [PATCH] powerpc/perf: Add missing break in power7_marked_instr_event()

[Madhavan Srinivasan <maddy@linux.vnet.ibm.com>]

On Thursday 20 September 2018 03:11 PM, Michael Ellerman wrote:
> In power7_marked_instr_event() there is a switch case that is missing
> a break or an explicit fallthrough, it's not immediately clear which
> it should be.
>
> The function determines based on the PMU event code, whether the event
> is a "marked" event (which then requires us to configure the PMU in a
> certain way). On Power7 there is no specific bit(s) in the event to
> tell us that, we just have to know.
>
> Rather than having a full list of every event and whether they are
> marked, we pull apart the event code and for events with certain
> values of certain fields we can say that those are all marked events.
>
> We take the psel (bits 0-7) of the event, and look at bits 4-7. For a
> value of 6 we say that if the entire psel == 0x64 then if the pmc == 3
> the event is marked, else not, and otherwise we continue.
>
> It is then that we fallthrough to the 8 case, where we return true if
> the unit == 0xd.
>
> The question is should the 6 case also fallthrough and check for
> unit == 0xd, or should it return.
>
> Looking at the full list of events we see that there are zero events
> where (psel >> 4) == 0x6 and unit == 0xd.
>
> So the answer is it doesn't really matter, there are no valid event
> codes that will return a different result whether we fallthrough or
> break.
>
> But equally, testing the 6 case events against unit == 0xd is slightly
> bogus, as there are no such events. So to make the code clearer, and
> avoid any future confusion, have the 6 case break rather than falling
> through.

Reviewed-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>

Just curious to know, how did you find this. Static code checker compiled
or any specific compiler warnings or just by code read?

Maddy
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>   arch/powerpc/perf/power7-pmu.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/perf/power7-pmu.c b/arch/powerpc/perf/power7-pmu.c
> index 7963658dbc22..6dbae9884ec4 100644
> --- a/arch/powerpc/perf/power7-pmu.c
> +++ b/arch/powerpc/perf/power7-pmu.c
> @@ -238,6 +238,7 @@ static int power7_marked_instr_event(u64 event)
>   	case 6:
>   		if (psel == 0x64)
>   			return pmc >= 3;
> +		break;
>   	case 8:
>   		return unit == 0xd;
>   	}