linux-kernel - Re: [PATCH v3 08/15] media: qcom: camss: Untangle if/else spaghetti in camss

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 4 Sep 2023 13:24:14 +0100
From:   Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     rfoss@...nel.org, todor.too@...il.com, agross@...nel.org,
        andersson@...nel.org, konrad.dybcio@...aro.org, mchehab@...nel.org,
        hverkuil-cisco@...all.nl, sakari.ailus@...ux.intel.com,
        andrey.konovalov@...aro.org, linux-media@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 08/15] media: qcom: camss: Untangle if/else spaghetti
 in camss

On 28/08/2023 19:51, Laurent Pinchart wrote:
>> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
>> @@ -592,15 +592,19 @@ int msm_csid_subdev_init(struct camss *camss, struct csid_device *csid,
>>   	csid->camss = camss;
>>   	csid->id = id;
>>   
>> -	if (camss->res->version == CAMSS_8x16) {
>> +	switch (camss->res->version) {
>> +	case CAMSS_8x16:
>>   		csid->ops = &csid_ops_4_1;
>> -	} else if (camss->res->version == CAMSS_8x96 ||
>> -		   camss->res->version == CAMSS_660) {
>> +		break;
>> +	case CAMSS_8x96:
>> +	case CAMSS_660:
>>   		csid->ops = &csid_ops_4_7;
>> -	} else if (camss->res->version == CAMSS_845 ||
>> -		   camss->res->version == CAMSS_8250) {
>> +		break;
>> +	case CAMSS_845:
>> +	case CAMSS_8250:
>>   		csid->ops = &csid_ops_gen2;
>> -	} else {
>> +		break;
>> +	default:
>>   		return -EINVAL;
> This should never happen, as adding support for a new SoC should come
> with an update for all the applicable switch/case statements. It's
> useful to let the compiler complain if someone forgets to do so, but
> with a default case, you will only see the issue at runtime. Could it be
> caught at compile time ?
> 

This can be done in fact.

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wswitch_002denum-303

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

typedef enum {
         MO = 0,
         LARRY,
         CURLY,
         BINGO,
}my_type;

int main (int argc, char *argv[])
{
         my_type x;
         time_t t;

         srand((unsigned) time(&t));

         x = rand() % BINGO;

         switch(x) {
         case MO:
                 printf("mo\n");
                 break;
         case LARRY:
                 printf("larry\n");
                 break;
         default:
                 printf("blargh\n");
                 break;

         }

         return 0;
}

gcc -o test test.c -Wswitch-enum
test.c: In function ‘main’:
test.c:38:9: warning: enumeration value ‘CURLY’ not handled in switch 
[-Wswitch-enum]
    38 |         switch(x) {
       |         ^~~~~~

It looks like we only enable that switch for tools though

grep -r "Wswitch-enum" *
tools/scripts/Makefile.include:EXTRA_WARNINGS += -Wswitch-enum
tools/bpf/bpftool/Makefile:CFLAGS += $(filter-out -Wswitch-enum 
-Wnested-externs,$(EXTRA_WARNINGS))

I'll still implement the code though, since if we do introduce the 
switch for the kernel it would be caught.

---
bod