2024-04-15 21:10:20

by Bitterblue Smith

Subject: [PATCH] wifi: rtl8xxxu: Fix gen1 rate mask command

The H2C (host to card) command which tells the firmware which TX rates
it can use is slightly wrong. Fix the order of the bytes.

Also put the macid in the command (relevant for AP mode).

This was tested with RTL8192CU. It also affects the RTL8723AU.

Cc: [email protected]
Signed-off-by: Bitterblue Smith <[email protected]>
---
.../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h | 10 +++++++---
.../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 18 +++++++++++-------
2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
index fd92d23c43d9..ca44d82cb5aa 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
@@ -1430,10 +1430,14 @@ struct h2c_cmd {
u8 data;
} __packed joinbss;
struct {
+#define RAID_MASK GENMASK(31, 28)
+#define RATE_MASK_MASK GENMASK(27, 0)
+#define MACID_MASK GENMASK(4, 0)
+#define SHORT_GI_MASK BIT(5)
+
u8 cmd;
- __le16 mask_hi;
- u8 arg;
- __le16 mask_lo;
+ __le32 rate_mask_and_raid;
+ u8 macid_and_short_gi;
} __packed ramask;
struct {
u8 cmd;
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index fac7824ae727..acbafc25c6e0 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -4641,15 +4641,19 @@ void rtl8xxxu_update_rate_mask(struct rtl8xxxu_priv *priv,
memset(&h2c, 0, sizeof(struct h2c_cmd));

h2c.ramask.cmd = H2C_SET_RATE_MASK;
- h2c.ramask.mask_lo = cpu_to_le16(ramask & 0xffff);
- h2c.ramask.mask_hi = cpu_to_le16(ramask >> 16);

- h2c.ramask.arg = 0x80;
- if (sgi)
- h2c.ramask.arg |= 0x20;
+ le32p_replace_bits(&h2c.ramask.rate_mask_and_raid, rateid, RAID_MASK);
+ le32p_replace_bits(&h2c.ramask.rate_mask_and_raid, ramask, RATE_MASK_MASK);
+
+ u8p_replace_bits(&h2c.ramask.macid_and_short_gi, macid, MACID_MASK);
+ u8p_replace_bits(&h2c.ramask.macid_and_short_gi, sgi, SHORT_GI_MASK);
+ u8p_replace_bits(&h2c.ramask.macid_and_short_gi, 1, BIT(7));
+
+ dev_dbg(&priv->udev->dev,
+ "%s: rate mask %08x, rate id %02x, arg %02x, size %zi\n",
+ __func__, ramask, rateid, h2c.ramask.macid_and_short_gi,
+ sizeof(h2c.ramask));

- dev_dbg(&priv->udev->dev, "%s: rate mask %08x, arg %02x, size %zi\n",
- __func__, ramask, h2c.ramask.arg, sizeof(h2c.ramask));
rtl8xxxu_gen1_h2c_cmd(priv, &h2c, sizeof(h2c.ramask));
}

--
2.44.0

2024-04-16 04:06:02

by Ping-Ke Shih

Subject: RE: [PATCH] wifi: rtl8xxxu: Fix gen1 rate mask command

Bitterblue Smith <[email protected]> wrote:
>
> The H2C (host to card) command which tells the firmware which TX rates
> it can use is slightly wrong. Fix the order of the bytes.
>
> Also put the macid in the command (relevant for AP mode).
>
> This was tested with RTL8192CU. It also affects the RTL8723AU.

Can you add test results before/after this patch?

I wonder if RTL8192CU and RTL8723AU use different command format, because
vendor driver of RTL8192CU seems use different command ID (w/o BIT(7)), see below.

>
> Cc: [email protected]
> Signed-off-by: Bitterblue Smith <[email protected]>
> ---
> .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h | 10 +++++++---
> .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 18 +++++++++++-------
> 2 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> index fd92d23c43d9..ca44d82cb5aa 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
> @@ -1430,10 +1430,14 @@ struct h2c_cmd {
> u8 data;
> } __packed joinbss;
> struct {
> +#define RAID_MASK GENMASK(31, 28)
> +#define RATE_MASK_MASK GENMASK(27, 0)
> +#define MACID_MASK GENMASK(4, 0)
> +#define SHORT_GI_MASK BIT(5)
> +
> u8 cmd;
> - __le16 mask_hi;
> - u8 arg;
> - __le16 mask_lo;
> + __le32 rate_mask_and_raid;
> + u8 macid_and_short_gi;
> } __packed ramask;
> struct {
> u8 cmd;
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index fac7824ae727..acbafc25c6e0 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -4641,15 +4641,19 @@ void rtl8xxxu_update_rate_mask(struct rtl8xxxu_priv *priv,
> memset(&h2c, 0, sizeof(struct h2c_cmd));
>
> h2c.ramask.cmd = H2C_SET_RATE_MASK;

rtl8xxxu: H2C_SET_RATE_MASK = (6 | H2C_EXT)
vendor driver of 8192cu: MACID_CONFIG_EID=6

Can you confirm if command "(6 | BIT(7))" works on 8192cu?
Maybe, it only works on 8723au? (this chip is too old, I don't have more info about it)


> - h2c.ramask.mask_lo = cpu_to_le16(ramask & 0xffff);
> - h2c.ramask.mask_hi = cpu_to_le16(ramask >> 16);
>
> - h2c.ramask.arg = 0x80;
> - if (sgi)
> - h2c.ramask.arg |= 0x20;
> + le32p_replace_bits(&h2c.ramask.rate_mask_and_raid, rateid, RAID_MASK);
> + le32p_replace_bits(&h2c.ramask.rate_mask_and_raid, ramask, RATE_MASK_MASK);
> +
> + u8p_replace_bits(&h2c.ramask.macid_and_short_gi, macid, MACID_MASK);
> + u8p_replace_bits(&h2c.ramask.macid_and_short_gi, sgi, SHORT_GI_MASK);
> + u8p_replace_bits(&h2c.ramask.macid_and_short_gi, 1, BIT(7));
> +
> + dev_dbg(&priv->udev->dev,
> + "%s: rate mask %08x, rate id %02x, arg %02x, size %zi\n",
> + __func__, ramask, rateid, h2c.ramask.macid_and_short_gi,
> + sizeof(h2c.ramask));
>
> - dev_dbg(&priv->udev->dev, "%s: rate mask %08x, arg %02x, size %zi\n",
> - __func__, ramask, h2c.ramask.arg, sizeof(h2c.ramask));
> rtl8xxxu_gen1_h2c_cmd(priv, &h2c, sizeof(h2c.ramask));
> }
>
> --
> 2.44.0

2024-04-16 11:58:17

by Bitterblue Smith

Subject: Re: [PATCH] wifi: rtl8xxxu: Fix gen1 rate mask command

On 16/04/2024 07:01, Ping-Ke Shih wrote:
> Bitterblue Smith <[email protected]> wrote:
>>
>> The H2C (host to card) command which tells the firmware which TX rates
>> it can use is slightly wrong. Fix the order of the bytes.
>>
>> Also put the macid in the command (relevant for AP mode).
>>
>> This was tested with RTL8192CU. It also affects the RTL8723AU.
>
> Can you add test results before/after this patch?
>
> I wonder if RTL8192CU and RTL8723AU use different command format, because
> vendor driver of RTL8192CU seems use different command ID (w/o BIT(7)), see below.
>

They use the same format, the vendor drivers just add BIT(7)
in rtl8192c_FillH2CCmd function:

https://github.com/lwfinger/rtl8192cu/blob/52b20929a8f68d5fbae38e6e253f75b713258487/hal/rtl8192c_cmd.c#L114

Please ignore this patch. I made a mistake: the order of the
bytes in the struct is weird, but it is correct for rtl8xxxu.
rtl8xxxu_gen1_h2c_cmd is aware of the changed order.

>>
>> Cc: [email protected]
>> Signed-off-by: Bitterblue Smith <[email protected]>
>> ---
>> .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h | 10 +++++++---
>> .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 18 +++++++++++-------
>> 2 files changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
>> index fd92d23c43d9..ca44d82cb5aa 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
>> @@ -1430,10 +1430,14 @@ struct h2c_cmd {
>> u8 data;
>> } __packed joinbss;
>> struct {
>> +#define RAID_MASK GENMASK(31, 28)
>> +#define RATE_MASK_MASK GENMASK(27, 0)
>> +#define MACID_MASK GENMASK(4, 0)
>> +#define SHORT_GI_MASK BIT(5)
>> +
>> u8 cmd;
>> - __le16 mask_hi;
>> - u8 arg;
>> - __le16 mask_lo;
>> + __le32 rate_mask_and_raid;
>> + u8 macid_and_short_gi;
>> } __packed ramask;
>> struct {
>> u8 cmd;
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> index fac7824ae727..acbafc25c6e0 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> @@ -4641,15 +4641,19 @@ void rtl8xxxu_update_rate_mask(struct rtl8xxxu_priv *priv,
>> memset(&h2c, 0, sizeof(struct h2c_cmd));
>>
>> h2c.ramask.cmd = H2C_SET_RATE_MASK;
>
> rtl8xxxu: H2C_SET_RATE_MASK = (6 | H2C_EXT)
> vendor driver of 8192cu: MACID_CONFIG_EID=6
>
> Can you confirm if command "(6 | BIT(7))" works on 8192cu?
> Maybe, it only works on 8723au? (this chip is too old, I don't have more info about it)
>
>
>> - h2c.ramask.mask_lo = cpu_to_le16(ramask & 0xffff);
>> - h2c.ramask.mask_hi = cpu_to_le16(ramask >> 16);
>>
>> - h2c.ramask.arg = 0x80;
>> - if (sgi)
>> - h2c.ramask.arg |= 0x20;
>> + le32p_replace_bits(&h2c.ramask.rate_mask_and_raid, rateid, RAID_MASK);
>> + le32p_replace_bits(&h2c.ramask.rate_mask_and_raid, ramask, RATE_MASK_MASK);
>> +
>> + u8p_replace_bits(&h2c.ramask.macid_and_short_gi, macid, MACID_MASK);
>> + u8p_replace_bits(&h2c.ramask.macid_and_short_gi, sgi, SHORT_GI_MASK);
>> + u8p_replace_bits(&h2c.ramask.macid_and_short_gi, 1, BIT(7));
>> +
>> + dev_dbg(&priv->udev->dev,
>> + "%s: rate mask %08x, rate id %02x, arg %02x, size %zi\n",
>> + __func__, ramask, rateid, h2c.ramask.macid_and_short_gi,
>> + sizeof(h2c.ramask));
>>
>> - dev_dbg(&priv->udev->dev, "%s: rate mask %08x, arg %02x, size %zi\n",
>> - __func__, ramask, h2c.ramask.arg, sizeof(h2c.ramask));
>> rtl8xxxu_gen1_h2c_cmd(priv, &h2c, sizeof(h2c.ramask));
>> }
>>
>> --
>> 2.44.0
>