staging: rtl8723au: fix/clean up two functions

PATCH In review Michel von Czettritz 4 participants 14 messages Jun 30, 2015 – Jul 2, 2015 Staging

Share & tools

Michel von Czettritz

author

2015-06-30 21:52 UTC #0 lore raw

cover staging: rtl8723au: fix/clean up two functions

Initialy fixing a sparse warning, resulted in 3 small patches cleaning rtl8723a_set_rssi_cmd an rtl8723a_set_raid_cmd up a bit.

Both functions now avoid implicit casts, return actual values, and rtl8723a_set_rssi_cmd cannot write in unallocated memory any more.

Michel von Czettritz (3):
staging: rtl8723au: adjust function param, u8* to u32*
staging: rtl8723au: fix "incorrect type in assignment"
staging: rtl8723au: function no longer discards return value

 drivers/staging/rtl8723au/hal/odm.c              |  2 +-
 drivers/staging/rtl8723au/hal/rtl8723a_cmd.c     | 18 ++++++++----------
 drivers/staging/rtl8723au/include/rtl8723a_cmd.h |  2 +-
 3 files changed, 10 insertions(+), 12 deletions(-)

Signature · 1 line

2.4.5

Michel von Czettritz

author

2015-06-30 21:53 UTC #1 lore raw

PATCH 1/3 staging: rtl8723au: adjust function param, u8* to u32*

If u8 *param, in the function rtl8723a_set_rssi_cmd, is only 3 Byte long the 4th Byte of cpu_to_le32 will be written in unallocated memory.

Change the function variable to u32*, so the problem can not occur.

rtl8723a_set_rssi_cmd currently is only called in hal/odm.c and is called with u32* as param. rtl8723a_set_rssi_cmd is never used as a function pointer, nor does it seem to conform to any API.

Signed-off-by Michel von Czettritz <[email protected]>

---
 drivers/staging/rtl8723au/hal/odm.c              | 2 +-
 drivers/staging/rtl8723au/hal/rtl8723a_cmd.c     | 6 +++---
 drivers/staging/rtl8723au/include/rtl8723a_cmd.h | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

drivers/staging/rtl8723au/hal/odm.c +1 -1

index f354f5e..5cbf8b6 100644
@@ -1274,7 +1274,7 @@ static void odm_RSSIMonitorCheck(struct dm_odm_t *pDM_Odm)
 
 	for (i = 0; i < sta_cnt; i++) {
 		if (PWDB_rssi[i] != (0))
-			rtl8723a_set_rssi_cmd(Adapter, (u8 *)&PWDB_rssi[i]);
+			rtl8723a_set_rssi_cmd(Adapter, &PWDB_rssi[i]);
 	}
 
 	pdmpriv->EntryMaxUndecoratedSmoothedPWDB = MaxDB;

drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +3 -3

index 9733aa6..1003365 100644
@@ -113,11 +113,11 @@ exit:
 	return ret;
 }
 
-int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)
+int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 {
-	*((u32 *)param) = cpu_to_le32(*((u32 *)param));
+	*((u32 *)param) = cpu_to_le32(*(param));
 
-	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, param);
+	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)param);
 
 	return _SUCCESS;
 }

drivers/staging/rtl8723au/include/rtl8723a_cmd.h +1 -1

index 014c02e..e39e38a 100644
@@ -149,7 +149,7 @@ void rtl8723a_set_BTCoex_AP_mode_FwRsvdPkt_cmd(struct rtw_adapter *padapter);
 #else
 #define rtl8723a_set_BTCoex_AP_mode_FwRsvdPkt_cmd(padapter) do {} while(0)
 #endif
-int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param);
+int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param);
 int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg);
 void rtl8723a_add_rateatid(struct rtw_adapter *padapter, u32 bitmap, u8 arg, u8 rssi_level);

Signature · 1 line

2.4.5

Michel von Czettritz

author

2015-06-30 21:54 UTC #2 lore raw

PATCH 2/3 staging: rtl8723au: fix "incorrect type in assignment"

In reply to Michel von Czettritz

Writing the output of cpu_to_le32 to an u32 or *(u32*) is an implicit cast and results in an sparse warning.

Since param and mask won't be changed, the implicit cast can be avoided by creating local variables.

Signed-off-by Michel von Czettritz <[email protected]>

---
 drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +6 -4

index 1003365..a5e6726 100644
@@ -115,9 +115,10 @@ exit:
 
 int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 {
-	*((u32 *)param) = cpu_to_le32(*(param));
+	__le32 param_le;
+	param_le = cpu_to_le32(*(param));
 
-	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)param);
+	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)&param_le);
 
 	return _SUCCESS;
 }

@@ -125,10 +126,11 @@ int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg)
 {
 	u8 buf[5];
+	__le32 mask_le;
 
 	memset(buf, 0, 5);
-	mask = cpu_to_le32(mask);
-	memcpy(buf, &mask, 4);
+	mask_le = cpu_to_le32(mask);
+	memcpy(buf, &mask_le, 4);
 	buf[4]  = arg;
 
 	FillH2CCmd(padapter, MACID_CONFIG_EID, 5, buf);

Signature · 1 line

2.4.5

Sudip Mukherjee

2015-07-01 07:06 UTC #3 lore raw

Re: [PATCH 2/3] staging: rtl8723au: fix "incorrect type in assignment"

Quoted 16 lines · Michel von Czettritz

Writing the output of cpu_to_le32 to an u32 or *(u32*) is an implicit cast and results in an sparse warning.
Since param and mask won't be changed, the implicit cast can be avoided by creating local variables.
Signed-off-by Michel von Czettritz <[email protected]>
---
 drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +0 -0
index 1003365..a5e6726 100644

drivers/staging/rtl8723au/hal/rtl8723a_cmd.c exit: · line 118

Quoted 5 lines

drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +1 -1

@@ -115,9 +115,10 @@ exit:
 
 int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 {
-	*((u32 *)param) = cpu_to_le32(*(param));
+	__le32 param_le;

checkpatch warns about "Missing a blank line after declarations"

regards
sudip

Dan Carpenter

2015-07-01 07:56 UTC #4 lore raw

Re: [PATCH 2/3] staging: rtl8723au: fix "incorrect type in assignment"

In reply to Michel von Czettritz

Quoted 16 lines · Michel von Czettritz

Writing the output of cpu_to_le32 to an u32 or *(u32*) is an implicit cast and results in an sparse warning.
Since param and mask won't be changed, the implicit cast can be avoided by creating local variables.
Signed-off-by Michel von Czettritz <[email protected]>
---
 drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +0 -0
index 1003365..a5e6726 100644

drivers/staging/rtl8723au/hal/rtl8723a_cmd.c exit: · line 119

Quoted 6 lines

drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +2 -1

@@ -115,9 +115,10 @@ exit:
 
 int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 {
-	*((u32 *)param) = cpu_to_le32(*(param));
+	__le32 param_le;
+	param_le = cpu_to_le32(*(param));

We shouldn't be passing param as a pointer anyway. It should just be u32 cmd.

	__le32 cmd_le = cpu_to_le32(cmd);

	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)&cmd_le);

Fold it together with patch 1.

regards,
dan carpenter

Jes Sorensen

2015-07-01 20:34 UTC #5 lore raw

Re: [PATCH 2/3] staging: rtl8723au: fix "incorrect type in assignment"

Quoted 28 lines · Dan Carpenter

Quoted 17 lines · Michel von Czettritz
Writing the output of cpu_to_le32 to an u32 or *(u32*) is an implicit cast and results in an sparse warning.
Since param and mask won't be changed, the implicit cast can be avoided by creating local variables.
Signed-off-by Michel von Czettritz <[email protected]>
---
 drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +0 -0
index 1003365..a5e6726 100644
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c exit: · line 119
Quoted 6 lines
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +2 -1
@@ -115,9 +115,10 @@ exit:
 
 int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 {
-	*((u32 *)param) = cpu_to_le32(*(param));
+	__le32 param_le;
+	param_le = cpu_to_le32(*(param));
We shouldn't be passing param as a pointer anyway. It should just be u32 cmd.

Quoted 5 lines

	__le32 cmd_le = cpu_to_le32(cmd);

	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)&cmd_le);

Fold it together with patch 1.

I agree, this would be cleaner.

Jes

Jes Sorensen

2015-07-01 20:31 UTC #6 lore raw

Re: [PATCH 2/3] staging: rtl8723au: fix "incorrect type in assignment"

In reply to Michel von Czettritz

Quoted 29 lines · Michel von Czettritz

Writing the output of cpu_to_le32 to an u32 or *(u32*) is an implicit cast and results in an sparse warning.
Since param and mask won't be changed, the implicit cast can be avoided by creating local variables.
Signed-off-by Michel von Czettritz <[email protected]>
---
 drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +3 -2
index 1003365..a5e6726 100644
@@ -115,9 +115,10 @@ exit:
 
 int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 {
-	*((u32 *)param) = cpu_to_le32(*(param));
+	__le32 param_le;
+	param_le = cpu_to_le32(*(param));
 
-	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)param);
+	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)&param_le);
 
 	return _SUCCESS;
 }

drivers/staging/rtl8723au/hal/rtl8723a_cmd.c int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param) · line 133

Quoted 10 lines

drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +3 -2

@@ -125,10 +126,11 @@ int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg)
 {
 	u8 buf[5];
+	__le32 mask_le;
 
 	memset(buf, 0, 5);
-	mask = cpu_to_le32(mask);
-	memcpy(buf, &mask, 4);
+	mask_le = cpu_to_le32(mask);
+	memcpy(buf, &mask_le, 4);

cpu_to_le32() + memcpy() is ugly - put_unaligned_le32() would make more sense.

Jes

Michel von Czettritz

author

2015-07-01 23:36 UTC #7 lore raw

Re: [PATCH 2/3] staging: rtl8723au: fix "incorrect type in assignment"

Quoted 44 lines · Jes Sorensen

Quoted 29 lines · Michel von Czettritz
Writing the output of cpu_to_le32 to an u32 or *(u32*) is an implicit cast and results in an sparse warning.
Since param and mask won't be changed, the implicit cast can be avoided by creating local variables.
Signed-off-by Michel von Czettritz <[email protected]>
---
 drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +3 -2
index 1003365..a5e6726 100644
@@ -115,9 +115,10 @@ exit:
 
 int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 {
-	*((u32 *)param) = cpu_to_le32(*(param));
+	__le32 param_le;
+	param_le = cpu_to_le32(*(param));
 
-	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)param);
+	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)&param_le);
 
 	return _SUCCESS;
 }
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param) · line 133
Quoted 10 lines
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +3 -2
@@ -125,10 +126,11 @@ int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg)
 {
 	u8 buf[5];
+	__le32 mask_le;
 
 	memset(buf, 0, 5);
-	mask = cpu_to_le32(mask);
-	memcpy(buf, &mask, 4);
+	mask_le = cpu_to_le32(mask);
+	memcpy(buf, &mask_le, 4);
cpu_to_le32() + memcpy() is ugly - put_unaligned_le32() would make more sense.

Quoted 1 line

Jes

I'm not sure if the fold, changing u32* to u32, and changing this, is a bit much. Should this be a second patch? Or maybe a patch for each function?

Regards Michel

Jes Sorensen

2015-07-02 02:00 UTC #8 lore raw

Re: [PATCH 2/3] staging: rtl8723au: fix "incorrect type in assignment"

Quoted 45 lines · Michel von Czettritz

Quoted 44 lines · Jes Sorensen
Quoted 29 lines · Michel von Czettritz
Writing the output of cpu_to_le32 to an u32 or *(u32*) is an implicit cast and results in an sparse warning.
Since param and mask won't be changed, the implicit cast can be avoided by creating local variables.
Signed-off-by Michel von Czettritz <[email protected]>
---
 drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +3 -2
index 1003365..a5e6726 100644
@@ -115,9 +115,10 @@ exit:
 
 int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 {
-	*((u32 *)param) = cpu_to_le32(*(param));
+	__le32 param_le;
+	param_le = cpu_to_le32(*(param));
 
-	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)param);
+	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)&param_le);
 
 	return _SUCCESS;
 }
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param) · line 133
Quoted 10 lines
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +3 -2
@@ -125,10 +126,11 @@ int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg)
 {
 	u8 buf[5];
+	__le32 mask_le;
 
 	memset(buf, 0, 5);
-	mask = cpu_to_le32(mask);
-	memcpy(buf, &mask, 4);
+	mask_le = cpu_to_le32(mask);
+	memcpy(buf, &mask_le, 4);
cpu_to_le32() + memcpy() is ugly - put_unaligned_le32() would make more sense.

Quoted 4 lines

Quoted 1 line
Jes
I'm not sure if the fold, changing u32* to u32, and changing this, is a bit much.
Should this be a second patch? Or maybe a patch for each function?

I would change the u32 * in one patch and the conversion and memcpy in another.

Cheers,
Jes

Michel von Czettritz

author

2015-06-30 21:55 UTC #9 lore raw

PATCH 3/3 staging: rtl8723au: function no longer discards return value

In reply to Michel von Czettritz

The return value of FillH2CCmd in rtl8723a_set_rssi_cmd and rtl8723a_set_raid_cmd is never checked. Both functions always return _SUCCESS.

Both functions now return the return value of FillH2CCmd.

Signed-off-by Michel von Czettritz <[email protected]>

---
 drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/staging/rtl8723au/hal/rtl8723a_cmd.c +2 -6

index a5e6726..72924a78 100644
@@ -118,9 +118,7 @@ int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u32 *param)
 	__le32 param_le;
 	param_le = cpu_to_le32(*(param));
 
-	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)&param_le);
-
-	return _SUCCESS;
+	return FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)&param_le);
 }
 
 int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg)

@@ -133,9 +131,7 @@ int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg)
 	memcpy(buf, &mask_le, 4);
 	buf[4]  = arg;
 
-	FillH2CCmd(padapter, MACID_CONFIG_EID, 5, buf);
-
-	return _SUCCESS;
+	return FillH2CCmd(padapter, MACID_CONFIG_EID, 5, buf);
 }
 
 /* bitmap[0:27] = tx_rate_bitmap */

Signature · 1 line

2.4.5

Sudip Mukherjee

2015-07-01 07:12 UTC #10 lore raw

Re: [PATCH 3/3] staging: rtl8723au: function no longer discards return value

Quoted 5 lines · Michel von Czettritz

The return value of FillH2CCmd in rtl8723a_set_rssi_cmd and rtl8723a_set_raid_cmd is never checked. Both functions always return _SUCCESS.
Both functions now return the return value of FillH2CCmd.

If they are never checked then why do we need to return some value? why not make them void?

regards
sudip

Dan Carpenter

2015-07-01 07:50 UTC #11 lore raw

Re: [PATCH 3/3] staging: rtl8723au: function no longer discards return value

Quoted 8 lines · Sudip Mukherjee

Quoted 5 lines · Michel von Czettritz
The return value of FillH2CCmd in rtl8723a_set_rssi_cmd and rtl8723a_set_raid_cmd is never checked. Both functions always return _SUCCESS.
Both functions now return the return value of FillH2CCmd.
If they are never checked then why do we need to return some value? why not make them void?

They probably *should* be checked eventually?

regards,
dan carpenter

Sudip Mukherjee

2015-07-01 08:08 UTC #12 lore raw

Re: [PATCH 3/3] staging: rtl8723au: function no longer discards return value

Quoted 11 lines · Dan Carpenter

Quoted 8 lines · Sudip Mukherjee
Quoted 5 lines · Michel von Czettritz
The return value of FillH2CCmd in rtl8723a_set_rssi_cmd and rtl8723a_set_raid_cmd is never checked. Both functions always return _SUCCESS.
Both functions now return the return value of FillH2CCmd.
If they are never checked then why do we need to return some value? why not make them void?
They probably *should* be checked eventually?

yes, they should check the sucess/failure of the commands. If that plan is there to check the values afterwards then the patch is correct.

regards
sudip

Michel von Czettritz

author

2015-07-01 16:50 UTC #13 lore raw

Re: [PATCH 3/3] staging: rtl8723au: function no longer discards return value

Quoted 7 lines · Sudip Mukherjee

Quoted 6 lines · Dan Carpenter
Quoted 5 lines · Sudip Mukherjee
Quoted 4 lines · Michel von Czettritz
The return value of FillH2CCmd in rtl8723a_set_rssi_cmd and rtl8723a_set_raid_cmd is never checked. Both functions always return _SUCCESS.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
Both functions now return the return value of FillH2CCmd.
If they are never checked then why do we need to return some value? why not make them void?
They probably *should* be checked eventually?
yes, they should check the sucess/failure of the commands. If that plan is there to check the values afterwards then the patch is correct.

Both functions are currently only called by void functions, which don't check on the return values of the functions.
Nonetheless I think rtl8723a_set_rssi_cmd and rtl8723a_set_raid_cmd should return correct values and another patch should take on the calling void functions error handling. As I'm not fully familiar with the inner workings of the driver, I'd leave the error handling for somebody else to implement, or would take it on later.

regards Michel