[PATCH 0/3] staging: r8188eu: Fix -Wuninitialized instances from clang

[PATCH 0/3] staging: r8188eu: Fix -Wuninitialized instances from clang

[Nathan Chancellor]

Hi all,

Commit 987219ad34a6 ("staging: r8188eu: remove lines from Makefile that
silence build warnings") exposed some instances of -Wuninitialized in
this driver. This series cleans them up. This passes my build tests with
GCC and clang against x86_64 allmodconfig.

Cheers,
Nathan

Nathan Chancellor (3):
  staging: r8188eu: Remove unused static inline functions in rtw_recv.h
  staging: r8188eu: Remove uninitialized use of ether_type in portctrl()
  staging: r8188eu: Reorganize error handling in rtw_drv_init()

 drivers/staging/r8188eu/core/rtw_recv.c    |  4 --
 drivers/staging/r8188eu/include/rtw_recv.h | 46 ----------------------
 drivers/staging/r8188eu/os_dep/usb_intf.c  | 20 +++++-----
 3 files changed, 10 insertions(+), 60 deletions(-)


base-commit: 626520f4ba27d92c8caaf2d1f70c4bca4ea3f9de
-- 
2.33.0.rc2

[PATCH 1/3] staging: r8188eu: Remove unused static inline functions in rtw_recv.h

[Nathan Chancellor]

After commit 987219ad34a6 ("staging: r8188eu: remove lines from Makefile
that silence build warnings"), clang warns several times:

In file included from
drivers/staging/r8188eu/os_dep/../include/drv_types.h:22:
drivers/staging/r8188eu/os_dep/../include/rtw_recv.h:395:9: warning:
variable 'buf_desc' is uninitialized when used here [-Wuninitialized]
        return buf_desc;
               ^~~~~~~~
drivers/staging/r8188eu/os_dep/../include/rtw_recv.h:391:25: note:
initialize the variable 'buf_desc' to silence this warning
        unsigned char *buf_desc;
                               ^
                                = NULL
drivers/staging/r8188eu/os_dep/../include/rtw_recv.h:412:52: warning:
variable 'buf_star' is uninitialized when used here [-Wuninitialized]
        precv_frame = rxmem_to_recvframe((unsigned char *)buf_star);
                                                          ^~~~~~~~
drivers/staging/r8188eu/os_dep/../include/rtw_recv.h:410:14: note:
initialize the variable 'buf_star' to silence this warning
        u8 *buf_star;
                    ^
                     = NULL
2 warnings generated.

The functions that these warnings come from are not used or are called
from functions that are not used so just remove them to remove the
warnings.

Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/staging/r8188eu/include/rtw_recv.h | 46 ----------------------
 1 file changed, 46 deletions(-)

diff --git a/drivers/staging/r8188eu/include/rtw_recv.h b/drivers/staging/r8188eu/include/rtw_recv.h
index 857269ae4209..81594e7aed51 100644
--- a/drivers/staging/r8188eu/include/rtw_recv.h
+++ b/drivers/staging/r8188eu/include/rtw_recv.h
@@ -386,52 +386,6 @@ static inline u8 *recvframe_pull_tail(struct recv_frame *precvframe, int sz)
 	return precvframe->rx_tail;
 }
 
-static inline unsigned char *get_rxbuf_desc(struct recv_frame *precvframe)
-{
-	unsigned char *buf_desc;
-
-	if (precvframe == NULL)
-		return NULL;
-	return buf_desc;
-}
-
-static inline struct recv_frame *rxmem_to_recvframe(u8 *rxmem)
-{
-	/* due to the design of 2048 bytes alignment of recv_frame,
-	 * we can reference the struct recv_frame */
-	/* from any given member of recv_frame. */
-	/*  rxmem indicates the any member/address in recv_frame */
-
-	return (struct recv_frame *)(((size_t)rxmem >> RXFRAME_ALIGN) << RXFRAME_ALIGN);
-}
-
-static inline struct recv_frame *pkt_to_recvframe(struct sk_buff *pkt)
-{
-	u8 *buf_star;
-	struct recv_frame *precv_frame;
-	precv_frame = rxmem_to_recvframe((unsigned char *)buf_star);
-
-	return precv_frame;
-}
-
-static inline u8 *pkt_to_recvmem(struct sk_buff *pkt)
-{
-	/*  return the rx_head */
-
-	struct recv_frame *precv_frame = pkt_to_recvframe(pkt);
-
-	return	precv_frame->rx_head;
-}
-
-static inline u8 *pkt_to_recvdata(struct sk_buff *pkt)
-{
-	/*  return the rx_data */
-
-	struct recv_frame *precv_frame = pkt_to_recvframe(pkt);
-
-	return	precv_frame->rx_data;
-}
-
 static inline int get_recvframe_len(struct recv_frame *precvframe)
 {
 	return precvframe->len;
-- 
2.33.0.rc2

Re: [PATCH 1/3] staging: r8188eu: Remove unused static inline functions in rtw_recv.h

[Phillip Potter]

On Thu, 12 Aug 2021 at 21:40, Nathan Chancellor <nathan@kernel.org> wrote:
>
> After commit 987219ad34a6 ("staging: r8188eu: remove lines from Makefile
> that silence build warnings"), clang warns several times:
>
> In file included from
> drivers/staging/r8188eu/os_dep/../include/drv_types.h:22:
> drivers/staging/r8188eu/os_dep/../include/rtw_recv.h:395:9: warning:
> variable 'buf_desc' is uninitialized when used here [-Wuninitialized]
>         return buf_desc;
>                ^~~~~~~~
> drivers/staging/r8188eu/os_dep/../include/rtw_recv.h:391:25: note:
> initialize the variable 'buf_desc' to silence this warning
>         unsigned char *buf_desc;
>                                ^
>                                 = NULL
> drivers/staging/r8188eu/os_dep/../include/rtw_recv.h:412:52: warning:
> variable 'buf_star' is uninitialized when used here [-Wuninitialized]
>         precv_frame = rxmem_to_recvframe((unsigned char *)buf_star);
>                                                           ^~~~~~~~
> drivers/staging/r8188eu/os_dep/../include/rtw_recv.h:410:14: note:
> initialize the variable 'buf_star' to silence this warning
>         u8 *buf_star;
>                     ^
>                      = NULL
> 2 warnings generated.
>
> The functions that these warnings come from are not used or are called
> from functions that are not used so just remove them to remove the
> warnings.
>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  drivers/staging/r8188eu/include/rtw_recv.h | 46 ----------------------
>  1 file changed, 46 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/include/rtw_recv.h b/drivers/staging/r8188eu/include/rtw_recv.h
> index 857269ae4209..81594e7aed51 100644
> --- a/drivers/staging/r8188eu/include/rtw_recv.h
> +++ b/drivers/staging/r8188eu/include/rtw_recv.h
> @@ -386,52 +386,6 @@ static inline u8 *recvframe_pull_tail(struct recv_frame *precvframe, int sz)
>         return precvframe->rx_tail;
>  }
>
> -static inline unsigned char *get_rxbuf_desc(struct recv_frame *precvframe)
> -{
> -       unsigned char *buf_desc;
> -
> -       if (precvframe == NULL)
> -               return NULL;
> -       return buf_desc;
> -}
> -
> -static inline struct recv_frame *rxmem_to_recvframe(u8 *rxmem)
> -{
> -       /* due to the design of 2048 bytes alignment of recv_frame,
> -        * we can reference the struct recv_frame */
> -       /* from any given member of recv_frame. */
> -       /*  rxmem indicates the any member/address in recv_frame */
> -
> -       return (struct recv_frame *)(((size_t)rxmem >> RXFRAME_ALIGN) << RXFRAME_ALIGN);
> -}
> -
> -static inline struct recv_frame *pkt_to_recvframe(struct sk_buff *pkt)
> -{
> -       u8 *buf_star;
> -       struct recv_frame *precv_frame;
> -       precv_frame = rxmem_to_recvframe((unsigned char *)buf_star);
> -
> -       return precv_frame;
> -}
> -
> -static inline u8 *pkt_to_recvmem(struct sk_buff *pkt)
> -{
> -       /*  return the rx_head */
> -
> -       struct recv_frame *precv_frame = pkt_to_recvframe(pkt);
> -
> -       return  precv_frame->rx_head;
> -}
> -
> -static inline u8 *pkt_to_recvdata(struct sk_buff *pkt)
> -{
> -       /*  return the rx_data */
> -
> -       struct recv_frame *precv_frame = pkt_to_recvframe(pkt);
> -
> -       return  precv_frame->rx_data;
> -}
> -
>  static inline int get_recvframe_len(struct recv_frame *precvframe)
>  {
>         return precvframe->len;
> --
> 2.33.0.rc2
>

Dear Nathan,

Thanks for this.

Acked-by: Phillip Potter <phil@philpotter.co.uk>

Regards,
Phil

[PATCH 2/3] staging: r8188eu: Remove uninitialized use of ether_type in portctrl()

[Nathan Chancellor]

After commit 987219ad34a6 ("staging: r8188eu: remove lines from Makefile
that silence build warnings"), clang warns:

drivers/staging/r8188eu/core/rtw_recv.c:499:8: warning: variable
'ether_type' is uninitialized when used here [-Wuninitialized]
                        if (ether_type == eapol_type)
                            ^~~~~~~~~~
drivers/staging/r8188eu/core/rtw_recv.c:458:16: note: initialize the
variable 'ether_type' to silence this warning
        u16     ether_type;
                          ^
                           = 0
1 warning generated.

This if statement sets the exact same assignment as above so just remove
it.

Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/staging/r8188eu/core/rtw_recv.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
index 9b3637e49052..8df38db9572c 100644
--- a/drivers/staging/r8188eu/core/rtw_recv.c
+++ b/drivers/staging/r8188eu/core/rtw_recv.c
@@ -495,10 +495,6 @@ static struct recv_frame *portctrl(struct adapter *adapter, struct recv_frame *p
 			/* allowed */
 			/* check decryption status, and decrypt the frame if needed */
 			prtnframe = precv_frame;
-			/* check is the EAPOL frame or not (Rekey) */
-			if (ether_type == eapol_type)
-				/* check Rekey */
-				prtnframe = precv_frame;
 		}
 	} else {
 		prtnframe = precv_frame;
-- 
2.33.0.rc2

Re: [PATCH 2/3] staging: r8188eu: Remove uninitialized use of ether_type in portctrl()

[Phillip Potter]

On Thu, 12 Aug 2021 at 21:40, Nathan Chancellor <nathan@kernel.org> wrote:
>
> After commit 987219ad34a6 ("staging: r8188eu: remove lines from Makefile
> that silence build warnings"), clang warns:
>
> drivers/staging/r8188eu/core/rtw_recv.c:499:8: warning: variable
> 'ether_type' is uninitialized when used here [-Wuninitialized]
>                         if (ether_type == eapol_type)
>                             ^~~~~~~~~~
> drivers/staging/r8188eu/core/rtw_recv.c:458:16: note: initialize the
> variable 'ether_type' to silence this warning
>         u16     ether_type;
>                           ^
>                            = 0
> 1 warning generated.
>
> This if statement sets the exact same assignment as above so just remove
> it.
>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  drivers/staging/r8188eu/core/rtw_recv.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/core/rtw_recv.c b/drivers/staging/r8188eu/core/rtw_recv.c
> index 9b3637e49052..8df38db9572c 100644
> --- a/drivers/staging/r8188eu/core/rtw_recv.c
> +++ b/drivers/staging/r8188eu/core/rtw_recv.c
> @@ -495,10 +495,6 @@ static struct recv_frame *portctrl(struct adapter *adapter, struct recv_frame *p
>                         /* allowed */
>                         /* check decryption status, and decrypt the frame if needed */
>                         prtnframe = precv_frame;
> -                       /* check is the EAPOL frame or not (Rekey) */
> -                       if (ether_type == eapol_type)
> -                               /* check Rekey */
> -                               prtnframe = precv_frame;
>                 }
>         } else {
>                 prtnframe = precv_frame;
> --
> 2.33.0.rc2
>

Thanks again.

Acked-by: Phillip Potter <phil@philpotter.co.uk>

Regards,
Phil

[PATCH 3/3] staging: r8188eu: Reorganize error handling in rtw_drv_init()

[Nathan Chancellor]

After commit 987219ad34a6 ("staging: r8188eu: remove lines from Makefile
that silence build warnings"), clang warns:

drivers/staging/r8188eu/os_dep/usb_intf.c:726:6: warning: variable
'status' is used uninitialized whenever 'if' condition is true
[-Wsometimes-uninitialized]
        if (!if1) {
            ^~~~
drivers/staging/r8188eu/os_dep/usb_intf.c:741:6: note: uninitialized use
occurs here
        if (status != _SUCCESS)
            ^~~~~~
drivers/staging/r8188eu/os_dep/usb_intf.c:726:2: note: remove the 'if'
if its condition is always false
        if (!if1) {
        ^~~~~~~~~~~
drivers/staging/r8188eu/os_dep/usb_intf.c:714:12: note: initialize the
variable 'status' to silence this warning
        int status;
                  ^
                   = 0
1 warning generated.

status is not initialized if the call to usb_dvobj_init() or
rtw_usb_if1_init() fails.

Looking at the error function as a whole, the error handling is odd
compared to the rest of the kernel, which prefers to set error codes on
goto paths, rather than a global "status" variable which determines the
error code at the end of the function and function calls in the case of
error.

Rearrange the error handling of this function to bring it more inline
with how the kernel does it in most cases, which helps readability.

The call to rtw_usb_if1_deinit() is eliminated because it is not
possible to ever hit it; if rtw_usb_if1_init() fails, the goto call
jumps over the call to rtw_usb_if1_deinit() and in the success case,
status is set to _SUCCESS.

Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 drivers/staging/r8188eu/os_dep/usb_intf.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
index a462cb6f3005..667f41125a87 100644
--- a/drivers/staging/r8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
@@ -704,20 +704,23 @@ static void rtw_usb_if1_deinit(struct adapter *if1)
 static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device_id *pdid)
 {
 	struct adapter *if1 = NULL;
-	int status;
 	struct dvobj_priv *dvobj;
+	int ret;
 
 	/* step 0. */
 	process_spec_devid(pdid);
 
 	/* Initialize dvobj_priv */
 	dvobj = usb_dvobj_init(pusb_intf);
-	if (!dvobj)
-		goto exit;
+	if (!dvobj) {
+		ret = -ENODEV;
+		goto err;
+	}
 
 	if1 = rtw_usb_if1_init(dvobj, pusb_intf);
 	if (!if1) {
 		DBG_88E("rtw_init_primarystruct adapter Failed!\n");
+		ret = -ENODEV;
 		goto free_dvobj;
 	}
 
@@ -726,15 +729,12 @@ static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device
 		rtw_signal_process(ui_pid[1], SIGUSR2);
 	}
 
-	status = _SUCCESS;
+	return 0;
 
-	if (status != _SUCCESS && if1)
-		rtw_usb_if1_deinit(if1);
 free_dvobj:
-	if (status != _SUCCESS)
-		usb_dvobj_deinit(pusb_intf);
-exit:
-	return status == _SUCCESS ? 0 : -ENODEV;
+	usb_dvobj_deinit(pusb_intf);
+err:
+	return ret;
 }
 
 /*
-- 
2.33.0.rc2

Re: [PATCH 3/3] staging: r8188eu: Reorganize error handling in rtw_drv_init()

[Phillip Potter]

On Thu, 12 Aug 2021 at 21:40, Nathan Chancellor <nathan@kernel.org> wrote:
>
> After commit 987219ad34a6 ("staging: r8188eu: remove lines from Makefile
> that silence build warnings"), clang warns:
>
> drivers/staging/r8188eu/os_dep/usb_intf.c:726:6: warning: variable
> 'status' is used uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
>         if (!if1) {
>             ^~~~
> drivers/staging/r8188eu/os_dep/usb_intf.c:741:6: note: uninitialized use
> occurs here
>         if (status != _SUCCESS)
>             ^~~~~~
> drivers/staging/r8188eu/os_dep/usb_intf.c:726:2: note: remove the 'if'
> if its condition is always false
>         if (!if1) {
>         ^~~~~~~~~~~
> drivers/staging/r8188eu/os_dep/usb_intf.c:714:12: note: initialize the
> variable 'status' to silence this warning
>         int status;
>                   ^
>                    = 0
> 1 warning generated.
>
> status is not initialized if the call to usb_dvobj_init() or
> rtw_usb_if1_init() fails.
>
> Looking at the error function as a whole, the error handling is odd
> compared to the rest of the kernel, which prefers to set error codes on
> goto paths, rather than a global "status" variable which determines the
> error code at the end of the function and function calls in the case of
> error.
>
> Rearrange the error handling of this function to bring it more inline
> with how the kernel does it in most cases, which helps readability.
>
> The call to rtw_usb_if1_deinit() is eliminated because it is not
> possible to ever hit it; if rtw_usb_if1_init() fails, the goto call
> jumps over the call to rtw_usb_if1_deinit() and in the success case,
> status is set to _SUCCESS.
>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  drivers/staging/r8188eu/os_dep/usb_intf.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
> index a462cb6f3005..667f41125a87 100644
> --- a/drivers/staging/r8188eu/os_dep/usb_intf.c
> +++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
> @@ -704,20 +704,23 @@ static void rtw_usb_if1_deinit(struct adapter *if1)
>  static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device_id *pdid)
>  {
>         struct adapter *if1 = NULL;
> -       int status;
>         struct dvobj_priv *dvobj;
> +       int ret;
>
>         /* step 0. */
>         process_spec_devid(pdid);
>
>         /* Initialize dvobj_priv */
>         dvobj = usb_dvobj_init(pusb_intf);
> -       if (!dvobj)
> -               goto exit;
> +       if (!dvobj) {
> +               ret = -ENODEV;
> +               goto err;
> +       }
>
>         if1 = rtw_usb_if1_init(dvobj, pusb_intf);
>         if (!if1) {
>                 DBG_88E("rtw_init_primarystruct adapter Failed!\n");
> +               ret = -ENODEV;
>                 goto free_dvobj;
>         }
>
> @@ -726,15 +729,12 @@ static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device
>                 rtw_signal_process(ui_pid[1], SIGUSR2);
>         }
>
> -       status = _SUCCESS;
> +       return 0;
>
> -       if (status != _SUCCESS && if1)
> -               rtw_usb_if1_deinit(if1);
>  free_dvobj:
> -       if (status != _SUCCESS)
> -               usb_dvobj_deinit(pusb_intf);
> -exit:
> -       return status == _SUCCESS ? 0 : -ENODEV;
> +       usb_dvobj_deinit(pusb_intf);
> +err:
> +       return ret;
>  }
>
>  /*
> --
> 2.33.0.rc2
>

Looks good as far as I can see, nicely done. Thanks.

Acked-by: Phillip Potter <phil@philpotter.co.uk>

Regards,
Phil

Re: [PATCH 3/3] staging: r8188eu: Reorganize error handling in rtw_drv_init()

[Fabio M. De Francesco]

On Thursday, August 12, 2021 10:40:27 PM CEST Nathan Chancellor wrote:
> [...]
> Looking at the error function as a whole, the error handling is odd
> compared to the rest of the kernel, which prefers to set error codes on
> goto paths, rather than a global "status" variable which determines the
> error code at the end of the function and function calls in the case of
> error.
>
"status" is not a global variable, instead it is a local variable with only 
in-function visibility and it has an automatic storage duration (i.e., it is 
allocated on the stack frame of the function and is destroyed when the stack 
is unwound at the return from the function). 

According to the above definition there's no difference in storage classes  
between the old "status" and the new "ret" (the latter being a merely rename 
of the former). However, "ret" is a more appropriate name for that variable.
Furthermore, your handling of errors and return value is more consistent with 
best practices. 

Therefore, apart that minor misuse of the "global" class in your commit 
message, it's a nice work and so...

Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Regards,

Fabio
>
> [...]
> 
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  drivers/staging/r8188eu/os_dep/usb_intf.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> [...]
>

Re: [PATCH 3/3] staging: r8188eu: Reorganize error handling in rtw_drv_init()

[Nathan Chancellor]

On 8/12/2021 4:11 PM, Fabio M. De Francesco wrote:
> On Thursday, August 12, 2021 10:40:27 PM CEST Nathan Chancellor wrote:
>> [...]
>> Looking at the error function as a whole, the error handling is odd
>> compared to the rest of the kernel, which prefers to set error codes on
>> goto paths, rather than a global "status" variable which determines the
>> error code at the end of the function and function calls in the case of
>> error.
>>
> "status" is not a global variable, instead it is a local variable with only
> in-function visibility and it has an automatic storage duration (i.e., it is
> allocated on the stack frame of the function and is destroyed when the stack
> is unwound at the return from the function).
> 
> According to the above definition there's no difference in storage classes
> between the old "status" and the new "ret" (the latter being a merely rename
> of the former). However, "ret" is a more appropriate name for that variable.
> Furthermore, your handling of errors and return value is more consistent with
> best practices.

Sorry, I meant "global" with regards to the function (as in "was there 
an error in the function"), rather than "global" as a storage class.

> Therefore, apart that minor misuse of the "global" class in your commit
> message, it's a nice work and so...

I am happy to redo the commit message if you and others so desire.

> Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>

Thank you for the review and ack!

Cheers,
Nathan

Re: [PATCH 3/3] staging: r8188eu: Reorganize error handling in rtw_drv_init()

[Fabio M. De Francesco]

On Friday, August 13, 2021 1:14:01 AM CEST Nathan Chancellor wrote:
> On 8/12/2021 4:11 PM, Fabio M. De Francesco wrote:
> > On Thursday, August 12, 2021 10:40:27 PM CEST Nathan Chancellor wrote:
> > [...]
> > Therefore, apart that minor misuse of the "global" class in your commit
> > message, it's a nice work and so...
> 
> I am happy to redo the commit message if you and others so desire.
> 
> > Acked-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> 
> Thank you for the review and ack!
> 
> Cheers,
> Nathan
>
Maybe that for that minor misuse of the definition of "global variable" it 
isn't worth to redo three patches. If I were you, I'd wait for feedback from 
Greg K-H and then I'd act accordingly. 

But, at my first read of your patch, I didn't notice that when you return the 
error from within the block starting at the "err:" label, "ret" is always set 
as "-ENODEV".

So, why not simply "return -ENODEV;" and get rid of the "ret" variable?

Thanks,

Fabio