-Wsometimes-uninitialized Clang warning in drivers/tty/serial/qcom_geni_serial.c

-Wsometimes-uninitialized Clang warning in drivers/tty/serial/qcom_geni_serial.c

[Nathan Chancellor]

Hi all,

We are trying to get Clang's -Wsometimes-uninitialized turned on for the
kernel as it can catch some bugs that GCC can't. This warning came up:

drivers/tty/serial/qcom_geni_serial.c:1079:6: warning: variable 'baud' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
        if (options)
            ^~~~~~~
drivers/tty/serial/qcom_geni_serial.c:1082:37: note: uninitialized use occurs here
        return uart_set_options(uport, co, baud, parity, bits, flow);
                                           ^~~~
drivers/tty/serial/qcom_geni_serial.c:1079:2: note: remove the 'if' if its condition is always true
        if (options)
        ^~~~~~~~~~~~
drivers/tty/serial/qcom_geni_serial.c:1053:10: note: initialize the variable 'baud' to silence this warning
        int baud;
                ^
                 = 0
1 warning generated.

While this is probably not an issue in practice (I assume baud is always
supplied as an option), we should clean up this warning. I would fix it
myself but I have no idea what baud's initial value should be as it
seems it is dependent on the driver. Your input would be much
appreciated.

Cheers,
Nathan

Re: -Wsometimes-uninitialized Clang warning in drivers/tty/serial/qcom_geni_serial.c

[Greg Kroah-Hartman]

On Thu, Mar 07, 2019 at 05:45:26PM -0700, Nathan Chancellor wrote:
> Hi all,
> 
> We are trying to get Clang's -Wsometimes-uninitialized turned on for the
> kernel as it can catch some bugs that GCC can't. This warning came up:
> 
> drivers/tty/serial/qcom_geni_serial.c:1079:6: warning: variable 'baud' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>         if (options)
>             ^~~~~~~
> drivers/tty/serial/qcom_geni_serial.c:1082:37: note: uninitialized use occurs here
>         return uart_set_options(uport, co, baud, parity, bits, flow);
>                                            ^~~~
> drivers/tty/serial/qcom_geni_serial.c:1079:2: note: remove the 'if' if its condition is always true
>         if (options)
>         ^~~~~~~~~~~~
> drivers/tty/serial/qcom_geni_serial.c:1053:10: note: initialize the variable 'baud' to silence this warning
>         int baud;
>                 ^
>                  = 0
> 1 warning generated.
> 
> While this is probably not an issue in practice (I assume baud is always
> supplied as an option), we should clean up this warning. I would fix it
> myself but I have no idea what baud's initial value should be as it
> seems it is dependent on the driver. Your input would be much
> appreciated.

this function can be called with the option variable set to NULL, so
there could not be a default baud rate set, nice catch.

As for the default, let's just be sane and set it to 9600 as that's a
normal default baudrate.

Do you want to make up a patch for this, or do you need me to?

thanks,

greg k-h

[PATCH] tty: serial: qcom_geni_serial: Initialize baud in qcom_geni_console_setup

[Nathan Chancellor]

When building with -Wsometimes-uninitialized, Clang warns:

drivers/tty/serial/qcom_geni_serial.c:1079:6: warning: variable 'baud'
is used uninitialized whenever 'if' condition is false
[-Wsometimes-uninitialized]

It's not wrong; when options is NULL, baud has no default value. Use
9600 as that is a sane default.

Link: https://github.com/ClangBuiltLinux/linux/issues/395
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 drivers/tty/serial/qcom_geni_serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 3bcec1c20219..35e5f9c5d5be 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1050,7 +1050,7 @@ static int __init qcom_geni_console_setup(struct console *co, char *options)
 {
 	struct uart_port *uport;
 	struct qcom_geni_serial_port *port;
-	int baud;
+	int baud = 9600;
 	int bits = 8;
 	int parity = 'n';
 	int flow = 'n';
-- 
2.21.0

Re: [PATCH] tty: serial: qcom_geni_serial: Initialize baud in qcom_geni_console_setup

[Nick Desaulniers]

On Fri, Mar 8, 2019 at 10:38 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> When building with -Wsometimes-uninitialized, Clang warns:
>
> drivers/tty/serial/qcom_geni_serial.c:1079:6: warning: variable 'baud'
> is used uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
>
> It's not wrong; when options is NULL, baud has no default value. Use
> 9600 as that is a sane default.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/395
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>

Great find. Thanks for the fix, and thanks Greg for the advice and
additional analysis here.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>  drivers/tty/serial/qcom_geni_serial.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3bcec1c20219..35e5f9c5d5be 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1050,7 +1050,7 @@ static int __init qcom_geni_console_setup(struct console *co, char *options)
>  {
>         struct uart_port *uport;
>         struct qcom_geni_serial_port *port;
> -       int baud;
> +       int baud = 9600;
>         int bits = 8;
>         int parity = 'n';
>         int flow = 'n';
> --
> 2.21.0
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] tty: serial: qcom_geni_serial: Initialize baud in qcom_geni_console_setup

[Arnd Bergmann]

On Fri, Mar 8, 2019 at 7:38 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> When building with -Wsometimes-uninitialized, Clang warns:
>
> drivers/tty/serial/qcom_geni_serial.c:1079:6: warning: variable 'baud'
> is used uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
>
> It's not wrong; when options is NULL, baud has no default value. Use
> 9600 as that is a sane default.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/395
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3bcec1c20219..35e5f9c5d5be 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1050,7 +1050,7 @@ static int __init qcom_geni_console_setup(struct console *co, char *options)
>  {
>         struct uart_port *uport;
>         struct qcom_geni_serial_port *port;
> -       int baud;
> +       int baud = 9600;
>         int bits = 8;
>         int parity = 'n';
>         int flow = 'n';

I made a similar patch here, but after looking at the driver concluded
that the bitrate had to be 115200, since that is used as the default
value otherwise.

        Arnd

Re: [PATCH] tty: serial: qcom_geni_serial: Initialize baud in qcom_geni_console_setup

[Nick Desaulniers]

On Fri, Mar 22, 2019 at 7:20 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Mar 8, 2019 at 7:38 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > When building with -Wsometimes-uninitialized, Clang warns:
> >
> > drivers/tty/serial/qcom_geni_serial.c:1079:6: warning: variable 'baud'
> > is used uninitialized whenever 'if' condition is false
> > [-Wsometimes-uninitialized]
> >
> > It's not wrong; when options is NULL, baud has no default value. Use
> > 9600 as that is a sane default.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/395
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/tty/serial/qcom_geni_serial.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > index 3bcec1c20219..35e5f9c5d5be 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -1050,7 +1050,7 @@ static int __init qcom_geni_console_setup(struct console *co, char *options)
> >  {
> >         struct uart_port *uport;
> >         struct qcom_geni_serial_port *port;
> > -       int baud;
> > +       int baud = 9600;
> >         int bits = 8;
> >         int parity = 'n';
> >         int flow = 'n';
>
> I made a similar patch here, but after looking at the driver concluded
> that the bitrate had to be 115200, since that is used as the default
> value otherwise.

Good point, we use this driver for Pixel, and I usually connect with a
baud rate of 115200.

-- 
Thanks,
~Nick Desaulniers