[v1] wifi: rtw88: sdio: Tx status for management frames

[RFC -v1] wifi: rtw88: sdio: Tx status for management frames

Posted by Zhen XIN 10 months, 1 week ago

Rtl8732ds doesn't work in AP-Mode due to the missing tx status for management frames
This patch enables tx status report for all tx skbs

Signed-off-by: Zhen XIN <zhen.xin@nokia-sbell.com>
---
 drivers/net/wireless/realtek/rtw88/sdio.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index e024061bdbf7..84f71e13b5ae 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -1186,7 +1186,7 @@ static int rtw_sdio_request_irq(struct rtw_dev *rtwdev,
 }
 
 static void rtw_sdio_indicate_tx_status(struct rtw_dev *rtwdev,
-					struct sk_buff *skb)
+					struct sk_buff *skb, enum rtw_tx_queue_type queue)
 {
 	struct rtw_sdio_tx_data *tx_data = rtw_sdio_get_tx_data(skb);
 	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
@@ -1195,7 +1195,7 @@ static void rtw_sdio_indicate_tx_status(struct rtw_dev *rtwdev,
 	skb_pull(skb, rtwdev->chip->tx_pkt_desc_sz);
 
 	/* enqueue to wait for tx report */
-	if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) {
+	if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS && queue <= RTW_TX_QUEUE_VO) {
 		rtw_tx_report_enqueue(rtwdev, skb, tx_data->sn);
 		return;
 	}
@@ -1227,10 +1227,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, queue);
 }
 
 static void rtw_sdio_tx_handler(struct work_struct *work)
-- 
2.25.1

RE: [RFC -v1] wifi: rtw88: sdio: Tx status for management frames

Posted by Ping-Ke Shih 10 months, 1 week ago

Hi Martin,

I replied original mail, because I think discussion would be clearer.

Zhen XIN <zhen.xin@nokia-sbell.com> wrote:
> Rtl8732ds doesn't work in AP-Mode due to the missing tx status for management frames
> This patch enables tx status report for all tx skbs
> 
> Signed-off-by: Zhen XIN <zhen.xin@nokia-sbell.com>
> ---
>  drivers/net/wireless/realtek/rtw88/sdio.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index e024061bdbf7..84f71e13b5ae 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -1186,7 +1186,7 @@ static int rtw_sdio_request_irq(struct rtw_dev *rtwdev,
>  }
> 
>  static void rtw_sdio_indicate_tx_status(struct rtw_dev *rtwdev,
> -                                       struct sk_buff *skb)
> +                                       struct sk_buff *skb, enum rtw_tx_queue_type queue)
>  {
>         struct rtw_sdio_tx_data *tx_data = rtw_sdio_get_tx_data(skb);
>         struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> @@ -1195,7 +1195,7 @@ static void rtw_sdio_indicate_tx_status(struct rtw_dev *rtwdev,
>         skb_pull(skb, rtwdev->chip->tx_pkt_desc_sz);
> 
>         /* enqueue to wait for tx report */
> -       if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) {
> +       if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS && queue <= RTW_TX_QUEUE_VO) {

Is this because you have seen "failed to get tx report"?
Have you tried to increasing RTW_TX_PROBE_TIMEOUT?

If it still can't get TX report, we might take this workaround with comments
to mention why we need it. Or a local variable with proper naming to point out
this, like

	bool queue_has_no_tx_report = queue > RTW_TX_QUEUE_VO;


By the way, USB behavior is very like to SDIO, but TX report seems to work well. 

>                 rtw_tx_report_enqueue(rtwdev, skb, tx_data->sn);
>                 return;
>         }
> @@ -1227,10 +1227,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, queue);

I think this change is reasonable, since skb via all kinds of queues could 
set IEEE80211_TX_CTL_REQ_TX_STATUS.

>  }
> 
>  static void rtw_sdio_tx_handler(struct work_struct *work)
> --
> 2.25.1

Re: [RFC -v1] wifi: rtw88: sdio: Tx status for management frames

Posted by Martin Blumenstingl 10 months ago

Hi Ping-Ke,

On Mon, Apr 7, 2025 at 5:30 AM Ping-Ke Shih <pkshih@realtek.com> wrote:
>
> Hi Martin,
>
> I replied original mail, because I think discussion would be clearer.
makes sense, thank you!

[...]
> > @@ -1195,7 +1195,7 @@ static void rtw_sdio_indicate_tx_status(struct rtw_dev *rtwdev,
> >         skb_pull(skb, rtwdev->chip->tx_pkt_desc_sz);
> >
> >         /* enqueue to wait for tx report */
> > -       if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) {
> > +       if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS && queue <= RTW_TX_QUEUE_VO) {
>
> Is this because you have seen "failed to get tx report"?
> Have you tried to increasing RTW_TX_PROBE_TIMEOUT?
>
> If it still can't get TX report, we might take this workaround with comments
> to mention why we need it. Or a local variable with proper naming to point out
> this, like
>
>         bool queue_has_no_tx_report = queue > RTW_TX_QUEUE_VO;
>
>
> By the way, USB behavior is very like to SDIO, but TX report seems to work well.
On my RTL8822CS I can confirm your thought:
I don't notice any extra "failed to get tx report" messages regardless
of whether I have "&& queue <= RTW_TX_QUEUE_VO" or not.


Best regards,
Martin

RE: [RFC -v1] wifi: rtw88: sdio: Tx status for management frames

Posted by Ping-Ke Shih 10 months ago

Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
> 
> Hi Ping-Ke,
> 
> On Mon, Apr 7, 2025 at 5:30 AM Ping-Ke Shih <pkshih@realtek.com> wrote:
> >
> > Hi Martin,
> >
> > I replied original mail, because I think discussion would be clearer.
> makes sense, thank you!
> 
> [...]
> > > @@ -1195,7 +1195,7 @@ static void rtw_sdio_indicate_tx_status(struct rtw_dev *rtwdev,
> > >         skb_pull(skb, rtwdev->chip->tx_pkt_desc_sz);
> > >
> > >         /* enqueue to wait for tx report */
> > > -       if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS) {
> > > +       if (info->flags & IEEE80211_TX_CTL_REQ_TX_STATUS && queue <= RTW_TX_QUEUE_VO) {
> >
> > Is this because you have seen "failed to get tx report"?
> > Have you tried to increasing RTW_TX_PROBE_TIMEOUT?
> >
> > If it still can't get TX report, we might take this workaround with comments
> > to mention why we need it. Or a local variable with proper naming to point out
> > this, like
> >
> >         bool queue_has_no_tx_report = queue > RTW_TX_QUEUE_VO;
> >
> >
> > By the way, USB behavior is very like to SDIO, but TX report seems to work well.
> On my RTL8822CS I can confirm your thought:
> I don't notice any extra "failed to get tx report" messages regardless
> of whether I have "&& queue <= RTW_TX_QUEUE_VO" or not.
> 

This workaround might need an chip attribute to enable then. 
Not sure if people in the GitHub thread have experiments on all
supported SDIO WiFi chips.

Re: [RFC -v1] wifi: rtw88: sdio: Tx status for management frames

Posted by Martin Blumenstingl 10 months, 1 week ago

Hello,

thank you for sending the patch!

On Wed, Apr 2, 2025 at 6:03 PM Zhen XIN <zhen.xin@nokia-sbell.com> wrote:
>
> Rtl8732ds doesn't work in AP-Mode due to the missing tx status for management frames
> This patch enables tx status report for all tx skbs
Ping-Ke, there's a bit more discussion at [0]
Do you have any suggestions regarding the approach used here?


Best regards,
Martin


[0] https://github.com/lwfinger/rtw88/issues/245#issuecomment-2765046681