Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753125Ab2JRAbV (ORCPT ); Wed, 17 Oct 2012 20:31:21 -0400 Received: from mail-ea0-f174.google.com ([209.85.215.174]:42324 "EHLO mail-ea0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752774Ab2JRAbU (ORCPT ); Wed, 17 Oct 2012 20:31:20 -0400 MIME-Version: 1.0 In-Reply-To: <1350508512-15562-2-git-send-email-mark.einon@gmail.com> References: <1350508512-15562-1-git-send-email-mark.einon@gmail.com> <1350508512-15562-2-git-send-email-mark.einon@gmail.com> Date: Wed, 17 Oct 2012 20:31:19 -0400 Message-ID: Subject: Re: [PATCH 2/2] staging: et131x: Fix 64bit tx dma address handling From: Jeffrey Ladouceur To: Mark Einon Cc: devel@driverdev.osuosl.org, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9989 Lines: 186 On Wed, Oct 17, 2012 at 5:15 PM, Mark Einon wrote: > The driver checks that the device can handle 64bit DMA addressing in > et131x_pci_setup(), but then assumes that the top dword of a tx dma > address is always zero when creating a dma mapping in nic_send_packet(). > Fix the mapping to use the higher dword of the dma_addr_t returned by > dma_map_single() and skb_frag_dma_map(). > > Also remove incorrect comments stating that dma_map_single() only returns > a 32 bit address. > > Signed-off-by: Mark Einon > --- > drivers/staging/et131x/et131x.c | 102 +++++++++++++++------------------------ > 1 file changed, 39 insertions(+), 63 deletions(-) > > diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c > index f0fe4bd..83643be 100644 > --- a/drivers/staging/et131x/et131x.c > +++ b/drivers/staging/et131x/et131x.c > @@ -3285,6 +3285,7 @@ static int nic_send_packet(struct et131x_adapter *adapter, struct tcb *tcb) > struct skb_frag_struct *frags = &skb_shinfo(skb)->frags[0]; > unsigned long flags; > struct phy_device *phydev = adapter->phydev; > + dma_addr_t dma_addr; > > /* Part of the optimizations of this send routine restrict us to > * sending 24 fragments at a pass. In practice we should never see > @@ -3314,77 +3315,46 @@ static int nic_send_packet(struct et131x_adapter *adapter, struct tcb *tcb) > * doesn't seem to like large fragments. > */ > if (skb_headlen(skb) <= 1514) { > - desc[frag].addr_hi = 0; > /* Low 16bits are length, high is vlan and > unused currently so zero */ > desc[frag].len_vlan = skb_headlen(skb); > - > - /* NOTE: Here, the dma_addr_t returned from > - * dma_map_single() is implicitly cast as a > - * u32. Although dma_addr_t can be > - * 64-bit, the address returned by > - * dma_map_single() is always 32-bit > - * addressable (as defined by the pci/dma > - * subsystem) > - */ > - desc[frag++].addr_lo = > - dma_map_single(&adapter->pdev->dev, > - skb->data, > - skb_headlen(skb), > - DMA_TO_DEVICE); > + dma_addr = dma_map_single(&adapter->pdev->dev, > + skb->data, > + skb_headlen(skb), > + DMA_TO_DEVICE); > + desc[frag].addr_lo = dma_addr & 0xFFFFFFFF; > + desc[frag].addr_hi = dma_addr >> 32; Maybe use macros defined in kernel.h instead: desc[frag].addr_lo = lower_32_bits(dma_addr); desc[frag].addr_hi = upper_32_bits(dma_addr); A few more instances below. > + frag++; > } else { > - desc[frag].addr_hi = 0; > desc[frag].len_vlan = skb_headlen(skb) / 2; > - > - /* NOTE: Here, the dma_addr_t returned from > - * dma_map_single() is implicitly cast as a > - * u32. Although dma_addr_t can be > - * 64-bit, the address returned by > - * dma_map_single() is always 32-bit > - * addressable (as defined by the pci/dma > - * subsystem) > - */ > - desc[frag++].addr_lo = > - dma_map_single(&adapter->pdev->dev, > - skb->data, > - (skb_headlen(skb) / 2), > - DMA_TO_DEVICE); > - desc[frag].addr_hi = 0; > + dma_addr = dma_map_single(&adapter->pdev->dev, > + skb->data, > + (skb_headlen(skb) / 2), > + DMA_TO_DEVICE); > + desc[frag].addr_lo = dma_addr & 0xFFFFFFFF; > + desc[frag].addr_hi = dma_addr >> 32; > + frag++; > > desc[frag].len_vlan = skb_headlen(skb) / 2; > - > - /* NOTE: Here, the dma_addr_t returned from > - * dma_map_single() is implicitly cast as a > - * u32. Although dma_addr_t can be > - * 64-bit, the address returned by > - * dma_map_single() is always 32-bit > - * addressable (as defined by the pci/dma > - * subsystem) > - */ > - desc[frag++].addr_lo = > - dma_map_single(&adapter->pdev->dev, > - skb->data + > - (skb_headlen(skb) / 2), > - (skb_headlen(skb) / 2), > - DMA_TO_DEVICE); > + dma_addr = dma_map_single(&adapter->pdev->dev, > + skb->data + > + (skb_headlen(skb) / 2), > + (skb_headlen(skb) / 2), > + DMA_TO_DEVICE); > + desc[frag].addr_lo = dma_addr & 0xFFFFFFFF; > + desc[frag].addr_hi = dma_addr >> 32; > + frag++; > } > } else { > - desc[frag].addr_hi = 0; > - desc[frag].len_vlan = > - frags[i - 1].size; > - > - /* NOTE: Here, the dma_addr_t returned from > - * dma_map_page() is implicitly cast as a u32. > - * Although dma_addr_t can be 64-bit, the address > - * returned by dma_map_page() is always 32-bit > - * addressable (as defined by the pci/dma subsystem) > - */ > - desc[frag++].addr_lo = skb_frag_dma_map( > - &adapter->pdev->dev, > - &frags[i - 1], > - 0, > - frags[i - 1].size, > - DMA_TO_DEVICE); > + desc[frag].len_vlan = frags[i - 1].size; > + dma_addr = skb_frag_dma_map(&adapter->pdev->dev, > + &frags[i - 1], > + 0, > + frags[i - 1].size, > + DMA_TO_DEVICE); > + desc[frag].addr_lo = dma_addr & 0xFFFFFFFF; > + desc[frag].addr_hi = dma_addr >> 32; > + frag++; > } > } > > @@ -3611,6 +3581,7 @@ static inline void free_send_packet(struct et131x_adapter *adapter, > unsigned long flags; > struct tx_desc *desc = NULL; > struct net_device_stats *stats = &adapter->net_stats; > + dma_addr_t dma_addr; > > if (tcb->flags & fMP_DEST_BROAD) > atomic_inc(&adapter->stats.broadcast_pkts_xmtd); > @@ -3631,8 +3602,13 @@ static inline void free_send_packet(struct et131x_adapter *adapter, > (adapter->tx_ring.tx_desc_ring + > INDEX10(tcb->index_start)); > > + dma_addr = desc->addr_lo; > + > + if (sizeof(dma_addr_t) == sizeof(u64)) > + dma_addr += ((dma_addr_t)desc->addr_hi) << 32; Probably end result the same, but don't see + too often. Not usually the way we think of it. dma_addr |= (dma_addr_t)desc->addr_hi << 32; > + > dma_unmap_single(&adapter->pdev->dev, > - desc->addr_lo, > + dma_addr, > desc->len_vlan, DMA_TO_DEVICE); > > add_10bit(&tcb->index_start, 1); > -- > 1.7.9.5 > > _______________________________________________ > devel mailing list > devel@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/devel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/