[RFC PATCH] staging: rtl8723au: fix byte order problems

[RFC PATCH] staging: rtl8723au: fix byte order problems

[Sven Dziadek]

Remove byte order conversions.
Conversion is already done in usb_ops_linux.c when accessing usb port.
The deleted lines convert to little-endian and then call FillH2CCmd to
convert back. Additionally, they are applied to wrong types and
process wrong parts of variables later on.

Signed-off-by: Sven Dziadek <sven.dziadek@gmx.de>
---

I was looking for some Sparse warnings that I could fix and found this:

drivers/staging/rtl8723au//hal/rtl8723a_cmd.c:96:38: warning: cast to
restricted __le16
drivers/staging/rtl8723au//hal/rtl8723a_cmd.c:100:27: warning: cast to
restricted __le32
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>

The rtl8723au drivers seems to work on many machines already, but
probably mostly on little-endian machines.
The file staging/rtl8723au/hal/rtl8723a_cmd.c contains four conversions
from or to little-endian that look suspicious.
The best example is:

int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)
{
	*((u32 *)param) = cpu_to_le32(*((u32 *)param));

	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, param);

	return _SUCCESS;
}

On little-endian, the conversion does nothing, but on big-endian, it
swaps the order and then only 3 bytes are used in FillH2CCmd (3rd
argument). So it actually ignores the original argument param.

I think it is best to delete these conversions because the actual
conversions are done when transferring data to or from the usb port already.

Unfortunately, I do not have the hardware to test it.
What do you think about the patch?

 drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
index 1662c03c..57f5941 100644
--- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
+++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
@@ -93,11 +93,9 @@ int FillH2CCmd(struct rtw_adapter *padapter, u8 ElementID, u32 CmdLen,
 
 		if (h2c_cmd & BIT(7)) {
 			msgbox_ex_addr = REG_HMEBOX_EXT_0 + (h2c_box_num * EX_MESSAGE_BOX_SIZE);
-			h2c_cmd_ex = le16_to_cpu(h2c_cmd_ex);
 			rtl8723au_write16(padapter, msgbox_ex_addr, h2c_cmd_ex);
 		}
 		msgbox_addr = REG_HMEBOX_0 + (h2c_box_num * MESSAGE_BOX_SIZE);
-		h2c_cmd = le32_to_cpu(h2c_cmd);
 		rtl8723au_write32(padapter, msgbox_addr, h2c_cmd);
 
 		bcmd_down = true;
@@ -115,8 +113,6 @@ exit:
 
 int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)
 {
-	*((u32 *)param) = cpu_to_le32(*((u32 *)param));
-
 	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, param);
 
 	return _SUCCESS;
@@ -127,7 +123,7 @@ int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg)
 	u8 buf[5];
 
 	memset(buf, 0, 5);
-	put_unaligned_le32(mask, buf);
+	memcpy(buf, &mask, 4);
 	buf[4]  = arg;
 
 	FillH2CCmd(padapter, MACID_CONFIG_EID, 5, buf);
-- 
2.1.4

Re: [RFC PATCH] staging: rtl8723au: fix byte order problems

[Jes Sorensen]

Sven Dziadek <sven.dziadek@gmx.de> writes:
> Remove byte order conversions.
> Conversion is already done in usb_ops_linux.c when accessing usb port.
> The deleted lines convert to little-endian and then call FillH2CCmd to
> convert back. Additionally, they are applied to wrong types and
> process wrong parts of variables later on.
>
> Signed-off-by: Sven Dziadek <sven.dziadek@gmx.de>
> ---
>
> I was looking for some Sparse warnings that I could fix and found this:
>
> drivers/staging/rtl8723au//hal/rtl8723a_cmd.c:96:38: warning: cast to
> restricted __le16
> drivers/staging/rtl8723au//hal/rtl8723a_cmd.c:100:27: warning: cast to
> restricted __le32
> 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>
>
> The rtl8723au drivers seems to work on many machines already, but
> probably mostly on little-endian machines.
> The file staging/rtl8723au/hal/rtl8723a_cmd.c contains four conversions
> from or to little-endian that look suspicious.
> The best example is:
>
> int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)
> {
> 	*((u32 *)param) = cpu_to_le32(*((u32 *)param));
>
> 	FillH2CCmd(padapter, RSSI_SETTING_EID, 3, param);
>
> 	return _SUCCESS;
> }
>
> On little-endian, the conversion does nothing, but on big-endian, it
> swaps the order and then only 3 bytes are used in FillH2CCmd (3rd
> argument). So it actually ignores the original argument param.
>
> I think it is best to delete these conversions because the actual
> conversions are done when transferring data to or from the usb port already.
>
> Unfortunately, I do not have the hardware to test it.
> What do you think about the patch?

Be *very* careful with this - there is a lot of dodgy passing back and
forth to the hardware and the format needs to match what the firmware
expects.

Unless you are going to test this and confirm that it actually works,
then please stay away from this.

NAK

Thanks,
Jes

Re: [RFC PATCH] staging: rtl8723au: fix byte order problems

[Julian Calaby]

Hi Sven,

On Tue, Jan 5, 2016 at 2:29 AM, Sven Dziadek <sven.dziadek@gmx.de> wrote:
> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> index 1662c03c..57f5941 100644
> --- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> @@ -93,11 +93,9 @@ int FillH2CCmd(struct rtw_adapter *padapter, u8 ElementID, u32 CmdLen,
>
>                 if (h2c_cmd & BIT(7)) {
>                         msgbox_ex_addr = REG_HMEBOX_EXT_0 + (h2c_box_num * EX_MESSAGE_BOX_SIZE);
> -                       h2c_cmd_ex = le16_to_cpu(h2c_cmd_ex);
>                         rtl8723au_write16(padapter, msgbox_ex_addr, h2c_cmd_ex);
>                 }
>                 msgbox_addr = REG_HMEBOX_0 + (h2c_box_num * MESSAGE_BOX_SIZE);
> -               h2c_cmd = le32_to_cpu(h2c_cmd);
>                 rtl8723au_write32(padapter, msgbox_addr, h2c_cmd);

While Jes has NACK'd this change, it does highlight that the h2c_cmd
and h2c_cmd_ex variables are being used to hold both cpu-endian and
little-endian data. A worthwhile change here might be to move the
conversion into the function call following these lines so that they
remain "clean".

That said, I'm not sure this particular snippet of code would work on
big-endian at all as I'm pretty sure that BIT() produces cpu-endian
values and we know from the line you remove that h2c_cmd is
little-endian at this point.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [RFC PATCH] staging: rtl8723au: fix byte order problems

[Jes Sorensen]

Julian Calaby <julian.calaby@gmail.com> writes:
> Hi Sven,
>
> On Tue, Jan 5, 2016 at 2:29 AM, Sven Dziadek <sven.dziadek@gmx.de> wrote:
>> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
>> index 1662c03c..57f5941 100644
>> --- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
>> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
>> @@ -93,11 +93,9 @@ int FillH2CCmd(struct rtw_adapter *padapter, u8 ElementID, u32 CmdLen,
>>
>>                 if (h2c_cmd & BIT(7)) {
>>                         msgbox_ex_addr = REG_HMEBOX_EXT_0 + (h2c_box_num * EX_MESSAGE_BOX_SIZE);
>> -                       h2c_cmd_ex = le16_to_cpu(h2c_cmd_ex);
>>                         rtl8723au_write16(padapter, msgbox_ex_addr, h2c_cmd_ex);
>>                 }
>>                 msgbox_addr = REG_HMEBOX_0 + (h2c_box_num * MESSAGE_BOX_SIZE);
>> -               h2c_cmd = le32_to_cpu(h2c_cmd);
>>                 rtl8723au_write32(padapter, msgbox_addr, h2c_cmd);
>
> While Jes has NACK'd this change, it does highlight that the h2c_cmd
> and h2c_cmd_ex variables are being used to hold both cpu-endian and
> little-endian data. A worthwhile change here might be to move the
> conversion into the function call following these lines so that they
> remain "clean".
>
> That said, I'm not sure this particular snippet of code would work on
> big-endian at all as I'm pretty sure that BIT() produces cpu-endian
> values and we know from the line you remove that h2c_cmd is
> little-endian at this point.

I am not opposed to cleaning it up, however I hope we can just remove
all of the code down the line instead. You may want to look at the h2c
implementation I did for rtl8xxxu. I believe it does work on big-endian,
at least I know Larry has been able to test it on big-endian systems.

Cheers,
Jes

Re: [RFC PATCH] staging: rtl8723au: fix byte order problems

[Larry Finger]

On 01/05/2016 09:53 AM, Jes Sorensen wrote:
> Julian Calaby <julian.calaby@gmail.com> writes:
>> Hi Sven,
>>
>> On Tue, Jan 5, 2016 at 2:29 AM, Sven Dziadek <sven.dziadek@gmx.de> wrote:
>>> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
>>> index 1662c03c..57f5941 100644
>>> --- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
>>> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
>>> @@ -93,11 +93,9 @@ int FillH2CCmd(struct rtw_adapter *padapter, u8 ElementID, u32 CmdLen,
>>>
>>>                  if (h2c_cmd & BIT(7)) {
>>>                          msgbox_ex_addr = REG_HMEBOX_EXT_0 + (h2c_box_num * EX_MESSAGE_BOX_SIZE);
>>> -                       h2c_cmd_ex = le16_to_cpu(h2c_cmd_ex);
>>>                          rtl8723au_write16(padapter, msgbox_ex_addr, h2c_cmd_ex);
>>>                  }
>>>                  msgbox_addr = REG_HMEBOX_0 + (h2c_box_num * MESSAGE_BOX_SIZE);
>>> -               h2c_cmd = le32_to_cpu(h2c_cmd);
>>>                  rtl8723au_write32(padapter, msgbox_addr, h2c_cmd);
>>
>> While Jes has NACK'd this change, it does highlight that the h2c_cmd
>> and h2c_cmd_ex variables are being used to hold both cpu-endian and
>> little-endian data. A worthwhile change here might be to move the
>> conversion into the function call following these lines so that they
>> remain "clean".
>>
>> That said, I'm not sure this particular snippet of code would work on
>> big-endian at all as I'm pretty sure that BIT() produces cpu-endian
>> values and we know from the line you remove that h2c_cmd is
>> little-endian at this point.
>
> I am not opposed to cleaning it up, however I hope we can just remove
> all of the code down the line instead. You may want to look at the h2c
> implementation I did for rtl8xxxu. I believe it does work on big-endian,
> at least I know Larry has been able to test it on big-endian systems.

I have tested rtl8xxxu for the RTL8192CU devices on a PowerBook G4 with a 
big-endian processor.

I do not have any RTL8723AU devices that can be tested this way, thus I have no 
idea if either of the drivers work on BE. If I were to guess, I would expect the 
staging driver to fail and rtl8xxxu to work.

Larry