Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 22 Jan 2022 12:22:29 +0100
From:   Lorenzo Bianconi <lorenzo@kernel.org>
To:     greearb@candelatech.com
Cc:     linux-wireless@vger.kernel.org
Subject: Re: [PATCH] mt76:  Ensure sale skb status list is processed.
Message-ID: <Yevo9fhaPeuS+e7C@lore-desk>
References: <20220121195548.17476-1-greearb@candelatech.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="xdtrgTUa6WyzUa/J"
Content-Disposition: inline
In-Reply-To: <20220121195548.17476-1-greearb@candelatech.com>
Precedence: bulk


--xdtrgTUa6WyzUa/J
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

> From: Ben Greear <greearb@candelatech.com>
>=20
> The old code might not ever run the stale skb status processing
> list, so change it to ensure the stale entries are cleaned up
> regularly.

I guess this work is already performed in mt76_tx_status_check() executed by
mac work (e.g. mt7921_mac_work()) where pid is set to 0 and the first lookup
will always fail. Have you identified an issue in the current codebase?

>=20
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  drivers/net/wireless/mediatek/mt76/mt76.h |  1 +
>  drivers/net/wireless/mediatek/mt76/tx.c   | 24 +++++++++++++++++------
>  2 files changed, 19 insertions(+), 6 deletions(-)
>=20
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wire=
less/mediatek/mt76/mt76.h
> index 37d82d806c09..bfb68788251f 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> @@ -271,6 +271,7 @@ struct mt76_wcid {
> =20
>  	struct list_head list;
>  	struct idr pktid;
> +	unsigned long last_idr_check_at; /* in jiffies */
>  };
> =20
>  struct mt76_txq {
> diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wirele=
ss/mediatek/mt76/tx.c
> index 938353ac272f..b6f0d74fd563 100644
> --- a/drivers/net/wireless/mediatek/mt76/tx.c
> +++ b/drivers/net/wireless/mediatek/mt76/tx.c
> @@ -157,24 +157,35 @@ mt76_tx_status_skb_get(struct mt76_dev *dev, struct=
 mt76_wcid *wcid, int pktid,
>  		       struct sk_buff_head *list)
>  {
>  	struct sk_buff *skb;
> +	struct sk_buff *skb2;
>  	int id;
> +	/* Check twice as often as the timeout value so that we mitigate
> +	 * worse-case timeout detection (where we do the check right before
> +	 * the per skb timer would have expired and so have to wait another int=
erval
> +	 * to detect the skb status timeout.)
> +	 */
> +	static const int check_interval =3D MT_TX_STATUS_SKB_TIMEOUT / 2;
> =20
>  	lockdep_assert_held(&dev->status_lock);
> =20
>  	skb =3D idr_remove(&wcid->pktid, pktid);
> -	if (skb)
> +
> +	/* If we have not checked for stale entries recently,
> +	 * then do that check now.
> +	 */
> +	if (time_is_after_jiffies(wcid->last_idr_check_at + check_interval))
>  		goto out;
> =20
>  	/* look for stale entries in the wcid idr queue */
> -	idr_for_each_entry(&wcid->pktid, skb, id) {
> -		struct mt76_tx_cb *cb =3D mt76_tx_skb_cb(skb);
> +	idr_for_each_entry(&wcid->pktid, skb2, id) {
> +		struct mt76_tx_cb *cb =3D mt76_tx_skb_cb(skb2);
> =20
>  		if (pktid >=3D 0) {
>  			if (!(cb->flags & MT_TX_CB_DMA_DONE))
>  				continue;
> =20
>  			if (time_is_after_jiffies(cb->jiffies +
> -						   MT_TX_STATUS_SKB_TIMEOUT))
> +						  MT_TX_STATUS_SKB_TIMEOUT))
>  				continue;
>  		}
> =20
> @@ -182,9 +193,10 @@ mt76_tx_status_skb_get(struct mt76_dev *dev, struct =
mt76_wcid *wcid, int pktid,
>  		 * and stop waiting for TXS callback.
>  		 */
>  		idr_remove(&wcid->pktid, cb->pktid);
> -		__mt76_tx_status_skb_done(dev, skb, MT_TX_CB_TXS_FAILED |
> -						    MT_TX_CB_TXS_DONE, list);

I guess it is more readable as it was before.

Regards,
Lorenzo

> +		__mt76_tx_status_skb_done(dev, skb2, MT_TX_CB_TXS_FAILED |
> +					  MT_TX_CB_TXS_DONE, list);
>  	}
> +	wcid->last_idr_check_at =3D jiffies;
> =20
>  out:
>  	if (idr_is_empty(&wcid->pktid))
> --=20
> 2.20.1
>=20

--xdtrgTUa6WyzUa/J
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCYevo9QAKCRA6cBh0uS2t
rNUnAQDf/7olCCQvApzArT+9vPGdEOZoxdabiKvDgaXXh5XbdAD/YJHlx4nzw2M5
GEdl1DitsnCZrn8W19w+1V9KSjCLNAI=
=Nds4
-----END PGP SIGNATURE-----

--xdtrgTUa6WyzUa/J--