Re: [PATCH 06/18] wifi: iwlwifi: keep the TSO and workaround pages mapped

["Berg, Benjamin" <benjamin.berg@intel.com>]

Hi Ben,

yes, you need to apply:

commit 003eae5a28c6c9d50290a4ac9b955be912f24c9f
Author: Benjamin Berg <benjamin.berg@intel.com>
Date:   Tue Jul 9 14:31:49 2024 +0200

    wifi: iwlwifi: correctly reference TSO page information


I had not fully tested the last revision and the error slipped
unfortunately.

Benjamin


On Wed, 2024-07-10 at 14:41 -0700, Ben Greear wrote:
> On 7/3/24 02:58, Miri Korenblit wrote:
> > From: Benjamin Berg <benjamin.berg@intel.com>
> > 
> > Map the pages when allocating them so that we will not need to map
> > each
> > of the used fragments at a later point.
> > 
> > For now the mapping is not used, this will be changed in a later
> > commit.
> > 
> > Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>
> > Signed-off-by: Miri Korenblit <miriam.rachel.korenblit@intel.com>
> > Reviewed-by: Johannes Berg <johannes.berg@intel.com>
> 
> Hello,
> 
> I patched every iwlwifi and mac80211 patch I found on the mailing
> list
> up to this one into my 6.10-ish tree.
> 
> I see immediate and reproducible crash when I send TCP traffic on
> be200
> radio.  I bisected it to this particular patch.
> 
> Previously I was only testing download direction and it ran fine, and
> UDP
> traffic upload seems to run OK, so it must be triggered by large TCP
> frames, and based on what this patch is messing with, that makes
> sense.
> Always possible I messed up when applying patches of course.
> 
> 
> Oops: general protection fault, probably for non-canonical address
> 0x5088000000ff0: 0000 [#1] PREEMPT SMP
> CPU: 3 PID: 800 Comm: irq/181-iwlwifi Tainted: G        W         
> 6.10.0-rc7+ #17
> Hardware name: Default string Default string/SKYBAY, BIOS 5.12
> 02/15/2023
> RIP: 0010:iwl_pcie_prep_tso+0x213/0x2b0 [iwlwifi]
> Code: 01 e0 49 c1 e4 06 41 b8 01 00 00 00 48 c1 f8 06 48 c1 e0 0c 48
> 01 d0 31 d2 48 89 45 08 48 b8 e8 0f 00 00 80 88 ff ff0
> RSP: 0018:ffffc900001e0678 EFLAGS: 00010207
> RAX: ffff888000000fe8 RBX: ffff8881263a3500 RCX: 0000000000001000
> RDX: 0000000000000000 RSI: ffffffff82b6ee60 RDI: ffff8881128d3748
> RBP: ffffe8ffffb882d0 R08: 0000000000000001 R09: 0000000000000020
> R10: 0000000000000002 R11: ffff8881128d35b8 R12: 0005088000000fe8
> R13: ffff888111b90028 R14: ffff88810f664438 R15: ffff8881263a3540
> FS:  0000000000000000(0000) GS:ffff88845db80000(0000)
> knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000040f70fe0 CR3: 0000000002a50002 CR4: 00000000003706f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>   <IRQ>
>   ? die_addr+0x2d/0x80
>   ? exc_general_protection+0x1aa/0x3f0
>   ? asm_exc_general_protection+0x22/0x30
>   ? iwl_pcie_prep_tso+0x213/0x2b0 [iwlwifi]
>   ? iwl_pcie_prep_tso+0x1b2/0x2b0 [iwlwifi]
>   iwl_txq_gen2_tx+0x7ec/0xf10 [iwlwifi]
>   iwl_mvm_tx_mpdu+0x1da/0x560 [iwlmvm]
>   iwl_mvm_tx_skb_sta+0x34c/0x540 [iwlmvm]
>   iwl_mvm_tx_skb+0x12/0x30 [iwlmvm]
>   iwl_mvm_mac_itxq_xmit+0xbe/0x1e0 [iwlmvm]
>   ieee80211_queue_skb+0x557/0x6d0 [mac80211]
>   __ieee80211_xmit_fast+0x7b2/0xad0 [mac80211]
>   ? lock_acquire+0xc7/0x2d0
>   ? lock_is_held_type+0xa5/0x110
>   ? find_held_lock+0x2b/0x80
>   ? skb_mac_gso_segment+0x130/0x1e0
>   ? lock_release+0xc6/0x290
>   ? skb_mac_gso_segment+0x13a/0x1e0
>   __ieee80211_subif_start_xmit+0x22d/0x5a0 [mac80211]
>   ieee80211_subif_start_xmit+0x57/0x570 [mac80211]
>   ? dev_queue_xmit_nit+0x2cd/0x3a0
>   ? lock_release+0xc6/0x290
>   ? dev_hard_start_xmit+0x76/0x250
>   dev_hard_start_xmit+0x76/0x250
>   __dev_queue_xmit+0x246/0x1270
>   ? lockdep_hardirqs_on_prepare+0xa7/0x170
>   ? eth_header+0x21/0xb0
>   ip_finish_output2+0x230/0xaa0
>   __ip_queue_xmit+0x1e6/0x760
>   __tcp_transmit_skb+0x521/0xdc0
>   ? find_held_lock+0x2b/0x80
>   tcp_write_xmit+0x4d7/0x17a0
>   ? lock_is_held_type+0xa5/0x110
>   tcp_tsq_handler+0x8a/0xd0
>   tcp_tasklet_func+0xbc/0x140
>   tasklet_action_common.constprop.0+0xfd/0x2f0
>   handle_softirqs+0xd0/0x440
>   ? iwl_pcie_irq_rx_msix_handler+0xe5/0x140 [iwlwifi]
>   ? iwl_pcie_irq_rx_msix_handler+0x5d/0x140 [iwlwifi]
>   do_softirq.part.0+0x3a/0x90
>   </IRQ>
>   <TASK>
>   __local_bh_enable_ip+0xbd/0xd0
>   ? iwl_pcie_irq_rx_msix_handler+0xe5/0x140 [iwlwifi]
>   iwl_pcie_irq_rx_msix_handler+0xed/0x140 [iwlwifi]
>   ? irq_thread+0x8c/0x200
>   irq_thread_fn+0x14/0x50
>   ? irq_thread+0x8c/0x200
>   irq_thread+0x12c/0x200
>   ? disable_irq_nosync+0x10/0x10
>   ? irq_set_affinity_notifier+0x140/0x140
>   ? irq_check_status_bit+0xf0/0xf0
>   kthread+0xd7/0x110
>   ? kthread_insert_work_sanity_check+0x50/0x50
>   ret_from_fork+0x28/0x40
>   ? kthread_insert_work_sanity_check+0x50/0x50
>   ret_from_fork_asm+0x11/0x20
>   </TASK>
> 
> 
> > ---
> >   .../wireless/intel/iwlwifi/pcie/internal.h    | 30 +++++++++-
> >   .../net/wireless/intel/iwlwifi/pcie/tx-gen2.c | 22 ++++++-
> >   drivers/net/wireless/intel/iwlwifi/pcie/tx.c  | 60
> > +++++++++++++++----
> >   3 files changed, 95 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
> > b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
> > index d63c1c284f70..b59de4f80b4b 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
> > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
> > @@ -603,6 +603,22 @@ struct iwl_tso_hdr_page {
> >   	u8 *pos;
> >   };
> >   
> > +/*
> > + * Note that we put this struct *last* in the page. By doing that,
> > we ensure
> > + * that no TB referencing this page can trigger the 32-bit
> > boundary hardware
> > + * bug.
> > + */
> > +struct iwl_tso_page_info {
> > +	dma_addr_t dma_addr;
> > +	struct page *next;
> > +	refcount_t use_count;
> > +};
> > +
> > +#define IWL_TSO_PAGE_DATA_SIZE	(PAGE_SIZE - sizeof(struct
> > iwl_tso_page_info))
> > +#define IWL_TSO_PAGE_INFO(addr)	\
> > +	((struct iwl_tso_page_info *)(((unsigned long)addr &
> > PAGE_MASK) + \
> > +				      IWL_TSO_PAGE_DATA_SIZE))
> > +
> >   int iwl_pcie_tx_init(struct iwl_trans *trans);
> >   void iwl_pcie_tx_start(struct iwl_trans *trans, u32
> > scd_base_addr);
> >   int iwl_pcie_tx_stop(struct iwl_trans *trans);
> > @@ -628,8 +644,18 @@ struct sg_table *iwl_pcie_prep_tso(struct
> > iwl_trans *trans, struct sk_buff *skb,
> >   				   struct iwl_cmd_meta *cmd_meta,
> >   				   u8 **hdr, unsigned int
> > hdr_room);
> >   
> > -void iwl_pcie_free_tso_page(struct iwl_trans *trans, struct
> > sk_buff *skb,
> > -			    struct iwl_cmd_meta *cmd_meta);
> > +void iwl_pcie_free_tso_pages(struct iwl_trans *trans, struct
> > sk_buff *skb,
> > +			     struct iwl_cmd_meta *cmd_meta);
> > +
> > +static inline dma_addr_t iwl_pcie_get_tso_page_phys(void *addr)
> > +{
> > +	dma_addr_t res;
> > +
> > +	res = IWL_TSO_PAGE_INFO(addr)->dma_addr;
> > +	res += (unsigned long)addr & ~PAGE_MASK;
> > +
> > +	return res;
> > +}
> >   
> >   static inline dma_addr_t
> >   iwl_txq_get_first_tb_dma(struct iwl_txq *txq, int idx)
> > diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
> > b/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
> > index 3dcce6a8da50..10ee2c328458 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
> > @@ -19,8 +19,10 @@ static struct page *get_workaround_page(struct
> > iwl_trans *trans,
> >   					struct sk_buff *skb)
> >   {
> >   	struct iwl_trans_pcie *trans_pcie =
> > IWL_TRANS_GET_PCIE_TRANS(trans);
> > +	struct iwl_tso_page_info *info;
> >   	struct page **page_ptr;
> >   	struct page *ret;
> > +	dma_addr_t phys;
> >   
> >   	page_ptr = (void *)((u8 *)skb->cb + trans_pcie-
> > >txqs.page_offs);
> >   
> > @@ -28,8 +30,22 @@ static struct page *get_workaround_page(struct
> > iwl_trans *trans,
> >   	if (!ret)
> >   		return NULL;
> >   
> > +	info = IWL_TSO_PAGE_INFO(page_address(ret));
> > +
> > +	/* Create a DMA mapping for the page */
> > +	phys = dma_map_page_attrs(trans->dev, ret, 0, PAGE_SIZE,
> > +				  DMA_TO_DEVICE,
> > DMA_ATTR_SKIP_CPU_SYNC);
> > +	if (unlikely(dma_mapping_error(trans->dev, phys))) {
> > +		__free_page(ret);
> > +		return NULL;
> > +	}
> > +
> > +	/* Store physical address and set use count */
> > +	info->dma_addr = phys;
> > +	refcount_set(&info->use_count, 1);
> > +
> >   	/* set the chaining pointer to the previous page if there
> > */
> > -	*(void **)((u8 *)page_address(ret) + PAGE_SIZE -
> > sizeof(void *)) = *page_ptr;
> > +	info->next = *page_ptr;
> >   	*page_ptr = ret;
> >   
> >   	return ret;
> > @@ -76,7 +92,7 @@ static int iwl_txq_gen2_set_tb_with_wa(struct
> > iwl_trans *trans,
> >   	 * a new mapping for it so the device will not fail.
> >   	 */
> >   
> > -	if (WARN_ON(len > PAGE_SIZE - sizeof(void *))) {
> > +	if (WARN_ON(len > IWL_TSO_PAGE_DATA_SIZE)) {
> >   		ret = -ENOBUFS;
> >   		goto unmap;
> >   	}
> > @@ -782,7 +798,7 @@ static void iwl_txq_gen2_unmap(struct iwl_trans
> > *trans, int txq_id)
> >   			struct sk_buff *skb = txq-
> > >entries[idx].skb;
> >   
> >   			if (!WARN_ON_ONCE(!skb))
> > -				iwl_pcie_free_tso_page(trans, skb,
> > cmd_meta);
> > +				iwl_pcie_free_tso_pages(trans,
> > skb, cmd_meta);
> >   		}
> >   		iwl_txq_gen2_free_tfd(trans, txq);
> >   		txq->read_ptr = iwl_txq_inc_wrap(trans, txq-
> > >read_ptr);
> > diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/tx.c
> > b/drivers/net/wireless/intel/iwlwifi/pcie/tx.c
> > index ac545a39ad2a..e00d85866de9 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/pcie/tx.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/pcie/tx.c
> > @@ -209,8 +209,22 @@ static void
> > iwl_pcie_clear_cmd_in_flight(struct iwl_trans *trans)
> >   	spin_unlock(&trans_pcie->reg_lock);
> >   }
> >   
> > -void iwl_pcie_free_tso_page(struct iwl_trans *trans, struct
> > sk_buff *skb,
> > -			    struct iwl_cmd_meta *cmd_meta)
> > +static void iwl_pcie_free_and_unmap_tso_page(struct iwl_trans
> > *trans,
> > +					     struct page *page)
> > +{
> > +	struct iwl_tso_page_info *info =
> > IWL_TSO_PAGE_INFO(page_address(page));
> > +
> > +	/* Decrease internal use count and unmap/free page if
> > needed */
> > +	if (refcount_dec_and_test(&info->use_count)) {
> > +		dma_unmap_page(trans->dev, info->dma_addr,
> > PAGE_SIZE,
> > +			       DMA_TO_DEVICE);
> > +
> > +		__free_page(page);
> > +	}
> > +}
> > +
> > +void iwl_pcie_free_tso_pages(struct iwl_trans *trans, struct
> > sk_buff *skb,
> > +			     struct iwl_cmd_meta *cmd_meta)
> >   {
> >   	struct iwl_trans_pcie *trans_pcie =
> > IWL_TRANS_GET_PCIE_TRANS(trans);
> >   	struct page **page_ptr;
> > @@ -221,10 +235,11 @@ void iwl_pcie_free_tso_page(struct iwl_trans
> > *trans, struct sk_buff *skb,
> >   	*page_ptr = NULL;
> >   
> >   	while (next) {
> > +		struct iwl_tso_page_info *info;
> >   		struct page *tmp = next;
> >   
> > -		next = *(void **)((u8 *)page_address(next) +
> > PAGE_SIZE -
> > -				  sizeof(void *));
> > +		info = IWL_TSO_PAGE_INFO(page_address(next));
> > +		next = info->next;
> >   
> >   		/* Unmap the scatter gather list that is on the
> > last page */
> >   		if (!next && cmd_meta->sg_offset) {
> > @@ -236,7 +251,7 @@ void iwl_pcie_free_tso_page(struct iwl_trans
> > *trans, struct sk_buff *skb,
> >   			dma_unmap_sgtable(trans->dev, sgt,
> > DMA_TO_DEVICE, 0);
> >   		}
> >   
> > -		__free_page(tmp);
> > +		iwl_pcie_free_and_unmap_tso_page(trans, tmp);
> >   	}
> >   }
> >   
> > @@ -381,7 +396,7 @@ static void iwl_pcie_txq_unmap(struct iwl_trans
> > *trans, int txq_id)
> >   			if (WARN_ON_ONCE(!skb))
> >   				continue;
> >   
> > -			iwl_pcie_free_tso_page(trans, skb,
> > cmd_meta);
> > +			iwl_pcie_free_tso_pages(trans, skb,
> > cmd_meta);
> >   		}
> >   		iwl_txq_free_tfd(trans, txq);
> >   		txq->read_ptr = iwl_txq_inc_wrap(trans, txq-
> > >read_ptr);
> > @@ -1722,7 +1737,9 @@ static void *iwl_pcie_get_page_hdr(struct
> > iwl_trans *trans,
> >   {
> >   	struct iwl_trans_pcie *trans_pcie =
> > IWL_TRANS_GET_PCIE_TRANS(trans);
> >   	struct iwl_tso_hdr_page *p = this_cpu_ptr(trans_pcie-
> > >txqs.tso_hdr_page);
> > +	struct iwl_tso_page_info *info;
> >   	struct page **page_ptr;
> > +	dma_addr_t phys;
> >   	void *ret;
> >   
> >   	page_ptr = (void *)((u8 *)skb->cb + trans_pcie-
> > >txqs.page_offs);
> > @@ -1743,23 +1760,42 @@ static void *iwl_pcie_get_page_hdr(struct
> > iwl_trans *trans,
> >   	 *
> >   	 * (see also get_workaround_page() in tx-gen2.c)
> >   	 */
> > -	if (p->pos + len < (u8 *)page_address(p->page) + PAGE_SIZE
> > -
> > -			   sizeof(void *))
> > +	if (((unsigned long)p->pos & ~PAGE_MASK) + len <
> > IWL_TSO_PAGE_DATA_SIZE) {
> > +		info = IWL_TSO_PAGE_INFO(page_address(ret));
> >   		goto out;
> > +	}
> >   
> >   	/* We don't have enough room on this page, get a new one.
> > */
> > -	__free_page(p->page);
> > +	iwl_pcie_free_and_unmap_tso_page(trans, p->page);
> >   
> >   alloc:
> >   	p->page = alloc_page(GFP_ATOMIC);
> >   	if (!p->page)
> >   		return NULL;
> >   	p->pos = page_address(p->page);
> > +
> > +	info = IWL_TSO_PAGE_INFO(page_address(ret));
> > +
> >   	/* set the chaining pointer to NULL */
> > -	*(void **)((u8 *)page_address(p->page) + PAGE_SIZE -
> > sizeof(void *)) = NULL;
> > +	info->next = NULL;
> > +
> > +	/* Create a DMA mapping for the page */
> > +	phys = dma_map_page_attrs(trans->dev, p->page, 0,
> > PAGE_SIZE,
> > +				  DMA_TO_DEVICE,
> > DMA_ATTR_SKIP_CPU_SYNC);
> > +	if (unlikely(dma_mapping_error(trans->dev, phys))) {
> > +		__free_page(p->page);
> > +		p->page = NULL;
> > +
> > +		return NULL;
> > +	}
> > +
> > +	/* Store physical address and set use count */
> > +	info->dma_addr = phys;
> > +	refcount_set(&info->use_count, 1);
> >   out:
> >   	*page_ptr = p->page;
> > -	get_page(p->page);
> > +	/* Return an internal reference for the caller */
> > +	refcount_inc(&info->use_count);
> >   	ret = p->pos;
> >   	p->pos += len;
> >   
> > @@ -2330,7 +2366,7 @@ void iwl_pcie_reclaim(struct iwl_trans
> > *trans, int txq_id, int ssn,
> >   			      read_ptr, txq->read_ptr, txq_id))
> >   			continue;
> >   
> > -		iwl_pcie_free_tso_page(trans, skb, cmd_meta);
> > +		iwl_pcie_free_tso_pages(trans, skb, cmd_meta);
> >   
> >   		__skb_queue_tail(skbs, skb);
> >   
> 

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928