| Message ID | 3068a7f8-0178-4ea7-bd18-4e377db07e76@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Ping-Ke Shih |
| Headers | show |
| Series | wifi: rtl8xxxu: Fix gen1 rate mask command | expand |
Bitterblue Smith <rtl8821cerfe2@gmail.com> 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: stable@vger.kernel.org > Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> > --- > .../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
On 16/04/2024 07:01, Ping-Ke Shih wrote: > Bitterblue Smith <rtl8821cerfe2@gmail.com> 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: stable@vger.kernel.org >> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> >> --- >> .../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 >
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)); }
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: stable@vger.kernel.org Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com> --- .../net/wireless/realtek/rtl8xxxu/rtl8xxxu.h | 10 +++++++--- .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 18 +++++++++++------- 2 files changed, 18 insertions(+), 10 deletions(-)