Re: kernel BUG at drivers/net/wireless/iwlwifi/iwl3945-base.c:3127!

[Jason Andryuk <jandryuk@gmail.com>]

On Sun, 2009-03-22 at 13:29 -0400, Jason Andryuk wrote:
> The patch did not work.
> 
> However, I do not think it's an alignment issue.
> 
> pci_map_single on x86_64 with >3G memory and no IOMMU uses the swiotlb
> by default.  swiotlb provides "bounce buffers" for physical addresses
> over 4GB.  So for memory where the kmalloc value is at a physical
> address over 2**32, pci_map_single will copy to a swiotlb slot.  That
> means the dma_handle return value of pci_map_single is not necessarily
> the physical address of the virtual address.
> 
> In iwl3945_tx_skb (iwl3945-base.c line 1099) use the pci_map_single
> which copies the memory of out_cmd at that time.  However,
> iwl3945_build_tx_cmd_basic, iwl3945_hw_build_tx_cmd_rate, and
> iwl3945_build_tx_cmd_hwcrypto are all passed out_cmd and modify memory
> located there.  These modifications are not reflected in the memory
> located at txcmd_phys.
> 
> pci_map_single should only be called once the tx command structures
> are completely written.
> 
> Additionally, pci_unmap_single should be called once the memory is no
> longer needed to free the swiotlb entry for that command.  I am not
> sure if this is currently handled for all cases.
> 
> Previous use of pci_alloc_coherent meant that memory would allocated
> at an physical address below 4GB and kept in-sync both the device and
> cpu.
> 
> Refer to Documentation/x86/x86_64/boot-options.txt and lib/swiotlb.c
> for information on IOMMU and swiotlb
> 
> Jason

I hacked up an inelegant patch that does get the driver working.

We call pci_dma_sync_single_for_device to ensure the correct contents
are visible to the device.  That is, we copy out_cmd to phys_addr, so
the desired contents are viewable for the device.

I tried moving the pci_map_single call to right before the call to
iwl_txq_update_write_ptr, but I never got that working successfully.
Presumably that is how the problem should be fixed.  Another failed
attempt was to map from "out_cmd + offsetof(struck iwl_cmd, hdr)"
instead of out_cmd.  The phys_addr + offsetof(struck iwl_cmd, hdr) is
what is passed to txq_attach_buf_to_tfd, so I thought that should work.
Unfortunately it did not.  The added benefit would have been that it
moved "len" and "mapping" of struct iwl_cmd_meta out of the mapped area.
Then losing them since they were written after the mapping would not
have been a problem. 

If struct iwl_cmd_meta's len and mapping members are read by one of the
"reclaim" functions, then they should probably be sync'ed with
pci_dma_sync_single_for_cpu beforehand.

Jason

diff --git a/drivers/net/wireless/iwlwifi/iwl-tx.c b/drivers/net/wireless/iwlwifi/iwl-tx.c
index b13862a..8ec26c7 100644
--- a/drivers/net/wireless/iwlwifi/iwl-tx.c
+++ b/drivers/net/wireless/iwlwifi/iwl-tx.c
@@ -975,6 +975,9 @@ int iwl_enqueue_hcmd(struct iwl_priv *priv, struct iwl_host_cmd *cmd)
 	pci_unmap_len_set(&out_cmd->meta, len, len);
 	phys_addr += offsetof(struct iwl_cmd, hdr);
 
+	pci_dma_sync_single_for_device(priv->pci_dev, phys_addr,
+				   len, PCI_DMA_BIDIRECTIONAL);
+
 	priv->cfg->ops->lib->txq_attach_buf_to_tfd(priv, txq,
 						   phys_addr, fix_size, 1,
 						   U32_PAD(cmd->len));
diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index 279d10c..a32ee4e 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -1151,6 +1151,12 @@ static int iwl3945_tx_skb(struct iwl_priv *priv, struct sk_buff *skb)
 	iwl_print_hex_dump(priv, IWL_DL_TX, (u8 *)tx->hdr,
 			   ieee80211_hdrlen(fc));
 
+	pci_dma_sync_single_for_device(priv->pci_dev,
+				    txcmd_phys, sizeof(struct iwl_cmd),
+				    PCI_DMA_TODEVICE);
+	pci_dma_sync_single_for_device(priv->pci_dev, phys_addr,
+					   skb->len - hdr_len,
+				PCI_DMA_TODEVICE);
 	/* Tell device the write index *just past* this latest filled TFD */
 	q->write_ptr = iwl_queue_inc_wrap(q->write_ptr, q->n_bd);
 	rc = iwl_txq_update_write_ptr(priv, txq);