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

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

[Arnd Bergmann]

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

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

[Ludovic BARRE]

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

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

[Geert Uytterhoeven]

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

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

[Ludovic BARRE]

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
> 

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

[Geert Uytterhoeven]

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

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

[Ludovic BARRE]

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
> 

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

[Arnd Bergmann]

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

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

[Ludovic BARRE]

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
>