Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1732218 > unrolled thread

[PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code

Started byArnd Bergmann <arnd@arndb.de>
First post2017-09-14 13:10 +0200
Last post2017-09-15 10:10 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code Arnd Bergmann <arnd@arndb.de> - 2017-09-14 13:10 +0200
    Re: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return  code Ludovic BARRE <ludovic.barre@st.com> - 2017-09-14 14:50 +0200
    Re: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code Geert Uytterhoeven <geert@linux-m68k.org> - 2017-09-14 15:40 +0200
      Re: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return  code Ludovic BARRE <ludovic.barre@st.com> - 2017-09-14 17:20 +0200
        Re: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code Geert Uytterhoeven <geert@linux-m68k.org> - 2017-09-14 17:30 +0200
          Re: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return  code Ludovic BARRE <ludovic.barre@st.com> - 2017-09-14 19:00 +0200
            Re: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code Arnd Bergmann <arnd@arndb.de> - 2017-09-14 23:40 +0200
              Re: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return  code Ludovic BARRE <ludovic.barre@st.com> - 2017-09-15 10:10 +0200

#1732218 — [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code

FromArnd Bergmann <arnd@arndb.de>
Date2017-09-14 13:10 +0200
Subject[PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code
Message-ID<upAbU-4fq-29@gated-at.bofh.it>
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;
-- 
2.9.0

[toc] | [next] | [standalone]

#1732256 — Re: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code

FromLudovic BARRE <ludovic.barre@st.com>
Date2017-09-14 14:50 +0200
SubjectRe: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code
Message-ID<upBKF-55T-1@gated-at.bofh.it>
In reply to#1732218
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;
>

[toc] | [prev] | [next] | [standalone]

#1732274

FromGeert Uytterhoeven <geert@linux-m68k.org>
Date2017-09-14 15:40 +0200
Message-ID<upCx4-5BF-9@gated-at.bofh.it>
In reply to#1732218
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

[toc] | [prev] | [next] | [standalone]

#1732350 — Re: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code

FromLudovic BARRE <ludovic.barre@st.com>
Date2017-09-14 17:20 +0200
SubjectRe: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code
Message-ID<upE5Q-6HF-15@gated-at.bofh.it>
In reply to#1732274
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
>

[toc] | [prev] | [next] | [standalone]

#1732352

FromGeert Uytterhoeven <geert@linux-m68k.org>
Date2017-09-14 17:30 +0200
Message-ID<upEfw-6KR-17@gated-at.bofh.it>
In reply to#1732350
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

[toc] | [prev] | [next] | [standalone]

#1732462 — Re: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code

FromLudovic BARRE <ludovic.barre@st.com>
Date2017-09-14 19:00 +0200
SubjectRe: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code
Message-ID<upFED-7zL-29@gated-at.bofh.it>
In reply to#1732352

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
>

[toc] | [prev] | [next] | [standalone]

#1732601

FromArnd Bergmann <arnd@arndb.de>
Date2017-09-14 23:40 +0200
Message-ID<upK1z-1Uw-5@gated-at.bofh.it>
In reply to#1732462
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

[toc] | [prev] | [next] | [standalone]

#1732769 — Re: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code

FromLudovic BARRE <ludovic.barre@st.com>
Date2017-09-15 10:10 +0200
SubjectRe: [PATCH] mtd: spi-nor: stm32-quadspi: avoid unintialized return code
Message-ID<upTRf-5K-11@gated-at.bofh.it>
In reply to#1732601

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
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web