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
| ||
|
Message-ID: <20130205142446.1998e92d@infradead.org> Date: Tue, 5 Feb 2013 14:24:46 -0200 From: Mauro Carvalho Chehab <mchehab@...radead.org> To: "Lad, Prabhakar" <prabhakar.csengg@...il.com> Cc: LMML <linux-media@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, DLOS <davinci-linux-open-source@...ux.davincidsp.com>, "Lad, Prabhakar" <prabhakar.lad@...com> Subject: Re: [PATCH] davinci: dm644x: fix enum ccdc_gama_width and enum ccdc_data_size comparision warning Em Wed, 2 Jan 2013 17:23:50 +0530 "Lad, Prabhakar" <prabhakar.csengg@...il.com> escreveu: > while the effect is harmless this patch I disagree that this is a harmless warning. It is here for a reason: you should not be relying on the enum "magic" value, since the main reason to use an enum is to fill/compare the enum fields only by their names, and not by their number. > fixes following build warning, > > drivers/media/platform/davinci/dm644x_ccdc.c: In function ‘validate_ccdc_param’: > drivers/media/platform/davinci/dm644x_ccdc.c:233:32: warning: comparison between > ‘enum ccdc_gama_width’ and ‘enum ccdc_data_size’ [-Wenum-compare] > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@...com> > --- > drivers/media/platform/davinci/dm644x_ccdc.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/platform/davinci/dm644x_ccdc.c b/drivers/media/platform/davinci/dm644x_ccdc.c > index ee7942b..42b473a 100644 > --- a/drivers/media/platform/davinci/dm644x_ccdc.c > +++ b/drivers/media/platform/davinci/dm644x_ccdc.c > @@ -228,9 +228,12 @@ static void ccdc_readregs(void) > static int validate_ccdc_param(struct ccdc_config_params_raw *ccdcparam) > { > if (ccdcparam->alaw.enable) { > + u32 gama_wd = ccdcparam->alaw.gama_wd; > + u32 data_sz = ccdcparam->data_sz; > + > if ((ccdcparam->alaw.gama_wd > CCDC_GAMMA_BITS_09_0) || > (ccdcparam->alaw.gama_wd < CCDC_GAMMA_BITS_15_6) || > - (ccdcparam->alaw.gama_wd < ccdcparam->data_sz)) { > + (gama_wd < data_sz)) { hmm... from include/media/davinci/dm644x_ccdc.h: enum ccdc_gama_width { CCDC_GAMMA_BITS_15_6, // 0 CCDC_GAMMA_BITS_14_5, // 1 CCDC_GAMMA_BITS_13_4, // 2 CCDC_GAMMA_BITS_12_3, // 3 CCDC_GAMMA_BITS_11_2, // 4 CCDC_GAMMA_BITS_10_1, // 5 CCDC_GAMMA_BITS_09_0 // 6 }; enum ccdc_data_size { CCDC_DATA_16BITS, // 0 CCDC_DATA_15BITS, // 1 CCDC_DATA_14BITS, // 2 CCDC_DATA_13BITS, // 3 CCDC_DATA_12BITS, // 4 CCDC_DATA_11BITS, // 5 CCDC_DATA_10BITS, // 6 CCDC_DATA_8BITS // 7 }; That doesn't seem right, as comparing the enum integer value won't warrant that the number of bits of gamma. For example, gamma == 6 means 9 bits, while ccdc == 6 means 10 bits. In any case, the code is just crappy, as one could anytime add more values at the enum or reorder. So, a better fix would be to have an array that would convert from the enum "magic" number into the number of bits. Hmm... wait a moment: why are you using an enum here at the first place??? It seems that it would be a way better to just use 2 unsigned integers: ccdc_data_num_bits and ccdc_gama_num_bits, and just fill it with the number of bits, instead of declaring an enum for it. Another alternative would be to merge them into just one enum, like: enum ccdc_bits { CCDC_8_BITS = 8, CCDC_9_BITS = 9, CCDC_10_BITS = 10, CCDC_11_BITS = 11, CCDC_12_BITS = 12, CCDC_13_BITS = 13, CCDC_14_BITS = 14, CCDC_15_BITS = 15, CCDC_16_BITS = 16, }; and replace all occurrences of ccdc_data_size and ccdc_gama_width by the new enum. This way, you could trust on compare one field with the other. > dev_dbg(ccdc_cfg.dev, "\nInvalid data line select"); > return -1; > } Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists