Rtw88-sdio is missing the tx status report for management frames

Fix this by mapping mgmt frames to queue TX_DESC_QSEL_MGMT

Signed-off-by: Zhen XIN <zhen.xin@nokia-sbell.com>
---
v2: have the right queue for mgmt frames as pointed out by Bitterblue Smith
---
 drivers/net/wireless/realtek/rtw88/sdio.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index e024061bdbf7..4311eb7cffef 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -718,10 +718,7 @@ static u8 rtw_sdio_get_tx_qsel(struct rtw_dev *rtwdev, struct sk_buff *skb,
 	case RTW_TX_QUEUE_H2C:
 		return TX_DESC_QSEL_H2C;
 	case RTW_TX_QUEUE_MGMT:
-		if (rtw_chip_wcpu_11n(rtwdev))
-			return TX_DESC_QSEL_HIGH;
-		else
-			return TX_DESC_QSEL_MGMT;
+		return TX_DESC_QSEL_MGMT;
 	case RTW_TX_QUEUE_HI0:
 		return TX_DESC_QSEL_HIGH;
 	default:
@@ -1227,10 +1224,7 @@ static void rtw_sdio_process_tx_queue(struct rtw_dev *rtwdev,
 		return;
 	}
 
-	if (queue <= RTW_TX_QUEUE_VO)
-		rtw_sdio_indicate_tx_status(rtwdev, skb);
-	else
-		dev_kfree_skb_any(skb);
+	rtw_sdio_indicate_tx_status(rtwdev, skb);
 }
 
 static void rtw_sdio_tx_handler(struct work_struct *work)
-- 
2.25.1

Hi Martin,

Zhen XIN <zhen.xin@nokia-sbell.com> wrote:
> Rtw88-sdio is missing the tx status report for management frames
> 
> Fix this by mapping mgmt frames to queue TX_DESC_QSEL_MGMT
> 
> Signed-off-by: Zhen XIN <zhen.xin@nokia-sbell.com>
> ---
> v2: have the right queue for mgmt frames as pointed out by Bitterblue Smith
> ---
>  drivers/net/wireless/realtek/rtw88/sdio.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index e024061bdbf7..4311eb7cffef 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -718,10 +718,7 @@ static u8 rtw_sdio_get_tx_qsel(struct rtw_dev *rtwdev, struct sk_buff *skb,
>         case RTW_TX_QUEUE_H2C:
>                 return TX_DESC_QSEL_H2C;
>         case RTW_TX_QUEUE_MGMT:
> -               if (rtw_chip_wcpu_11n(rtwdev))
> -                       return TX_DESC_QSEL_HIGH;
> -               else
> -                       return TX_DESC_QSEL_MGMT;
> +               return TX_DESC_QSEL_MGMT;

Do you remember why you did the special deal with 11n chips? 
And this RFC looks good to me. (except to commit message, but this is RFC)

>         case RTW_TX_QUEUE_HI0:
>                 return TX_DESC_QSEL_HIGH;
>         default:
> @@ -1227,10 +1224,7 @@ static void rtw_sdio_process_tx_queue(struct rtw_dev *rtwdev,
>                 return;
>         }
> 
> -       if (queue <= RTW_TX_QUEUE_VO)
> -               rtw_sdio_indicate_tx_status(rtwdev, skb);
> -       else
> -               dev_kfree_skb_any(skb);
> +       rtw_sdio_indicate_tx_status(rtwdev, skb);
>  }
> 
>  static void rtw_sdio_tx_handler(struct work_struct *work)
> --
> 2.25.1

Hi Ping-Ke,

On Thu, Apr 10, 2025 at 6:30 AM Ping-Ke Shih <pkshih@realtek.com> wrote:
[...]
> > @@ -718,10 +718,7 @@ static u8 rtw_sdio_get_tx_qsel(struct rtw_dev *rtwdev, struct sk_buff *skb,
> >         case RTW_TX_QUEUE_H2C:
> >                 return TX_DESC_QSEL_H2C;
> >         case RTW_TX_QUEUE_MGMT:
> > -               if (rtw_chip_wcpu_11n(rtwdev))
> > -                       return TX_DESC_QSEL_HIGH;
> > -               else
> > -                       return TX_DESC_QSEL_MGMT;
> > +               return TX_DESC_QSEL_MGMT;
>
> Do you remember why you did the special deal with 11n chips?
> And this RFC looks good to me. (except to commit message, but this is RFC)
I don't remember - and Jernej said the same thing.
However, since we got the first 802.11n hardware for testing long
after this part was written my suggestion is: let's roll this into a
proper patch, Cc Fiona Klute <fiona.klute@gmx.de> (author of RTL8723CS
support) on the resulting patch(es) and then apply the patches
(assuming nobody observes any problems).

To make this a non-RFC patch the following steps are needed (in my opinion):
- split the change into two patches (one which unconditionally calls
rtw_sdio_indicate_tx_status())
- another one for the TX_DESC_QSEL_MGMT mapping
- each of the patches should include their own description
- I checked the history and it seems that both problems were
introduced with the original commit, meaning both patches should get
the following line (above the Signed-off-by): Fixes: 65371a3f14e7
("wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets")
- (plus anything Ping-Ke has to add :-) )


Best regards,
Martin

Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
> Hi Ping-Ke,
> 
> On Thu, Apr 10, 2025 at 6:30 AM Ping-Ke Shih <pkshih@realtek.com> wrote:
> [...]
> > > @@ -718,10 +718,7 @@ static u8 rtw_sdio_get_tx_qsel(struct rtw_dev *rtwdev, struct sk_buff *skb,
> > >         case RTW_TX_QUEUE_H2C:
> > >                 return TX_DESC_QSEL_H2C;
> > >         case RTW_TX_QUEUE_MGMT:
> > > -               if (rtw_chip_wcpu_11n(rtwdev))
> > > -                       return TX_DESC_QSEL_HIGH;
> > > -               else
> > > -                       return TX_DESC_QSEL_MGMT;
> > > +               return TX_DESC_QSEL_MGMT;
> >
> > Do you remember why you did the special deal with 11n chips?
> > And this RFC looks good to me. (except to commit message, but this is RFC)
> I don't remember - and Jernej said the same thing.
> However, since we got the first 802.11n hardware for testing long
> after this part was written my suggestion is: let's roll this into a
> proper patch, Cc Fiona Klute <fiona.klute@gmx.de> (author of RTL8723CS
> support) on the resulting patch(es) and then apply the patches
> (assuming nobody observes any problems).
> 
> To make this a non-RFC patch the following steps are needed (in my opinion):
> - split the change into two patches (one which unconditionally calls
> rtw_sdio_indicate_tx_status())
> - another one for the TX_DESC_QSEL_MGMT mapping
> - each of the patches should include their own description
> - I checked the history and it seems that both problems were
> introduced with the original commit, meaning both patches should get
> the following line (above the Signed-off-by): Fixes: 65371a3f14e7
> ("wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets")
> - (plus anything Ping-Ke has to add :-) )

That's super clear. No other opinion from me.  :-)