From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1D67FCD5BD1 for ; Mon, 1 Jun 2026 05:58:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=N8ZmOAtQgvXuoLmKd6WCIumh3x0pvVxkYVcX8hy5mGg=; b=pwDyYcM/V5T6gXHhlMnazGWb2K NSFCdYpT3gQDQi9HAXjZaHv22TS1JF/ipEy2kUxL98cNx88K6ZWu/kJejDNPPPR+OHIkfTKkodqK3 GRrBWUnKUfsCCNums698HI1iO8P0mWh5TpnNWWrRvcxLoFhVVC1YbSwRSTZBe9z2GAvyEgU/7UnrE t8L8/TpBVcgD6/qgljFook2knfNwsczNB3vu1fpD63HP3iv1MfuI6p0L0AMxunz5/ElsFl4vpZTC+ UN/ATpZZYdZ+6cFhbhsK9HYUMbrIFsF4Kq5JzVwPvIqwZMdBcUxzqWbudUt7D7AK2PwM8XdqzVYLK /VI633Ng==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1wTvff-0000000AAmw-2jfg; Mon, 01 Jun 2026 05:58:43 +0000 Received: from sea.source.kernel.org ([2600:3c0a:e001:78e:0:1991:8:25]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1wTvfd-0000000AAmO-0RJC; Mon, 01 Jun 2026 05:58:42 +0000 Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) by sea.source.kernel.org (Postfix) with ESMTP id 33670415FE; Mon, 1 Jun 2026 05:58:40 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1DD361F00893; Mon, 1 Jun 2026 05:58:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1780293520; bh=N8ZmOAtQgvXuoLmKd6WCIumh3x0pvVxkYVcX8hy5mGg=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=TX5kBpWogNBnq7ueXJBbRDG8RSiNQDJYQsZsVJwAw+Sx0ec5M2tfUdiR51Vqqw5jn JnUO4Dg67KKMRNHS1GnWVVqS1ptyGmtdJrtaDl3tjLIPHwb64pDhJnIuAJU55/IKzn 7aCIwjaoKQHla2d/N/C6yV1jVXWzLSFb8XsIqnWT1yqoKZVVCEUMJCc19zFhmYzdon snynCKO/dAgGkM4PsZ98pWd54qURST2GDjaUkOkoCMhZFp0FBnlKwHyUdoZ++e5xQ4 Nf9kX6MDL3sv5m36Hh8D2PrH3uN32dpUQm+5yaaqrwRH5yszxkXtvl8FjHGXAoxwmp ZTQeKagSpkzOA== Date: Mon, 1 Jun 2026 07:56:41 +0200 From: "lorenzo@kernel.org" To: Ryder Lee Cc: Shayne Chen =?utf-8?B?KOmZs+i7kuS4nik=?= , "nbd@nbd.name" , Roy-CH Luo , Chui-hao Chiu =?utf-8?B?KOmCseWegua1qSk=?= , AngeloGioacchino Del Regno , "linux-kernel@vger.kernel.org" , "linux-wireless@vger.kernel.org" , Sean Wang , Bo Jiao =?utf-8?B?KOeEpuazoik=?= , "linux-mediatek@lists.infradead.org" , "matthias.bgg@gmail.com" , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH v2] wifi: mt76: mt7996: fix reading zeroed info->control.flags after mt76_tx_status_skb_add() Message-ID: References: <20260531-mt76_tx_status_skb_add-overwrite-fix-v2-1-b73c4b4a9798@kernel.org> <44c54ed4da0d294c567b3b0ad750f082a6f1be9f.camel@mediatek.com> <7f02be7c4f919413718a0218b3792d4b0a222ca3.camel@mediatek.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="wzjsFj6KDcPkGXlo" Content-Disposition: inline In-Reply-To: <7f02be7c4f919413718a0218b3792d4b0a222ca3.camel@mediatek.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.9.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20260531_225841_192973_2F17B809 X-CRM114-Status: GOOD ( 49.71 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --wzjsFj6KDcPkGXlo Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On May 31, Ryder Lee wrote: > On Sun, 2026-05-31 at 15:12 +0200, lorenzo@kernel.org wrote: > > On May 31, Ryder Lee wrote: > > > On Sun, 2026-05-31 at 14:11 +0200, lorenzo@kernel.org=A0wrote: > > > > > On Sun, 2026-05-31 at 10:55 +0200, Lorenzo Bianconi wrote: > > > > > > mt76_tx_status_skb_add() zeroes the mt76_tx_cb struct stored > > > > > > at > > > > > > info->status.status_driver_data via memset(). Since info- > > > > > > >control > > > > > > and > > > > > > info->status are members of the same union in > > > > > > ieee80211_tx_info, > > > > > > this overwrites info->control.flags. > > > > > > In mt7996_tx_prepare_skb(), mt76_tx_status_skb_add() is > > > > > > called > > > > > > before > > > > > > mt7996_mac_write_txwi(), which re-reads info->control.flags > > > > > > to > > > > > > extract > > > > > > IEEE80211_TX_CTRL_MLO_LINK. Because the field has been > > > > > > zeroed, > > > > > > the > > > > > > link_id always resolves to 0 for frames using global_wcid, > > > > > > leading to > > > > > > incorrect TXWI configuration. > > > > > > Fix this by passing link_id as an explicit parameter to > > > > > > mt7996_mac_write_txwi(). In mt7996_tx_prepare_skb(), the > > > > > > link_id > > > > > > is > > > > > > already extracted from info->control.flags before the > > > > > > destructive > > > > > > mt76_tx_status_skb_add() call. For the beacon and inband > > > > > > discovery > > > > > > callers in mcu.c, use link_conf->link_id directly. > > > > > >=20 > > > > > > Fixes: f0b0b239b8f36 ("wifi: mt76: mt7996: rework > > > > > > mt7996_mac_write_txwi() for MLO support") > > > > > > Signed-off-by: Lorenzo Bianconi > > > > > > --- > > > > > > Changes in v2: > > > > > > - Do not use link_id in mt7996_mac_write_txwi if it is > > > > > > IEEE80211_LINK_UNSPECIFIED > > > > > > - In mt7996_mac_write_txwi() rely on link_id calculated in > > > > > > =A0 mt7996_tx_prepare_skb(). > > > > > > - Link to v1: > > > > > > https://lore.kernel.org/r/20260530-mt76_tx_status_skb_add-overw= rite-fix-v1-1-e2c3151c391a@kernel.org > > > > > > =A0 > > > > > > --- > > > > > > =A0drivers/net/wireless/mediatek/mt76/mt7996/mac.c=A0=A0=A0 | 14 > > > > > > ++++---- > > > > > > ---- > > > > > > -- > > > > > > =A0drivers/net/wireless/mediatek/mt76/mt7996/mcu.c=A0=A0=A0 |= =A0 5 +++- > > > > > > - > > > > > > =A0drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h |=A0 3 ++- > > > > > > =A03 files changed, 9 insertions(+), 13 deletions(-) > > > > > >=20 > > > > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c > > > > > > b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c > > > > > > index c98446057282..95b3078d9667 100644 > > > > > > --- a/drivers/net/wireless/mediatek/mt76/mt7996/mac.c > > > > > > +++ b/drivers/net/wireless/mediatek/mt76/mt7996/mac.c > > > > > > @@ -856,7 +856,8 @@ mt7996_mac_write_txwi_80211(struct > > > > > > mt7996_dev > > > > > > *dev, __le32 *txwi, > > > > > > =A0void mt7996_mac_write_txwi(struct mt7996_dev *dev, __le32 > > > > > > *txwi, > > > > > > =A0 =A0=A0 struct sk_buff *skb, struct > > > > > > mt76_wcid > > > > > > *wcid, > > > > > > =A0 =A0=A0 struct ieee80211_key_conf *key, > > > > > > int > > > > > > pid, > > > > > > - =A0=A0 enum mt76_txq_id qid, u32 > > > > > > changed) > > > > > > + =A0=A0 enum mt76_txq_id qid, u32 > > > > > > changed, > > > > > > + =A0=A0 unsigned int link_id) > > > > > > =A0{ > > > > > > =A0 struct ieee80211_hdr *hdr =3D (struct ieee80211_hdr > > > > > > *)skb- > > > > > > > data; > > > > > > =A0 struct ieee80211_tx_info *info =3D > > > > > > IEEE80211_SKB_CB(skb); > > > > > > @@ -866,7 +867,6 @@ void mt7996_mac_write_txwi(struct > > > > > > mt7996_dev > > > > > > *dev, __le32 *txwi, > > > > > > =A0 bool is_8023 =3D info->flags & > > > > > > IEEE80211_TX_CTL_HW_80211_ENCAP; > > > > > > =A0 struct mt76_vif_link *mlink =3D NULL; > > > > > > =A0 struct mt7996_vif *mvif; > > > > > > - unsigned int link_id; > > > > > > =A0 u16 tx_count =3D 15; > > > > > > =A0 u32 val; > > > > > > =A0 bool inband_disc =3D !!(changed & > > > > > > (BSS_CHANGED_UNSOL_BCAST_PROBE_RESP | > > > > > > @@ -874,17 +874,11 @@ void mt7996_mac_write_txwi(struct > > > > > > mt7996_dev > > > > > > *dev, __le32 *txwi, > > > > > > =A0 bool beacon =3D !!(changed & (BSS_CHANGED_BEACON | > > > > > > =A0 =A0=A0=A0 > > > > > > BSS_CHANGED_BEACON_ENABLED)) > > > > > > && > > > > > > (!inband_disc); > > > > > > =A0 > > > > > > - if (wcid !=3D &dev->mt76.global_wcid) > > > > > > - link_id =3D wcid->link_id; > > > > > > - else > > > > > > - link_id =3D u32_get_bits(info->control.flags, > > > > > > - =A0=A0=A0=A0=A0=A0 > > > > > > IEEE80211_TX_CTRL_MLO_LINK); > > > > > > - > > > > > > =A0 mvif =3D vif ? (struct mt7996_vif *)vif->drv_priv : > > > > > > NULL; > > > > > > =A0 if (mvif) { > > > > > > =A0 if (wcid->offchannel) > > > > > > =A0 mlink =3D rcu_dereference(mvif- > > > > > > > mt76.offchannel_link); > > > > > > - if (!mlink) > > > > > > + if (!mlink && link_id !=3D > > > > > > IEEE80211_LINK_UNSPECIFIED) > > > > > > =A0 mlink =3D rcu_dereference(mvif- > > > > > > > mt76.link[link_id]); > > > > > > =A0 } > > > > > > =A0 > > > > > > @@ -1096,7 +1090,7 @@ int mt7996_tx_prepare_skb(struct > > > > > > mt76_dev > > > > > > *mdev, void *txwi_ptr, > > > > > > =A0 /* Transmit non qos data by 802.11 header and need > > > > > > to > > > > > > fill > > > > > > txd by host*/ > > > > > > =A0 if (!is_8023 || pid >=3D MT_PACKET_ID_FIRST) > > > > > > =A0 mt7996_mac_write_txwi(dev, txwi_ptr, > > > > > > tx_info- > > > > > > > skb, > > > > > > wcid, key, > > > > > > - =A0=A0=A0=A0=A0 pid, qid, 0); > > > > > > + =A0=A0=A0=A0=A0 pid, qid, 0, link_id); > > > > > > =A0 > > > > > > =A0 /* MT7996 and MT7992 require driver to provide the > > > > > > MAC > > > > > > TXP > > > > > > for AddBA > > > > > > =A0 * req > > > > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/mcu.c > > > > > > b/drivers/net/wireless/mediatek/mt76/mt7996/mcu.c > > > > > > index 8be40d60ad29..a14c63438923 100644 > > > > > > --- a/drivers/net/wireless/mediatek/mt76/mt7996/mcu.c > > > > > > +++ b/drivers/net/wireless/mediatek/mt76/mt7996/mcu.c > > > > > > @@ -3103,7 +3103,7 @@ mt7996_mcu_beacon_cont(struct > > > > > > mt7996_dev > > > > > > *dev, > > > > > > =A0 > > > > > > =A0 buf =3D (u8 *)bcn + sizeof(*bcn); > > > > > > =A0 mt7996_mac_write_txwi(dev, (__le32 *)buf, skb, wcid, > > > > > > NULL, > > > > > > 0, 0, > > > > > > - =A0=A0=A0=A0=A0 BSS_CHANGED_BEACON); > > > > > > + =A0=A0=A0=A0=A0 BSS_CHANGED_BEACON, link_conf- > > > > > > > link_id); > > > > > > =A0 > > > > > > =A0 memcpy(buf + MT_TXD_SIZE, skb->data, skb->len); > > > > > > =A0} > > > > > > @@ -3249,7 +3249,8 @@ int > > > > > > mt7996_mcu_beacon_inband_discov(struct > > > > > > mt7996_dev *dev, > > > > > > =A0 > > > > > > =A0 buf =3D (u8 *)tlv + sizeof(*discov); > > > > > > =A0 > > > > > > - mt7996_mac_write_txwi(dev, (__le32 *)buf, skb, wcid, > > > > > > NULL, > > > > > > 0, 0, changed); > > > > > > + mt7996_mac_write_txwi(dev, (__le32 *)buf, skb, wcid, > > > > > > NULL, > > > > > > 0, 0, > > > > > > + =A0=A0=A0=A0=A0 changed, link_conf->link_id); > > > > > > =A0 > > > > > > =A0 memcpy(buf + MT_TXD_SIZE, skb->data, skb->len); > > > > > > =A0 > > > > > > diff --git > > > > > > a/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h > > > > > > b/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h > > > > > > index 0dc4198fcf8b..0d6488522ba7 100644 > > > > > > --- a/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h > > > > > > +++ b/drivers/net/wireless/mediatek/mt76/mt7996/mt7996.h > > > > > > @@ -874,7 +874,8 @@ void mt7996_mac_enable_nf(struct > > > > > > mt7996_dev > > > > > > *dev, > > > > > > u8 band); > > > > > > =A0void mt7996_mac_write_txwi(struct mt7996_dev *dev, __le32 > > > > > > *txwi, > > > > > > =A0 =A0=A0 struct sk_buff *skb, struct > > > > > > mt76_wcid > > > > > > *wcid, > > > > > > =A0 =A0=A0 struct ieee80211_key_conf *key, > > > > > > int > > > > > > pid, > > > > > > - =A0=A0 enum mt76_txq_id qid, u32 > > > > > > changed); > > > > > > + =A0=A0 enum mt76_txq_id qid, u32 > > > > > > changed, > > > > > > + =A0=A0 unsigned int link_id); > > > > > > =A0void mt7996_mac_update_beacons(struct mt7996_phy *phy); > > > > > > =A0void mt7996_mac_set_coverage_class(struct mt7996_phy *phy); > > > > > > =A0void mt7996_mac_work(struct work_struct *work); > > > > > >=20 > > > > > > --- > > > > > > base-commit: 4913f44167cf35a9536e9eec7352e15b2de0c573 > > > > > > change-id: 20260530-mt76_tx_status_skb_add-overwrite-fix- > > > > > > 85818a9bb31f > > > > > >=20 > > > > > > Best regards, > > > > > >=20 > > > > > >=20 > > > > > We might expand flags further so this still doesn't solve the > > > > > issue > > > > > of > > > > > flags being cleared - it only works for MLO flag. And the > > > > > developers > > > > > still won't easily notice that the flags are being cleared. > > > >=20 > > > > My opinion is we should consider just upstream code and then > > > > change > > > > it as soon > > > > as you post this new feature upstream, but I will let Felix > > > > comments > > > > on it. > > > > Moreover, the proposed approach aligns link_id used in > > > > mt7996_tx_prepare_skb() > > > > to the one used in mt7996_mac_write_txwi() and fix a possible OOB > > > > bug > > > > in > > > > mt7996_mac_write_txwi(). > > > >=20 > > > > Regards, > > > > Lorenzo > > > >=20 > > > > >=20 > > >=20 > > > Just to tie in with this patch subject - I'm just thinking of a way > > > to > > > solve this once and for all. If the problem is reading zeroed info- > > > > control.flags, wouldn't it be better to just pass a u32 flags, > > > something like this: > > >=20 > > > u32 flags =3D info->control.flags > > >=20 > > > mt7996_mac_write_txwi(dev, (__le32 *)buf, skb, wcid, NULL, 0, 0, > > > =A0=A0=A0=A0=A0 changed, flags); > > >=20 > > > We can use all flags then. > >=20 > > what about link_id? Should it be the same between > > mt7996_tx_prepare_skb() > > and mt7996_mac_write_txwi()? > >=20 > >=20 > =20 > I mean the link_id is only corresponds to one specific flags bit of > mac80211_tx_control_flags. But there are other bits that aren't > handled. Wouldn't u32 flags make it more cleaner? Yes, I got your point, but my concern is if we need to sync link_id between mt7996_tx_prepare_skb() and mt7996_mac_write_txwi(). If so, I guess it is much better to pass link_id explicitly to mt7996_mac_write_txwi() since it does not just depended on mac80211_tx_control_flags and I think we should not duplicate the logic in mt7996_mac_write_txwi(). Got my point? If in the future (not required now) we need to pass mac80211_tx_control_fla= gs to mt7996_mac_write_txwi(), we will do it easily. Regards, Lorenzo >=20 > Ryder >=20 >=20 --wzjsFj6KDcPkGXlo Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCah0fFgAKCRA6cBh0uS2t rEUJAP9cw7n0ld9kt7adipEUeq8Xa+Mo1z5RCm5s8tUkFyHaogD+PFq1JsPllni7 lolW/EqZMwI69tCxVZdr80J41s02Vws= =st0d -----END PGP SIGNATURE----- --wzjsFj6KDcPkGXlo--