Re: [PATCH wireless v2 1/2] wifi: mac80211: Update skb's control block key in ieee80211_tx_dequeue()

Re: [PATCH wireless v2 1/2] wifi: mac80211: Update skb's control block key in ieee80211_tx_dequeue()

[Bert Karwatzki]

Am Freitag, dem 11.04.2025 um 12:06 +0200 schrieb Remi Pommarel:
> Hi Bert,
>
> On Thu, Apr 10, 2025 at 11:55:26PM +0200, Bert Karwatzki wrote:
> > This commit breaks the mediatek mt7921 wireless driver. In linux-next-20250410
> > my mt7921e Wi-Fi controller is no longer able to connect to a network.
> > I bisected this to commit a104042e2bf6 ("wifi: mac80211: Update skb's control
> > block key in ieee80211_tx_dequeue()").
> >
> > Hardware:
> > 04:00.0 Network controller: MEDIATEK Corp. MT7921K (RZ608) Wi-Fi 6E 80MHz
> >
> > This debugging patch reveals that the change causes key to be NULL in
> > mt7921_tx_prepare_skb().
> >
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> > index 881812ba03ff..3b8552a1055c 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> > @@ -13,6 +13,7 @@ int mt7921e_tx_prepare_skb(struct mt76_dev *mdev, void *txwi_ptr,
> >         struct mt792x_dev *dev = container_of(mdev, struct mt792x_dev, mt76);
> >         struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx_info->skb);
> >         struct ieee80211_key_conf *key = info->control.hw_key;
> > +       dev_info(mdev->dev, "%s: key = %px\n", __func__, key);
> >         struct mt76_connac_hw_txp *txp;
> >         struct mt76_txwi_cache *t;
> >         int id, pid;
> >
> >
> > So why is info->control.hw_key not updated by ieee80211_tx_h_select_key()?
> >
> > diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> > index 34f229a6eab0..2510e3533d13 100644
> > --- a/net/mac80211/tx.c
> > +++ b/net/mac80211/tx.c
> > @@ -631,8 +631,10 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
> >  		case WLAN_CIPHER_SUITE_WEP40:
> >  		case WLAN_CIPHER_SUITE_WEP104:
> >  		case WLAN_CIPHER_SUITE_TKIP:
> > -			if (!ieee80211_is_data_present(hdr->frame_control))
> > +			if (!ieee80211_is_data_present(hdr->frame_control)) {
> > +				printk(KERN_INFO "%s %d: setting tx->key = NULL\n", __func__, __LINE__);
> >  				tx->key = NULL;
> > +			}
> >  			break;
> >  		case WLAN_CIPHER_SUITE_CCMP:
> >  		case WLAN_CIPHER_SUITE_CCMP_256:
> > @@ -641,19 +643,23 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
> >  			if (!ieee80211_is_data_present(hdr->frame_control) &&
> >  			    !ieee80211_use_mfp(hdr->frame_control, tx->sta,
> >  					       tx->skb) &&
> > -			    !ieee80211_is_group_privacy_action(tx->skb))
> > +			    !ieee80211_is_group_privacy_action(tx->skb)) {
> > +				printk(KERN_INFO "%s %d: setting tx->key = NULL\n", __func__, __LINE__);
> >  				tx->key = NULL;
> > -			else
> > +			} else {
> >  				skip_hw = (tx->key->conf.flags &
> >  					   IEEE80211_KEY_FLAG_SW_MGMT_TX) &&
> >  					ieee80211_is_mgmt(hdr->frame_control);
> > +			}
> >  			break;
> >  		case WLAN_CIPHER_SUITE_AES_CMAC:
> >  		case WLAN_CIPHER_SUITE_BIP_CMAC_256:
> >  		case WLAN_CIPHER_SUITE_BIP_GMAC_128:
> >  		case WLAN_CIPHER_SUITE_BIP_GMAC_256:
> > -			if (!ieee80211_is_mgmt(hdr->frame_control))
> > +			if (!ieee80211_is_mgmt(hdr->frame_control)) {
> > +				printk(KERN_INFO "%s %d: setting tx->key = NULL\n", __func__, __LINE__);
> >  				tx->key = NULL;
> > +			}
> >  			break;
> >  		}
> >
> > @@ -662,9 +668,13 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
> >  			     tx->skb->protocol != tx->sdata->control_port_protocol)
> >  			return TX_DROP;
> >
> > +		printk(KERN_INFO "%s: skip_hw=%d tx->key=%px\n",
> > +				__func__, skip_hw, tx->key);
> >  		if (!skip_hw && tx->key &&
> > -		    tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)
> > +		    tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
> >  			info->control.hw_key = &tx->key->conf;
> > +			printk(KERN_INFO "%s: info->control.hw_key = %px\n", __func__, info->control.hw_key);
> > +		}
> >  	} else if (ieee80211_is_data_present(hdr->frame_control) && tx->sta &&
> >  		   test_sta_flag(tx->sta, WLAN_STA_USES_ENCRYPTION)) {
> >  		return TX_DROP;
> > @@ -3894,6 +3904,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
> >  	 * The key can be removed while the packet was queued, so need to call
> >  	 * this here to get the current key.
> >  	 */
> > +	printk(KERN_INFO "%s: info->control.hw_key = %px, setting to NULL\n",
> > +			__func__, info->control.hw_key);
> >  	info->control.hw_key = NULL;
> >  	r = ieee80211_tx_h_select_key(&tx);
> >  	if (r != TX_CONTINUE) {
> >
> > This patch reveals that tx->key is set to NULL (in the @@ -641,19 +643,23 @@ chunk)
> > and so the updating of info->contro.hw_key is skipped:
> >
> > [   17.411298] [   T1232] ieee80211_tx_h_select_key 647: setting tx->key = NULL
>
> That means that we are trying to send non management frames using
> AES_CMAC, or BIP_* cipher, aren't those ciphers used only for group
> management frames ?
>
> > [   17.411300] [   T1232] ieee80211_tx_h_select_key: skip_hw=0 tx->key=0000000000000000
> > [   17.411307] [   T1232] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key = 0000000000000000
> >
> > If I revert commit a104042e2bf6 while keeping the debug patches it shows that
> > the for mt7921e the key is never updated in ieee80211_tx_h_select_key(), mt7921e
> > relies on the key your patch is setting to NULL.
> >
> > Is this a problem with your patch or with the mt7921e driver that just got
> > revealed by your patch?
>
> Not sure yet, do you happen to know which kind of frame mt7921e is
> trying to be sent using this NULL key ?
>
> Thanks,

I modified my debugging patch to print mgmt->frame_control, if needed I could
also insert a nore complicated function printing out frame types using the
ieee80211_is_*() functions:

diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
index 881812ba03ff..cfbe7e1e4713 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
@@ -13,6 +13,9 @@ int mt7921e_tx_prepare_skb(struct mt76_dev *mdev, void
*txwi_ptr,
        struct mt792x_dev *dev = container_of(mdev, struct mt792x_dev, mt76);
        struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx_info->skb);
        struct ieee80211_key_conf *key = info->control.hw_key;
+       struct ieee80211_mgmt *mgmt = (void *)tx_info->skb->data;
+       __le16 fc = mgmt->frame_control;
+       dev_info(mdev->dev, "%s: key = %px fc = 0x%hx\n", __func__, key, fc);
        struct mt76_connac_hw_txp *txp;
        struct mt76_txwi_cache *t;
        int id, pid;

and get this, while unsuccesfully trying to connect (also note that one time
getting a key worked):

$ dmesg | grep prepare_skb
[   11.775642] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xb0
[   11.800047] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x0
[   13.365330] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xb0
[   13.370257] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x0
[   16.468481] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xb0
[   16.472407] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x0
[   16.542017] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x188
[   16.549581] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x188
[   16.597120] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[   16.612263] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xd0

Here we actually go a key:
[   16.614478] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
ffff89c275297230 fc = 0x4188

[   16.654273] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   16.698286] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   17.735855] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   17.837355] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   17.851029] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   18.613079] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[   18.786202] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   21.027478] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   21.150212] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[   21.843201] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   25.769981] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[   29.776926] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   34.424966] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[   44.945880] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   50.670382] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[   62.054907] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xc0
[   65.042457] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xb0
[   65.047387] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x0
[   65.391881] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x188
[   65.405189] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x188
[   65.445775] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[   65.449289] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   65.479305] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xd0
[   65.854396] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xb0
[   65.878897] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x0
[   65.954047] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x188
[   65.961191] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x188
[   65.966296] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   65.977287] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xd0
[   66.117317] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   66.626938] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   66.700300] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   66.818440] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   67.347089] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   67.448947] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[   67.649713] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   68.535890] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   69.537434] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   69.697331] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[   69.866666] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   70.500294] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   71.537535] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   74.615436] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[   75.543129] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   77.741585] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   83.309171] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[   83.550827] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   91.650611] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   99.564147] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[   99.625088] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[  111.050345] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xc0
[  114.346190] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xb0
[  114.370450] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x0
[  114.447350] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x188
[  114.454710] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x188
[  114.494605] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[  114.520269] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xd0
[  114.522520] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
ffff89c2e7227230 fc = 0x4188
[  115.138564] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[  115.394552] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[  116.435866] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[  116.494988] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[  116.612522] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[  116.724047] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[  117.459651] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[  117.600286] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[  118.816266] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[  118.961067] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[  120.304632] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[  121.614000] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[  123.281756] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0xffff
[  127.930929] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333
[  129.617550] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
0000000000000000 fc = 0x3333

Bert Karwatzki

Re: [PATCH wireless v2 1/2] wifi: mac80211: Update skb's control block key in ieee80211_tx_dequeue()

[Remi Pommarel]

On Fri, Apr 11, 2025 at 01:10:02PM +0200, Bert Karwatzki wrote:
> Am Freitag, dem 11.04.2025 um 12:06 +0200 schrieb Remi Pommarel:
> > Hi Bert,
> >
> > On Thu, Apr 10, 2025 at 11:55:26PM +0200, Bert Karwatzki wrote:
> > > This commit breaks the mediatek mt7921 wireless driver. In linux-next-20250410
> > > my mt7921e Wi-Fi controller is no longer able to connect to a network.
> > > I bisected this to commit a104042e2bf6 ("wifi: mac80211: Update skb's control
> > > block key in ieee80211_tx_dequeue()").
> > >
> > > Hardware:
> > > 04:00.0 Network controller: MEDIATEK Corp. MT7921K (RZ608) Wi-Fi 6E 80MHz
> > >
> > > This debugging patch reveals that the change causes key to be NULL in
> > > mt7921_tx_prepare_skb().
> > >
> > > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> > > index 881812ba03ff..3b8552a1055c 100644
> > > --- a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> > > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> > > @@ -13,6 +13,7 @@ int mt7921e_tx_prepare_skb(struct mt76_dev *mdev, void *txwi_ptr,
> > >         struct mt792x_dev *dev = container_of(mdev, struct mt792x_dev, mt76);
> > >         struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx_info->skb);
> > >         struct ieee80211_key_conf *key = info->control.hw_key;
> > > +       dev_info(mdev->dev, "%s: key = %px\n", __func__, key);
> > >         struct mt76_connac_hw_txp *txp;
> > >         struct mt76_txwi_cache *t;
> > >         int id, pid;
> > >
> > >
> > > So why is info->control.hw_key not updated by ieee80211_tx_h_select_key()?
> > >
> > > diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> > > index 34f229a6eab0..2510e3533d13 100644
> > > --- a/net/mac80211/tx.c
> > > +++ b/net/mac80211/tx.c
> > > @@ -631,8 +631,10 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
> > >  		case WLAN_CIPHER_SUITE_WEP40:
> > >  		case WLAN_CIPHER_SUITE_WEP104:
> > >  		case WLAN_CIPHER_SUITE_TKIP:
> > > -			if (!ieee80211_is_data_present(hdr->frame_control))
> > > +			if (!ieee80211_is_data_present(hdr->frame_control)) {
> > > +				printk(KERN_INFO "%s %d: setting tx->key = NULL\n", __func__, __LINE__);
> > >  				tx->key = NULL;
> > > +			}
> > >  			break;
> > >  		case WLAN_CIPHER_SUITE_CCMP:
> > >  		case WLAN_CIPHER_SUITE_CCMP_256:
> > > @@ -641,19 +643,23 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
> > >  			if (!ieee80211_is_data_present(hdr->frame_control) &&
> > >  			    !ieee80211_use_mfp(hdr->frame_control, tx->sta,
> > >  					       tx->skb) &&
> > > -			    !ieee80211_is_group_privacy_action(tx->skb))
> > > +			    !ieee80211_is_group_privacy_action(tx->skb)) {
> > > +				printk(KERN_INFO "%s %d: setting tx->key = NULL\n", __func__, __LINE__);
> > >  				tx->key = NULL;
> > > -			else
> > > +			} else {
> > >  				skip_hw = (tx->key->conf.flags &
> > >  					   IEEE80211_KEY_FLAG_SW_MGMT_TX) &&
> > >  					ieee80211_is_mgmt(hdr->frame_control);
> > > +			}
> > >  			break;
> > >  		case WLAN_CIPHER_SUITE_AES_CMAC:
> > >  		case WLAN_CIPHER_SUITE_BIP_CMAC_256:
> > >  		case WLAN_CIPHER_SUITE_BIP_GMAC_128:
> > >  		case WLAN_CIPHER_SUITE_BIP_GMAC_256:
> > > -			if (!ieee80211_is_mgmt(hdr->frame_control))
> > > +			if (!ieee80211_is_mgmt(hdr->frame_control)) {
> > > +				printk(KERN_INFO "%s %d: setting tx->key = NULL\n", __func__, __LINE__);
> > >  				tx->key = NULL;
> > > +			}
> > >  			break;
> > >  		}
> > >
> > > @@ -662,9 +668,13 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
> > >  			     tx->skb->protocol != tx->sdata->control_port_protocol)
> > >  			return TX_DROP;
> > >
> > > +		printk(KERN_INFO "%s: skip_hw=%d tx->key=%px\n",
> > > +				__func__, skip_hw, tx->key);
> > >  		if (!skip_hw && tx->key &&
> > > -		    tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)
> > > +		    tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
> > >  			info->control.hw_key = &tx->key->conf;
> > > +			printk(KERN_INFO "%s: info->control.hw_key = %px\n", __func__, info->control.hw_key);
> > > +		}
> > >  	} else if (ieee80211_is_data_present(hdr->frame_control) && tx->sta &&
> > >  		   test_sta_flag(tx->sta, WLAN_STA_USES_ENCRYPTION)) {
> > >  		return TX_DROP;
> > > @@ -3894,6 +3904,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
> > >  	 * The key can be removed while the packet was queued, so need to call
> > >  	 * this here to get the current key.
> > >  	 */
> > > +	printk(KERN_INFO "%s: info->control.hw_key = %px, setting to NULL\n",
> > > +			__func__, info->control.hw_key);
> > >  	info->control.hw_key = NULL;
> > >  	r = ieee80211_tx_h_select_key(&tx);
> > >  	if (r != TX_CONTINUE) {
> > >
> > > This patch reveals that tx->key is set to NULL (in the @@ -641,19 +643,23 @@ chunk)
> > > and so the updating of info->contro.hw_key is skipped:
> > >
> > > [   17.411298] [   T1232] ieee80211_tx_h_select_key 647: setting tx->key = NULL
> >
> > That means that we are trying to send non management frames using
> > AES_CMAC, or BIP_* cipher, aren't those ciphers used only for group
> > management frames ?

Thanks for the debug.

I completely miscalculated the line here sorry. In fact it means here
that mt7921e needs a key for null data frames or for non protected
management frames.

As a lot of your frame control dump does not make sense to me (e.g.
0xffff) maybe you could try to print the fc here at line 647. Just a
wild guess maybe mt7921e needs MT_TXD3_PROTECT_FRAME for NULL data
frames ?

> >
> > > [   17.411300] [   T1232] ieee80211_tx_h_select_key: skip_hw=0 tx->key=0000000000000000
> > > [   17.411307] [   T1232] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key = 0000000000000000
> > >
> > > If I revert commit a104042e2bf6 while keeping the debug patches it shows that
> > > the for mt7921e the key is never updated in ieee80211_tx_h_select_key(), mt7921e
> > > relies on the key your patch is setting to NULL.
> > >
> > > Is this a problem with your patch or with the mt7921e driver that just got
> > > revealed by your patch?
> >
> > Not sure yet, do you happen to know which kind of frame mt7921e is
> > trying to be sent using this NULL key ?
> >
> > Thanks,
> 
> I modified my debugging patch to print mgmt->frame_control, if needed I could
> also insert a nore complicated function printing out frame types using the
> ieee80211_is_*() functions:
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> index 881812ba03ff..cfbe7e1e4713 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> @@ -13,6 +13,9 @@ int mt7921e_tx_prepare_skb(struct mt76_dev *mdev, void
> *txwi_ptr,
>         struct mt792x_dev *dev = container_of(mdev, struct mt792x_dev, mt76);
>         struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx_info->skb);
>         struct ieee80211_key_conf *key = info->control.hw_key;
> +       struct ieee80211_mgmt *mgmt = (void *)tx_info->skb->data;
> +       __le16 fc = mgmt->frame_control;
> +       dev_info(mdev->dev, "%s: key = %px fc = 0x%hx\n", __func__, key, fc);
>         struct mt76_connac_hw_txp *txp;
>         struct mt76_txwi_cache *t;
>         int id, pid;
> 
> and get this, while unsuccesfully trying to connect (also note that one time
> getting a key worked):
> 
> $ dmesg | grep prepare_skb
> [   11.775642] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0xb0
> [   11.800047] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0x0
> [   13.365330] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0xb0
> [   13.370257] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0x0
> [   16.468481] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0xb0
> [   16.472407] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0x0
> [   16.542017] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0x188
> [   16.549581] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0x188
> [   16.597120] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0xffff

Thanks again for taking time to debug that.
As said above some of those fc does not make sense to me as I am not
expected first two bits (Protocol version) to be non null.

> [   16.612263] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0xd0
> 
> Here we actually go a key:
> [   16.614478] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> ffff89c275297230 fc = 0x4188
> [...]

-- 
Remi

Re: [PATCH wireless v2 1/2] wifi: mac80211: Update skb's control block key in ieee80211_tx_dequeue()

[Remi Pommarel]

Hello Bert, Johannes

On Fri, Apr 11, 2025 at 01:10:02PM +0200, Bert Karwatzki wrote:
> Am Freitag, dem 11.04.2025 um 12:06 +0200 schrieb Remi Pommarel:
> > Hi Bert,
> >
> > On Thu, Apr 10, 2025 at 11:55:26PM +0200, Bert Karwatzki wrote:
> > > This commit breaks the mediatek mt7921 wireless driver. In linux-next-20250410
> > > my mt7921e Wi-Fi controller is no longer able to connect to a network.
> > > I bisected this to commit a104042e2bf6 ("wifi: mac80211: Update skb's control
> > > block key in ieee80211_tx_dequeue()").
> > >
> > > Hardware:
> > > 04:00.0 Network controller: MEDIATEK Corp. MT7921K (RZ608) Wi-Fi 6E 80MHz
> > >
> > > This debugging patch reveals that the change causes key to be NULL in
> > > mt7921_tx_prepare_skb().
> > >
> > > diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> > > index 881812ba03ff..3b8552a1055c 100644
> > > --- a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> > > +++ b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> > > @@ -13,6 +13,7 @@ int mt7921e_tx_prepare_skb(struct mt76_dev *mdev, void *txwi_ptr,
> > >         struct mt792x_dev *dev = container_of(mdev, struct mt792x_dev, mt76);
> > >         struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx_info->skb);
> > >         struct ieee80211_key_conf *key = info->control.hw_key;
> > > +       dev_info(mdev->dev, "%s: key = %px\n", __func__, key);
> > >         struct mt76_connac_hw_txp *txp;
> > >         struct mt76_txwi_cache *t;
> > >         int id, pid;
> > >
> > >
> > > So why is info->control.hw_key not updated by ieee80211_tx_h_select_key()?
> > >
> > > diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> > > index 34f229a6eab0..2510e3533d13 100644
> > > --- a/net/mac80211/tx.c
> > > +++ b/net/mac80211/tx.c
> > > @@ -631,8 +631,10 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
> > >  		case WLAN_CIPHER_SUITE_WEP40:
> > >  		case WLAN_CIPHER_SUITE_WEP104:
> > >  		case WLAN_CIPHER_SUITE_TKIP:
> > > -			if (!ieee80211_is_data_present(hdr->frame_control))
> > > +			if (!ieee80211_is_data_present(hdr->frame_control)) {
> > > +				printk(KERN_INFO "%s %d: setting tx->key = NULL\n", __func__, __LINE__);
> > >  				tx->key = NULL;
> > > +			}
> > >  			break;
> > >  		case WLAN_CIPHER_SUITE_CCMP:
> > >  		case WLAN_CIPHER_SUITE_CCMP_256:
> > > @@ -641,19 +643,23 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
> > >  			if (!ieee80211_is_data_present(hdr->frame_control) &&
> > >  			    !ieee80211_use_mfp(hdr->frame_control, tx->sta,
> > >  					       tx->skb) &&
> > > -			    !ieee80211_is_group_privacy_action(tx->skb))
> > > +			    !ieee80211_is_group_privacy_action(tx->skb)) {
> > > +				printk(KERN_INFO "%s %d: setting tx->key = NULL\n", __func__, __LINE__);
> > >  				tx->key = NULL;
> > > -			else
> > > +			} else {
> > >  				skip_hw = (tx->key->conf.flags &
> > >  					   IEEE80211_KEY_FLAG_SW_MGMT_TX) &&
> > >  					ieee80211_is_mgmt(hdr->frame_control);
> > > +			}
> > >  			break;
> > >  		case WLAN_CIPHER_SUITE_AES_CMAC:
> > >  		case WLAN_CIPHER_SUITE_BIP_CMAC_256:
> > >  		case WLAN_CIPHER_SUITE_BIP_GMAC_128:
> > >  		case WLAN_CIPHER_SUITE_BIP_GMAC_256:
> > > -			if (!ieee80211_is_mgmt(hdr->frame_control))
> > > +			if (!ieee80211_is_mgmt(hdr->frame_control)) {
> > > +				printk(KERN_INFO "%s %d: setting tx->key = NULL\n", __func__, __LINE__);
> > >  				tx->key = NULL;
> > > +			}
> > >  			break;
> > >  		}
> > >
> > > @@ -662,9 +668,13 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
> > >  			     tx->skb->protocol != tx->sdata->control_port_protocol)
> > >  			return TX_DROP;
> > >
> > > +		printk(KERN_INFO "%s: skip_hw=%d tx->key=%px\n",
> > > +				__func__, skip_hw, tx->key);
> > >  		if (!skip_hw && tx->key &&
> > > -		    tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)
> > > +		    tx->key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
> > >  			info->control.hw_key = &tx->key->conf;
> > > +			printk(KERN_INFO "%s: info->control.hw_key = %px\n", __func__, info->control.hw_key);
> > > +		}
> > >  	} else if (ieee80211_is_data_present(hdr->frame_control) && tx->sta &&
> > >  		   test_sta_flag(tx->sta, WLAN_STA_USES_ENCRYPTION)) {
> > >  		return TX_DROP;
> > > @@ -3894,6 +3904,8 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
> > >  	 * The key can be removed while the packet was queued, so need to call
> > >  	 * this here to get the current key.
> > >  	 */
> > > +	printk(KERN_INFO "%s: info->control.hw_key = %px, setting to NULL\n",
> > > +			__func__, info->control.hw_key);
> > >  	info->control.hw_key = NULL;
> > >  	r = ieee80211_tx_h_select_key(&tx);
> > >  	if (r != TX_CONTINUE) {
> > >
> > > This patch reveals that tx->key is set to NULL (in the @@ -641,19 +643,23 @@ chunk)
> > > and so the updating of info->contro.hw_key is skipped:
> > >
> > > [   17.411298] [   T1232] ieee80211_tx_h_select_key 647: setting tx->key = NULL
> >
> > That means that we are trying to send non management frames using
> > AES_CMAC, or BIP_* cipher, aren't those ciphers used only for group
> > management frames ?
> >
> > > [   17.411300] [   T1232] ieee80211_tx_h_select_key: skip_hw=0 tx->key=0000000000000000
> > > [   17.411307] [   T1232] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key = 0000000000000000
> > >
> > > If I revert commit a104042e2bf6 while keeping the debug patches it shows that
> > > the for mt7921e the key is never updated in ieee80211_tx_h_select_key(), mt7921e
> > > relies on the key your patch is setting to NULL.
> > >
> > > Is this a problem with your patch or with the mt7921e driver that just got
> > > revealed by your patch?
> >
> > Not sure yet, do you happen to know which kind of frame mt7921e is
> > trying to be sent using this NULL key ?
> >
> > Thanks,
> 
> I modified my debugging patch to print mgmt->frame_control, if needed I could
> also insert a nore complicated function printing out frame types using the
> ieee80211_is_*() functions:
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> index 881812ba03ff..cfbe7e1e4713 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt7921/pci_mac.c
> @@ -13,6 +13,9 @@ int mt7921e_tx_prepare_skb(struct mt76_dev *mdev, void
> *txwi_ptr,
>         struct mt792x_dev *dev = container_of(mdev, struct mt792x_dev, mt76);
>         struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx_info->skb);
>         struct ieee80211_key_conf *key = info->control.hw_key;
> +       struct ieee80211_mgmt *mgmt = (void *)tx_info->skb->data;
> +       __le16 fc = mgmt->frame_control;
> +       dev_info(mdev->dev, "%s: key = %px fc = 0x%hx\n", __func__, key, fc);
>         struct mt76_connac_hw_txp *txp;
>         struct mt76_txwi_cache *t;
>         int id, pid;
> 
> and get this, while unsuccesfully trying to connect (also note that one time
> getting a key worked):
> 
> $ dmesg | grep prepare_skb
> [   11.775642] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0xb0
> [   11.800047] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0x0
> [   13.365330] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0xb0
> [   13.370257] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0x0
> [   16.468481] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0xb0
> [   16.472407] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0x0
> [   16.542017] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0x188
> [   16.549581] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0x188
> [   16.597120] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0xffff
> [   16.612263] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0xd0
> 
> Here we actually go a key:
> [   16.614478] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> ffff89c275297230 fc = 0x4188
> 
> [   16.654273] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0x3333
> [   16.698286] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0x3333
> [   17.735855] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =
> 0000000000000000 fc = 0x3333
> [   17.837355] [   T1227] mt7921e 0000:04:00.0: mt7921e_tx_prepare_skb: key =

[....]

I have ordered a mt7921 card so I could reproduce this and finally took
time to debug that. The issue comes to the fact that mt7921 uses 802.11
encapsulation offloading and as such we are calling
ieee80211_tx_h_select_key() on a 802.3 frame.

This function casts the skb data as a 802.11 header regardless the skb
is 802.11 or not in order to decide if the info->control.hw_key needs to
be set. So the hw_key is likely to remain NULL in ieee80211_tx_dequeue()
and because mt7921 driver needs this key to be valid data frames are
dropped.

Will send a patch so that ieee80211_tx_h_select_key() does not try to
get info from a ieee80211_hdr mapped on 802.3 packet data (i.e. when
IEEE80211_TX_CTL_HW_80211_ENCAP is set).

Thanks

-- 
Remi

Re: [PATCH wireless v2 1/2] wifi: mac80211: Update skb's control block key in ieee80211_tx_dequeue()

[Johannes Berg]

Hi,

On Thu, 2025-07-17 at 15:21 +0200, Remi Pommarel wrote:
> I have ordered a mt7921 card so I could reproduce this and finally took
> time to debug that. 

Oh wow, talk about dedication. Thank you!

> The issue comes to the fact that mt7921 uses 802.11
> encapsulation offloading and as such we are calling
> ieee80211_tx_h_select_key() on a 802.3 frame.

Oh... Guess we could've thought about that, sorry. Though I would've
thought ath12k will also do that. Maybe not in the config you were
seeing the issue in?

> This function casts the skb data as a 802.11 header regardless the skb
> is 802.11 or not in order to decide if the info->control.hw_key needs to
> be set. So the hw_key is likely to remain NULL in ieee80211_tx_dequeue()
> and because mt7921 driver needs this key to be valid data frames are
> dropped.
> 
> Will send a patch so that ieee80211_tx_h_select_key() does not try to
> get info from a ieee80211_hdr mapped on 802.3 packet data (i.e. when
> IEEE80211_TX_CTL_HW_80211_ENCAP is set).
> 

Sounds good!

johannes

Re: [PATCH wireless v2 1/2] wifi: mac80211: Update skb's control block key in ieee80211_tx_dequeue()

[Remi Pommarel]

On Thu, Jul 17, 2025 at 03:46:00PM +0200, Johannes Berg wrote:
> Hi,
> 
> On Thu, 2025-07-17 at 15:21 +0200, Remi Pommarel wrote:
> > I have ordered a mt7921 card so I could reproduce this and finally took
> > time to debug that. 
> 
> Oh wow, talk about dedication. Thank you!
> 
> > The issue comes to the fact that mt7921 uses 802.11
> > encapsulation offloading and as such we are calling
> > ieee80211_tx_h_select_key() on a 802.3 frame.
> 
> Oh... Guess we could've thought about that, sorry. Though I would've
> thought ath12k will also do that. Maybe not in the config you were
> seeing the issue in?

I've mainly tested on ath*k HW. Either without 802.11 encapsulation HW
offloading or with it but only on ath12k and it seems that this driver
does not need the key in this mode. After a quick look into ath11k and
10k that also seems to be the case for those chips.

For the record, if I am not mistaken, ath12k driver does not seem to
support 802.11 encaps HW offloading mainline yet, I do seem to have some
vendor patches to support it though.

> 
> > This function casts the skb data as a 802.11 header regardless the skb
> > is 802.11 or not in order to decide if the info->control.hw_key needs to
> > be set. So the hw_key is likely to remain NULL in ieee80211_tx_dequeue()
> > and because mt7921 driver needs this key to be valid data frames are
> > dropped.
> > 
> > Will send a patch so that ieee80211_tx_h_select_key() does not try to
> > get info from a ieee80211_hdr mapped on 802.3 packet data (i.e. when
> > IEEE80211_TX_CTL_HW_80211_ENCAP is set).
> > 
> 
> Sounds good!
> 
> johannes

-- 
Remi

Re: [PATCH wireless v2 1/2] wifi: mac80211: Update skb's control block key in ieee80211_tx_dequeue()

[Jeff Johnson]

On 7/17/2025 7:22 AM, Remi Pommarel wrote:
> For the record, if I am not mistaken, ath12k driver does not seem to
> support 802.11 encaps HW offloading mainline yet, I do seem to have some
> vendor patches to support it though.

Upstreaming this functionality is in the internal pipeline but won't make the
v6.17 merge window.

Re: [PATCH wireless v2 1/2] wifi: mac80211: Update skb's control block key in ieee80211_tx_dequeue()

[Jeff Johnson]

On 7/17/2025 8:46 AM, Jeff Johnson wrote:
> On 7/17/2025 7:22 AM, Remi Pommarel wrote:
>> For the record, if I am not mistaken, ath12k driver does not seem to
>> support 802.11 encaps HW offloading mainline yet, I do seem to have some
>> vendor patches to support it though.
> 
> Upstreaming this functionality is in the internal pipeline but won't make the
> v6.17 merge window.

I make a comment like that and all of a sudden internal reviews get expedited.

s/but won't/and should/

/jeff