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 <[email protected]>
---
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
Hi Sven,
On Tue, Jan 5, 2016 at 2:29 AM, Sven Dziadek <[email protected]> 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: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
Julian Calaby <[email protected]> writes:
> Hi Sven,
>
> On Tue, Jan 5, 2016 at 2:29 AM, Sven Dziadek <[email protected]> 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
On 01/05/2016 09:53 AM, Jes Sorensen wrote:
> Julian Calaby <[email protected]> writes:
>> Hi Sven,
>>
>> On Tue, Jan 5, 2016 at 2:29 AM, Sven Dziadek <[email protected]> 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
Sven Dziadek <[email protected]> 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 <[email protected]>
> ---
>
> 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