[PATCH] drm/xe: Fix warning on unreachable statement

[Lucas De Marchi]

On Fri, Jul 19, 2024 at 07:59:21PM GMT, Matthew Brost wrote:
>On Fri, Jul 19, 2024 at 12:15:34PM -0700, Lucas De Marchi wrote:
>> eu_type_to_str() relies on -Wswitch to warn (and -Werror) to make sure
>> it handles all enum values. However it's perfectly legal to pass an int
>> to that function so in the end that function may happen to return
>> nothing. A smart compiler could notice eu_type is never assigned to
>> anything other than those values.
>>
>> Trying to reproduce this issue, none of gcc-9, gcc-10 and gcc-13
>> triggered for me, but this was reported in a different system with
>> gcc-10:
>>
>> 	drivers/gpu/drm/xe/xe.o: warning: objtool: xe_gt_topology_dump() falls through to next function xe_gt_topology_init()
>>
>> Since that is not really possible, just take the simple approach and
>> return NULL.
>>
>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_gt_topology.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_gt_topology.c b/drivers/gpu/drm/xe/xe_gt_topology.c
>> index 5a1559edf3e9..0662f71c6ede 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_topology.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_topology.c
>> @@ -233,7 +233,7 @@ static const char *eu_type_to_str(enum xe_gt_eu_type eu_type)
>>  		return "simd8";
>
>Typically elsewhere in the KMD issue done this:
>
>        default:
>                drm_warn(&xe->drm, "NOT POSSIBLE");
>
>Never really agreed on approach but this is used in a lot of places.

which defeats the purpose of -Wswitch (enabled by default on
semi-recent compilers with -Wall). We'd have to start using -Wswitch-enum
to get a build warning with default case. Kernel has -Wswitch-enums
for tools in tools/scripts/Makefile.include, but even then some tools opt
out ... it's often too strict and I don't see we enabling it for kernel
any time soon..

It seems entirely bogus to trade the buil time warning to a runtime one.
And if we have it in build-time, then I don't see why we need one in
runtime. In this case.

>
>Should we do this here?

I don't think so. Some other cases may have a good reason to do a
switch() in something that is not an enum, then resort to default for
that. But when we are handling all the values for an enum var, I think
this pattern is better than using a default case.

Lucas De Marchi

>
>Matt
>
>>  	}
>>
>> -	unreachable();
>> +	return NULL;
>>  }
>>
>>  void
>> --
>> 2.43.0
>>