Patchwork [net-next] atl1e:fix 2.6.31-git4 -- ATL1E 0000:03:00.0: DMA-API: device driver frees DMA

login
register
mail settings
Submitter jie.yang@atheros.com
Date Sept. 16, 2009, 6:21 a.m.
Message ID <12530821133392-git-send-email-jie.yang@atheros.com>
Download mbox | patch
Permalink /patch/33686/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

jie.yang@atheros.com - Sept. 16, 2009, 6:21 a.m.
[   25.059969] WARNING: at lib/dma-debug.c:816 check_unmap+0x383/0x55c()
[   25.059976] Hardware name: 1000HE
[   25.059984] ATL1E 0000:03:00.0: DMA-API: device driver frees DMA
    memory with wrong function [device address=0x0000000036b92802] 
    [size=90 bytes] [mapped as single] [unmapped as page]

use the wrong API when free dma. So when map dma use a flag to demostrate
whether it is 'pci_map_single' or 'pci_map_page'. When free the dma, check
the flags to select the right APIs('pci_unmap_single' or 'pci_unmap_page').
 
Signed-off-by: Jie Yang <jie.yang@atheros.com>
---

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Sept. 16, 2009, 6:57 a.m.
From: <jie.yang@atheros.com>
Date: Wed, 16 Sep 2009 14:21:53 +0800

> 
> [   25.059969] WARNING: at lib/dma-debug.c:816 check_unmap+0x383/0x55c()
> [   25.059976] Hardware name: 1000HE
> [   25.059984] ATL1E 0000:03:00.0: DMA-API: device driver frees DMA
>     memory with wrong function [device address=0x0000000036b92802] 
>     [size=90 bytes] [mapped as single] [unmapped as page]
> 
> use the wrong API when free dma. So when map dma use a flag to demostrate
> whether it is 'pci_map_single' or 'pci_map_page'. When free the dma, check
> the flags to select the right APIs('pci_unmap_single' or 'pci_unmap_page').
>  
> Signed-off-by: Jie Yang <jie.yang@atheros.com>

An 'unsigned long' is an enormous type to use just to store
3 bits of information.  Use a u16 or similar to compact the
size of struct atl1e_tx_buffer

Also, it looks terribly ugle to define the flag macros, some with
three leading zeros in the hexadecimal values and one without.

Please make them consistent, thanks.

Generally, I see that this change was put together very hastily
and without very much care.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Walker - Sept. 16, 2009, 9:10 a.m.
On Wed, 2009-09-16 at 14:21 +0800, jie.yang@atheros.com wrote:
> -           pci_unmap_page(pdev, tx_buffer->dma,
> +           if (tx_buffer->flags & ATL1E_TX_PCIMAP_SINGLE)
> +               pci_unmap_single(pdev, tx_buffer->dma,
> +                   tx_buffer->length, PCI_DMA_TODEVICE);
> +           else if (tx_buffer->flags & ATL1E_TX_PCIMAP_PAGE)
> +               pci_unmap_page(pdev, tx_buffer->dma,

Could you run this through checkpatch, and fix any errors if find? It
looks like you uses spaces for tabs in the code above, maybe in the
other block too..

Daniel

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jie.yang@atheros.com - Sept. 16, 2009, 9:31 a.m.
On Wednesday, September 16, 2009 5:11 PM
Daniel Walker <dwalker@fifo99.com> wrote:

> On Wed, 2009-09-16 at 14:21 +0800, jie.yang@atheros.com wrote:
> > -           pci_unmap_page(pdev, tx_buffer->dma,
> > +           if (tx_buffer->flags & ATL1E_TX_PCIMAP_SINGLE)
> > +               pci_unmap_single(pdev, tx_buffer->dma,
> > +                   tx_buffer->length, PCI_DMA_TODEVICE);
> > +           else if (tx_buffer->flags & ATL1E_TX_PCIMAP_PAGE)
> > +               pci_unmap_page(pdev, tx_buffer->dma,
>
> Could you run this through checkpatch, and fix any errors if
> find? It looks like you uses spaces for tabs in the code
> above, maybe in the other block too..
>
> Daniel
>
>

ou, my mistake, just resend.

Best  wishes
jie
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Sept. 16, 2009, 9:47 a.m.
From: Daniel Walker <dwalker@fifo99.com>
Date: Wed, 16 Sep 2009 02:10:51 -0700

> On Wed, 2009-09-16 at 14:21 +0800, jie.yang@atheros.com wrote:
>> -           pci_unmap_page(pdev, tx_buffer->dma,
>> +           if (tx_buffer->flags & ATL1E_TX_PCIMAP_SINGLE)
>> +               pci_unmap_single(pdev, tx_buffer->dma,
>> +                   tx_buffer->length, PCI_DMA_TODEVICE);
>> +           else if (tx_buffer->flags & ATL1E_TX_PCIMAP_PAGE)
>> +               pci_unmap_page(pdev, tx_buffer->dma,
> 
> Could you run this through checkpatch, and fix any errors if find? It
> looks like you uses spaces for tabs in the code above, maybe in the
> other block too..
> 

It's because of his email client, it corrupted the whole patch
like that.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Cyrill Gorcunov - Sept. 16, 2009, 9:51 a.m.
[David Miller - Wed, Sep 16, 2009 at 02:47:59AM -0700]
| From: Daniel Walker <dwalker@fifo99.com>
| Date: Wed, 16 Sep 2009 02:10:51 -0700
| 
| > On Wed, 2009-09-16 at 14:21 +0800, jie.yang@atheros.com wrote:
| >> -           pci_unmap_page(pdev, tx_buffer->dma,
| >> +           if (tx_buffer->flags & ATL1E_TX_PCIMAP_SINGLE)
| >> +               pci_unmap_single(pdev, tx_buffer->dma,
| >> +                   tx_buffer->length, PCI_DMA_TODEVICE);
| >> +           else if (tx_buffer->flags & ATL1E_TX_PCIMAP_PAGE)
| >> +               pci_unmap_page(pdev, tx_buffer->dma,
| > 
| > Could you run this through checkpatch, and fix any errors if find? It
| > looks like you uses spaces for tabs in the code above, maybe in the
| > other block too..
| > 
| 
| It's because of his email client, it corrupted the whole patch
| like that.

well, client seems to be

| X-Mailer: git-send-email 1.5.2.2

more probably the text editor is the reason.

	-- Cyrill
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jie.yang@atheros.com - Sept. 16, 2009, 9:54 a.m.
On Wednesday, September 16, 2009 5:52 PM
Cyrill Gorcunov <gorcunov@gmail.com>  wrote:

> [David Miller - Wed, Sep 16, 2009 at 02:47:59AM -0700]
> | From: Daniel Walker <dwalker@fifo99.com>
> | Date: Wed, 16 Sep 2009 02:10:51 -0700
> |
> | > On Wed, 2009-09-16 at 14:21 +0800, jie.yang@atheros.com wrote:
> | >> -           pci_unmap_page(pdev, tx_buffer->dma,
> | >> +           if (tx_buffer->flags & ATL1E_TX_PCIMAP_SINGLE)
> | >> +               pci_unmap_single(pdev, tx_buffer->dma,
> | >> +                   tx_buffer->length, PCI_DMA_TODEVICE);
> | >> +           else if (tx_buffer->flags & ATL1E_TX_PCIMAP_PAGE)
> | >> +               pci_unmap_page(pdev, tx_buffer->dma,
> | >
> | > Could you run this through checkpatch, and fix any errors
> if find?
> | > It looks like you uses spaces for tabs in the code above,
> maybe in
> | > the other block too..
> | >
> |
> | It's because of his email client, it corrupted the whole patch like
> | that.
>
> well, client seems to be
>
> | X-Mailer: git-send-email 1.5.2.2
>
> more probably the text editor is the reason.
>
>         -- Cyrill
>

yes, the editor 'vi' have configed "set expandtab".
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/atl1e/atl1e.h b/drivers/net/atl1e/atl1e.h
index ba48220..d63be5c 100644
--- a/drivers/net/atl1e/atl1e.h
+++ b/drivers/net/atl1e/atl1e.h
@@ -377,10 +377,19 @@  struct atl1e_hw {
  */
 struct atl1e_tx_buffer {
    struct sk_buff *skb;
+   unsigned long flags;
+#define ATL1E_TX_PCIMAP_SINGLE     0x0001
+#define ATL1E_TX_PCIMAP_PAGE       0x0002
+#define ATL1E_TX_PCIMAP_TYPE_MASK  0x3
    u16 length;
    dma_addr_t dma;
 };
 
+#define ATL1E_SET_PCIMAP_TYPE(tx_buff, type) do {      \
+   ((tx_buff)->flags) &= ~ATL1E_TX_PCIMAP_TYPE_MASK;   \
+   ((tx_buff)->flags) |= (type);               \
+   } while (0)
+
 struct atl1e_rx_page {
    dma_addr_t  dma;    /* receive rage DMA address */
    u8      *addr;   /* receive rage virtual address */
diff --git a/drivers/net/atl1e/atl1e_main.c b/drivers/net/atl1e/atl1e_main.c
index 69b830f..955da73 100644
--- a/drivers/net/atl1e/atl1e_main.c
+++ b/drivers/net/atl1e/atl1e_main.c
@@ -635,7 +635,11 @@  static void atl1e_clean_tx_ring(struct atl1e_adapter *adapter)
    for (index = 0; index < ring_count; index++) {
        tx_buffer = &tx_ring->tx_buffer[index];
        if (tx_buffer->dma) {
-           pci_unmap_page(pdev, tx_buffer->dma,
+           if (tx_buffer->flags & ATL1E_TX_PCIMAP_SINGLE)
+               pci_unmap_single(pdev, tx_buffer->dma,
+                   tx_buffer->length, PCI_DMA_TODEVICE);
+           else if (tx_buffer->flags & ATL1E_TX_PCIMAP_PAGE)
+               pci_unmap_page(pdev, tx_buffer->dma,
                    tx_buffer->length, PCI_DMA_TODEVICE);
            tx_buffer->dma = 0;
        }
@@ -1220,7 +1224,11 @@  static bool atl1e_clean_tx_irq(struct atl1e_adapter *adapter)
    while (next_to_clean != hw_next_to_clean) {
        tx_buffer = &tx_ring->tx_buffer[next_to_clean];
        if (tx_buffer->dma) {
-           pci_unmap_page(adapter->pdev, tx_buffer->dma,
+           if (tx_buffer->flags & ATL1E_TX_PCIMAP_SINGLE)
+               pci_unmap_single(adapter->pdev, tx_buffer->dma,
+                   tx_buffer->length, PCI_DMA_TODEVICE);
+           else if (tx_buffer->flags & ATL1E_TX_PCIMAP_PAGE)
+               pci_unmap_page(adapter->pdev, tx_buffer->dma,
                    tx_buffer->length, PCI_DMA_TODEVICE);
            tx_buffer->dma = 0;
        }
@@ -1741,6 +1749,7 @@  static void atl1e_tx_map(struct atl1e_adapter *adapter,
        tx_buffer->length = map_len;
        tx_buffer->dma = pci_map_single(adapter->pdev,
                    skb->data, hdr_len, PCI_DMA_TODEVICE);
+       ATL1E_SET_PCIMAP_TYPE(tx_buffer, ATL1E_TX_PCIMAP_SINGLE);
        mapped_len += map_len;
        use_tpd->buffer_addr = cpu_to_le64(tx_buffer->dma);
        use_tpd->word2 = (use_tpd->word2 & (~TPD_BUFLEN_MASK)) |
@@ -1766,6 +1775,7 @@  static void atl1e_tx_map(struct atl1e_adapter *adapter,
        tx_buffer->dma =
            pci_map_single(adapter->pdev, skb->data + mapped_len,
                    map_len, PCI_DMA_TODEVICE);
+       ATL1E_SET_PCIMAP_TYPE(tx_buffer, ATL1E_TX_PCIMAP_SINGLE);
        mapped_len  += map_len;
        use_tpd->buffer_addr = cpu_to_le64(tx_buffer->dma);
        use_tpd->word2 = (use_tpd->word2 & (~TPD_BUFLEN_MASK)) |
@@ -1801,6 +1811,7 @@  static void atl1e_tx_map(struct atl1e_adapter *adapter,
                        (i * MAX_TX_BUF_LEN),
                        tx_buffer->length,
                        PCI_DMA_TODEVICE);
+           ATL1E_SET_PCIMAP_TYPE(tx_buffer, ATL1E_TX_PCIMAP_PAGE);
            use_tpd->buffer_addr = cpu_to_le64(tx_buffer->dma);
            use_tpd->word2 = (use_tpd->word2 & (~TPD_BUFLEN_MASK)) |
                    ((cpu_to_le32(tx_buffer->length) &