On 19/04/2023 04:20, Ping-Ke Shih wrote:
>
>
>> -----Original Message-----
>> From: Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx>
>> Sent: Tuesday, April 18, 2023 9:05 PM
>> To: linux-wireless@xxxxxxxxxxxxxxx
>> Cc: Jes Sorensen <Jes.Sorensen@xxxxxxxxx>; Ping-Ke Shih <pkshih@xxxxxxxxxxx>
>> Subject: [PATCH] wifi: rtl8xxxu: Support USB RX aggregation for the newer chips
>>
>> The driver can receive several frames in the same USB transfer.
>> Add the code to handle this in rtl8xxxu_parse_rxdesc24(), even though
>> currently all the relevant chips send only one frame per USB transfer
>> (RTL8723BU, RTL8192EU, RTL8188FU, RTL8710BU).
>>
>> rtl8xxxu_parse_rxdesc16() used by RTL8723AU, RTL8192CU, and RTL8188EU
>> already handles RX aggregation.
>>
>> This was tested with RTL8188FU, RTL8192EU, RTL8710BU, and RTL8192FU.
>>
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@xxxxxxxxx>
>> ---
>> .../wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 117 ++++++++++++------
>> 1 file changed, 76 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> index fd8c8c6d53d6..5fccd898d607 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> @@ -6197,61 +6197,96 @@ int rtl8xxxu_parse_rxdesc16(struct rtl8xxxu_priv *priv, struct sk_buff *skb)
>> int rtl8xxxu_parse_rxdesc24(struct rtl8xxxu_priv *priv, struct sk_buff *skb)
>> {
>> struct ieee80211_hw *hw = priv->hw;
>> - struct ieee80211_rx_status *rx_status = IEEE80211_SKB_RXCB(skb);
>> - struct rtl8xxxu_rxdesc24 *rx_desc =
>> - (struct rtl8xxxu_rxdesc24 *)skb->data;
>> + struct ieee80211_rx_status *rx_status;
>> + struct rtl8xxxu_rxdesc24 *rx_desc;
>> struct rtl8723au_phy_stats *phy_stats;
>> - __le32 *_rx_desc_le = (__le32 *)skb->data;
>> - u32 *_rx_desc = (u32 *)skb->data;
>> + struct sk_buff *next_skb = NULL;
>> + __le32 *_rx_desc_le;
>> + u32 *_rx_desc;
>> int drvinfo_sz, desc_shift;
>> - int i;
>> + int i, pkt_len, urb_len, pkt_offset;
>>
>> - for (i = 0; i < (sizeof(struct rtl8xxxu_rxdesc24) / sizeof(u32)); i++)
>> - _rx_desc[i] = le32_to_cpu(_rx_desc_le[i]);
>> + urb_len = skb->len;
>>
>> - memset(rx_status, 0, sizeof(struct ieee80211_rx_status));
>> + if (urb_len < sizeof(struct rtl8xxxu_rxdesc24)) {
>> + kfree_skb(skb);
>> + return RX_TYPE_ERROR;
>> + }
>>
>> - skb_pull(skb, sizeof(struct rtl8xxxu_rxdesc24));
>> + do {
>> + rx_desc = (struct rtl8xxxu_rxdesc24 *)skb->data;
>> + _rx_desc_le = (__le32 *)skb->data;
>> + _rx_desc = (u32 *)skb->data;
>>
>> - phy_stats = (struct rtl8723au_phy_stats *)skb->data;
>> + for (i = 0; i < (sizeof(struct rtl8xxxu_rxdesc24) / sizeof(u32)); i++)
>> + _rx_desc[i] = le32_to_cpu(_rx_desc_le[i]);
>>
>> - drvinfo_sz = rx_desc->drvinfo_sz * 8;
>> - desc_shift = rx_desc->shift;
>> - skb_pull(skb, drvinfo_sz + desc_shift);
>> + pkt_len = rx_desc->pktlen;
>>
>> - if (rx_desc->rpt_sel) {
>> - struct device *dev = &priv->udev->dev;
>> - dev_dbg(dev, "%s: C2H packet\n", __func__);
>> - rtl8723bu_handle_c2h(priv, skb);
>> - return RX_TYPE_C2H;
>> - }
>> + drvinfo_sz = rx_desc->drvinfo_sz * 8;
>> + desc_shift = rx_desc->shift;
>> + pkt_offset = roundup(pkt_len + drvinfo_sz + desc_shift +
>> + sizeof(struct rtl8xxxu_rxdesc24), 8);
>>
>> - if (rx_desc->phy_stats)
>> - priv->fops->parse_phystats(priv, rx_status, phy_stats,
>> - rx_desc->rxmcs, (struct ieee80211_hdr *)skb->data,
>> - rx_desc->crc32 || rx_desc->icverr);
>> + /*
>> + * Only clone the skb if there's enough data at the end to
>> + * at least cover the rx descriptor
>> + */
>> + if (urb_len >= (pkt_offset + sizeof(struct rtl8xxxu_rxdesc24)))
>
> That means you use 'sizeof(struct rtl8xxxu_rxdesc24)' as clue to know if this
> is the last packet or not, right?
>
> Do you think if it is needed to handle malformed packet for the last skb if
> its length is shorter than pkt_offset or pkt_len?
>
I'm not sure. I just copied what rtl8xxxu_parse_rxdesc16() was doing.
>> + next_skb = skb_clone(skb, GFP_ATOMIC);
>>
>> - rx_status->mactime = rx_desc->tsfl;
>> - rx_status->flag |= RX_FLAG_MACTIME_START;
>> + rx_status = IEEE80211_SKB_RXCB(skb);
>> + memset(rx_status, 0, sizeof(struct ieee80211_rx_status));
>>
>> - if (!rx_desc->swdec)
>> - rx_status->flag |= RX_FLAG_DECRYPTED;
>> - if (rx_desc->crc32)
>> - rx_status->flag |= RX_FLAG_FAILED_FCS_CRC;
>> - if (rx_desc->bw)
>> - rx_status->bw = RATE_INFO_BW_40;
>> + skb_pull(skb, sizeof(struct rtl8xxxu_rxdesc24));
>>
>> - if (rx_desc->rxmcs >= DESC_RATE_MCS0) {
>> - rx_status->encoding = RX_ENC_HT;
>> - rx_status->rate_idx = rx_desc->rxmcs - DESC_RATE_MCS0;
>> - } else {
>> - rx_status->rate_idx = rx_desc->rxmcs;
>> - }
>> + phy_stats = (struct rtl8723au_phy_stats *)skb->data;
>> +
>> + skb_pull(skb, drvinfo_sz + desc_shift);
>> +
>> + skb_trim(skb, pkt_len);
>> +
>> + if (rx_desc->rpt_sel) {
>> + struct device *dev = &priv->udev->dev;
>> + dev_dbg(dev, "%s: C2H packet\n", __func__);
>> + rtl8723bu_handle_c2h(priv, skb);
>> + } else {
>> + if (rx_desc->phy_stats)
>> + priv->fops->parse_phystats(priv, rx_status, phy_stats,
>> + rx_desc->rxmcs, (struct ieee80211_hdr
>> *)skb->data,
>> + rx_desc->crc32 || rx_desc->icverr);
>
> This statement is too long. Add a helper 'struct ieee80211_hdr *hdr', like
>
> if (rx_desc->phy_stats) {
> struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
>
> priv->fops->parse_phystats(priv, rx_status, phy_stats,
> rx_desc->rxmcs, hdr,
> rx_desc->crc32 || rx_desc->icverr);
> }
>
Okay.
>
>> +
>> + rx_status->mactime = rx_desc->tsfl;
>> + rx_status->flag |= RX_FLAG_MACTIME_START;
>> +
>> + if (!rx_desc->swdec)
>> + rx_status->flag |= RX_FLAG_DECRYPTED;
>> + if (rx_desc->crc32)
>> + rx_status->flag |= RX_FLAG_FAILED_FCS_CRC;
>> + if (rx_desc->bw)
>> + rx_status->bw = RATE_INFO_BW_40;
>>
>> - rx_status->freq = hw->conf.chandef.chan->center_freq;
>> - rx_status->band = hw->conf.chandef.chan->band;
>> + if (rx_desc->rxmcs >= DESC_RATE_MCS0) {
>> + rx_status->encoding = RX_ENC_HT;
>> + rx_status->rate_idx = rx_desc->rxmcs - DESC_RATE_MCS0;
>> + } else {
>> + rx_status->rate_idx = rx_desc->rxmcs;
>> + }
>> +
>> + rx_status->freq = hw->conf.chandef.chan->center_freq;
>> + rx_status->band = hw->conf.chandef.chan->band;
>> +
>> + ieee80211_rx_irqsafe(hw, skb);
>> + }
>> +
>> + skb = next_skb;
>> + if (skb)
>> + skb_pull(next_skb, pkt_offset);
>> +
>> + urb_len -= pkt_offset;
>> + next_skb = NULL;
>> + } while (skb && urb_len >= sizeof(struct rtl8xxxu_rxdesc24));
>>
>> - ieee80211_rx_irqsafe(hw, skb);
>> return RX_TYPE_DATA_PKT;
>> }
>>
>> --
>> 2.39.2
>>
>> ------Please consider the environment before printing this e-mail.
