Re: [PATCH] mtd: nand: ecc-hamming: Clarify the logic around rp17

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Dan,

Dan Carpenter <dan.carpenter@oracle.com> wrote on Thu, 29 Oct 2020
13:27:05 +0300:

> On Thu, Oct 29, 2020 at 09:38:47AM +0100, Miquel Raynal wrote:
> > This code has been written in 2008 and is fine, but in order to keep
> > robots happy, I think it's time to change a little bit this code just
> > to clarify the different possible values of eccsize_mult. Indeed, this
> > variable may only take the value 1 or 2 because step_size, in the case
> > of the software Hamming ECC engine may only be 256 or 512. Depending
> > on the value of eccsize_mult, an extra rp17 variable is set, or not
> > and triggers the following warning:
> > 
> >      smatch warnings:
> >      ecc_sw_hamming_calculate() error: uninitialized symbol 'rp17'.
> > 
> > As highlighted by Dan Carpenter, if the only possible values for
> > eccsize_mult are 1 and 2, then the code is fine, but "it's hard to
> > tell just from looking".
> > 
> > So instead of shifting step_size, let's use a ternary condition to
> > assign to eccsize_mult the only two possible values and clarify the
> > driver's logic.
> > 
> > Now that the situation is clarified for humans, set rp17 to 0 in an
> > else block to keep robots silent as well.  
> 
> Smatch will parse it correctly with just the ternary change but there
> might be other checkers which want the the else statement.  I'm not
> sure.  GCC doesn't seem to warn about uninitialized variables these
> days.

Well, I thought about this after having written the commit log, I
amended the commit to just keep the ternary change and I forgot to drop
this part of the commit log.

So if the change looks good to you I can simply drop the last paragraph
when applying.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/