velocity driver unmaps incorrect size.

velocity driver unmaps incorrect size.

[Dave Jones]

Seen during boot when the interface comes up..

	Dave

------------[ cut here ]------------
WARNING: at lib/dma-debug.c:505 check_unmap+0x1f8/0x4d4()
Hardware name:  
via-velocity 0000:00:0e.0: DMA-API: device driver frees DMA memory with different size [device address=0x000000001a456242] [map size=90 bytes] [unmap size=1 bytes]
Modules linked in: nf_conntrack_netbios_ns ip6t_REJECT ip6table_filter ip6_tables ipv6 cpufreq_ondemand acpi_cpufreq serio_raw pcspkr i2c_viapro 3c59x mii i2c_core usb_storage firewire_ohci via_velocity firewire_core crc_ccitt crc_itu_t ata_generic pata_acpi pata_via ext2 [last unloaded: scsi_wait_scan]
Pid: 0, comm: swapper Tainted: G        W  2.6.30-6.fc12.i686.PAE #1
Call Trace:
 [<c04434a2>] warn_slowpath_common+0x75/0x9d
 [<c05e1029>] ? check_unmap+0x1f8/0x4d4
 [<c0443533>] warn_slowpath_fmt+0x34/0x48
 [<c05e1029>] check_unmap+0x1f8/0x4d4
 [<c05e15a8>] debug_dma_unmap_page+0x71/0x8a
 [<dcf88829>] pci_unmap_single+0x74/0x90 [via_velocity]
 [<dcf88922>] velocity_tx_srv+0xdd/0x1a0 [via_velocity]
 [<dcf89e76>] velocity_intr+0x52f/0x5a1 [via_velocity]
 [<dd0c60d6>] ? ip6_output_finish+0x0/0x104 [ipv6]
 [<dd0c6172>] ? ip6_output_finish+0x9c/0x104 [ipv6]
 [<c0494938>] ? handle_fasteoi_irq+0x27/0xcf
 [<c046c8fb>] ? print_lock_contention_bug+0x1f/0xd1
 [<c0494938>] ? handle_fasteoi_irq+0x27/0xcf
 [<c0494938>] ? handle_fasteoi_irq+0x27/0xcf
 [<c0492c06>] handle_IRQ_event+0x56/0x112
 [<c0492c06>] ? handle_IRQ_event+0x56/0x112
 [<c0494997>] handle_fasteoi_irq+0x86/0xcf
 [<c040c350>] handle_irq+0x50/0x6c
 [<c040bc4f>] do_IRQ+0x52/0xa4
 [<c040a715>] common_interrupt+0x35/0x3c
 [<c0757f3f>] ? netif_receive_skb+0x1b9/0x3ca
 [<c07581de>] process_backlog+0x8e/0xcf
 [<c0756262>] net_rx_action+0xa7/0x1b9
 [<c0449064>] __do_softirq+0xaa/0x173
 [<c0449176>] do_softirq+0x49/0x7f
 [<c04492ca>] irq_exit+0x48/0x87
 [<c040bc7c>] do_IRQ+0x7f/0xa4
 [<c040a715>] common_interrupt+0x35/0x3c
 [<c046b2ff>] ? trace_hardirqs_on+0x19/0x2c
 [<c06437a7>] ? acpi_idle_enter_simple+0x11d/0x161
 [<c07316ff>] cpuidle_idle_call+0x73/0xbc
 [<c0409091>] cpu_idle+0xac/0xcb
 [<c07e024a>] rest_init+0x66/0x79
 [<c0a38acd>] start_kernel+0x36b/0x383
 [<c0a38090>] i386_start_kernel+0x7f/0x98
---[ end trace 6db0eec1553c1cdc ]---
Mapped at:
 [<c05e196f>] debug_dma_map_page+0x85/0x168
 [<dcf891fb>] pci_map_single+0x8e/0xab [via_velocity]
 [<dcf8979a>] velocity_xmit+0xae/0x1c2 [via_velocity]
 [<c0758d2d>] dev_hard_start_xmit+0x233/0x2a5
 [<c076a13e>] __qdisc_run+0xd0/0x1c5

Re: velocity driver unmaps incorrect size.

[David Miller]

From: Dave Jones <davej@redhat.com>
Date: Sun, 21 Jun 2009 13:37:45 -0400

> ------------[ cut here ]------------
> WARNING: at lib/dma-debug.c:505 check_unmap+0x1f8/0x4d4()
> Hardware name:  
> via-velocity 0000:00:0e.0: DMA-API: device driver frees DMA memory with different size [device address=0x000000001a456242] [map size=90 bytes] [unmap size=1 bytes]

Ok, bad unmap size is "1".

> Call Trace:
>  [<c04434a2>] warn_slowpath_common+0x75/0x9d
>  [<c05e1029>] ? check_unmap+0x1f8/0x4d4
>  [<c0443533>] warn_slowpath_fmt+0x34/0x48
>  [<c05e1029>] check_unmap+0x1f8/0x4d4
>  [<c05e15a8>] debug_dma_unmap_page+0x71/0x8a
>  [<dcf88829>] pci_unmap_single+0x74/0x90 [via_velocity]
>  [<dcf88922>] velocity_tx_srv+0xdd/0x1a0 [via_velocity]
>  [<dcf89e76>] velocity_intr+0x52f/0x5a1 [via_velocity]

So since this is happening in velocity_tx_srv() it has to be
velocity_free_tx_buf().  It has two cases, one for when
VELOCITY_ZERO_COPY_SUPPORT is defined and one for when that
is not defined.

There is no way to set that define that I can see in the
tree, so we only need to consider the case where this
macro is not defined:

static void velocity_free_tx_buf(struct velocity_info *vptr, struct velocity_td_info *tdinfo)
{
 ...
	if (tdinfo->skb_dma) {
		pktlen = (skb->len > ETH_ZLEN ? : ETH_ZLEN);
		for (i = 0; i < tdinfo->nskb_dma; i++) {
			pci_unmap_single(vptr->pdev, tdinfo->skb_dma[i], pktlen, PCI_DMA_TODEVICE);
			tdinfo->skb_dma[i] = 0;
		}
	}
 ...
}

It seems to me that it's impossible for 'pktlen' to every be
'1' here as the DMA debug code is claiming.  It must always
be at least ETH_ZLEN.  And that is what is passed in for the
unmap length.

David is there something wonky in your build or do you have
any local patches applied?  Is it possible that for some reason
your build is forcing VELOCITY_ZERO_COPY_SUPPORT to be defined
for some reason?

Thanks.

Re: velocity driver unmaps incorrect size.

[Dave Jones]

On Sun, Jun 21, 2009 at 06:43:45PM -0700, David Miller wrote:

 > > ------------[ cut here ]------------
 > > WARNING: at lib/dma-debug.c:505 check_unmap+0x1f8/0x4d4()
 > > Hardware name:  
 > > via-velocity 0000:00:0e.0: DMA-API: device driver frees DMA memory with different size [device address=0x000000001a456242] [map size=90 bytes] [unmap size=1 bytes]
 > 
 > Ok, bad unmap size is "1".
 > 
 > >  [<c05e1029>] ? check_unmap+0x1f8/0x4d4
 > >  [<c0443533>] warn_slowpath_fmt+0x34/0x48
 > >  [<c05e1029>] check_unmap+0x1f8/0x4d4
 > >  [<c05e15a8>] debug_dma_unmap_page+0x71/0x8a
 > >  [<dcf88829>] pci_unmap_single+0x74/0x90 [via_velocity]
 > >  [<dcf88922>] velocity_tx_srv+0xdd/0x1a0 [via_velocity]
 > >  [<dcf89e76>] velocity_intr+0x52f/0x5a1 [via_velocity]
 > 
 > So since this is happening in velocity_tx_srv() it has to be
 > velocity_free_tx_buf().  It has two cases, one for when
 > VELOCITY_ZERO_COPY_SUPPORT is defined and one for when that
 > is not defined.
 > 
 > There is no way to set that define that I can see in the
 > tree, so we only need to consider the case where this
 > macro is not defined:
 > 
 > static void velocity_free_tx_buf(struct velocity_info *vptr, struct velocity_td_info *tdinfo)
 > {
 >  ...
 > 	if (tdinfo->skb_dma) {
 > 		pktlen = (skb->len > ETH_ZLEN ? : ETH_ZLEN);
 > 		for (i = 0; i < tdinfo->nskb_dma; i++) {
 > 			pci_unmap_single(vptr->pdev, tdinfo->skb_dma[i], pktlen, PCI_DMA_TODEVICE);
 > 			tdinfo->skb_dma[i] = 0;
 > 		}
 > 	}
 >  ...
 > }
 > 
 > It seems to me that it's impossible for 'pktlen' to every be
 > '1' here as the DMA debug code is claiming.  It must always
 > be at least ETH_ZLEN.  And that is what is passed in for the
 > unmap length.
 > 
 > David is there something wonky in your build or do you have
 > any local patches applied?

Nothing obvious that could explain it.
(It's the Fedora 11 kernel, which has no patches to this driver,
 nor to net/)

 > Is it possible that for some reason
 > your build is forcing VELOCITY_ZERO_COPY_SUPPORT to be defined
 > for some reason?

Memory corruption maybe?
It's especially odd in that it only happens once during boot,
and never happens again. This is my firewall/router, so there's
more packets going through that box than any other I have.


btw, given the zerocopy stuff has been disabled for so long,
is it worth keeping it ?

	Dave

Re: velocity driver unmaps incorrect size.

[David Miller]

From: Dave Jones <davej@redhat.com>
Date: Sun, 21 Jun 2009 22:40:44 -0400

> On Sun, Jun 21, 2009 at 06:43:45PM -0700, David Miller wrote:
> 
>  > Is it possible that for some reason
>  > your build is forcing VELOCITY_ZERO_COPY_SUPPORT to be defined
>  > for some reason?
> 
> Memory corruption maybe?
> It's especially odd in that it only happens once during boot,
> and never happens again. This is my firewall/router, so there's
> more packets going through that box than any other I have.

I would not be inclined to this theory.  Regardless of what garbage is
read from skb->len, it's always going to be at least ETH_ZLEN due to
the "max of skb->len and ETH_ZLEN" assignment there.

> btw, given the zerocopy stuff has been disabled for so long,
> is it worth keeping it ?

I'm pretty sure it should be tossed.  I wonder if anyone even tries
to build that code these days.

Re: velocity driver unmaps incorrect size.

[Dave Jones]

On Sun, Jun 21, 2009 at 07:42:48PM -0700, David Miller wrote:
 > From: Dave Jones <davej@redhat.com>
 > Date: Sun, 21 Jun 2009 22:40:44 -0400
 > 
 > > On Sun, Jun 21, 2009 at 06:43:45PM -0700, David Miller wrote:
 > > 
 > >  > Is it possible that for some reason
 > >  > your build is forcing VELOCITY_ZERO_COPY_SUPPORT to be defined
 > >  > for some reason?
 > > 
 > > Memory corruption maybe?
 > > It's especially odd in that it only happens once during boot,
 > > and never happens again. This is my firewall/router, so there's
 > > more packets going through that box than any other I have.
 > 
 > I would not be inclined to this theory.  Regardless of what garbage is
 > read from skb->len, it's always going to be at least ETH_ZLEN due to
 > the "max of skb->len and ETH_ZLEN" assignment there.

Yeah, seems unlikely.  Compiler bug seems unlikely too, given it's a one-off.
I'll see if I can come up with some other debug patches to figure out what's
going on in there.

 > > btw, given the zerocopy stuff has been disabled for so long,
 > > is it worth keeping it ?
 > 
 > I'm pretty sure it should be tossed.  I wonder if anyone even tries
 > to build that code these days.



Remove unused Velocity zero copy code.

This code hasn't been enabled in years.

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
index b02f7ad..faae32c 100644
--- a/drivers/net/via-velocity.c
+++ b/drivers/net/via-velocity.c
@@ -976,9 +976,6 @@ static int __devinit velocity_found1(struct pci_dev *pdev, const struct pci_devi
 	dev->netdev_ops = &velocity_netdev_ops;
 	dev->ethtool_ops = &velocity_ethtool_ops;
 
-#ifdef  VELOCITY_ZERO_COPY_SUPPORT
-	dev->features |= NETIF_F_SG;
-#endif
 	dev->features |= NETIF_F_HW_VLAN_TX | NETIF_F_HW_VLAN_FILTER |
 		NETIF_F_HW_VLAN_RX;
 
@@ -1849,11 +1846,7 @@ static void velocity_free_tx_buf(struct velocity_info *vptr, struct velocity_td_
 
 		pktlen = (skb->len > ETH_ZLEN ? : ETH_ZLEN);
 		for (i = 0; i < tdinfo->nskb_dma; i++) {
-#ifdef VELOCITY_ZERO_COPY_SUPPORT
-			pci_unmap_single(vptr->pdev, tdinfo->skb_dma[i], le16_to_cpu(td->tdesc1.len), PCI_DMA_TODEVICE);
-#else
 			pci_unmap_single(vptr->pdev, tdinfo->skb_dma[i], pktlen, PCI_DMA_TODEVICE);
-#endif
 			tdinfo->skb_dma[i] = 0;
 		}
 	}
@@ -2095,13 +2088,6 @@ static int velocity_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	len = cpu_to_le16(pktlen);
 
-#ifdef VELOCITY_ZERO_COPY_SUPPORT
-	if (skb_shinfo(skb)->nr_frags > 6 && __skb_linearize(skb)) {
-		kfree_skb(skb);
-		return 0;
-	}
-#endif
-
 	spin_lock_irqsave(&vptr->lock, flags);
 
 	index = vptr->tx.curr[qnum];
@@ -2111,59 +2097,18 @@ static int velocity_xmit(struct sk_buff *skb, struct net_device *dev)
 	td_ptr->tdesc1.TCR = TCR0_TIC;
 	td_ptr->td_buf[0].size &= ~TD_QUEUE;
 
-#ifdef VELOCITY_ZERO_COPY_SUPPORT
-	if (skb_shinfo(skb)->nr_frags > 0) {
-		int nfrags = skb_shinfo(skb)->nr_frags;
-		tdinfo->skb = skb;
-		if (nfrags > 6) {
-			skb_copy_from_linear_data(skb, tdinfo->buf, skb->len);
-			tdinfo->skb_dma[0] = tdinfo->buf_dma;
-			td_ptr->tdesc0.len = len;
-			td_ptr->tx.buf[0].pa_low = cpu_to_le32(tdinfo->skb_dma[0]);
-			td_ptr->tx.buf[0].pa_high = 0;
-			td_ptr->tx.buf[0].size = len;	/* queue is 0 anyway */
-			tdinfo->nskb_dma = 1;
-		} else {
-			int i = 0;
-			tdinfo->nskb_dma = 0;
-			tdinfo->skb_dma[i] = pci_map_single(vptr->pdev, skb->data,
-						skb_headlen(skb), PCI_DMA_TODEVICE);
-
-			td_ptr->tdesc0.len = len;
-
-			/* FIXME: support 48bit DMA later */
-			td_ptr->tx.buf[i].pa_low = cpu_to_le32(tdinfo->skb_dma);
-			td_ptr->tx.buf[i].pa_high = 0;
-			td_ptr->tx.buf[i].size = cpu_to_le16(skb_headlen(skb));
-
-			for (i = 0; i < nfrags; i++) {
-				skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
-				void *addr = (void *)page_address(frag->page) + frag->page_offset;
-
-				tdinfo->skb_dma[i + 1] = pci_map_single(vptr->pdev, addr, frag->size, PCI_DMA_TODEVICE);
-
-				td_ptr->tx.buf[i + 1].pa_low = cpu_to_le32(tdinfo->skb_dma[i + 1]);
-				td_ptr->tx.buf[i + 1].pa_high = 0;
-				td_ptr->tx.buf[i + 1].size = cpu_to_le16(frag->size);
-			}
-			tdinfo->nskb_dma = i - 1;
-		}
+	/*
+	 *	Map the linear network buffer into PCI space and
+	 *	add it to the transmit ring.
+	 */
+	tdinfo->skb = skb;
+	tdinfo->skb_dma[0] = pci_map_single(vptr->pdev, skb->data, pktlen, PCI_DMA_TODEVICE);
+	td_ptr->tdesc0.len = len;
+	td_ptr->td_buf[0].pa_low = cpu_to_le32(tdinfo->skb_dma[0]);
+	td_ptr->td_buf[0].pa_high = 0;
+	td_ptr->td_buf[0].size = len;
+	tdinfo->nskb_dma = 1;
 
-	} else
-#endif
-	{
-		/*
-		 *	Map the linear network buffer into PCI space and
-		 *	add it to the transmit ring.
-		 */
-		tdinfo->skb = skb;
-		tdinfo->skb_dma[0] = pci_map_single(vptr->pdev, skb->data, pktlen, PCI_DMA_TODEVICE);
-		td_ptr->tdesc0.len = len;
-		td_ptr->td_buf[0].pa_low = cpu_to_le32(tdinfo->skb_dma[0]);
-		td_ptr->td_buf[0].pa_high = 0;
-		td_ptr->td_buf[0].size = len;
-		tdinfo->nskb_dma = 1;
-	}
 	td_ptr->tdesc1.cmd = TCPLS_NORMAL + (tdinfo->nskb_dma + 1) * 16;
 
 	if (vptr->vlgrp && vlan_tx_tag_present(skb)) {

[PATCH] Fix velocity driver unmapping incorrect size.

[Dave Jones]

Oh man, this was subtle.  How did this never bite us before ?
-- 

When a packet is greater than ETH_ZLEN, we end up assigning the
boolean result of a comparison to the size we unmap.

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
index b02f7ad..46405e9 100644
--- a/drivers/net/via-velocity.c
+++ b/drivers/net/via-velocity.c
@@ -1847,7 +1847,7 @@ static void velocity_free_tx_buf(struct velocity_info *vptr, struct velocity_td_
 	 */
 	if (tdinfo->skb_dma) {
 
-		pktlen = (skb->len > ETH_ZLEN ? : ETH_ZLEN);
+		pktlen = (skb->len > ETH_ZLEN ? skb->len : ETH_ZLEN);
 		for (i = 0; i < tdinfo->nskb_dma; i++) {
 #ifdef VELOCITY_ZERO_COPY_SUPPORT
 			pci_unmap_single(vptr->pdev, tdinfo->skb_dma[i], le16_to_cpu(td->tdesc1.len), PCI_DMA_TODEVICE);

Re: [PATCH] Fix velocity driver unmapping incorrect size.

[Dave Jones]

On Sun, Jun 21, 2009 at 11:37:35PM -0400, Dave Jones wrote:
 > Oh man, this was subtle.  How did this never bite us before ?
 
bah. I introduced that back in 59f8e169e25c5fce91826412c38359ecaf940b82

I suck at ternary operators.

	Dave

[PATCH v2] Fix velocity driver unmapping incorrect size.

[Dave Jones]

this makes it even clearer what's going on.
-- 

When a packet is greater than ETH_ZLEN, we end up assigning the
boolean result of a comparison to the size we unmap.

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/drivers/net/via-velocity.c b/drivers/net/via-velocity.c
index b02f7ad..3ba3595 100644
--- a/drivers/net/via-velocity.c
+++ b/drivers/net/via-velocity.c
@@ -1847,7 +1847,7 @@ static void velocity_free_tx_buf(struct velocity_info *vptr, struct velocity_td_
 	 */
 	if (tdinfo->skb_dma) {
 
-		pktlen = (skb->len > ETH_ZLEN ? : ETH_ZLEN);
+		pktlen = max_t(unsigned int, skb->len, ETH_ZLEN);
 		for (i = 0; i < tdinfo->nskb_dma; i++) {
 #ifdef VELOCITY_ZERO_COPY_SUPPORT
 			pci_unmap_single(vptr->pdev, tdinfo->skb_dma[i], le16_to_cpu(td->tdesc1.len), PCI_DMA_TODEVICE);

Re: [PATCH v2] Fix velocity driver unmapping incorrect size.

[David Miller]

From: Dave Jones <davej@redhat.com>
Date: Mon, 22 Jun 2009 01:35:30 -0400

> this makes it even clearer what's going on.
> -- 
> 
> When a packet is greater than ETH_ZLEN, we end up assigning the
> boolean result of a comparison to the size we unmap.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>

Looks great Dave, thanks for figuring this out.

I'll queue this up for -stable too.