From: Jacob Kiefer <[email protected]>
This patch fixes the following sparse errors:
CHECK drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
...
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:118:25: \
warning: incorrect type in assignment (different base types)
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:118:25: \
expected unsigned int [unsigned] [usertype] <noident>
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:118:25: \
got restricted __le32 [usertype] <noident>
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:130:14: \
warning: incorrect type in assignment (different base types)
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:130:14: \
expected unsigned int [unsigned] [usertype] mask
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:130:14: \
got restricted __le32 [usertype] <noident>
...
Signed-off-by: Jacob Kiefer <[email protected]>
---
Took a fresh look at the code. In v2, verified that the converted
buffer logic is the same as previous: i.e. an 8-bit pointer to
what really is a 32-bit little endian buffer is passed to
FillH2CCmd and used in a memcpy within. Likewise, a 32-bit little endian
mask is used in memcpy. This patch makes the code cleaner with
no confusing reuse of buffers for both little- and (possibly)
big-endian data and clears the sparse errors.
As for __le32 typing, le32 is not available in this scope,
since it is defined under fs/ntfs/types.h.
---
drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
index 9733aa6..cceda59 100644
--- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
+++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
@@ -115,9 +115,11 @@ exit:
int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)
{
- *((u32 *)param) = cpu_to_le32(*((u32 *)param));
+ __le32 leparam;
- FillH2CCmd(padapter, RSSI_SETTING_EID, 3, param);
+ leparam = cpu_to_le32(*((u32 *)param));
+
+ FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)&leparam);
return _SUCCESS;
}
@@ -125,10 +127,11 @@ int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)
int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg)
{
u8 buf[5];
+ __le32 lemask;
memset(buf, 0, 5);
- mask = cpu_to_le32(mask);
- memcpy(buf, &mask, 4);
+ lemask = cpu_to_le32(mask);
+ memcpy(buf, &lemask, 4);
buf[4] = arg;
FillH2CCmd(padapter, MACID_CONFIG_EID, 5, buf);
--
1.8.3.2
Hello
This patch set fixes the same sparse errors as v2, taking Al's
advice into consideration and changing the interfaces to little-endian.
Jake
On Tue, Oct 06, 2015 at 12:32:28AM -0400, Jacob Kiefer wrote:
> int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)
> {
> - *((u32 *)param) = cpu_to_le32(*((u32 *)param));
> + __le32 leparam;
>
> - FillH2CCmd(padapter, RSSI_SETTING_EID, 3, param);
> + leparam = cpu_to_le32(*((u32 *)param));
> +
> + FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)&leparam);
Why not fill the thing we are passing already with little-endian? There's
only one caller, after all...
> int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg)
> {
> u8 buf[5];
> + __le32 lemask;
>
> memset(buf, 0, 5);
> - mask = cpu_to_le32(mask);
> - memcpy(buf, &mask, 4);
> + lemask = cpu_to_le32(mask);
> + memcpy(buf, &lemask, 4);
> buf[4] = arg;
>
> FillH2CCmd(padapter, MACID_CONFIG_EID, 5, buf);
Ugh...
struct macid_config_eid {__le32 mask; u8 arg;} buf = {
.mask = cpu_to_le32(mask), .arg = arg
};
FillH2CCmd(padapter, MACID_CONFIG_EID, 5, &buf);
Why bother with memcpy/memset/whatnot when all you are trying to do is to
initialize a temporary structure? And no, it's not going to have any gaps...
On Wed, Oct 07, 2015 at 12:11:34AM +0100, Al Viro wrote:
> On Tue, Oct 06, 2015 at 12:32:28AM -0400, Jacob Kiefer wrote:
>
> > int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)
> > {
> > - *((u32 *)param) = cpu_to_le32(*((u32 *)param));
> > + __le32 leparam;
> >
> > - FillH2CCmd(padapter, RSSI_SETTING_EID, 3, param);
> > + leparam = cpu_to_le32(*((u32 *)param));
> > +
> > + FillH2CCmd(padapter, RSSI_SETTING_EID, 3, (u8 *)&leparam);
>
> Why not fill the thing we are passing already with little-endian? There's
> only one caller, after all...
>
This is a good point. Adding to that, I looked at the one caller: they
take a u32, dereference it and convert it to a u8 pointer and then pass
it into rtl8723a_set_rssi_cmd -- it seems that the u8 *param passed should just
be a __le32 or little-endian u32, and then get deferenced and cast for
FillH2CCmd.
rtl8723a_set_rssi_cmd caller code for reference:
> for (i = 0; i < sta_cnt; i++) {
> if (PWDB_rssi[i] != (0))
> rtl8723a_set_rssi_cmd(Adapter, (u8 *)&PWDB_rssi[i]);
> }
> > int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg)
> > {
> > u8 buf[5];
> > + __le32 lemask;
> >
> > memset(buf, 0, 5);
> > - mask = cpu_to_le32(mask);
> > - memcpy(buf, &mask, 4);
> > + lemask = cpu_to_le32(mask);
> > + memcpy(buf, &lemask, 4);
> > buf[4] = arg;
> >
> > FillH2CCmd(padapter, MACID_CONFIG_EID, 5, buf);
>
> Ugh...
> struct macid_config_eid {__le32 mask; u8 arg;} buf = {
> .mask = cpu_to_le32(mask), .arg = arg
> };
>
> FillH2CCmd(padapter, MACID_CONFIG_EID, 5, &buf);
>
> Why bother with memcpy/memset/whatnot when all you are trying to do is to
> initialize a temporary structure? And no, it's not going to have any gaps...
This is also a good point -- we can definitely avoid the memcpy/memsets
this way. Additionally, there are only two callers to rtl8723a_set_raid_cmd;
there's no reason the u32 mask can't be passed in as little-endian
__le32 or u32.
One question -- which is preferable: a u32 that we know is
little-endian, or an explicit __le32? __le32 would not cause sparse
errors, so I'm inclined to go that route, is there something wrong with
this?
Going forward, I'm going to split this up into two patches, one for
each function. In each I'll change the interface to take a __le32
instead of what is going on here and make the caller do the le32
conversion. I'll also update the caller references and the .h file
for each. I'll submit them as a patch set later this week.
Let me know your thoughts on this.
Jake
On Sat, Oct 10, 2015 at 02:50:44PM -0400, Jacob Kiefer wrote:
> Hello
>
> This patch set fixes the same sparse errors as v2, taking Al's
> advice into consideration and changing the interfaces to little-endian.
Please resend these in a format that I can apply them in (i.e. one patch
per email in a series, like you see on the mailing list.)
thanks,
greg k-h