From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 17DB11DA60D; Sun, 31 May 2026 13:12:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780233156; cv=none; b=umWAtyO0gNX5oKCzlYx8xDDGdgL1bOPcWYXUB/2xYwKwIwL5kSQl0KZHRJV3SA5E8SziQOnV02DSzUpKDUU5HuW9GiJowFrfJ3HX9MyUH7u0xa3RYffMbW2GZqjihp5/Uu6nER4rc5ojVM4zISWw+kZLMD+oCFjcCEQoq01GKXs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780233156; c=relaxed/simple; bh=guQtl8M7IjJ2QF5QA0efzkLPAd9vU0qZWP/hQt0sGK8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=WwWNEaRo3vGwP1OHFpv+aNOAVelXcO9ToxPgRYNqYHjqGVyzwVIs4KQoBzX9UIkGiiM6peOGxyIbtp+5aa4X9ow6Lh3LPF5Q+T6RO4jPfWTB3LsSGYFca3Znu6A6rNGi+osfmthtv3orWfkZ1WvuKk/XKo7wKmWAWr2TsxNpHbg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=f4d+ei5s; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="f4d+ei5s" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3C1E11F00893; Sun, 31 May 2026 13:12:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1780233154; bh=wl7h/nP2W/+/m5djoDmhJ64JCsZ6GM2Cvfrk76r+gLs=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=f4d+ei5sYqMZEWD3KJD9Nj2yKOWAhbOgRkN/kOcjgLYeKtTcyVEhnrBkyNZ1HBW0g 9SUts+yAv67XmNihdT2OqfRi3VFGmDmKbxj02XbEEiZ8pDiVBtKgDkqFfzNWEYfkj+ p1oPu1CHWWO/AxGWt9X4ZC3OBi6D8E/AdunQgXVDT0j1R3TdffoFzMjEKpxChAPex/ AokYRhREuOlYOnZavWxaeNOdHZ50Ef5mRFN6pv0x4KSzIAPJHGEKEiDREBpmhqf2CD M713WnWUS/XdsMXptAt2hHOgPDzgrrE9hgVKNNUkbV+m3JlNr77F6HhplKNLzh/ISn bVtOgVFIq3n8w== Date: Sun, 31 May 2026 15:12:32 +0200 From: "lorenzo@kernel.org" To: Ryder Lee Cc: "linux-mediatek@lists.infradead.org" , Shayne Chen =?utf-8?B?KOmZs+i7kuS4nik=?= , "nbd@nbd.name" , Roy-CH Luo , AngeloGioacchino Del Regno , "linux-kernel@vger.kernel.org" , Chui-hao Chiu =?utf-8?B?KOmCseWegua1qSk=?= , Sean Wang , Bo Jiao =?utf-8?B?KOeEpuazoik=?= , "matthias.bgg@gmail.com" , "linux-arm-kernel@lists.infradead.org" , "linux-wireless@vger.kernel.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> Precedence: bulk X-Mailing-List: linux-wireless@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="iV0pCa8YK5XNI+fk" Content-Disposition: inline In-Reply-To: <44c54ed4da0d294c567b3b0ad750f082a6f1be9f.camel@mediatek.com> --iV0pCa8YK5XNI+fk 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 14:11 +0200, lorenzo@kernel.org wrote: > > > 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-overwrite= -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, > changed, flags); >=20 > We can use all flags then. what about link_id? Should it be the same between mt7996_tx_prepare_skb() and mt7996_mac_write_txwi()? Regards, Lorenzo >=20 > Ryder --iV0pCa8YK5XNI+fk Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iHUEABYKAB0WIQTquNwa3Txd3rGGn7Y6cBh0uS2trAUCahwzwAAKCRA6cBh0uS2t rD4YAP9hHyxN5pAAliX9U3KZa/cZBB69MpDTmStOuUDGtpwWTAEA1CPZPxSRh8dU yjwjrj7e/lPn9UmJlgYuTfrKZNsR9QA= =c0Am -----END PGP SIGNATURE----- --iV0pCa8YK5XNI+fk--