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

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Richard Weinberger <richard@nod.at>,
	kernel test robot <lkp@intel.com>,
	linux-mtd@lists.infradead.org,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>
Subject: Re: [PATCH] mtd: nand: ecc-hamming: Clarify the logic around rp17
Date: Fri, 30 Oct 2020 18:20:09 +0100	[thread overview]
Message-ID: <20201030182009.646d4ce0@xps13> (raw)
In-Reply-To: <20201029113425.26f83231@xps13>

Hi Dan,

Miquel Raynal <miquel.raynal@bootlin.com> wrote on Thu, 29 Oct 2020
11:34:25 +0100:

> 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.

GCC keeps complaining...

gcc_recent_errors
|-- h8300-randconfig-m031-20201029
|   `-- drivers-mtd-nand-ecc-sw-hamming.c-ecc_sw_hamming_calculate()-error:uninitialized-symbol-rp17-.
`-- x86_64-randconfig-m001-20201029
    `-- drivers-mtd-nand-ecc-sw-hamming.c-ecc_sw_hamming_calculate()-error:uninitialized-symbol-rp17-.

I'll fix my patch and initialize rp17 to 0, it does not hurt anyway...

Thanks,
Miquèl

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