staging: r8188eu: Fix -Wuninitialized instances from clang

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 <[email protected]>

---
 drivers/staging/r8188eu/include/rtw_recv.h | 46 ----------------------
 1 file changed, 46 deletions(-)

drivers/staging/r8188eu/include/rtw_recv.h +0 -47

index 857269ae4209..81594e7aed51 100644
@@ -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

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 <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_recv.c | 4 ----
 1 file changed, 4 deletions(-)

drivers/staging/r8188eu/core/rtw_recv.c +0 -5

index 9b3637e49052..8df38db9572c 100644
@@ -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

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 <[email protected]>

---
 drivers/staging/r8188eu/os_dep/usb_intf.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

drivers/staging/r8188eu/os_dep/usb_intf.c +10 -11

index a462cb6f3005..667f41125a87 100644
@@ -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

[...]
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.

[...]

Signed-off-by Nathan Chancellor <[email protected]>

---
 drivers/staging/r8188eu/os_dep/usb_intf.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

[...]

Quoted 7 lines · CEST Nathan Chancellor

[...]
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 <[email protected]>

Quoted 4 lines · Fabio M. De Francesco

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.

Quoted 1 line

Acked-by Fabio M. De Francesco <[email protected]>

Thank you for the review and ack!

Cheers,
Nathan