Re: [PATCH -next 5/6] staging: rtl8723bs: mark some variables as __maybe_unused

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Edmundo Carmona Antoranz <eantoranz@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Marco Cesati <marcocesati@gmail.com>,
	Ross Schmidt <ross.schm.dev@gmail.com>,
	fabioaiuto83@gmail.com, linux-staging@lists.linux.dev,
	kernel-janitors@vger.kernel.org
Subject: Re: [PATCH -next 5/6] staging: rtl8723bs: mark some variables as __maybe_unused
Date: Sat, 27 Mar 2021 19:27:53 +0300	[thread overview]
Message-ID: <20210327162752.GG1717@kadam> (raw)
In-Reply-To: <CAOc6etZaqnDXU4dgBmx_qd8iwpmEjDhs=7JnLuCUFxs65=TRHQ@mail.gmail.com>

On Sat, Mar 27, 2021 at 08:44:29AM -0600, Edmundo Carmona Antoranz wrote:
> On Sat, Mar 27, 2021 at 2:18 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > This is just papering over badness.  Leave the warnings as is so someone
> > will notice it and fix it properly in the future.
> >

Take a look at the first example:

  1277  /* define REJOIN */
           ^^^^^^^^^^^^^
This is developer code which can't be turned on through the Kconfig.
Automatically that means we are allowed to delete all the #ifdef REJOIN
code.

  1278  void rtw_joinbss_event_prehandle(struct adapter *adapter, u8 *pbuf)
  1279  {
  1280          static u8 retry;
                ^^^^^^^^^^^^^^^

  1397          } else {/* if join_res < 0 (join fails), then try again */
  1398  
  1399                  #ifdef REJOIN
  1400                  res = _FAIL;
  1401                  if (retry < 2) {

This function is now only allowed to be called twice.  What the heck
sort of function is that where we can only call it twice???  Who calls
this anyway?  Something to investigate.

  1402                          res = rtw_select_and_join_from_scanned_queue(pmlmepriv);
                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Here we are setting "res".

  1403                          RT_TRACE(_module_rtl871x_mlme_c_, _drv_err_, ("rtw_select_and_join_from_scanned_queue again! res:%d\n", res));
  1404                  }
  1405  
  1406                  if (res == _SUCCESS) {
  1407                          /* extend time of assoc_timer */
  1408                          _set_timer(&pmlmepriv->assoc_timer, MAX_JOIN_TIMEOUT);
  1409                          retry++;

So, okay, we're only allowed to succeed twice.  We can fail as many
times as we want.  Weird.

  1410                  } else if (res == 2) {/* there is no need to wait for join */

What does 1 is _SUCCESS, 0 is failure, but what is 2?  It turns out that
rtw_select_and_join_from_scanned_queue() never returns 2...

  1411                          _clr_fwstate_(pmlmepriv, _FW_UNDER_LINKING);
  1412                          rtw_indicate_connect(adapter);
  1413                  } else {
  1414                          RT_TRACE(_module_rtl871x_mlme_c_, _drv_err_, ("Set Assoc_Timer = 1; can't find match ssid in scanned_q\n"));
  1415                  #endif
  1416  
  1417                          _set_timer(&pmlmepriv->assoc_timer, 1);
  1418                          /* rtw_free_assoc_resources(adapter, 1); */
  1419                          _clr_fwstate_(pmlmepriv, _FW_UNDER_LINKING);
  1420  
  1421                  #ifdef REJOIN
  1422                          retry = 0;
  1423                  }
  1424                  #endif
  1425          }
  1426  
  1427  ignore_joinbss_callback:
  1428  
  1429          spin_unlock_bh(&pmlmepriv->lock);
  1430  }

So a lot of this is nonsensical dead code, and the "unused variable" was
a useful clue about that.  Probably all these are nonsensical in their
own way so there isn't going to be one answer.  Look carefully at each
warning and then think about how to re-write it completely.

regards,
dan carpenter