[PATCH] mt76: remove empty flag in mt76_txq_schedule_list

[PATCH] mt76: remove empty flag in mt76_txq_schedule_list

[Lorenzo Bianconi]

Remove empty flag in mt76_txq_schedule_list and mt76_txq_send_burst
since we just need retry_q length to notify mac80211 to reschedule the
current tx queue

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 drivers/net/wireless/mediatek/mt76/tx.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
index d7982aa83c11..51d69329ed06 100644
--- a/drivers/net/wireless/mediatek/mt76/tx.c
+++ b/drivers/net/wireless/mediatek/mt76/tx.c
@@ -378,7 +378,7 @@ EXPORT_SYMBOL_GPL(mt76_release_buffered_frames);
 
 static int
 mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
-		    struct mt76_txq *mtxq, bool *empty)
+		    struct mt76_txq *mtxq)
 {
 	struct ieee80211_txq *txq = mtxq_to_txq(mtxq);
 	enum mt76_txq_id qid = mt76_txq_get_qid(txq);
@@ -392,16 +392,12 @@ mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
 	bool probe;
 	int idx;
 
-	if (test_bit(MT_WCID_FLAG_PS, &wcid->flags)) {
-		*empty = true;
+	if (test_bit(MT_WCID_FLAG_PS, &wcid->flags))
 		return 0;
-	}
 
 	skb = mt76_txq_dequeue(dev, mtxq, false);
-	if (!skb) {
-		*empty = true;
+	if (!skb)
 		return 0;
-	}
 
 	info = IEEE80211_SKB_CB(skb);
 	if (!(wcid->tx_info & MT_WCID_TX_INFO_SET))
@@ -432,10 +428,8 @@ mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
 			return -EBUSY;
 
 		skb = mt76_txq_dequeue(dev, mtxq, false);
-		if (!skb) {
-			*empty = true;
+		if (!skb)
 			break;
-		}
 
 		info = IEEE80211_SKB_CB(skb);
 		cur_ampdu = info->flags & IEEE80211_TX_CTL_AMPDU;
@@ -482,8 +476,6 @@ mt76_txq_schedule_list(struct mt76_dev *dev, enum mt76_txq_id qid)
 
 	spin_lock_bh(&hwq->lock);
 	while (1) {
-		bool empty = false;
-
 		if (sq->swq_queued >= 4)
 			break;
 
@@ -515,10 +507,9 @@ mt76_txq_schedule_list(struct mt76_dev *dev, enum mt76_txq_id qid)
 			spin_lock_bh(&hwq->lock);
 		}
 
-		ret += mt76_txq_send_burst(dev, sq, mtxq, &empty);
-		if (skb_queue_empty(&mtxq->retry_q))
-			empty = true;
-		ieee80211_return_txq(dev->hw, txq, !empty);
+		ret += mt76_txq_send_burst(dev, sq, mtxq);
+		ieee80211_return_txq(dev->hw, txq,
+				     !skb_queue_empty(&mtxq->retry_q));
 	}
 	spin_unlock_bh(&hwq->lock);
 
-- 
2.21.0

Re: [PATCH] mt76: remove empty flag in mt76_txq_schedule_list

[Stanislaw Gruszka]

On Thu, Aug 22, 2019 at 11:49:10AM +0200, Lorenzo Bianconi wrote:
> Remove empty flag in mt76_txq_schedule_list and mt76_txq_send_burst
> since we just need retry_q length to notify mac80211 to reschedule the
> current tx queue
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/net/wireless/mediatek/mt76/tx.c | 23 +++++++----------------
>  1 file changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
> index d7982aa83c11..51d69329ed06 100644
> --- a/drivers/net/wireless/mediatek/mt76/tx.c
> +++ b/drivers/net/wireless/mediatek/mt76/tx.c
> @@ -378,7 +378,7 @@ EXPORT_SYMBOL_GPL(mt76_release_buffered_frames);
>  
>  static int
>  mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
> -		    struct mt76_txq *mtxq, bool *empty)
> +		    struct mt76_txq *mtxq)
>  {
>  	struct ieee80211_txq *txq = mtxq_to_txq(mtxq);
>  	enum mt76_txq_id qid = mt76_txq_get_qid(txq);
> @@ -392,16 +392,12 @@ mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
>  	bool probe;
>  	int idx;
>  
> -	if (test_bit(MT_WCID_FLAG_PS, &wcid->flags)) {
> -		*empty = true;
> +	if (test_bit(MT_WCID_FLAG_PS, &wcid->flags))
>  		return 0;

This changes behaviour for station in PS state. If retry_q is not
empty, now we will be rescheduling tx queue for STA in PS mode.
Not sure if this is problem or not, though.

Stanislaw

Re: [PATCH] mt76: remove empty flag in mt76_txq_schedule_list

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

> On Thu, Aug 22, 2019 at 11:49:10AM +0200, Lorenzo Bianconi wrote:
> > Remove empty flag in mt76_txq_schedule_list and mt76_txq_send_burst
> > since we just need retry_q length to notify mac80211 to reschedule the
> > current tx queue
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  drivers/net/wireless/mediatek/mt76/tx.c | 23 +++++++----------------
> >  1 file changed, 7 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
> > index d7982aa83c11..51d69329ed06 100644
> > --- a/drivers/net/wireless/mediatek/mt76/tx.c
> > +++ b/drivers/net/wireless/mediatek/mt76/tx.c
> > @@ -378,7 +378,7 @@ EXPORT_SYMBOL_GPL(mt76_release_buffered_frames);
> >  
> >  static int
> >  mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
> > -		    struct mt76_txq *mtxq, bool *empty)
> > +		    struct mt76_txq *mtxq)
> >  {
> >  	struct ieee80211_txq *txq = mtxq_to_txq(mtxq);
> >  	enum mt76_txq_id qid = mt76_txq_get_qid(txq);
> > @@ -392,16 +392,12 @@ mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
> >  	bool probe;
> >  	int idx;
> >  
> > -	if (test_bit(MT_WCID_FLAG_PS, &wcid->flags)) {
> > -		*empty = true;
> > +	if (test_bit(MT_WCID_FLAG_PS, &wcid->flags))
> >  		return 0;
> 
> This changes behaviour for station in PS state. If retry_q is not
> empty, now we will be rescheduling tx queue for STA in PS mode.
> Not sure if this is problem or not, though.

good point..looking at the code it seems not a issue since we will not actually
tx frames for PS stations. What do you think?

Regards,
Lorenzo

> 
> Stanislaw

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] mt76: remove empty flag in mt76_txq_schedule_list

[Stanislaw Gruszka]

On Thu, Aug 22, 2019 at 01:50:52PM +0200, Lorenzo Bianconi wrote:
> > On Thu, Aug 22, 2019 at 11:49:10AM +0200, Lorenzo Bianconi wrote:
> > > Remove empty flag in mt76_txq_schedule_list and mt76_txq_send_burst
> > > since we just need retry_q length to notify mac80211 to reschedule the
> > > current tx queue
> > > 
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > >  drivers/net/wireless/mediatek/mt76/tx.c | 23 +++++++----------------
> > >  1 file changed, 7 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
> > > index d7982aa83c11..51d69329ed06 100644
> > > --- a/drivers/net/wireless/mediatek/mt76/tx.c
> > > +++ b/drivers/net/wireless/mediatek/mt76/tx.c
> > > @@ -378,7 +378,7 @@ EXPORT_SYMBOL_GPL(mt76_release_buffered_frames);
> > >  
> > >  static int
> > >  mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
> > > -		    struct mt76_txq *mtxq, bool *empty)
> > > +		    struct mt76_txq *mtxq)
> > >  {
> > >  	struct ieee80211_txq *txq = mtxq_to_txq(mtxq);
> > >  	enum mt76_txq_id qid = mt76_txq_get_qid(txq);
> > > @@ -392,16 +392,12 @@ mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
> > >  	bool probe;
> > >  	int idx;
> > >  
> > > -	if (test_bit(MT_WCID_FLAG_PS, &wcid->flags)) {
> > > -		*empty = true;
> > > +	if (test_bit(MT_WCID_FLAG_PS, &wcid->flags))
> > >  		return 0;
> > 
> > This changes behaviour for station in PS state. If retry_q is not
> > empty, now we will be rescheduling tx queue for STA in PS mode.
> > Not sure if this is problem or not, though.
> 
> good point..looking at the code it seems not a issue since we will not actually
> tx frames for PS stations. What do you think?

I do not see how changing this could possibly break things, but it 
was explicitly added by below commit, with changelog sugesting it is
needed:

commit d225581df3147060bc99e931b11f7cf2dcb1b2ca
Author: Felix Fietkau <nbd@nbd.name>
Date:   Mon Jan 21 17:33:38 2019 +0100

    mt76: avoid scheduling tx queues for powersave stations
    
    In case a tx queue wake call arrives after a client has transitioned to
    powersave, make sure that the queue is not kept active until the client
    has left powersave mode

Stanislaw

Re: [PATCH] mt76: remove empty flag in mt76_txq_schedule_list

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2673 bytes --]

> On Thu, Aug 22, 2019 at 01:50:52PM +0200, Lorenzo Bianconi wrote:
> > > On Thu, Aug 22, 2019 at 11:49:10AM +0200, Lorenzo Bianconi wrote:
> > > > Remove empty flag in mt76_txq_schedule_list and mt76_txq_send_burst
> > > > since we just need retry_q length to notify mac80211 to reschedule the
> > > > current tx queue
> > > > 
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > >  drivers/net/wireless/mediatek/mt76/tx.c | 23 +++++++----------------
> > > >  1 file changed, 7 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
> > > > index d7982aa83c11..51d69329ed06 100644
> > > > --- a/drivers/net/wireless/mediatek/mt76/tx.c
> > > > +++ b/drivers/net/wireless/mediatek/mt76/tx.c
> > > > @@ -378,7 +378,7 @@ EXPORT_SYMBOL_GPL(mt76_release_buffered_frames);
> > > >  
> > > >  static int
> > > >  mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
> > > > -		    struct mt76_txq *mtxq, bool *empty)
> > > > +		    struct mt76_txq *mtxq)
> > > >  {
> > > >  	struct ieee80211_txq *txq = mtxq_to_txq(mtxq);
> > > >  	enum mt76_txq_id qid = mt76_txq_get_qid(txq);
> > > > @@ -392,16 +392,12 @@ mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
> > > >  	bool probe;
> > > >  	int idx;
> > > >  
> > > > -	if (test_bit(MT_WCID_FLAG_PS, &wcid->flags)) {
> > > > -		*empty = true;
> > > > +	if (test_bit(MT_WCID_FLAG_PS, &wcid->flags))
> > > >  		return 0;
> > > 
> > > This changes behaviour for station in PS state. If retry_q is not
> > > empty, now we will be rescheduling tx queue for STA in PS mode.
> > > Not sure if this is problem or not, though.
> > 
> > good point..looking at the code it seems not a issue since we will not actually
> > tx frames for PS stations. What do you think?
> 
> I do not see how changing this could possibly break things, but it 
> was explicitly added by below commit, with changelog sugesting it is
> needed:
> 
> commit d225581df3147060bc99e931b11f7cf2dcb1b2ca
> Author: Felix Fietkau <nbd@nbd.name>
> Date:   Mon Jan 21 17:33:38 2019 +0100
> 
>     mt76: avoid scheduling tx queues for powersave stations
>     
>     In case a tx queue wake call arrives after a client has transitioned to
>     powersave, make sure that the queue is not kept active until the client
>     has left powersave mode
> 
> Stanislaw

This is exactly what I am saying, we need the 'if block' in mt76_txq_send_burst
in order to avoid scheduling tx queue for station in powersave but the empty
flag seems not necessary.

Regards,
Lorenzo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]