[PATCH 00/28] Reenable maybe-uninitialized warnings

[PATCH 00/28] Reenable maybe-uninitialized warnings

[Arnd Bergmann]

This is a set of patches that I hope to get into v4.9 in some form
in order to turn on the -Wmaybe-uninitialized warnings again.

After talking to Linus in person at Linaro Connect about this, I
spent some time on finding all the remaining warnings, and this
is the resulting patch series. More details are in the description
of the last patch that actually enables the warning.

Let me know if there are other warnings that I missed, and whether
you think these are still appropriate for v4.9 or not.
A couple of patches are non-obvious, and could use some more
detailed review.

	Arnd

Arnd Bergmann (28):
  [v2] netfilter: nf_tables: avoid uninitialized variable warning
  [v2] mtd: mtk: avoid warning in mtk_ecc_encode
  [v2] infiniband: shut up a maybe-uninitialized warning
  f2fs: replace a build-time warning with runtime WARN_ON
  ext2: avoid bogus -Wmaybe-uninitialized warning
  NFSv4.1: work around -Wmaybe-uninitialized warning
  ceph: avoid false positive maybe-uninitialized warning
  staging: lustre: restore initialization of return code
  staging: lustre: remove broken dead code in
    cfs_cpt_table_create_pattern
  UBI: fix uninitialized access of vid_hdr pointer
  block: rdb: false-postive gcc-4.9 -Wmaybe-uninitialized
  [media] rc: print correct variable for z8f0811
  [media] dib0700: fix uninitialized data on 'repeat' event
  iio: accel: sca3000_core: avoid potentially uninitialized variable
  crypto: aesni: avoid -Wmaybe-uninitialized warning
  pcmcia: fix return value of soc_pcmcia_regulator_set
  spi: fsl-espi: avoid processing uninitalized data on error
  drm: avoid uninitialized timestamp use in wait_vblank
  brcmfmac: avoid maybe-uninitialized warning in brcmf_cfg80211_start_ap
  net: bcm63xx: avoid referencing uninitialized variable
  net/hyperv: avoid uninitialized variable
  x86: apm: avoid uninitialized data
  x86: mark target address as output in 'insb' asm
  x86: math-emu: possible uninitialized variable use
  s390: pci: don't print uninitialized data for debugging
  nios2: fix timer initcall return value
  rocker: fix maybe-uninitialized warning
  Kbuild: bring back -Wmaybe-uninitialized warning

 Makefile                                           |  10 +-
 arch/arc/Makefile                                  |   4 +-
 arch/nios2/kernel/time.c                           |   1 +
 arch/s390/pci/pci_dma.c                            |   2 +-
 arch/x86/crypto/aesni-intel_glue.c                 | 121 +++++++++++++--------
 arch/x86/include/asm/io.h                          |   4 +-
 arch/x86/kernel/apm_32.c                           |   5 +-
 arch/x86/math-emu/Makefile                         |   4 +-
 arch/x86/math-emu/reg_compare.c                    |  16 +--
 drivers/block/rbd.c                                |   1 +
 drivers/gpu/drm/drm_irq.c                          |   4 +-
 drivers/infiniband/core/cma.c                      |  56 +++++-----
 drivers/media/i2c/ir-kbd-i2c.c                     |   2 +-
 drivers/media/usb/dvb-usb/dib0700_core.c           |  10 +-
 drivers/mtd/nand/mtk_ecc.c                         |  19 ++--
 drivers/mtd/ubi/eba.c                              |   2 +-
 drivers/net/ethernet/broadcom/bcm63xx_enet.c       |   3 +-
 drivers/net/ethernet/rocker/rocker_ofdpa.c         |   4 +-
 drivers/net/hyperv/netvsc_drv.c                    |   2 +-
 .../broadcom/brcm80211/brcmfmac/cfg80211.c         |   2 +-
 drivers/pcmcia/soc_common.c                        |   2 +-
 drivers/spi/spi-fsl-espi.c                         |   2 +-
 drivers/staging/iio/accel/sca3000_core.c           |   2 +
 .../staging/lustre/lnet/libcfs/linux/linux-cpu.c   |   7 --
 drivers/staging/lustre/lustre/lov/lov_pack.c       |   2 +
 fs/ceph/super.c                                    |   3 +-
 fs/ext2/inode.c                                    |   7 +-
 fs/f2fs/data.c                                     |   7 ++
 fs/nfs/nfs4session.c                               |  10 +-
 net/netfilter/nft_range.c                          |  10 +-
 scripts/Makefile.ubsan                             |   4 +
 31 files changed, 187 insertions(+), 141 deletions(-)

-- 
Cc: x86@kernel.org
Cc: linux-media@vger.kernel.org
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: linux-s390@vger.kernel.org
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-mtd@lists.infradead.org
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: linux-crypto@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: ceph-devel@vger.kernel.org
Cc: linux-f2fs-devel@lists.sourceforge.net
Cc: linux-ext4@vger.kernel.org
Cc: netfilter-devel@vger.kernel.org
2.9.0

[PATCH 02/28] [v2] mtd: mtk: avoid warning in mtk_ecc_encode

[Arnd Bergmann]

When building with -Wmaybe-uninitialized, gcc produces a silly false positive
warning for the mtk_ecc_encode function:

drivers/mtd/nand/mtk_ecc.c: In function 'mtk_ecc_encode':
drivers/mtd/nand/mtk_ecc.c:402:15: error: 'val' may be used uninitialized in this function [-Werror=maybe-uninitialized]

The function for some reason contains a double byte swap on big-endian
builds to get the OOB data into the correct order again, and is written
in a slightly confusing way.

Using a simple memcpy32_fromio() to read the data simplifies it a lot
so it becomes more readable and produces no warning. However, the
output might not have 32-bit alignment, so we have to use another
memcpy to avoid taking alignment faults or writing beyond the end
of the array.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
v2: move temporary buffer into struct mtk_ecc instead of having it
    on the stack, as suggested by Boris Brezillon
---
 drivers/mtd/nand/mtk_ecc.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
index d54f666..dbf2562 100644
--- a/drivers/mtd/nand/mtk_ecc.c
+++ b/drivers/mtd/nand/mtk_ecc.c
@@ -86,6 +86,8 @@ struct mtk_ecc {
 	struct completion done;
 	struct mutex lock;
 	u32 sectors;
+
+	u8 eccdata[112];
 };
 
 static inline void mtk_ecc_wait_idle(struct mtk_ecc *ecc,
@@ -366,9 +368,8 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
 		   u8 *data, u32 bytes)
 {
 	dma_addr_t addr;
-	u8 *p;
-	u32 len, i, val;
-	int ret = 0;
+	u32 len;
+	int ret;
 
 	addr = dma_map_single(ecc->dev, data, bytes, DMA_TO_DEVICE);
 	ret = dma_mapping_error(ecc->dev, addr);
@@ -393,14 +394,12 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
 
 	/* Program ECC bytes to OOB: per sector oob = FDM + ECC + SPARE */
 	len = (config->strength * ECC_PARITY_BITS + 7) >> 3;
-	p = data + bytes;
 
-	/* write the parity bytes generated by the ECC back to the OOB region */
-	for (i = 0; i < len; i++) {
-		if ((i % 4) == 0)
-			val = readl(ecc->regs + ECC_ENCPAR(i / 4));
-		p[i] = (val >> ((i % 4) * 8)) & 0xff;
-	}
+	/* write the parity bytes generated by the ECC back to temp buffer */
+	__ioread32_copy(ecc->eccdata, ecc->regs + ECC_ENCPAR(0), round_up(len, 4));
+
+	/* copy into possibly unaligned OOB region with actual length */
+	memcpy(data + bytes, ecc->eccdata, len);
 timeout:
 
 	dma_unmap_single(ecc->dev, addr, bytes, DMA_TO_DEVICE);
-- 
2.9.0

[PATCH 10/28] UBI: fix uninitialized access of vid_hdr pointer

[Arnd Bergmann]

A rework of UBI that just appeared in linux-next during the merge
window introduced caused the recover_peb to use a variable that
is never initialized as seen from this gcc warning:

drivers/mtd/ubi/eba.c: In function ‘recover_peb’:
drivers/mtd/ubi/eba.c:744:40: error: ‘vid_hdr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]

It seems clear that the change to the function arguments was missing
the initialization that I'm now adding back to restore the
way the function was working before.

Fixes: 3291b52f9ff0 ("UBI: introduce the VID buffer concept")
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Richard Weinberger <richard@nod.at>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/mtd/ubi/eba.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index 95c4048..2e152be 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -719,7 +719,7 @@ static int try_recover_peb(struct ubi_volume *vol, int pnum, int lnum,
 			   struct ubi_vid_io_buf *vidb, bool *retry)
 {
 	struct ubi_device *ubi = vol->ubi;
-	struct ubi_vid_hdr *vid_hdr;
+	struct ubi_vid_hdr *vid_hdr = ubi_get_vid_hdr(vidb);
 	int new_pnum, err, vol_id = vol->vol_id, data_size;
 	uint32_t crc;
 
-- 
2.9.0

[PATCH 28/28] Kbuild: bring back -Wmaybe-uninitialized warning

[Arnd Bergmann]

Traditionally, we have always had warnings about uninitialized variables
enabled, as this is part of -Wall, and generally a good idea [1], but it
also always produced false positives, mainly because this is a variation
of the halting problem and provably impossible to get right in all cases
[2].

Various people have identified cases that are particularly bad for false
positives, and in commit e74fc973b6e5 ("Turn off -Wmaybe-uninitialized
when building with -Os"), I turned off the warning for any build that
was done with CC_OPTIMIZE_FOR_SIZE.  This drastically reduced the number
of false positive warnings in the default build but unfortunately had
the side effect of turning the warning off completely in 'allmodconfig'
builds, which in turn led to a lot of warnings (both actual bugs, and
remaining false positives) to go in unnoticed.

With commit 877417e6ffb9 ("Kbuild: change CC_OPTIMIZE_FOR_SIZE
definition") enabled the warning again for allmodconfig builds in v4.7
and in v4.8-rc1, I had finally managed to address all warnings I get in
an ARM allmodconfig build and most other maybe-uninitialized warnings
for ARM randconfig builds.

However, commit 6e8d666e9253 ("Disable "maybe-uninitialized" warning
globally") was merged at the same time and disabled it completely for
all configurations, because of false-positive warnings on x86 that
I had not addressed until then. This caused a lot of actual bugs to
get merged into mainline, and I sent several dozen patches for these
during the v4.9 development cycle. Most of these are actual bugs,
some are for correct code that is safe because it is only called
under external constraints that make it impossible to run into
the case that gcc sees, and in a few cases gcc is just stupid and
finds something that can obviously never happen.

I have now done a few thousand randconfig builds on x86 and collected
all patches that I needed to address every single warning I got
(I can provide the combined patch for the other warnings if anyone
is interested), so I hope we can get the warning back and let people
catch the actual bugs earlier.

Note that the majority of the patches I created are for the third kind
of problem (stupid false-positives), for one of two reasons:
- some of them only get triggered in certain combinations of config
  options, so we don't always run into them, and
- the actual bugs tend to get addressed much quicker as they also
  lead to incorrect runtime behavior.

These 27 patches address the warnings that either occur in one of the more
common configurations (defconfig, allmodconfig, or something built by the
kbuild robot or kernelci.org), or they are about a real bug. It would be
good to get these all into v4.9 if we want to turn on the warning again.
I have tested these extensively with gcc-4.9 and gcc-6 and done a bit
of testing with gcc-5, and all of these should now be fine. gcc-4.8
is much worse about the false-positive warnings and is also fairly old
now, so I'm leaving the warning disabled with that version. gcc-4.7 and
older don't understand the -Wno-maybe-uninitialized option and are not
affected by this patch either way.

I have another (smaller) series of patches for warnings that are both
harmless and not as easy to trigger, and I will send them for inclusion
in v4.10.

Link: https://rusty.ozlabs.org/?p=232 [1]
Link: https://gcc.gnu.org/wiki/Better_Uninitialized_Warnings [2]
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 Makefile               | 10 ++++++----
 arch/arc/Makefile      |  4 +++-
 scripts/Makefile.ubsan |  4 ++++
 3 files changed, 13 insertions(+), 5 deletions(-)

Cc: x86@kernel.org
Cc: linux-media@vger.kernel.org
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: linux-s390@vger.kernel.org
Cc: Ilya Dryomov <idryomov@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-mtd@lists.infradead.org
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: linux-crypto@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: ceph-devel@vger.kernel.org
Cc: linux-f2fs-devel@lists.sourceforge.net
Cc: linux-ext4@vger.kernel.org
Cc: netfilter-devel@vger.kernel.org

diff --git a/Makefile b/Makefile
index 512e47a..43cd3d9 100644
--- a/Makefile
+++ b/Makefile
@@ -370,7 +370,7 @@ LDFLAGS_MODULE  =
 CFLAGS_KERNEL	=
 AFLAGS_KERNEL	=
 LDFLAGS_vmlinux =
-CFLAGS_GCOV	= -fprofile-arcs -ftest-coverage -fno-tree-loop-im
+CFLAGS_GCOV	= -fprofile-arcs -ftest-coverage -fno-tree-loop-im  -Wno-maybe-uninitialized
 CFLAGS_KCOV	:= $(call cc-option,-fsanitize-coverage=trace-pc,)
 
 
@@ -620,7 +620,6 @@ ARCH_CFLAGS :=
 include arch/$(SRCARCH)/Makefile
 
 KBUILD_CFLAGS	+= $(call cc-option,-fno-delete-null-pointer-checks,)
-KBUILD_CFLAGS	+= $(call cc-disable-warning,maybe-uninitialized,)
 KBUILD_CFLAGS	+= $(call cc-disable-warning,frame-address,)
 
 ifdef CONFIG_LD_DEAD_CODE_DATA_ELIMINATION
@@ -629,15 +628,18 @@ KBUILD_CFLAGS	+= $(call cc-option,-fdata-sections,)
 endif
 
 ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE
-KBUILD_CFLAGS	+= -Os
+KBUILD_CFLAGS	+= -Os $(call cc-disable-warning,maybe-uninitialized,)
 else
 ifdef CONFIG_PROFILE_ALL_BRANCHES
-KBUILD_CFLAGS	+= -O2
+KBUILD_CFLAGS	+= -O2 $(call cc-disable-warning,maybe-uninitialized,)
 else
 KBUILD_CFLAGS   += -O2
 endif
 endif
 
+KBUILD_CFLAGS += $(call cc-ifversion, -lt, 0409, \
+			$(call cc-disable-warning,maybe-uninitialized,))
+
 # Tell gcc to never replace conditional load with a non-conditional one
 KBUILD_CFLAGS	+= $(call cc-option,--param=allow-store-data-races=0)
 
diff --git a/arch/arc/Makefile b/arch/arc/Makefile
index aa82d13..19cce22 100644
--- a/arch/arc/Makefile
+++ b/arch/arc/Makefile
@@ -71,7 +71,9 @@ cflags-$(CONFIG_ARC_DW2_UNWIND)		+= -fasynchronous-unwind-tables $(cfi)
 ifndef CONFIG_CC_OPTIMIZE_FOR_SIZE
 # Generic build system uses -O2, we want -O3
 # Note: No need to add to cflags-y as that happens anyways
-ARCH_CFLAGS += -O3
+#
+# Disable the false maybe-uninitialized warings gcc spits out at -O3
+ARCH_CFLAGS += -O3 $(call cc-disable-warning,maybe-uninitialized,)
 endif
 
 # small data is default for elf32 tool-chain. If not usable, disable it
diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
index dd779c4..3b1b138 100644
--- a/scripts/Makefile.ubsan
+++ b/scripts/Makefile.ubsan
@@ -17,4 +17,8 @@ endif
 ifdef CONFIG_UBSAN_NULL
       CFLAGS_UBSAN += $(call cc-option, -fsanitize=null)
 endif
+
+      # -fsanitize=* options makes GCC less smart than usual and
+      # increase number of 'maybe-uninitialized false-positives
+      CFLAGS_UBSAN += $(call cc-option, -Wno-maybe-uninitialized)
 endif
-- 
2.9.0

Re: [PATCH 00/28] Reenable maybe-uninitialized warnings

[Christoph Hellwig]

On Tue, Oct 18, 2016 at 12:03:28AM +0200, Arnd Bergmann wrote:
> This is a set of patches that I hope to get into v4.9 in some form
> in order to turn on the -Wmaybe-uninitialized warnings again.

Hi Arnd,

I jsut complained to Geert that I was introducing way to many
bugs or pointless warnings for some compilers lately, but gcc didn't
warn me about them.  From a little research the lack of
-Wmaybe-uninitialized seems to be the reason for it, so I'm all
for re-enabling it.

Re: [PATCH 10/28] UBI: fix uninitialized access of vid_hdr pointer

[Boris Brezillon]

Hi Arnd,

On Tue, 18 Oct 2016 00:10:13 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> A rework of UBI that just appeared in linux-next during the merge
> window introduced caused the recover_peb to use a variable that
> is never initialized as seen from this gcc warning:
> 
> drivers/mtd/ubi/eba.c: In function ‘recover_peb’:
> drivers/mtd/ubi/eba.c:744:40: error: ‘vid_hdr’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> It seems clear that the change to the function arguments was missing
> the initialization that I'm now adding back to restore the
> way the function was working before.

Thanks for the fix, but Geert already sent a patch for this bug a few
days ago.

Regards,

Boris

> 
> Fixes: 3291b52f9ff0 ("UBI: introduce the VID buffer concept")
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Richard Weinberger <richard@nod.at>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/mtd/ubi/eba.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
> index 95c4048..2e152be 100644
> --- a/drivers/mtd/ubi/eba.c
> +++ b/drivers/mtd/ubi/eba.c
> @@ -719,7 +719,7 @@ static int try_recover_peb(struct ubi_volume *vol, int pnum, int lnum,
>  			   struct ubi_vid_io_buf *vidb, bool *retry)
>  {
>  	struct ubi_device *ubi = vol->ubi;
> -	struct ubi_vid_hdr *vid_hdr;
> +	struct ubi_vid_hdr *vid_hdr = ubi_get_vid_hdr(vidb);
>  	int new_pnum, err, vol_id = vol->vol_id, data_size;
>  	uint32_t crc;
>  

Re: [PATCH 02/28] [v2] mtd: mtk: avoid warning in mtk_ecc_encode

[Boris Brezillon]

On Tue, 18 Oct 2016 00:05:31 +0200
Arnd Bergmann <arnd@arndb.de> wrote:

> When building with -Wmaybe-uninitialized, gcc produces a silly false positive
> warning for the mtk_ecc_encode function:
> 
> drivers/mtd/nand/mtk_ecc.c: In function 'mtk_ecc_encode':
> drivers/mtd/nand/mtk_ecc.c:402:15: error: 'val' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> The function for some reason contains a double byte swap on big-endian
> builds to get the OOB data into the correct order again, and is written
> in a slightly confusing way.
> 
> Using a simple memcpy32_fromio() to read the data simplifies it a lot
> so it becomes more readable and produces no warning. However, the
> output might not have 32-bit alignment, so we have to use another
> memcpy to avoid taking alignment faults or writing beyond the end
> of the array.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Jorge, RogerCC, can I have an Acked-by and/or Tested-by for this patch?

> ---
> v2: move temporary buffer into struct mtk_ecc instead of having it
>     on the stack, as suggested by Boris Brezillon
> ---
>  drivers/mtd/nand/mtk_ecc.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
> index d54f666..dbf2562 100644
> --- a/drivers/mtd/nand/mtk_ecc.c
> +++ b/drivers/mtd/nand/mtk_ecc.c
> @@ -86,6 +86,8 @@ struct mtk_ecc {
>  	struct completion done;
>  	struct mutex lock;
>  	u32 sectors;
> +
> +	u8 eccdata[112];
>  };
>  
>  static inline void mtk_ecc_wait_idle(struct mtk_ecc *ecc,
> @@ -366,9 +368,8 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
>  		   u8 *data, u32 bytes)
>  {
>  	dma_addr_t addr;
> -	u8 *p;
> -	u32 len, i, val;
> -	int ret = 0;
> +	u32 len;
> +	int ret;
>  
>  	addr = dma_map_single(ecc->dev, data, bytes, DMA_TO_DEVICE);
>  	ret = dma_mapping_error(ecc->dev, addr);
> @@ -393,14 +394,12 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
>  
>  	/* Program ECC bytes to OOB: per sector oob = FDM + ECC + SPARE */
>  	len = (config->strength * ECC_PARITY_BITS + 7) >> 3;
> -	p = data + bytes;
>  
> -	/* write the parity bytes generated by the ECC back to the OOB region */
> -	for (i = 0; i < len; i++) {
> -		if ((i % 4) == 0)
> -			val = readl(ecc->regs + ECC_ENCPAR(i / 4));
> -		p[i] = (val >> ((i % 4) * 8)) & 0xff;
> -	}
> +	/* write the parity bytes generated by the ECC back to temp buffer */
> +	__ioread32_copy(ecc->eccdata, ecc->regs + ECC_ENCPAR(0), round_up(len, 4));
> +
> +	/* copy into possibly unaligned OOB region with actual length */
> +	memcpy(data + bytes, ecc->eccdata, len);
>  timeout:
>  
>  	dma_unmap_single(ecc->dev, addr, bytes, DMA_TO_DEVICE);

Re: [PATCH 02/28] [v2] mtd: mtk: avoid warning in mtk_ecc_encode

[RogerCC.Lin]

On Tue, 2016-10-18 at 07:19 +0200, Boris Brezillon wrote:
> On Tue, 18 Oct 2016 00:05:31 +0200
> Arnd Bergmann <arnd@arndb.de> wrote:
> 
> > When building with -Wmaybe-uninitialized, gcc produces a silly false positive
> > warning for the mtk_ecc_encode function:
> > 
> > drivers/mtd/nand/mtk_ecc.c: In function 'mtk_ecc_encode':
> > drivers/mtd/nand/mtk_ecc.c:402:15: error: 'val' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > 
> > The function for some reason contains a double byte swap on big-endian
> > builds to get the OOB data into the correct order again, and is written
> > in a slightly confusing way.
> > 
> > Using a simple memcpy32_fromio() to read the data simplifies it a lot
> > so it becomes more readable and produces no warning. However, the
> > output might not have 32-bit alignment, so we have to use another
> > memcpy to avoid taking alignment faults or writing beyond the end
> > of the array.
> > 
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> Jorge, RogerCC, can I have an Acked-by and/or Tested-by for this patch?
Tested, this patch is OK,
Tested-by: RogerCC Lin <rogercc.lin@mediatek.com>

> 
> > ---
> > v2: move temporary buffer into struct mtk_ecc instead of having it
> >     on the stack, as suggested by Boris Brezillon
> > ---
> >  drivers/mtd/nand/mtk_ecc.c | 19 +++++++++----------
> >  1 file changed, 9 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
> > index d54f666..dbf2562 100644
> > --- a/drivers/mtd/nand/mtk_ecc.c
> > +++ b/drivers/mtd/nand/mtk_ecc.c
> > @@ -86,6 +86,8 @@ struct mtk_ecc {
> >  	struct completion done;
> >  	struct mutex lock;
> >  	u32 sectors;
> > +
> > +	u8 eccdata[112];
> >  };
> >  
> >  static inline void mtk_ecc_wait_idle(struct mtk_ecc *ecc,
> > @@ -366,9 +368,8 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
> >  		   u8 *data, u32 bytes)
> >  {
> >  	dma_addr_t addr;
> > -	u8 *p;
> > -	u32 len, i, val;
> > -	int ret = 0;
> > +	u32 len;
> > +	int ret;
> >  
> >  	addr = dma_map_single(ecc->dev, data, bytes, DMA_TO_DEVICE);
> >  	ret = dma_mapping_error(ecc->dev, addr);
> > @@ -393,14 +394,12 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
> >  
> >  	/* Program ECC bytes to OOB: per sector oob = FDM + ECC + SPARE */
> >  	len = (config->strength * ECC_PARITY_BITS + 7) >> 3;
> > -	p = data + bytes;
> >  
> > -	/* write the parity bytes generated by the ECC back to the OOB region */
> > -	for (i = 0; i < len; i++) {
> > -		if ((i % 4) == 0)
> > -			val = readl(ecc->regs + ECC_ENCPAR(i / 4));
> > -		p[i] = (val >> ((i % 4) * 8)) & 0xff;
> > -	}
> > +	/* write the parity bytes generated by the ECC back to temp buffer */
> > +	__ioread32_copy(ecc->eccdata, ecc->regs + ECC_ENCPAR(0), round_up(len, 4));
> > +
> > +	/* copy into possibly unaligned OOB region with actual length */
> > +	memcpy(data + bytes, ecc->eccdata, len);
> >  timeout:
> >  
> >  	dma_unmap_single(ecc->dev, addr, bytes, DMA_TO_DEVICE);
> 

Re: [PATCH 02/28] [v2] mtd: mtk: avoid warning in mtk_ecc_encode

[Boris Brezillon]

On Tue, 18 Oct 2016 18:12:32 +0800
RogerCC.Lin <rogercc.lin@mediatek.com> wrote:

> On Tue, 2016-10-18 at 07:19 +0200, Boris Brezillon wrote:
> > On Tue, 18 Oct 2016 00:05:31 +0200
> > Arnd Bergmann <arnd@arndb.de> wrote:
> >   
> > > When building with -Wmaybe-uninitialized, gcc produces a silly false positive
> > > warning for the mtk_ecc_encode function:
> > > 
> > > drivers/mtd/nand/mtk_ecc.c: In function 'mtk_ecc_encode':
> > > drivers/mtd/nand/mtk_ecc.c:402:15: error: 'val' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> > > 
> > > The function for some reason contains a double byte swap on big-endian
> > > builds to get the OOB data into the correct order again, and is written
> > > in a slightly confusing way.
> > > 
> > > Using a simple memcpy32_fromio() to read the data simplifies it a lot
> > > so it becomes more readable and produces no warning. However, the
> > > output might not have 32-bit alignment, so we have to use another
> > > memcpy to avoid taking alignment faults or writing beyond the end
> > > of the array.
> > > 
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>  
> > 
> > Jorge, RogerCC, can I have an Acked-by and/or Tested-by for this patch?  
> Tested, this patch is OK,
> Tested-by: RogerCC Lin <rogercc.lin@mediatek.com>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

Brian, can you take this patch for the next -rc?

> 
> >   
> > > ---
> > > v2: move temporary buffer into struct mtk_ecc instead of having it
> > >     on the stack, as suggested by Boris Brezillon
> > > ---
> > >  drivers/mtd/nand/mtk_ecc.c | 19 +++++++++----------
> > >  1 file changed, 9 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/mtk_ecc.c b/drivers/mtd/nand/mtk_ecc.c
> > > index d54f666..dbf2562 100644
> > > --- a/drivers/mtd/nand/mtk_ecc.c
> > > +++ b/drivers/mtd/nand/mtk_ecc.c
> > > @@ -86,6 +86,8 @@ struct mtk_ecc {
> > >  	struct completion done;
> > >  	struct mutex lock;
> > >  	u32 sectors;
> > > +
> > > +	u8 eccdata[112];
> > >  };
> > >  
> > >  static inline void mtk_ecc_wait_idle(struct mtk_ecc *ecc,
> > > @@ -366,9 +368,8 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
> > >  		   u8 *data, u32 bytes)
> > >  {
> > >  	dma_addr_t addr;
> > > -	u8 *p;
> > > -	u32 len, i, val;
> > > -	int ret = 0;
> > > +	u32 len;
> > > +	int ret;
> > >  
> > >  	addr = dma_map_single(ecc->dev, data, bytes, DMA_TO_DEVICE);
> > >  	ret = dma_mapping_error(ecc->dev, addr);
> > > @@ -393,14 +394,12 @@ int mtk_ecc_encode(struct mtk_ecc *ecc, struct mtk_ecc_config *config,
> > >  
> > >  	/* Program ECC bytes to OOB: per sector oob = FDM + ECC + SPARE */
> > >  	len = (config->strength * ECC_PARITY_BITS + 7) >> 3;
> > > -	p = data + bytes;
> > >  
> > > -	/* write the parity bytes generated by the ECC back to the OOB region */
> > > -	for (i = 0; i < len; i++) {
> > > -		if ((i % 4) == 0)
> > > -			val = readl(ecc->regs + ECC_ENCPAR(i / 4));
> > > -		p[i] = (val >> ((i % 4) * 8)) & 0xff;
> > > -	}
> > > +	/* write the parity bytes generated by the ECC back to temp buffer */
> > > +	__ioread32_copy(ecc->eccdata, ecc->regs + ECC_ENCPAR(0), round_up(len, 4));
> > > +
> > > +	/* copy into possibly unaligned OOB region with actual length */
> > > +	memcpy(data + bytes, ecc->eccdata, len);
> > >  timeout:
> > >  
> > >  	dma_unmap_single(ecc->dev, addr, bytes, DMA_TO_DEVICE);  
> >   
> 
>