diff mbox series

[v2,1/5] staging: rtl8192e: Remove variable SetWirelessMode

Message ID fba56522e419351b05e33df8cd0ac31806534d8c.1686149467.git.yogi.kernel@gmail.com
State New
Headers
Series Trivial code cleanup patches |

Commit Message

Yogesh Hegde June 7, 2023, 3:01 p.m. UTC 
  The variable SetWirelessMode is set in only one place throughout the
driver. This patch removes the variable and calls the real function
directly instead, eliminating the unnecessary indirection.
Additionally, the removal of the variable aligns with the checkpatch
guidelines by removing the use of CamelCase.

Signed-off-by: Yogesh Hegde <yogi.kernel@gmail.com>
---

v2: Removed the variable and called the function direction instead of
    just renaming the variable as suggested by Greg Kroah-Hartman
    <gregkh@linuxfoundation.org>.
---
 drivers/staging/rtl8192e/rtl8192e/rtl_core.c |  1 -
 drivers/staging/rtl8192e/rtllib.h            |  1 -
 drivers/staging/rtl8192e/rtllib_softmac.c    | 16 +++++++---------
 3 files changed, 7 insertions(+), 11 deletions(-)

Comments

Philipp Hortmann June 7, 2023, 8:18 p.m. UTC | #1 
On 6/7/23 17:07, Dan Carpenter wrote:
> On Wed, Jun 07, 2023 at 08:31:41PM +0530, Yogesh Hegde wrote:
>> The variable SetWirelessMode is set in only one place throughout the
>> driver. This patch removes the variable and calls the real function
>> directly instead, eliminating the unnecessary indirection.
>> Additionally, the removal of the variable aligns with the checkpatch
>> guidelines by removing the use of CamelCase.
>>
>> Signed-off-by: Yogesh Hegde <yogi.kernel@gmail.com>
>> ---
>>
>> v2: Removed the variable and called the function direction instead of
>>      just renaming the variable as suggested by Greg Kroah-Hartman
>>      <gregkh@linuxfoundation.org>.
> 
> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
> 
> regards,
> dan carpenter
> 
> 

Hi Dan,

thanks for all the work you do. I always appreciate your comments.

But I think it does not work because this driver is divided into two 
modules.

To load the driver I am using the following lines:
sudo insmod drivers/staging/rtl8192e/rtllib.ko
sudo insmod drivers/staging/rtl8192e/rtl8192e/r8192e_pci.ko

So this line is required:
priv->rtllib->SetWirelessMode = rtl92e_set_wireless_mode;
as one name is used in one module and one in the other module.

But the change was proposed by the masters so I thought that I must be 
wrong.

My compiler does not compile this patch:
   LD [M]  drivers/staging/rtl8192e/rtl8192e/r8192e_pci.o
   MODPOST drivers/staging/rtl8192e/Module.symvers
ERROR: modpost: "rtl92e_set_wireless_mode" 
[drivers/staging/rtl8192e/rtllib.ko] undefined!
make[1]: *** [scripts/Makefile.modpost:136: 
drivers/staging/rtl8192e/Module.symvers] Error 1
make: *** [Makefile:1978: modpost] Error 2

Did this compile on your system Yogesh?

So I am looking forward to your response what I am doing wrong.

Bye Philipp
Dan Carpenter June 8, 2023, 9 a.m. UTC | #2 
On Wed, Jun 07, 2023 at 10:18:28PM +0200, Philipp Hortmann wrote:
> On 6/7/23 17:07, Dan Carpenter wrote:
> > On Wed, Jun 07, 2023 at 08:31:41PM +0530, Yogesh Hegde wrote:
> > > The variable SetWirelessMode is set in only one place throughout the
> > > driver. This patch removes the variable and calls the real function
> > > directly instead, eliminating the unnecessary indirection.
> > > Additionally, the removal of the variable aligns with the checkpatch
> > > guidelines by removing the use of CamelCase.
> > > 
> > > Signed-off-by: Yogesh Hegde <yogi.kernel@gmail.com>
> > > ---
> > > 
> > > v2: Removed the variable and called the function direction instead of
> > >      just renaming the variable as suggested by Greg Kroah-Hartman
> > >      <gregkh@linuxfoundation.org>.
> > 
> > Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>
> > 
> > regards,
> > dan carpenter
> > 
> > 
> 
> Hi Dan,
> 
> thanks for all the work you do. I always appreciate your comments.
> 

No need to butter me up.  :P


> But I think it does not work because this driver is divided into two
> modules.
> 

Yeah, you're right.  I'm wrong.  We can't break the compile.

regards,
dan carpenter
Yogesh Hegde June 8, 2023, 9:59 a.m. UTC | #3 
On Wed, Jun 07, 2023 at 10:18:28PM +0200, Philipp Hortmann wrote:
> My compiler does not compile this patch:
>   LD [M]  drivers/staging/rtl8192e/rtl8192e/r8192e_pci.o
>   MODPOST drivers/staging/rtl8192e/Module.symvers
> ERROR: modpost: "rtl92e_set_wireless_mode"
> [drivers/staging/rtl8192e/rtllib.ko] undefined!
> make[1]: *** [scripts/Makefile.modpost:136:
> drivers/staging/rtl8192e/Module.symvers] Error 1
> make: *** [Makefile:1978: modpost] Error 2
> 
> Did this compile on your system Yogesh?
No, while sending the patch I partially compiled it 
`make drivers/staging/rtl8192e` but when I fully compile it `make all` it gives 
me the same error. It is a mistake on my part. 

> But I think it does not work because this driver is divided into two
> modules.
> 
> To load the driver I am using the following lines:
> sudo insmod drivers/staging/rtl8192e/rtllib.ko
> sudo insmod drivers/staging/rtl8192e/rtl8192e/r8192e_pci.ko 
I was not aware of this and assumed that ideally the driver has only one (PCI) 
interface so all the files should be compiled into one `.ko` file and loaded.

> So this line is required:
> priv->rtllib->SetWirelessMode = rtl92e_set_wireless_mode;
> as one name is used in one module and one in the other module.
Yes, this makes sense now. 

Moving forward, 
Dan, Greg and Philipp what would be the best fix for this issue, 
1.  Patchset to combine both the modules into one module 
2.  Revert back this patchset to v1.

Looking forward for your response. 

Regards 
Yogesh
Dan Carpenter June 8, 2023, 10:27 a.m. UTC | #4 
On Thu, Jun 08, 2023 at 03:29:38PM +0530, Yogesh Hegde wrote:
> On Wed, Jun 07, 2023 at 10:18:28PM +0200, Philipp Hortmann wrote:
> Dan, Greg and Philipp what would be the best fix for this issue, 
> 1.  Patchset to combine both the modules into one module 
> 2.  Revert back this patchset to v1.

Neither of those options are good.  I haven't investigated this problem
to figure out what the correct option is but we're unlikely to merge
either of those unless you make some very persuasive arguments.

regards,
dan carpenter
Philipp Hortmann June 8, 2023, 12:43 p.m. UTC | #5 
On 6/8/23 11:59, Yogesh Hegde wrote:
> On Wed, Jun 07, 2023 at 10:18:28PM +0200, Philipp Hortmann wrote:
>> My compiler does not compile this patch:
>>    LD [M]  drivers/staging/rtl8192e/rtl8192e/r8192e_pci.o
>>    MODPOST drivers/staging/rtl8192e/Module.symvers
>> ERROR: modpost: "rtl92e_set_wireless_mode"
>> [drivers/staging/rtl8192e/rtllib.ko] undefined!
>> make[1]: *** [scripts/Makefile.modpost:136:
>> drivers/staging/rtl8192e/Module.symvers] Error 1
>> make: *** [Makefile:1978: modpost] Error 2
>>
>> Did this compile on your system Yogesh?
> No, while sending the patch I partially compiled it
> `make drivers/staging/rtl8192e` but when I fully compile it `make all` it gives
> me the same error. It is a mistake on my part.
To build one module you need to use:
make -C . M=drivers/staging/rtl8192e

> 
>> But I think it does not work because this driver is divided into two
>> modules.
>>
>> To load the driver I am using the following lines:
>> sudo insmod drivers/staging/rtl8192e/rtllib.ko
>> sudo insmod drivers/staging/rtl8192e/rtl8192e/r8192e_pci.ko
> I was not aware of this and assumed that ideally the driver has only one (PCI)
> interface so all the files should be compiled into one `.ko` file and loaded.
> 
>> So this line is required:
>> priv->rtllib->SetWirelessMode = rtl92e_set_wireless_mode;
>> as one name is used in one module and one in the other module.
> Yes, this makes sense now.
> 
> Moving forward,
> Dan, Greg and Philipp what would be the best fix for this issue,
> 1.  Patchset to combine both the modules into one module
I had a look for this and it was not difficult at all to combine. But a 
patch long ago divided the two modules, to make it more like the driver 
this one should be merged to, in the wireless subsystem. Please see TODO 
file.

> 2.  Revert back this patchset to v1.
> 
> Looking forward for your response.
> 
> Regards
> Yogesh

Bye Philipp
kernel test robot June 12, 2023, 1:52 a.m. UTC | #6 
Hi Yogesh,

kernel test robot noticed the following build errors:

[auto build test ERROR on staging/staging-testing]
[also build test ERROR on staging/staging-next staging/staging-linus linus/master v6.4-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yogesh-Hegde/staging-rtl8192e-Remove-variable-SetWirelessMode/20230607-230636
base:   staging/staging-testing
patch link:    https://lore.kernel.org/r/fba56522e419351b05e33df8cd0ac31806534d8c.1686149467.git.yogi.kernel%40gmail.com
patch subject: [PATCH v2 1/5] staging: rtl8192e: Remove variable SetWirelessMode
config: x86_64-randconfig-s053-20230611 (https://download.01.org/0day-ci/archive/20230612/202306120923.uMeOfojk-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/823187b988791852e562dba90e5eb7c7e7df8eca
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Yogesh-Hegde/staging-rtl8192e-Remove-variable-SetWirelessMode/20230607-230636
        git checkout 823187b988791852e562dba90e5eb7c7e7df8eca
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 olddefconfig
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306120923.uMeOfojk-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "rtl92e_set_wireless_mode" [drivers/staging/rtl8192e/rtllib.ko] undefined!
diff mbox series

Patch

diff --git a/drivers/staging/rtl8192e/rtl8192e/rtl_core.c b/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
index 17b70dde7eeb..bbe0864e1348 100644
--- a/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
+++ b/drivers/staging/rtl8192e/rtl8192e/rtl_core.c
@@ -716,7 +716,6 @@  static void _rtl92e_init_priv_handler(struct net_device *dev)
 	priv->rtllib->check_nic_enough_desc	= _rtl92e_check_nic_enough_desc;
 	priv->rtllib->handle_assoc_response	= _rtl92e_handle_assoc_response;
 	priv->rtllib->handle_beacon		= _rtl92e_handle_beacon;
-	priv->rtllib->SetWirelessMode		= rtl92e_set_wireless_mode;
 	priv->rtllib->LeisurePSLeave		= rtl92e_leisure_ps_leave;
 	priv->rtllib->SetBWModeHandler		= rtl92e_set_bw_mode;
 	priv->rf_set_chan			= rtl92e_set_channel;
diff --git a/drivers/staging/rtl8192e/rtllib.h b/drivers/staging/rtl8192e/rtllib.h
index 4aa5ce9f7792..68a3b8af3119 100644
--- a/drivers/staging/rtl8192e/rtllib.h
+++ b/drivers/staging/rtl8192e/rtllib.h
@@ -1710,7 +1710,6 @@  struct rtllib_device {
 				 enum ht_channel_width bandwidth,
 				 enum ht_extchnl_offset Offset);
 	bool (*GetNmodeSupportBySecCfg)(struct net_device *dev);
-	void (*SetWirelessMode)(struct net_device *dev, u8 wireless_mode);
 	bool (*GetHalfNmodeSupportByAPsHandler)(struct net_device *dev);
 	u8   (*rtllib_ap_sec_type)(struct rtllib_device *ieee);
 	void (*InitialGainHandler)(struct net_device *dev, u8 Operation);
diff --git a/drivers/staging/rtl8192e/rtllib_softmac.c b/drivers/staging/rtl8192e/rtllib_softmac.c
index 7c4cba6dcf46..ed171eca4916 100644
--- a/drivers/staging/rtl8192e/rtllib_softmac.c
+++ b/drivers/staging/rtl8192e/rtllib_softmac.c
@@ -19,6 +19,7 @@ 
 #include <linux/etherdevice.h>
 #include <linux/ieee80211.h>
 #include "dot11d.h"
+#include "rtl8192e/rtl_core.h"
 
 static void rtllib_sta_wakeup(struct rtllib_device *ieee, short nl);
 
@@ -1503,7 +1504,7 @@  static void rtllib_associate_complete_wq(void *data)
 		netdev_info(ieee->dev, "Using G rates:%d\n", ieee->rate);
 	} else {
 		ieee->rate = 22;
-		ieee->SetWirelessMode(ieee->dev, IEEE_B);
+		rtl92e_set_wireless_mode(ieee->dev, IEEE_B);
 		netdev_info(ieee->dev, "Using B rates:%d\n", ieee->rate);
 	}
 	if (ieee->ht_info->bCurrentHTSupport && ieee->ht_info->enable_ht) {
@@ -1687,14 +1688,12 @@  inline void rtllib_softmac_new_net(struct rtllib_device *ieee,
 				    (ieee->modulation &
 				     RTLLIB_OFDM_MODULATION)) {
 					ieee->rate = 108;
-					ieee->SetWirelessMode(ieee->dev,
-							      IEEE_G);
+					rtl92e_set_wireless_mode(ieee->dev, IEEE_G);
 					netdev_info(ieee->dev,
 						    "Using G rates\n");
 				} else {
 					ieee->rate = 22;
-					ieee->SetWirelessMode(ieee->dev,
-							      IEEE_B);
+					rtl92e_set_wireless_mode(ieee->dev, IEEE_B);
 					netdev_info(ieee->dev,
 						    "Using B rates\n");
 				}
@@ -2276,11 +2275,10 @@  static void rtllib_rx_auth_resp(struct rtllib_device *ieee, struct sk_buff *skb)
 		}
 		/* Dummy wirless mode setting to avoid encryption issue */
 		if (bSupportNmode) {
-			ieee->SetWirelessMode(ieee->dev,
-					      ieee->current_network.mode);
+			rtl92e_set_wireless_mode(ieee->dev, ieee->current_network.mode);
 		} else {
 			/*TODO*/
-			ieee->SetWirelessMode(ieee->dev, IEEE_G);
+			rtl92e_set_wireless_mode(ieee->dev, IEEE_G);
 		}
 
 		if ((ieee->current_network.mode == IEEE_N_24G) &&
@@ -2615,7 +2613,7 @@  static void rtllib_start_ibss_wq(void *data)
 		}
 
 		ieee->current_network.qos_data.supported = 0;
-		ieee->SetWirelessMode(ieee->dev, IEEE_G);
+		rtl92e_set_wireless_mode(ieee->dev, IEEE_G);
 		ieee->current_network.mode = ieee->mode;
 		ieee->current_network.atim_window = 0;
 		ieee->current_network.capability = WLAN_CAPABILITY_IBSS;