| Message ID | 20170914110709.3591691-1-arnd@arndb.de |
|---|---|
| State | Superseded |
| Delegated to: | Cyrille Pitchen |
| Headers | show |
| Series | mtd: spi-nor: stm32-quadspi: avoid unintialized return code | expand |
hi Arnd thx Arnd for compilation warning (gcc <= 4.6) Acked-by: Ludovic Barre <ludovic.barre@st.com> PS: at runtime stm32_qspi_tx_poll can't be call, because stm32_qspi_tx check if there is tx data available if (!cmd->tx_data) return 0; BR Ludo On 09/14/2017 01:06 PM, Arnd Bergmann wrote: > If we send zero-length data to stm32_qspi_tx_poll() on older > compiler versions such as gcc-4.6, we get warned that the > return code is uninitialized: > > drivers/mtd/spi-nor/stm32-quadspi.c:248:2: error: ‘ret’ may be used uninitialized in this function [-Werror=uninitialized] > > On newer compiler versions, the return code is always zero > in this case, as the local variable gets optimized away and > is assumed to be zero after the loop completes without error. > > This changes the function to instead return -EINVAL if it > ever gets called with a zero length buffer. > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82203 > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/mtd/spi-nor/stm32-quadspi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/spi-nor/stm32-quadspi.c b/drivers/mtd/spi-nor/stm32-quadspi.c > index 86c0931543c5..711cfe7aa4bf 100644 > --- a/drivers/mtd/spi-nor/stm32-quadspi.c > +++ b/drivers/mtd/spi-nor/stm32-quadspi.c > @@ -227,7 +227,7 @@ static int stm32_qspi_tx_poll(struct stm32_qspi *qspi, > void (*tx_fifo)(u8 *, void __iomem *); > u32 len = cmd->len, sr; > u8 *buf = cmd->buf; > - int ret; > + int ret = -EINVAL; > > if (cmd->qspimode == CCR_FMODE_INDW) > tx_fifo = stm32_qspi_write_fifo; >
Hi Arnd, On Thu, Sep 14, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote: > If we send zero-length data to stm32_qspi_tx_poll() on older > compiler versions such as gcc-4.6, we get warned that the > return code is uninitialized: > > drivers/mtd/spi-nor/stm32-quadspi.c:248:2: error: ‘ret’ may be used uninitialized in this function [-Werror=uninitialized] > > On newer compiler versions, the return code is always zero > in this case, as the local variable gets optimized away and > is assumed to be zero after the loop completes without error. > > This changes the function to instead return -EINVAL if it > ever gets called with a zero length buffer. > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82203 > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/mtd/spi-nor/stm32-quadspi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/spi-nor/stm32-quadspi.c b/drivers/mtd/spi-nor/stm32-quadspi.c > index 86c0931543c5..711cfe7aa4bf 100644 > --- a/drivers/mtd/spi-nor/stm32-quadspi.c > +++ b/drivers/mtd/spi-nor/stm32-quadspi.c > @@ -227,7 +227,7 @@ static int stm32_qspi_tx_poll(struct stm32_qspi *qspi, > void (*tx_fifo)(u8 *, void __iomem *); > u32 len = cmd->len, sr; > u8 *buf = cmd->buf; > - int ret; > + int ret = -EINVAL; > > if (cmd->qspimode == CCR_FMODE_INDW) > tx_fifo = stm32_qspi_write_fifo; See also "[PATCH] mtd: spi-nor: stm32-quadspi: Fix uninitialized error return code" (https://patchwork.kernel.org/patch/9842173/) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
hi Arnd, Geert On 09/14/2017 03:38 PM, Geert Uytterhoeven wrote: > Hi Arnd, > > On Thu, Sep 14, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> If we send zero-length data to stm32_qspi_tx_poll() on older >> compiler versions such as gcc-4.6, we get warned that the >> return code is uninitialized: >> >> drivers/mtd/spi-nor/stm32-quadspi.c:248:2: error: ‘ret’ may be used uninitialized in this function [-Werror=uninitialized] >> >> On newer compiler versions, the return code is always zero >> in this case, as the local variable gets optimized away and >> is assumed to be zero after the loop completes without error. >> >> This changes the function to instead return -EINVAL if it >> ever gets called with a zero length buffer. >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82203 >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> --- >> drivers/mtd/spi-nor/stm32-quadspi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/mtd/spi-nor/stm32-quadspi.c b/drivers/mtd/spi-nor/stm32-quadspi.c >> index 86c0931543c5..711cfe7aa4bf 100644 >> --- a/drivers/mtd/spi-nor/stm32-quadspi.c >> +++ b/drivers/mtd/spi-nor/stm32-quadspi.c >> @@ -227,7 +227,7 @@ static int stm32_qspi_tx_poll(struct stm32_qspi *qspi, >> void (*tx_fifo)(u8 *, void __iomem *); >> u32 len = cmd->len, sr; >> u8 *buf = cmd->buf; >> - int ret; >> + int ret = -EINVAL; >> >> if (cmd->qspimode == CCR_FMODE_INDW) >> tx_fifo = stm32_qspi_write_fifo; > > See also "[PATCH] mtd: spi-nor: stm32-quadspi: Fix uninitialized error > return code" > (https://patchwork.kernel.org/patch/9842173/) hi Arnd, Geert sorry, I was forgot this thread while my holidays Geert: what do you mean like "similar bugs in the future" in "If you initialized ret at the beginning, you lose the ability to catch newly introduced similar bugs in the future." > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds >
Hi Ludovic, On Thu, Sep 14, 2017 at 5:13 PM, Ludovic BARRE <ludovic.barre@st.com> wrote: > On 09/14/2017 03:38 PM, Geert Uytterhoeven wrote: >> On Thu, Sep 14, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>> If we send zero-length data to stm32_qspi_tx_poll() on older >>> compiler versions such as gcc-4.6, we get warned that the >>> return code is uninitialized: >>> >>> drivers/mtd/spi-nor/stm32-quadspi.c:248:2: error: ‘ret’ may be used >>> uninitialized in this function [-Werror=uninitialized] >>> >>> On newer compiler versions, the return code is always zero >>> in this case, as the local variable gets optimized away and >>> is assumed to be zero after the loop completes without error. >>> >>> This changes the function to instead return -EINVAL if it >>> ever gets called with a zero length buffer. >>> >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82203 >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>> --- >>> drivers/mtd/spi-nor/stm32-quadspi.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/mtd/spi-nor/stm32-quadspi.c >>> b/drivers/mtd/spi-nor/stm32-quadspi.c >>> index 86c0931543c5..711cfe7aa4bf 100644 >>> --- a/drivers/mtd/spi-nor/stm32-quadspi.c >>> +++ b/drivers/mtd/spi-nor/stm32-quadspi.c >>> @@ -227,7 +227,7 @@ static int stm32_qspi_tx_poll(struct stm32_qspi >>> *qspi, >>> void (*tx_fifo)(u8 *, void __iomem *); >>> u32 len = cmd->len, sr; >>> u8 *buf = cmd->buf; >>> - int ret; >>> + int ret = -EINVAL; >>> >>> if (cmd->qspimode == CCR_FMODE_INDW) >>> tx_fifo = stm32_qspi_write_fifo; >> >> >> See also "[PATCH] mtd: spi-nor: stm32-quadspi: Fix uninitialized error >> return code" >> (https://patchwork.kernel.org/patch/9842173/) > > hi Arnd, Geert > > sorry, I was forgot this thread while my holidays > > Geert: what do you mean like "similar bugs in the future" in "If you > initialized ret at the beginning, you lose the ability to catch newly > introduced similar bugs in the future." If you pre-initialize ret at the top, you loose the ability of the compiler to detect at compile-time if ret is never written to later. It will just return -EINVAL at runtime. With my version, if the code is modified later and another "return ret" is added, the compiler will detect if there's a code path that forgets to assign a value to ret. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 09/14/2017 05:24 PM, Geert Uytterhoeven wrote: > Hi Ludovic, > > On Thu, Sep 14, 2017 at 5:13 PM, Ludovic BARRE <ludovic.barre@st.com> wrote: >> On 09/14/2017 03:38 PM, Geert Uytterhoeven wrote: >>> On Thu, Sep 14, 2017 at 1:06 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>>> If we send zero-length data to stm32_qspi_tx_poll() on older >>>> compiler versions such as gcc-4.6, we get warned that the >>>> return code is uninitialized: >>>> >>>> drivers/mtd/spi-nor/stm32-quadspi.c:248:2: error: ‘ret’ may be used >>>> uninitialized in this function [-Werror=uninitialized] >>>> >>>> On newer compiler versions, the return code is always zero >>>> in this case, as the local variable gets optimized away and >>>> is assumed to be zero after the loop completes without error. >>>> >>>> This changes the function to instead return -EINVAL if it >>>> ever gets called with a zero length buffer. >>>> >>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82203 >>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>>> --- >>>> drivers/mtd/spi-nor/stm32-quadspi.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mtd/spi-nor/stm32-quadspi.c >>>> b/drivers/mtd/spi-nor/stm32-quadspi.c >>>> index 86c0931543c5..711cfe7aa4bf 100644 >>>> --- a/drivers/mtd/spi-nor/stm32-quadspi.c >>>> +++ b/drivers/mtd/spi-nor/stm32-quadspi.c >>>> @@ -227,7 +227,7 @@ static int stm32_qspi_tx_poll(struct stm32_qspi >>>> *qspi, >>>> void (*tx_fifo)(u8 *, void __iomem *); >>>> u32 len = cmd->len, sr; >>>> u8 *buf = cmd->buf; >>>> - int ret; >>>> + int ret = -EINVAL; >>>> >>>> if (cmd->qspimode == CCR_FMODE_INDW) >>>> tx_fifo = stm32_qspi_write_fifo; >>> >>> >>> See also "[PATCH] mtd: spi-nor: stm32-quadspi: Fix uninitialized error >>> return code" >>> (https://patchwork.kernel.org/patch/9842173/) >> >> hi Arnd, Geert >> >> sorry, I was forgot this thread while my holidays >> >> Geert: what do you mean like "similar bugs in the future" in "If you >> initialized ret at the beginning, you lose the ability to catch newly >> introduced similar bugs in the future." > > If you pre-initialize ret at the top, you loose the ability of the compiler > to detect at compile-time if ret is never written to later. It will just return > -EINVAL at runtime. > > With my version, if the code is modified later and another "return ret" is > added, the compiler will detect if there's a code path that forgets > to assign a value to ret. Ok, it's clear for me. I favor geert's solution. Arnd what do you think ? > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds >
On Thu, Sep 14, 2017 at 6:55 PM, Ludovic BARRE <ludovic.barre@st.com> wrote: > > > On 09/14/2017 05:24 PM, Geert Uytterhoeven wrote: >> >> Hi Ludovic, >> >> On Thu, Sep 14, 2017 at 5:13 PM, Ludovic BARRE <ludovic.barre@st.com> >> wrote: >>> >>> On 09/14/2017 03:38 PM, Geert Uytterhoeven wrote: >>> >>> hi Arnd, Geert >>> >>> sorry, I was forgot this thread while my holidays >>> >>> Geert: what do you mean like "similar bugs in the future" in "If you >>> initialized ret at the beginning, you lose the ability to catch newly >>> introduced similar bugs in the future." >> >> >> If you pre-initialize ret at the top, you loose the ability of the >> compiler >> to detect at compile-time if ret is never written to later. It will just >> return >> -EINVAL at runtime. >> >> With my version, if the code is modified later and another "return ret" is >> added, the compiler will detect if there's a code path that forgets >> to assign a value to ret. > > Ok, it's clear for me. > I favor geert's solution. > Arnd what do you think ? I usually follow the same rule that Geert explained (and quote https://rusty.ozlabs.org/?p=232 when I do so). In this case, there did not seem to be much value as the variable is not used afterwards, and I kept the 'single return statement' guideline. In the end, either version seems totally fine to me here, so please use Geert's if you prefer that. Arnd
On 09/14/2017 11:38 PM, Arnd Bergmann wrote: > On Thu, Sep 14, 2017 at 6:55 PM, Ludovic BARRE <ludovic.barre@st.com> wrote: >> >> >> On 09/14/2017 05:24 PM, Geert Uytterhoeven wrote: >>> >>> Hi Ludovic, >>> >>> On Thu, Sep 14, 2017 at 5:13 PM, Ludovic BARRE <ludovic.barre@st.com> >>> wrote: >>>> >>>> On 09/14/2017 03:38 PM, Geert Uytterhoeven wrote: >>>> >>>> hi Arnd, Geert >>>> >>>> sorry, I was forgot this thread while my holidays >>>> >>>> Geert: what do you mean like "similar bugs in the future" in "If you >>>> initialized ret at the beginning, you lose the ability to catch newly >>>> introduced similar bugs in the future." >>> >>> >>> If you pre-initialize ret at the top, you loose the ability of the >>> compiler >>> to detect at compile-time if ret is never written to later. It will just >>> return >>> -EINVAL at runtime. >>> >>> With my version, if the code is modified later and another "return ret" is >>> added, the compiler will detect if there's a code path that forgets >>> to assign a value to ret. >> >> Ok, it's clear for me. >> I favor geert's solution. >> Arnd what do you think ? > > I usually follow the same rule that Geert explained (and quote > https://rusty.ozlabs.org/?p=232 when I do so). In this case, there > did not seem to be much value as the variable is not used > afterwards, and I kept the 'single return statement' guideline. > > In the end, either version seems totally fine to me here, so > please use Geert's if you prefer that. thank Arnd for your answer, great link :-) we take geert's patch. Geert: I will acked your patch. thanks everybody > > Arnd >
diff --git a/drivers/mtd/spi-nor/stm32-quadspi.c b/drivers/mtd/spi-nor/stm32-quadspi.c index 86c0931543c5..711cfe7aa4bf 100644 --- a/drivers/mtd/spi-nor/stm32-quadspi.c +++ b/drivers/mtd/spi-nor/stm32-quadspi.c @@ -227,7 +227,7 @@ static int stm32_qspi_tx_poll(struct stm32_qspi *qspi, void (*tx_fifo)(u8 *, void __iomem *); u32 len = cmd->len, sr; u8 *buf = cmd->buf; - int ret; + int ret = -EINVAL; if (cmd->qspimode == CCR_FMODE_INDW) tx_fifo = stm32_qspi_write_fifo;
If we send zero-length data to stm32_qspi_tx_poll() on older compiler versions such as gcc-4.6, we get warned that the return code is uninitialized: drivers/mtd/spi-nor/stm32-quadspi.c:248:2: error: ‘ret’ may be used uninitialized in this function [-Werror=uninitialized] On newer compiler versions, the return code is always zero in this case, as the local variable gets optimized away and is assumed to be zero after the loop completes without error. This changes the function to instead return -EINVAL if it ever gets called with a zero length buffer. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82203 Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/mtd/spi-nor/stm32-quadspi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)