staging: wilc1000: need feedback about a patch

Hello there,

I just started patching some drivers to familiarize myself better with the kernel and perhaps contribute more to it if I find things that I can actually handle! (hoping I'll be able to handle more and more throughout time).

Doing so, I found out that in the wilc1000 (staging) driver, I got compiler warnings for casts from u32 variables to pointers and pointers to u32 variables. Of course, as my system is 64 bits, the pointers are stored on that format, while u32 is stored on only 32 bits, making this cast insecure. Replacing the u32 variables by size_t ones corrects this behavior, as size_t is stored on 32 bits for 32 bits systems and 64 bits for 64 bits systems. But I'm wondering if doing so breaks everything here? I mean, why use u32 instead of int at first if 32 bits wasn't needed? Also, there is a variable named u32address, which clearly states that u32 was researched here.

So, you'll find following this email the patch I made for the file host_interface.c which was reporting these warnings a lot. I'd like to get some advice on that patch: is it a good idea to do that? If so, can I improve it? If no, what can I do differently to correct those warnings? I'm here to learn, all feedbacks are welcome!

Once everything is figured out on how to correct that, I'll be happy to apply that correction to the other files of the modules that still report that warning!

Thanks,
Rapha?l

Rapha?l Beamonte (1):
staging: wilc1000: replace addresses variables type from u32 to size_t
for x64 systems

 drivers/staging/wilc1000/coreconfigurator.c |   2 +-
 drivers/staging/wilc1000/coreconfigurator.h |   2 +-
 drivers/staging/wilc1000/host_interface.c   | 126 ++++++++++++++--------------
 drivers/staging/wilc1000/host_interface.h   |   2 +-
 4 files changed, 66 insertions(+), 66 deletions(-)

2.1.4

On x64, moving from pointers to u32 variables raises a compiler warning as u32 variables are stored on 32 bits while pointers are stored on 64 bits. Using size_t solve this problem as size_t is stored on 32 or 64 bits depending on the system.

---
 drivers/staging/wilc1000/coreconfigurator.c |   2 +-
 drivers/staging/wilc1000/coreconfigurator.h |   2 +-
 drivers/staging/wilc1000/host_interface.c   | 126 ++++++++++++++--------------
 drivers/staging/wilc1000/host_interface.h   |   2 +-
 4 files changed, 66 insertions(+), 66 deletions(-)

index c81c41d..f9ddf2a 100644
@@ -1983,7 +1983,7 @@ extern wilc_wlan_oup_t *gpstrWlanOps;
  *  @version	1.0
  */
 s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
-			  u32 u32WIDsCount, bool bRespRequired, u32 drvHandler)
+			  u32 u32WIDsCount, bool bRespRequired, size_t drvHandler)
 {
 	s32 counter = 0, ret = 0;
 	if (gpstrWlanOps == NULL) {

index 3af1935..35b62e6 100644
@@ -167,7 +167,7 @@ s32 CoreConfiguratorInit(void);
 s32 CoreConfiguratorDeInit(void);
 
 s32 SendConfigPkt(u8 u8Mode, tstrWID *pstrWIDs,
-		  u32 u32WIDsCount, bool bRespRequired, u32 drvHandler);
+		  u32 u32WIDsCount, bool bRespRequired, size_t drvHandler);
 s32 ParseNetworkInfo(u8 *pu8MsgBuffer, tstrNetworkInfo **ppstrNetworkInfo);
 s32 DeallocateNetworkInfo(tstrNetworkInfo *pstrNetworkInfo);
 

index e66dee9..cb46a34 100644
@@ -272,7 +272,7 @@ typedef struct {
 } tstrWILC_UsrConnReq;
 
 typedef struct {
-	u32 u32Address;
+	size_t u32Address;
 } tstrHostIfSetDrvHandler;
 
 typedef struct {

2.1.4

2015-08-17 13:12 GMT-04:00 <[email protected]>:

You're right. I tried first to use the struct that is used to declare pstrWFIDrv, but
as it needed a lot of code refactoring I switched to find an easier path, but that's
not as elegant and clean.

Actually, that's what I wondered also! With further researchs, I just ended up finding those patches in the linux-wireless mailing list:
http://www.spinics.net/lists/linux-wireless/msg138655.html
http://www.spinics.net/lists/linux-wireless/msg138656.html

These patches are not integrated with Greg's staging tree yet it seems. That's probably why I missed them at first... They however seem more clean than mine.

This driver is known broken in this way, please see the many discussions about this on the driverdevel mailing list as to what the original developers of the code are trying to do to fix this, and attempts for others who are also trying to clean it up in this way.

I'm also not going to take this type of changes from anyone that does not have the hardware to properly test it, do you have the hardware that this driver controls?

thanks,

greg k-h

2015-08-17 15:55 GMT-04:00 Greg Kroah-Hartman <[email protected]>:

Good tip! I looked up different mailing lists but didn't come across the driver development one, even though I've been sending patches to it in cc... Sorry for that.

No, you're right. I'm trying to contribute with what I find I can do, but I'm still just cutting my teeth at that for the time being. I'm trying to acquire a better knowledge of kernel development to try to contribute at a deeper level in the end. I thought I could work on that, but that'll indeed be difficult to test it without the hardware.

Thanks for the time you're taking to review my patches and give advices Greg!

Rapha?l