Netdev List
 help / color / mirror / Atom feed
* Question about way that NICs deliver packets to the kernel
From: Junchang Wang @ 2010-07-15 14:24 UTC (permalink / raw)
  To: romieu, netdev

Hi list,
My understand of the way that NICs deliver packets to the kernel is
as follows. Correct me if any of this is wrong. Thanks.

1) The device buffer is fixed. When the kernel is acknowledged arrival of a 
new packet, it dynamically allocate a new skb and copy the packet into it. 
For example, 8139too.

2) The device buffer is mapped by streaming DMA. When the kernel is 
acknowledged arrival of a new packet, it unmaps the region previously mapped. 
Obviously, there is NO memcpy operation. Additional cost is streaming DMA 
map/unmap operations. For example, e100 and e1000.

Here comes my question:
1) Is there a principle indicating which one is better? Is streaming DMA
map/unmap operations more expensive than memcpy operation?


2) Why does r8169 bias towards the first approach even if it support both? I 
convert r8169 to the second one and get a 5% performance boost. Below is result
running netperf TCP_STREAM test with 1.6K byte packet length.
        scheme 1    scheme 2    Imp.
r8169     683M        718M       5%

The following patch shows what I did:

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 239d7ef..707876f 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -4556,15 +4556,9 @@ static int rtl8169_rx_interrupt(struct net_device *dev,
 
 			rtl8169_rx_csum(skb, desc);
 
-			if (rtl8169_try_rx_copy(&skb, tp, pkt_size, addr)) {
-				pci_dma_sync_single_for_device(pdev, addr,
-					pkt_size, PCI_DMA_FROMDEVICE);
-				rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
-			} else {
-				pci_unmap_single(pdev, addr, tp->rx_buf_sz,
-						 PCI_DMA_FROMDEVICE);
-				tp->Rx_skbuff[entry] = NULL;
-			}
+			pci_unmap_single(pdev, addr, tp->rx_buf_sz,
+					 PCI_DMA_FROMDEVICE);
+			tp->Rx_skbuff[entry] = NULL;
 
 			skb_put(skb, pkt_size);
 			skb->protocol = eth_type_trans(skb, dev);

Thanks in advance.

--Junchang

^ permalink raw reply related
* [PATCH -next 0/2] bnx2: allow sleep during allocation
From: Stanislaw Gruszka @ 2010-07-15 14:25 UTC (permalink / raw)
  To: netdev; +Cc: Michael Chan

We have Fedora bug report about memory allocation failure in bnx2_open
(https://bugzilla.redhat.com/show_bug.cgi?id=612861). To prevent
failure we can allow allocator to sleep. Both patches add
GFP_KERNEL flag where possible, first patch in alloc API, second
in DMA API (after conversion from pci_dma_*).

Stanislaw

^ permalink raw reply
* [PATCH 1/2] bnx2: allocate with GFP_KERNEL flag on RX path init
From: Stanislaw Gruszka @ 2010-07-15 14:25 UTC (permalink / raw)
  To: netdev; +Cc: Michael Chan
In-Reply-To: <20100715142530.12504.80404.send-patch@dhcp-lab-109.englab.brq.redhat.com>

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/bnx2.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index a203f39..6de4cb7 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -2664,13 +2664,13 @@ bnx2_set_mac_addr(struct bnx2 *bp, u8 *mac_addr, u32 pos)
 }
 
 static inline int
-bnx2_alloc_rx_page(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, u16 index)
+bnx2_alloc_rx_page(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, u16 index, gfp_t gfp)
 {
 	dma_addr_t mapping;
 	struct sw_pg *rx_pg = &rxr->rx_pg_ring[index];
 	struct rx_bd *rxbd =
 		&rxr->rx_pg_desc_ring[RX_RING(index)][RX_IDX(index)];
-	struct page *page = alloc_page(GFP_ATOMIC);
+	struct page *page = alloc_page(gfp);
 
 	if (!page)
 		return -ENOMEM;
@@ -2705,7 +2705,7 @@ bnx2_free_rx_page(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, u16 index)
 }
 
 static inline int
-bnx2_alloc_rx_skb(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, u16 index)
+bnx2_alloc_rx_skb(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, u16 index, gfp_t gfp)
 {
 	struct sk_buff *skb;
 	struct sw_bd *rx_buf = &rxr->rx_buf_ring[index];
@@ -2713,7 +2713,7 @@ bnx2_alloc_rx_skb(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, u16 index)
 	struct rx_bd *rxbd = &rxr->rx_desc_ring[RX_RING(index)][RX_IDX(index)];
 	unsigned long align;
 
-	skb = netdev_alloc_skb(bp->dev, bp->rx_buf_size);
+	skb = __netdev_alloc_skb(bp->dev, bp->rx_buf_size, gfp);
 	if (skb == NULL) {
 		return -ENOMEM;
 	}
@@ -2974,7 +2974,7 @@ bnx2_rx_skb(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, struct sk_buff *skb,
 	int err;
 	u16 prod = ring_idx & 0xffff;
 
-	err = bnx2_alloc_rx_skb(bp, rxr, prod);
+	err = bnx2_alloc_rx_skb(bp, rxr, prod, GFP_KERNEL);
 	if (unlikely(err)) {
 		bnx2_reuse_rx_skb(bp, rxr, skb, (u16) (ring_idx >> 16), prod);
 		if (hdr_len) {
@@ -3039,7 +3039,8 @@ bnx2_rx_skb(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, struct sk_buff *skb,
 			rx_pg->page = NULL;
 
 			err = bnx2_alloc_rx_page(bp, rxr,
-						 RX_PG_RING_IDX(pg_prod));
+						 RX_PG_RING_IDX(pg_prod),
+						 GFP_ATOMIC);
 			if (unlikely(err)) {
 				rxr->rx_pg_cons = pg_cons;
 				rxr->rx_pg_prod = pg_prod;
@@ -5179,7 +5180,7 @@ bnx2_init_rx_ring(struct bnx2 *bp, int ring_num)
 
 	ring_prod = prod = rxr->rx_pg_prod;
 	for (i = 0; i < bp->rx_pg_ring_size; i++) {
-		if (bnx2_alloc_rx_page(bp, rxr, ring_prod) < 0) {
+		if (bnx2_alloc_rx_page(bp, rxr, ring_prod, GFP_KERNEL) < 0) {
 			netdev_warn(bp->dev, "init'ed rx page ring %d with %d/%d pages only\n",
 				    ring_num, i, bp->rx_pg_ring_size);
 			break;
@@ -5191,7 +5192,7 @@ bnx2_init_rx_ring(struct bnx2 *bp, int ring_num)
 
 	ring_prod = prod = rxr->rx_prod;
 	for (i = 0; i < bp->rx_ring_size; i++) {
-		if (bnx2_alloc_rx_skb(bp, rxr, ring_prod) < 0) {
+		if (bnx2_alloc_rx_skb(bp, rxr, ring_prod, GFP_KERNEL) < 0) {
 			netdev_warn(bp->dev, "init'ed rx ring %d with %d/%d skbs only\n",
 				    ring_num, i, bp->rx_ring_size);
 			break;
-- 
1.7.1


^ permalink raw reply related
* [PATCH 2/2] bnx2: use device model DMA API
From: Stanislaw Gruszka @ 2010-07-15 14:25 UTC (permalink / raw)
  To: netdev; +Cc: Michael Chan
In-Reply-To: <20100715142530.12504.80404.send-patch@dhcp-lab-109.englab.brq.redhat.com>

Use DMA API as PCI equivalents will be deprecated. This change also allow
to allocate with GFP_KERNEL in some places.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/bnx2.c |  111 +++++++++++++++++++++++++++-------------------------
 1 files changed, 58 insertions(+), 53 deletions(-)

diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
index 6de4cb7..98aed05 100644
--- a/drivers/net/bnx2.c
+++ b/drivers/net/bnx2.c
@@ -692,9 +692,9 @@ bnx2_free_tx_mem(struct bnx2 *bp)
 		struct bnx2_tx_ring_info *txr = &bnapi->tx_ring;
 
 		if (txr->tx_desc_ring) {
-			pci_free_consistent(bp->pdev, TXBD_RING_SIZE,
-					    txr->tx_desc_ring,
-					    txr->tx_desc_mapping);
+			dma_free_coherent(&bp->pdev->dev, TXBD_RING_SIZE,
+					  txr->tx_desc_ring,
+					  txr->tx_desc_mapping);
 			txr->tx_desc_ring = NULL;
 		}
 		kfree(txr->tx_buf_ring);
@@ -714,9 +714,9 @@ bnx2_free_rx_mem(struct bnx2 *bp)
 
 		for (j = 0; j < bp->rx_max_ring; j++) {
 			if (rxr->rx_desc_ring[j])
-				pci_free_consistent(bp->pdev, RXBD_RING_SIZE,
-						    rxr->rx_desc_ring[j],
-						    rxr->rx_desc_mapping[j]);
+				dma_free_coherent(&bp->pdev->dev, RXBD_RING_SIZE,
+						  rxr->rx_desc_ring[j],
+						  rxr->rx_desc_mapping[j]);
 			rxr->rx_desc_ring[j] = NULL;
 		}
 		vfree(rxr->rx_buf_ring);
@@ -724,9 +724,9 @@ bnx2_free_rx_mem(struct bnx2 *bp)
 
 		for (j = 0; j < bp->rx_max_pg_ring; j++) {
 			if (rxr->rx_pg_desc_ring[j])
-				pci_free_consistent(bp->pdev, RXBD_RING_SIZE,
-						    rxr->rx_pg_desc_ring[j],
-						    rxr->rx_pg_desc_mapping[j]);
+				dma_free_coherent(&bp->pdev->dev, RXBD_RING_SIZE,
+						  rxr->rx_pg_desc_ring[j],
+						  rxr->rx_pg_desc_mapping[j]);
 			rxr->rx_pg_desc_ring[j] = NULL;
 		}
 		vfree(rxr->rx_pg_ring);
@@ -748,8 +748,8 @@ bnx2_alloc_tx_mem(struct bnx2 *bp)
 			return -ENOMEM;
 
 		txr->tx_desc_ring =
-			pci_alloc_consistent(bp->pdev, TXBD_RING_SIZE,
-					     &txr->tx_desc_mapping);
+			dma_alloc_coherent(&bp->pdev->dev, TXBD_RING_SIZE,
+					   &txr->tx_desc_mapping, GFP_KERNEL);
 		if (txr->tx_desc_ring == NULL)
 			return -ENOMEM;
 	}
@@ -776,8 +776,10 @@ bnx2_alloc_rx_mem(struct bnx2 *bp)
 
 		for (j = 0; j < bp->rx_max_ring; j++) {
 			rxr->rx_desc_ring[j] =
-				pci_alloc_consistent(bp->pdev, RXBD_RING_SIZE,
-						     &rxr->rx_desc_mapping[j]);
+				dma_alloc_coherent(&bp->pdev->dev,
+						   RXBD_RING_SIZE,
+						   &rxr->rx_desc_mapping[j],
+						   GFP_KERNEL);
 			if (rxr->rx_desc_ring[j] == NULL)
 				return -ENOMEM;
 
@@ -795,8 +797,10 @@ bnx2_alloc_rx_mem(struct bnx2 *bp)
 
 		for (j = 0; j < bp->rx_max_pg_ring; j++) {
 			rxr->rx_pg_desc_ring[j] =
-				pci_alloc_consistent(bp->pdev, RXBD_RING_SIZE,
-						&rxr->rx_pg_desc_mapping[j]);
+				dma_alloc_coherent(&bp->pdev->dev,
+						   RXBD_RING_SIZE,
+						   &rxr->rx_pg_desc_mapping[j],
+						   GFP_KERNEL);
 			if (rxr->rx_pg_desc_ring[j] == NULL)
 				return -ENOMEM;
 
@@ -816,16 +820,16 @@ bnx2_free_mem(struct bnx2 *bp)
 
 	for (i = 0; i < bp->ctx_pages; i++) {
 		if (bp->ctx_blk[i]) {
-			pci_free_consistent(bp->pdev, BCM_PAGE_SIZE,
-					    bp->ctx_blk[i],
-					    bp->ctx_blk_mapping[i]);
+			dma_free_coherent(&bp->pdev->dev, BCM_PAGE_SIZE,
+					  bp->ctx_blk[i],
+					  bp->ctx_blk_mapping[i]);
 			bp->ctx_blk[i] = NULL;
 		}
 	}
 	if (bnapi->status_blk.msi) {
-		pci_free_consistent(bp->pdev, bp->status_stats_size,
-				    bnapi->status_blk.msi,
-				    bp->status_blk_mapping);
+		dma_free_coherent(&bp->pdev->dev, bp->status_stats_size,
+				  bnapi->status_blk.msi,
+				  bp->status_blk_mapping);
 		bnapi->status_blk.msi = NULL;
 		bp->stats_blk = NULL;
 	}
@@ -846,8 +850,8 @@ bnx2_alloc_mem(struct bnx2 *bp)
 	bp->status_stats_size = status_blk_size +
 				sizeof(struct statistics_block);
 
-	status_blk = pci_alloc_consistent(bp->pdev, bp->status_stats_size,
-					  &bp->status_blk_mapping);
+	status_blk = dma_alloc_coherent(&bp->pdev->dev, bp->status_stats_size,
+					&bp->status_blk_mapping, GFP_KERNEL);
 	if (status_blk == NULL)
 		goto alloc_mem_err;
 
@@ -885,9 +889,10 @@ bnx2_alloc_mem(struct bnx2 *bp)
 		if (bp->ctx_pages == 0)
 			bp->ctx_pages = 1;
 		for (i = 0; i < bp->ctx_pages; i++) {
-			bp->ctx_blk[i] = pci_alloc_consistent(bp->pdev,
+			bp->ctx_blk[i] = dma_alloc_coherent(&bp->pdev->dev,
 						BCM_PAGE_SIZE,
-						&bp->ctx_blk_mapping[i]);
+						&bp->ctx_blk_mapping[i],
+						GFP_KERNEL);
 			if (bp->ctx_blk[i] == NULL)
 				goto alloc_mem_err;
 		}
@@ -2674,9 +2679,9 @@ bnx2_alloc_rx_page(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, u16 index, gf
 
 	if (!page)
 		return -ENOMEM;
-	mapping = pci_map_page(bp->pdev, page, 0, PAGE_SIZE,
+	mapping = dma_map_page(&bp->pdev->dev, page, 0, PAGE_SIZE,
 			       PCI_DMA_FROMDEVICE);
-	if (pci_dma_mapping_error(bp->pdev, mapping)) {
+	if (dma_mapping_error(&bp->pdev->dev, mapping)) {
 		__free_page(page);
 		return -EIO;
 	}
@@ -2697,8 +2702,8 @@ bnx2_free_rx_page(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, u16 index)
 	if (!page)
 		return;
 
-	pci_unmap_page(bp->pdev, dma_unmap_addr(rx_pg, mapping), PAGE_SIZE,
-		       PCI_DMA_FROMDEVICE);
+	dma_unmap_page(&bp->pdev->dev, dma_unmap_addr(rx_pg, mapping),
+		       PAGE_SIZE, PCI_DMA_FROMDEVICE);
 
 	__free_page(page);
 	rx_pg->page = NULL;
@@ -2721,9 +2726,9 @@ bnx2_alloc_rx_skb(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, u16 index, gfp
 	if (unlikely((align = (unsigned long) skb->data & (BNX2_RX_ALIGN - 1))))
 		skb_reserve(skb, BNX2_RX_ALIGN - align);
 
-	mapping = pci_map_single(bp->pdev, skb->data, bp->rx_buf_use_size,
-		PCI_DMA_FROMDEVICE);
-	if (pci_dma_mapping_error(bp->pdev, mapping)) {
+	mapping = dma_map_single(&bp->pdev->dev, skb->data, bp->rx_buf_use_size,
+				 PCI_DMA_FROMDEVICE);
+	if (dma_mapping_error(&bp->pdev->dev, mapping)) {
 		dev_kfree_skb(skb);
 		return -EIO;
 	}
@@ -2829,7 +2834,7 @@ bnx2_tx_int(struct bnx2 *bp, struct bnx2_napi *bnapi, int budget)
 			}
 		}
 
-		pci_unmap_single(bp->pdev, dma_unmap_addr(tx_buf, mapping),
+		dma_unmap_single(&bp->pdev->dev, dma_unmap_addr(tx_buf, mapping),
 			skb_headlen(skb), PCI_DMA_TODEVICE);
 
 		tx_buf->skb = NULL;
@@ -2838,7 +2843,7 @@ bnx2_tx_int(struct bnx2 *bp, struct bnx2_napi *bnapi, int budget)
 		for (i = 0; i < last; i++) {
 			sw_cons = NEXT_TX_BD(sw_cons);
 
-			pci_unmap_page(bp->pdev,
+			dma_unmap_page(&bp->pdev->dev,
 				dma_unmap_addr(
 					&txr->tx_buf_ring[TX_RING_IDX(sw_cons)],
 					mapping),
@@ -2945,7 +2950,7 @@ bnx2_reuse_rx_skb(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr,
 	cons_rx_buf = &rxr->rx_buf_ring[cons];
 	prod_rx_buf = &rxr->rx_buf_ring[prod];
 
-	pci_dma_sync_single_for_device(bp->pdev,
+	dma_sync_single_for_device(&bp->pdev->dev,
 		dma_unmap_addr(cons_rx_buf, mapping),
 		BNX2_RX_OFFSET + BNX2_RX_COPY_THRESH, PCI_DMA_FROMDEVICE);
 
@@ -2987,7 +2992,7 @@ bnx2_rx_skb(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, struct sk_buff *skb,
 	}
 
 	skb_reserve(skb, BNX2_RX_OFFSET);
-	pci_unmap_single(bp->pdev, dma_addr, bp->rx_buf_use_size,
+	dma_unmap_single(&bp->pdev->dev, dma_addr, bp->rx_buf_use_size,
 			 PCI_DMA_FROMDEVICE);
 
 	if (hdr_len == 0) {
@@ -3049,7 +3054,7 @@ bnx2_rx_skb(struct bnx2 *bp, struct bnx2_rx_ring_info *rxr, struct sk_buff *skb,
 				return err;
 			}
 
-			pci_unmap_page(bp->pdev, mapping_old,
+			dma_unmap_page(&bp->pdev->dev, mapping_old,
 				       PAGE_SIZE, PCI_DMA_FROMDEVICE);
 
 			frag_size -= frag_len;
@@ -3120,7 +3125,7 @@ bnx2_rx_int(struct bnx2 *bp, struct bnx2_napi *bnapi, int budget)
 
 		dma_addr = dma_unmap_addr(rx_buf, mapping);
 
-		pci_dma_sync_single_for_cpu(bp->pdev, dma_addr,
+		dma_sync_single_for_cpu(&bp->pdev->dev, dma_addr,
 			BNX2_RX_OFFSET + BNX2_RX_COPY_THRESH,
 			PCI_DMA_FROMDEVICE);
 
@@ -5338,7 +5343,7 @@ bnx2_free_tx_skbs(struct bnx2 *bp)
 				continue;
 			}
 
-			pci_unmap_single(bp->pdev,
+			dma_unmap_single(&bp->pdev->dev,
 					 dma_unmap_addr(tx_buf, mapping),
 					 skb_headlen(skb),
 					 PCI_DMA_TODEVICE);
@@ -5349,7 +5354,7 @@ bnx2_free_tx_skbs(struct bnx2 *bp)
 			j++;
 			for (k = 0; k < last; k++, j++) {
 				tx_buf = &txr->tx_buf_ring[TX_RING_IDX(j)];
-				pci_unmap_page(bp->pdev,
+				dma_unmap_page(&bp->pdev->dev,
 					dma_unmap_addr(tx_buf, mapping),
 					skb_shinfo(skb)->frags[k].size,
 					PCI_DMA_TODEVICE);
@@ -5379,7 +5384,7 @@ bnx2_free_rx_skbs(struct bnx2 *bp)
 			if (skb == NULL)
 				continue;
 
-			pci_unmap_single(bp->pdev,
+			dma_unmap_single(&bp->pdev->dev,
 					 dma_unmap_addr(rx_buf, mapping),
 					 bp->rx_buf_use_size,
 					 PCI_DMA_FROMDEVICE);
@@ -5732,9 +5737,9 @@ bnx2_run_loopback(struct bnx2 *bp, int loopback_mode)
 	for (i = 14; i < pkt_size; i++)
 		packet[i] = (unsigned char) (i & 0xff);
 
-	map = pci_map_single(bp->pdev, skb->data, pkt_size,
-		PCI_DMA_TODEVICE);
-	if (pci_dma_mapping_error(bp->pdev, map)) {
+	map = dma_map_single(&bp->pdev->dev, skb->data, pkt_size,
+			     PCI_DMA_TODEVICE);
+	if (dma_mapping_error(&bp->pdev->dev, map)) {
 		dev_kfree_skb(skb);
 		return -EIO;
 	}
@@ -5772,7 +5777,7 @@ bnx2_run_loopback(struct bnx2 *bp, int loopback_mode)
 
 	udelay(5);
 
-	pci_unmap_single(bp->pdev, map, pkt_size, PCI_DMA_TODEVICE);
+	dma_unmap_single(&bp->pdev->dev, map, pkt_size, PCI_DMA_TODEVICE);
 	dev_kfree_skb(skb);
 
 	if (bnx2_get_hw_tx_cons(tx_napi) != txr->tx_prod)
@@ -5789,7 +5794,7 @@ bnx2_run_loopback(struct bnx2 *bp, int loopback_mode)
 	rx_hdr = rx_buf->desc;
 	skb_reserve(rx_skb, BNX2_RX_OFFSET);
 
-	pci_dma_sync_single_for_cpu(bp->pdev,
+	dma_sync_single_for_cpu(&bp->pdev->dev,
 		dma_unmap_addr(rx_buf, mapping),
 		bp->rx_buf_size, PCI_DMA_FROMDEVICE);
 
@@ -6457,8 +6462,8 @@ bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	} else
 		mss = 0;
 
-	mapping = pci_map_single(bp->pdev, skb->data, len, PCI_DMA_TODEVICE);
-	if (pci_dma_mapping_error(bp->pdev, mapping)) {
+	mapping = dma_map_single(&bp->pdev->dev, skb->data, len, PCI_DMA_TODEVICE);
+	if (dma_mapping_error(&bp->pdev->dev, mapping)) {
 		dev_kfree_skb(skb);
 		return NETDEV_TX_OK;
 	}
@@ -6486,9 +6491,9 @@ bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		txbd = &txr->tx_desc_ring[ring_prod];
 
 		len = frag->size;
-		mapping = pci_map_page(bp->pdev, frag->page, frag->page_offset,
-			len, PCI_DMA_TODEVICE);
-		if (pci_dma_mapping_error(bp->pdev, mapping))
+		mapping = dma_map_page(&bp->pdev->dev, frag->page, frag->page_offset,
+				       len, PCI_DMA_TODEVICE);
+		if (dma_mapping_error(&bp->pdev->dev, mapping))
 			goto dma_error;
 		dma_unmap_addr_set(&txr->tx_buf_ring[ring_prod], mapping,
 				   mapping);
@@ -6527,7 +6532,7 @@ dma_error:
 	ring_prod = TX_RING_IDX(prod);
 	tx_buf = &txr->tx_buf_ring[ring_prod];
 	tx_buf->skb = NULL;
-	pci_unmap_single(bp->pdev, dma_unmap_addr(tx_buf, mapping),
+	dma_unmap_single(&bp->pdev->dev, dma_unmap_addr(tx_buf, mapping),
 			 skb_headlen(skb), PCI_DMA_TODEVICE);
 
 	/* unmap remaining mapped pages */
@@ -6535,7 +6540,7 @@ dma_error:
 		prod = NEXT_TX_BD(prod);
 		ring_prod = TX_RING_IDX(prod);
 		tx_buf = &txr->tx_buf_ring[ring_prod];
-		pci_unmap_page(bp->pdev, dma_unmap_addr(tx_buf, mapping),
+		dma_unmap_page(&bp->pdev->dev, dma_unmap_addr(tx_buf, mapping),
 			       skb_shinfo(skb)->frags[i].size,
 			       PCI_DMA_TODEVICE);
 	}
-- 
1.7.1


^ permalink raw reply related
* Re: Question about way that NICs deliver packets to the kernel
From: Ben Hutchings @ 2010-07-15 14:33 UTC (permalink / raw)
  To: Junchang Wang; +Cc: romieu, netdev
In-Reply-To: <20100715142418.GA26491@host-a-229.ustcsz.edu.cn>

On Thu, 2010-07-15 at 22:24 +0800, Junchang Wang wrote:
> Hi list,
> My understand of the way that NICs deliver packets to the kernel is
> as follows. Correct me if any of this is wrong. Thanks.
> 
> 1) The device buffer is fixed. When the kernel is acknowledged arrival of a 
> new packet, it dynamically allocate a new skb and copy the packet into it. 
> For example, 8139too.
> 
> 2) The device buffer is mapped by streaming DMA. When the kernel is 
> acknowledged arrival of a new packet, it unmaps the region previously mapped. 
> Obviously, there is NO memcpy operation. Additional cost is streaming DMA 
> map/unmap operations. For example, e100 and e1000.
> 
> Here comes my question:
> 1) Is there a principle indicating which one is better? Is streaming DMA
> map/unmap operations more expensive than memcpy operation?

DMA should result in lower CPU usage and higher maximum performance.

> 2) Why does r8169 bias towards the first approach even if it support both? I 
> convert r8169 to the second one and get a 5% performance boost. Below is result
> running netperf TCP_STREAM test with 1.6K byte packet length.
>         scheme 1    scheme 2    Imp.
> r8169     683M        718M       5%
[...]

You should also compare the CPU usage.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


^ permalink raw reply
* Re: [PATCH 1/2] bnx2: allocate with GFP_KERNEL flag on RX path init
From: Michael Chan @ 2010-07-15 14:48 UTC (permalink / raw)
  To: 'Stanislaw Gruszka', netdev@vger.kernel.org
In-Reply-To: <20100715142537.12504.60051.send-patch@dhcp-lab-109.englab.brq.redhat.com>

Stanislaw Gruszka wrote:

> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> @@ -2974,7 +2974,7 @@ bnx2_rx_skb(struct bnx2 *bp, struct
> bnx2_rx_ring_info *rxr, struct sk_buff *skb,
>       int err;
>       u16 prod = ring_idx & 0xffff;
>
> -     err = bnx2_alloc_rx_skb(bp, rxr, prod);
> +     err = bnx2_alloc_rx_skb(bp, rxr, prod, GFP_KERNEL);

This should be GFP_ATOMIC since it is called from NAPI softirq
context.



^ permalink raw reply
* Re: [PATCH 2/2] netfilter: nfnetlink_queue: add definition of aligned_be64 for user-space apps
From: Patrick McHardy @ 2010-07-15 15:07 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel, NetDev
In-Reply-To: <4C3EF819.8080801@netfilter.org>

Am 15.07.2010 13:59, schrieb Pablo Neira Ayuso:
> On 15/07/10 11:19, Patrick McHardy wrote:
>> Am 12.07.2010 19:00, schrieb Pablo Neira Ayuso:
>>> Currently, libnl and libnetfilter_queue include in one of their
>>> user-space header files an ad-hoc definition of aligned_be64.
>>> However, applications that use the BSD socket API to communicate
>>> via Netlink sockets (ie. those that do not use these libraries)
>>> would need to define this type by hand if they include the
>>> kernel-space header nfnetlink_queue.h.
>>>
>>> This patch adds the definition of aligned_bed64 for user-space
>>> applications in the kernel header. Otherwise, they have to define
>>> it to avoid the following compilation problem:
>>>
>>> /usr/include/linux/netfilter/nfnetlink_queue.h:28: error: expected specifier-qualifier-list before ‘aligned_be64’
>>
>> Why can't these applications simply include linux/types.h?
> 
> Including it doesn't fix the problem here:
> 
> #include <linux/types.h>
> #include <linux/netfilter/nfnetlink_queue.h>
> 
> I still get:
> 
> /usr/include/linux/netfilter/nfnetlink_queue.h:28: error: expected
> specifier-qualifier-list before ‘aligned_be64’
> 
> aligned_be64 is only define in the kernel (it's included under the
> __KERNEL__ definition).
>

In that case I think we should export a __aligned_be64 in types.h
and use that instead of aligned_be64.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply
* Re: [PATCH] netfilter: add CHECKSUM target
From: Patrick McHardy @ 2010-07-15 15:17 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: David S. Miller, Alexey Kuznetsov, Pekka Savola (ipv6),
	James Morris, Hideaki YOSHIFUJI, linux-kernel, netfilter-devel,
	netfilter, coreteam, netdev, herbert.xu, kvm
In-Reply-To: <20100711104705.GA18017@redhat.com>

Am 11.07.2010 12:47, schrieb Michael S. Tsirkin:
> On Fri, Jul 09, 2010 at 05:17:36PM +0200, Patrick McHardy wrote:
>> Am 09.07.2010 00:29, schrieb Michael S. Tsirkin:
>>> This adds a `CHECKSUM' target, which can be used in the iptables mangle
>>> table.
>>>
>>> You can use this target to compute and fill in the checksum in
>>> an IP packet that lacks a checksum.  This is particularly useful,
>>> if you need to work around old applications such as dhcp clients,
>>> that do not work well with checksum offloads, but don't want to
>>> disable checksum offload in your device.
>>>
>>> The problem happens in the field with virtualized applications.
>>> For reference, see Red Hat bz 605555, as well as
>>> http://www.spinics.net/lists/kvm/msg37660.html
>>>
>>> Typical expected use (helps old dhclient binary running in a VM):
>>> iptables -A POSTROUTING -t mangle -p udp --dport 68 -j CHECKSUM
>>> --checksum-fill
>>
>> I'm not sure this is something we want to merge upstream and
>> support indefinitely. Dave suggested this as a temporary
>> out-of-tree workaround until the majority of guest dhcp clients
>> are fixed. Has anything changed that makes this course of
>> action impractical?
> 
> If I understand what Dave said correctly, it's up to you ...
> 
> The arguments for putting this upstream are:
> 
> Given the track record, I wouldn't hope for quick fix in the majority of
> guest dhcp clients, unfortunately :(.  We are talking years here.
> Even after that, one of the uses of virtualization is
> to keep old guests running. So yes, I think we'll
> keep using work-arounds for this for a very long time.
> 
> Further, since we have to add the module and we have to teach management
> to program it, it will be much less painful for everyone
> involved if we can put the code upstream, rather than forking
> management code.

Fair enough, its simple enough that I don't expect much maintenance
overhead.

^ permalink raw reply
* Re: [PATCHv4] netfilter: add CHECKSUM target
From: Patrick McHardy @ 2010-07-15 15:22 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Daniel P. Berrange, Jes Sorensen, David S. Miller, Jan Engelhardt,
	Randy Dunlap, netfilter-devel, netfilter, coreteam, linux-kernel,
	netdev
In-Reply-To: <20100715115217.GA6737@redhat.com>

Am 15.07.2010 13:52, schrieb Michael S. Tsirkin:
> This adds a `CHECKSUM' target, which can be used in the iptables mangle
> table.
> 
> You can use this target to compute and fill in the checksum in
> a packet that lacks a checksum.  This is particularly useful,
> if you need to work around old applications such as dhcp clients,
> that do not work well with checksum offloads, but don't want to
> disable checksum offload in your device.
> 
> The problem happens in the field with virtualized applications.
> For reference, see Red Hat bz 605555, as well as
> http://www.spinics.net/lists/kvm/msg37660.html
> 
> Typical expected use (helps old dhclient binary running in a VM):
> iptables -A POSTROUTING -t mangle -p udp --dport bootpc \
> 	-j CHECKSUM --checksum-fill
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Includes fixes by Jan Engelhardt <jengelh@medozas.de>

Applied, thanks Michael.

^ permalink raw reply
* Re: [PATCHv4] extensions: libxt_CHECKSUM extension
From: Patrick McHardy @ 2010-07-15 15:24 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Daniel P. Berrange, Jes Sorensen, David S. Miller, Jan Engelhardt,
	Randy Dunlap, netfilter-devel, netfilter, coreteam, linux-kernel,
	netdev
In-Reply-To: <20100715115229.GB6737@redhat.com>

Am 15.07.2010 13:52, schrieb Michael S. Tsirkin:
> This adds a `CHECKSUM' target, which can be used in the iptables mangle
> table.
> 
> You can use this target to compute and fill in the checksum in
> a packet that lacks a checksum.  This is particularly useful,
> if you need to work around old applications such as dhcp clients,
> that do not work well with checksum offloads, but don't want to disable
> checksum offload in your device.
> 
> The problem happens in the field with virtualized applications.
> For reference, see Red Hat bz 605555, as well as
> http://www.spinics.net/lists/kvm/msg37660.html
> 
> Typical expected use (helps old dhclient binary running in a VM):
> iptables -A POSTROUTING -t mangle -p udp --dport bootpc \
> -j CHECKSUM --checksum-fill

Applied to iptables-next, thanks.

^ permalink raw reply
* Re: Question about way that NICs deliver packets to the kernel
From: Stephen Hemminger @ 2010-07-15 15:59 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: Junchang Wang, romieu, netdev
In-Reply-To: <1279204417.2118.12.camel@achroite.uk.solarflarecom.com>

On Thu, 15 Jul 2010 15:33:37 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> On Thu, 2010-07-15 at 22:24 +0800, Junchang Wang wrote:
> > Hi list,
> > My understand of the way that NICs deliver packets to the kernel is
> > as follows. Correct me if any of this is wrong. Thanks.
> > 
> > 1) The device buffer is fixed. When the kernel is acknowledged arrival of a 
> > new packet, it dynamically allocate a new skb and copy the packet into it. 
> > For example, 8139too.
> > 
> > 2) The device buffer is mapped by streaming DMA. When the kernel is 
> > acknowledged arrival of a new packet, it unmaps the region previously mapped. 
> > Obviously, there is NO memcpy operation. Additional cost is streaming DMA 
> > map/unmap operations. For example, e100 and e1000.
> > 
> > Here comes my question:
> > 1) Is there a principle indicating which one is better? Is streaming DMA
> > map/unmap operations more expensive than memcpy operation?
> 
> DMA should result in lower CPU usage and higher maximum performance.
> 
> > 2) Why does r8169 bias towards the first approach even if it support both? I 
> > convert r8169 to the second one and get a 5% performance boost. Below is result
> > running netperf TCP_STREAM test with 1.6K byte packet length.
> >         scheme 1    scheme 2    Imp.
> > r8169     683M        718M       5%
> [...]
> 
> You should also compare the CPU usage.

Also many drivers copy small receives into a new buffer
which saves space and often gives better performance.

^ permalink raw reply
* Re: multiqueue, skb_get_queue_mapping() and netdev_get_tx_queue()
From: Eldon Koyle @ 2010-07-15 16:22 UTC (permalink / raw)
  To: netdev
In-Reply-To: <20100714231352.GA32397@esk.cs.usu.edu>

On  Jul 14 17:13-0600, Eldon Koyle wrote:
> It looks like there is a potential for an out of bounds index anywhere
> skb_get_queue_mapping(skb) (which just returns skb->queue_mapping) is
> used to get an index for netdev_get_tx_queue() (and probably other
> places) on a device with multiple rx/tx queues.

Looking more closely, it looks like skb->queue_mapping is treated
differently between rx and tx.  In net/core/dev.c in dev_pick_tx, it
uses skb_tx_hash to get the tx queue it should use and then does:
	skb_set_queue_mapping(skb, queue_index);
	return netdev_get_tx_queue(dev, queue_index);

Sorry for the noise.

-- 
Eldon Koyle
-- 
He who renders warfare fatal to all engaged in it will be the greatest
benefactor the world has yet known.
		-- Sir Richard Burton

^ permalink raw reply
* Re: [PATCH RFC] act_cpu: packet distributing
From: Eric Dumazet @ 2010-07-15 16:54 UTC (permalink / raw)
  To: hadi; +Cc: Changli Gao, David S. Miller, Patrick McHardy, Tom Herbert,
	netdev
In-Reply-To: <1279198139.4510.710.camel@bigi>

Le jeudi 15 juillet 2010 à 08:48 -0400, jamal a écrit :
> On Wed, 2010-07-14 at 12:17 +0800, Changli Gao wrote:
> 
> > Thanks, I'll try. It is a write critical section, and for me it is
> > difficult to convert this lock to RCU. Could you show me some
> > examples?
> 
> RCU maybe a little trickier here Eric. Actions could be shared i.e. 
> example, it is possible to have a policer action restricting rates for a
> group of flows  across multiple netdevices etc. Since action stats get
> written to by different CPUs concurrently. It could be probably done if
> one was to implement per-cpu stats which get summed-up when user space
> asks.

It's certainly tricky, but is act_cpu useful in its current shape, based
on an infrastructure that had to use a lock because of exact
rates/accounting ?

I dont understand how distributing packets to different cpus, if going
through a central lock can be an improvement. Changli patches are most
of the time not documented, and no performance data is provided.

Even if we solve this locking problem, using percpu variables, act_cpu
hits another problem :

The socket refcount, taken by the 'master' cpu, and released by the
consumer cpu.

RFS provides sort of a lazy flow-based distribution without central lock
or cache line ping pongs. Why Changli dont use this, we dont know yet.




^ permalink raw reply
* Re: Raise initial congestion window size / speedup slow start?
From: Jerry Chu @ 2010-07-15 17:36 UTC (permalink / raw)
  To: Ed W
  Cc: Tom Herbert, Hagen Paul Pfeifer, Rick Jones, David Miller,
	davidsen, linux-kernel, netdev, Nandita Dukkipati
In-Reply-To: <4C3EBD51.80406@wildgooses.com>

On Thu, Jul 15, 2010 at 12:48 AM, Ed W <lists@wildgooses.com> wrote:
>
> On 15/07/2010 05:12, Tom Herbert wrote:
>>
>> There is an Internet draft
>> (http://datatracker.ietf.org/doc/draft-hkchu-tcpm-initcwnd/) on
>> raising the default Initial Congestion window to 10 segments, as well
>> as a SIGCOMM paper (http://ccr.sigcomm.org/online/?q=node/621).
>>
>
> You guys have obviously done a lot of work on this, however, it seems that there is a case for introducing some heuristics into the choice of init cwnd as well as offering the option to go larger?  An initial size of 10 packets is just another magic number that obviously works with the median bandwidth delay product on today's networks - can we not do better still?
>
> Seems like a bunch of clever folks have already suggested tweaks to the steady stage congestion avoidance, but so far everyone is afraid to touch the early stage heuristics?

This is because there is not enough info for deriving any heuristic.
For initcwnd one is constrained to
only info from 3WHS. This includes a rough estimate of RTT plus all
the bits in the SYN/SYN-ACK
headers. I'm assuming a stateless approach. We've tried a stateful
solution (i.e., seeding initcwnd from
past history) but found its complexity outweigh the gain.
(See http://www.ietf.org/proceedings/77/slides/tcpm-4.pdf)

>
> Also would you guys not benefit from wider deployment of ECN?  Can you not help find some ways that deployment could be increased?  At present there are big warnings all over the option that it causes some problems, but there is no quantification of how much and really whether this warning is still appropriate?

That will add yet another hoop for us to jump over. Also I'm not sure
a couple of bits are sufficient for a
guesstimate of what initcwnd ought to be.

Our reasoning is simple - there has been tremendous b/w growth since
rfc2414 was published. Even the
lowest common denominator (i.e., dialup links) has moved from 9.6Kbps
to 56Kbps. That's a six fold
increase. If you believe initcwnd should grow proportionally to the
buffer sizes in access links, and the
buffer sizes grows proportionally to b/w, then the initcwnd outght to
be 3*6 = 18 today.

We chose a modest increase (10) with the hope to expedite the
standardization process (and would
certainly appreciate helps from folks on this list). 10 is very
conservative considering many deployment
has gone beyond 3, including Linux stack, which allows one additional
pkt if it's the last data pkt.

Longer term it will be nice to find a way to get rid of this fixed,
somewhat arbitrary initcwnd. Mark
Allman's JumpStart is one idea, but it'd be a much longer route.

Jerry

>
> Ed W
>

^ permalink raw reply
* Re: [PATCHv4] netfilter: add CHECKSUM target
From: Michael S. Tsirkin @ 2010-07-15 17:42 UTC (permalink / raw)
  To: Patrick McHardy
  Cc: Daniel P. Berrange, Jes Sorensen, David S. Miller, Jan Engelhardt,
	Randy Dunlap, netfilter-devel, netfilter, coreteam, linux-kernel,
	netdev
In-Reply-To: <4C3F27A2.3030207@trash.net>

On Thu, Jul 15, 2010 at 05:22:10PM +0200, Patrick McHardy wrote:
> Am 15.07.2010 13:52, schrieb Michael S. Tsirkin:
> > This adds a `CHECKSUM' target, which can be used in the iptables mangle
> > table.
> > 
> > You can use this target to compute and fill in the checksum in
> > a packet that lacks a checksum.  This is particularly useful,
> > if you need to work around old applications such as dhcp clients,
> > that do not work well with checksum offloads, but don't want to
> > disable checksum offload in your device.
> > 
> > The problem happens in the field with virtualized applications.
> > For reference, see Red Hat bz 605555, as well as
> > http://www.spinics.net/lists/kvm/msg37660.html
> > 
> > Typical expected use (helps old dhclient binary running in a VM):
> > iptables -A POSTROUTING -t mangle -p udp --dport bootpc \
> > 	-j CHECKSUM --checksum-fill
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Includes fixes by Jan Engelhardt <jengelh@medozas.de>
> 
> Applied, thanks Michael.

I forgot to export the header. And once I added it, I got warning
on header_check, so here's a fixup. Sorry about the noise.

netfilter: correct CHECKSUM header and export it

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

--

diff --git a/include/linux/netfilter/Kbuild b/include/linux/netfilter/Kbuild
index 48767cd..0c3098f 100644
--- a/include/linux/netfilter/Kbuild
+++ b/include/linux/netfilter/Kbuild
@@ -3,6 +3,7 @@ header-y += nf_conntrack_tuple_common.h
 header-y += nfnetlink_conntrack.h
 header-y += nfnetlink_log.h
 header-y += nfnetlink_queue.h
+header-y += xt_CHECKSUM.h
 header-y += xt_CLASSIFY.h
 header-y += xt_CONNMARK.h
 header-y += xt_CONNSECMARK.h
diff --git a/include/linux/netfilter/xt_CHECKSUM.h b/include/linux/netfilter/xt_CHECKSUM.h
index 3b4fb77..9a2e466 100644
--- a/include/linux/netfilter/xt_CHECKSUM.h
+++ b/include/linux/netfilter/xt_CHECKSUM.h
@@ -6,8 +6,10 @@
  *
  * This software is distributed under GNU GPL v2, 1991
 */
-#ifndef _IPT_CHECKSUM_TARGET_H
-#define _IPT_CHECKSUM_TARGET_H
+#ifndef _XT_CHECKSUM_TARGET_H
+#define _XT_CHECKSUM_TARGET_H
+
+#include <linux/types.h>
 
 #define XT_CHECKSUM_OP_FILL	0x01	/* fill in checksum in IP header */
 
@@ -15,4 +17,4 @@ struct xt_CHECKSUM_info {
 	__u8 operation;	/* bitset of operations */
 };
 
-#endif /* _IPT_CHECKSUM_TARGET_H */
+#endif /* _XT_CHECKSUM_TARGET_H */

^ permalink raw reply related
* Re: [PATCH] vhost-net: avoid flush under lock
From: Sridhar Samudrala @ 2010-07-15 18:17 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: David S. Miller, Arnd Bergmann, Paul E. McKenney, kvm,
	virtualization, netdev, linux-kernel
In-Reply-To: <20100715121912.GA7176@redhat.com>

On Thu, 2010-07-15 at 15:19 +0300, Michael S. Tsirkin wrote:
> We flush under vq mutex when changing backends.
> This creates a deadlock as workqueue being flushed
> needs this lock as well.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=612421
> 
> Drop the vq mutex before flush: we have the device mutex
> which is sufficient to prevent another ioctl from touching
> the vq.

Why do we need to flush the vq when trying to set the backend and
we find that it is already set. Is this just an optimization?

Thanks
Sridhar
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/vhost/net.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 28d7786..50df58e6 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -534,11 +534,16 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
>  	rcu_assign_pointer(vq->private_data, sock);
>  	vhost_net_enable_vq(n, vq);
>  done:
> +	mutex_unlock(&vq->mutex);
> +
>  	if (oldsock) {
>  		vhost_net_flush_vq(n, index);
>  		fput(oldsock->file);
>  	}
> 
> +	mutex_unlock(&n->dev.mutex);
> +	return 0;
> +
>  err_vq:
>  	mutex_unlock(&vq->mutex);
>  err:


^ permalink raw reply
* [PATCH 03/25] atm: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)
From: Peter Huewe @ 2010-07-15 18:38 UTC (permalink / raw)
  To: Kernel Janitors
  Cc: Chas Williams, David S. Miller, Ben Hutchings, Tejun Heo,
	linux-atm-general, netdev, linux-kernel

From: Peter Huewe <peterhuewe@gmx.de>

This patch converts pci_table entries, where .subvendor=PCI_ANY_ID and
.subdevice=PCI_ANY_ID, .class=0 and .class_mask=0, to use the
PCI_VDEVICE macro, and thus improves readability.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/atm/ambassador.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/atm/ambassador.c b/drivers/atm/ambassador.c
index 9d18644..a33896a 100644
--- a/drivers/atm/ambassador.c
+++ b/drivers/atm/ambassador.c
@@ -2371,10 +2371,8 @@ MODULE_PARM_DESC(pci_lat, "PCI latency in bus cycles");
 /********** module entry **********/
 
 static struct pci_device_id amb_pci_tbl[] = {
-	{ PCI_VENDOR_ID_MADGE, PCI_DEVICE_ID_MADGE_AMBASSADOR, PCI_ANY_ID, PCI_ANY_ID,
-	  0, 0, 0 },
-	{ PCI_VENDOR_ID_MADGE, PCI_DEVICE_ID_MADGE_AMBASSADOR_BAD, PCI_ANY_ID, PCI_ANY_ID,
-	  0, 0, 0 },
+	{ PCI_VDEVICE(MADGE, PCI_DEVICE_ID_MADGE_AMBASSADOR), 0 },
+	{ PCI_VDEVICE(MADGE, PCI_DEVICE_ID_MADGE_AMBASSADOR_BAD), 0 },
 	{ 0, }
 };
 
-- 
1.7.1

^ permalink raw reply related
* Re: [PATCH] CAN: Add Flexcan CAN controller driver
From: Wolfgang Grandegger @ 2010-07-15 18:38 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: socketcan-core-0fE9KPoRgkgATYTw5x5z8w,
	netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1279144811-12251-1-git-send-email-mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Hi Marc,

On 07/15/2010 12:00 AM, Marc Kleine-Budde wrote:
> From: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 
> This core is found on some Freescale SoCs and also some Coldfire
> SoCs. Support for Coldfire is missing though at the moment as
> they have an older revision of the core which does not have RX FIFO
> support.
> 
> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
> 
> Changes to prev version:
> * The is now GPLv2 (only) as no one complained.
> 
> The patch applies to current net-next-2.6/master.
> If there aren't any objections please consider applying this patch.
> Wolfgang, can I an Acked-by?

I realized a few issues. You can add my "acked-by" when they are fixed.

> 
> Cheers, Marc
> 
> P.S.:
> This patch can be pulled, too:
> 
> The following changes since commit fae88f7eedae42c955075aec7a0cd27545f81511:
> 
>   Merge branch 'for-davem' of git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-next-2.6 (2010-07-13 14:25:13 -0700)
> 
> are available in the git repository at:
> 
>   git://git.pengutronix.de/git/mkl/linux-2.6.git for-net-next-2.6
> 
> Sascha Hauer (1):
>       CAN: Add Flexcan CAN controller driver
> 
>  drivers/net/can/Kconfig              |    6 +
>  drivers/net/can/Makefile             |    1 +
>  drivers/net/can/flexcan.c            | 1005 ++++++++++++++++++++++++++++++++++
>  include/linux/can/platform/flexcan.h |   20 +
>  4 files changed, 1032 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/net/can/flexcan.c
>  create mode 100644 include/linux/can/platform/flexcan.h
> 
> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> index 2c5227c..3f13299 100644
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -73,6 +73,12 @@ config CAN_JANZ_ICAN3
>  	  This driver can also be built as a module. If so, the module will be
>  	  called janz-ican3.ko.
>  
> +config CAN_FLEXCAN
> +	tristate "Support for Freescale FLEXCAN based chips"
> +	depends on CAN_DEV

Some more arch specific dependencies would be nice.

> +	---help---
> +	  Say Y here if you want to support for Freescale FlexCAN.
> +
>  source "drivers/net/can/mscan/Kconfig"
>  
>  source "drivers/net/can/sja1000/Kconfig"
> diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
> index 9047cd0..0057537 100644
> --- a/drivers/net/can/Makefile
> +++ b/drivers/net/can/Makefile
> @@ -16,5 +16,6 @@ obj-$(CONFIG_CAN_TI_HECC)	+= ti_hecc.o
>  obj-$(CONFIG_CAN_MCP251X)	+= mcp251x.o
>  obj-$(CONFIG_CAN_BFIN)		+= bfin_can.o
>  obj-$(CONFIG_CAN_JANZ_ICAN3)	+= janz-ican3.o
> +obj-$(CONFIG_CAN_FLEXCAN)	+= flexcan.o
>  
>  ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> new file mode 100644
> index 0000000..a3180ba
> --- /dev/null
> +++ b/drivers/net/can/flexcan.c
> @@ -0,0 +1,1005 @@
> +/*
> + * flexcan.c - FLEXCAN CAN controller driver
> + *
> + * Copyright (c) 2005-2006 Varma Electronics Oy
> + * Copyright (c) 2009 Sascha Hauer, Pengutronix
> + * Copyright (c) 2010 Marc Kleine-Budde, Pengutronix
> + *
> + * Based on code originally by Andrey Volkov <avolkov-ppI4tVfbJvJWk0Htik3J/w@public.gmane.org>
> + *
> + * LICENCE:
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/netdevice.h>
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +#include <linux/can/platform/flexcan.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/if_arp.h>
> +#include <linux/if_ether.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +
> +#include <mach/clock.h>
> +
> +#define DRV_NAME		"flexcan"
> +#define FLEXCAN_NAPI_WEIGHT	8
> +
> +
> +/* FLEXCAN module configuration register (CANMCR) bits */
> +#define FLEXCAN_MCR_MDIS		BIT(31)
> +#define FLEXCAN_MCR_FRZ			BIT(30)
> +#define FLEXCAN_MCR_FEN			BIT(29)
> +#define FLEXCAN_MCR_HALT		BIT(28)
> +#define FLEXCAN_MCR_NOT_RDY		BIT(27)
> +#define FLEXCAN_MCR_WAK_MSK		BIT(26)
> +#define FLEXCAN_MCR_SOFTRST		BIT(25)
> +#define FLEXCAN_MCR_FRZ_ACK		BIT(24)
> +#define FLEXCAN_MCR_SUPV		BIT(23)
> +#define FLEXCAN_MCR_SLF_WAK		BIT(22)
> +#define FLEXCAN_MCR_WRN_EN		BIT(21)
> +#define FLEXCAN_MCR_LPM_ACK		BIT(20)
> +#define FLEXCAN_MCR_WAK_SRC		BIT(19)
> +#define FLEXCAN_MCR_DOZE		BIT(18)
> +#define FLEXCAN_MCR_SRX_DIS		BIT(17)
> +#define FLEXCAN_MCR_BCC			BIT(16)
> +#define FLEXCAN_MCR_LPRIO_EN		BIT(13)
> +#define FLEXCAN_MCR_AEN			BIT(12)
> +#define FLEXCAN_MCR_MAXMB(x)		((x) & 0xf)
> +#define FLEXCAN_MCR_IDAM_A		(0 << 8)
> +#define FLEXCAN_MCR_IDAM_B		(1 << 8)
> +#define FLEXCAN_MCR_IDAM_C		(2 << 8)
> +#define FLEXCAN_MCR_IDAM_D		(3 << 8)
> +
> +/* FLEXCAN control register (CANCTRL) bits */
> +#define FLEXCAN_CTRL_PRESDIV(x)		(((x) & 0xff) << 24)
> +#define FLEXCAN_CTRL_RJW(x)		(((x) & 0x03) << 22)
> +#define FLEXCAN_CTRL_PSEG1(x)		(((x) & 0x07) << 19)
> +#define FLEXCAN_CTRL_PSEG2(x)		(((x) & 0x07) << 16)
> +#define FLEXCAN_CTRL_BOFF_MSK		BIT(15)
> +#define FLEXCAN_CTRL_ERR_MSK		BIT(14)
> +#define FLEXCAN_CTRL_CLK_SRC		BIT(13)
> +#define FLEXCAN_CTRL_LPB		BIT(12)
> +#define FLEXCAN_CTRL_TWRN_MSK		BIT(11)
> +#define FLEXCAN_CTRL_RWRN_MSK		BIT(10)
> +#define FLEXCAN_CTRL_SMP		BIT(7)
> +#define FLEXCAN_CTRL_BOFF_REC		BIT(6)
> +#define FLEXCAN_CTRL_TSYNC		BIT(5)
> +#define FLEXCAN_CTRL_LBUF		BIT(4)
> +#define FLEXCAN_CTRL_LOM		BIT(3)
> +#define FLEXCAN_CTRL_PROPSEG(x)		((x) & 0x07)
> +
> +/* FLEXCAN error and status register (ESR) bits */
> +#define FLEXCAN_ESR_TWRN_INT		BIT(17)
> +#define FLEXCAN_ESR_RWRN_INT		BIT(16)
> +#define FLEXCAN_ESR_BIT1_ERR		BIT(15)
> +#define FLEXCAN_ESR_BIT0_ERR		BIT(14)
> +#define FLEXCAN_ESR_ACK_ERR		BIT(13)
> +#define FLEXCAN_ESR_CRC_ERR		BIT(12)
> +#define FLEXCAN_ESR_FRM_ERR		BIT(11)
> +#define FLEXCAN_ESR_STF_ERR		BIT(10)
> +#define FLEXCAN_ESR_TX_WRN		BIT(9)
> +#define FLEXCAN_ESR_RX_WRN		BIT(8)
> +#define FLEXCAN_ESR_IDLE		BIT(7)
> +#define FLEXCAN_ESR_TXRX		BIT(6)
> +#define FLEXCAN_EST_FLT_CONF_SHIFT	(4)
> +#define FLEXCAN_ESR_FLT_CONF_MASK	(0x2 << FLEXCAN_EST_FLT_CONF_SHIFT)
> +#define FLEXCAN_ESR_FLT_CONF_ACTIVE	(0x0 << FLEXCAN_EST_FLT_CONF_SHIFT)
> +#define FLEXCAN_ESR_FLT_CONF_PASSIVE	(0x1 << FLEXCAN_EST_FLT_CONF_SHIFT)
> +#define FLEXCAN_ESR_BOFF_INT		BIT(2)
> +#define FLEXCAN_ESR_ERR_INT		BIT(1)
> +#define FLEXCAN_ESR_WAK_INT		BIT(0)
> +#define FLEXCAN_ESR_ERR_FRAME \
> +	(FLEXCAN_ESR_BIT1_ERR | FLEXCAN_ESR_BIT0_ERR | \
> +	 FLEXCAN_ESR_ACK_ERR | FLEXCAN_ESR_CRC_ERR | \
> +	 FLEXCAN_ESR_FRM_ERR | FLEXCAN_ESR_STF_ERR)
> +#define FLEXCAN_ESR_ERR_LINE \
> +	(FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | FLEXCAN_ESR_BOFF_INT)
> +
> +/* FLEXCAN interrupt flag register (IFLAG) bits */
> +#define FLEXCAN_TX_BUF_ID		8
> +#define FLEXCAN_IFLAG_BUF(x)		BIT(x)
> +#define FLEXCAN_IFLAG_RX_FIFO_OVERFLOW	BIT(7)
> +#define FLEXCAN_IFLAG_RX_FIFO_WARN	BIT(6)
> +#define FLEXCAN_IFLAG_RX_FIFO_AVAILABLE	BIT(5)
> +#define FLEXCAN_IFLAG_DEFAULT \
> +	(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW | FLEXCAN_IFLAG_RX_FIFO_AVAILABLE | \
> +	 FLEXCAN_IFLAG_BUF(FLEXCAN_TX_BUF_ID))
> +
> +/* FLEXCAN message buffers */
> +#define FLEXCAN_MB_CNT_CODE(x)		(((x) & 0xf) << 24)
> +#define FLEXCAN_MB_CNT_SRR		BIT(22)
> +#define FLEXCAN_MB_CNT_IDE		BIT(21)
> +#define FLEXCAN_MB_CNT_RTR		BIT(20)
> +#define FLEXCAN_MB_CNT_LENGTH(x)	(((x) & 0xf) << 16)
> +#define FLEXCAN_MB_CNT_TIMESTAMP(x)	((x) & 0xffff)
> +
> +#define FLEXCAN_MB_CODE_MASK		(0xf0ffffff)
> +
> +/* Structure of the message buffer */
> +struct flexcan_mb {
> +	u32 can_ctrl;
> +	u32 can_id;
> +	u32 data[2];
> +};
> +
> +/* Structure of the hardware registers */
> +struct flexcan_regs {
> +	u32 mcr;		/* 0x00 */
> +	u32 ctrl;		/* 0x04 */
> +	u32 timer;		/* 0x08 */
> +	u32 _reserved1;		/* 0x0c */
> +	u32 rxgmask;		/* 0x10 */
> +	u32 rx14mask;		/* 0x14 */
> +	u32 rx15mask;		/* 0x18 */
> +	u32 ecr;		/* 0x1c */
> +	u32 esr;		/* 0x20 */
> +	u32 imask2;		/* 0x24 */
> +	u32 imask1;		/* 0x28 */
> +	u32 iflag2;		/* 0x2c */
> +	u32 iflag1;		/* 0x30 */
> +	u32 _reserved2[19];
> +	struct flexcan_mb cantxfg[64];
> +};
> +
> +struct flexcan_priv {
> +	struct can_priv can;
> +	struct net_device *dev;
> +	struct napi_struct napi;
> +
> +	void __iomem *base;
> +	u32 reg_esr;
> +	u32 reg_ctrl_default;
> +
> +	struct clk *clk;
> +	struct flexcan_platform_data *pdata;
> +};
> +
> +static struct can_bittiming_const flexcan_bittiming_const = {
> +	.name = DRV_NAME,
> +	.tseg1_min = 4,
> +	.tseg1_max = 16,
> +	.tseg2_min = 2,
> +	.tseg2_max = 8,
> +	.sjw_max = 4,
> +	.brp_min = 1,
> +	.brp_max = 256,
> +	.brp_inc = 1,
> +};
> +
> +/*
> + * Swtich transceiver on or off
> + */
> +static void flexcan_transceiver_switch(const struct flexcan_priv *priv, int on)
> +{
> +	if (priv->pdata && priv->pdata->transceiver_switch)
> +		priv->pdata->transceiver_switch(on);
> +}
> +
> +static inline void flexcan_chip_enable(struct flexcan_priv *priv)
> +{
> +	struct flexcan_regs __iomem *regs = priv->base;
> +	u32 reg;
> +
> +	reg = readl(&regs->mcr);
> +	reg &= ~FLEXCAN_MCR_MDIS;
> +	writel(reg, &regs->mcr);
> +
> +	udelay(10);
> +}
> +
> +static inline void flexcan_chip_disable(struct flexcan_priv *priv)
> +{
> +	struct flexcan_regs __iomem *regs = priv->base;
> +	u32 reg;
> +
> +	reg = readl(&regs->mcr);
> +	reg |= FLEXCAN_MCR_MDIS;
> +	writel(reg, &regs->mcr);
> +}
> +
> +static int flexcan_get_berr_counter(const struct net_device *dev,
> +				    struct can_berr_counter *bec)
> +{
> +	const struct flexcan_priv *priv = netdev_priv(dev);
> +	struct flexcan_regs __iomem *regs = priv->base;
> +	u32 reg = readl(&regs->ecr);
> +
> +	bec->txerr = (reg >> 0) & 0xff;
> +	bec->rxerr = (reg >> 8) & 0xff;
> +
> +	return 0;
> +}
> +
> +
> +static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	const struct flexcan_priv *priv = netdev_priv(dev);
> +	struct net_device_stats *stats = &dev->stats;
> +	struct flexcan_regs __iomem *regs = priv->base;
> +	struct can_frame *frame = (struct can_frame *)skb->data;
> +	u32 can_id;
> +	u32 ctrl = FLEXCAN_MB_CNT_CODE(0xc) | (frame->can_dlc << 16);
> +
> +	if (can_dropped_invalid_skb(dev, skb))
> +		return NETDEV_TX_OK;
> +
> +	netif_stop_queue(dev);
> +
> +	if (frame->can_id & CAN_EFF_FLAG) {
> +		can_id = frame->can_id & CAN_EFF_MASK;
> +		ctrl |= FLEXCAN_MB_CNT_IDE | FLEXCAN_MB_CNT_SRR;
> +	} else {
> +		can_id = (frame->can_id & CAN_SFF_MASK) << 18;
> +	}
> +
> +	if (frame->can_id & CAN_RTR_FLAG)
> +		ctrl |= FLEXCAN_MB_CNT_RTR;
> +
> +	if (frame->can_dlc > 0) {
> +		u32 data;
> +		data = frame->data[0] << 24;
> +		data |= frame->data[1] << 16;
> +		data |= frame->data[2] << 8;
> +		data |= frame->data[3];
> +		writel(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[0]);
> +	}

IIRC, in your review of Jürgens patch you suggested to use be32_to_cpu here.

> +	if (frame->can_dlc > 3) {
> +		u32 data;
> +		data = frame->data[4] << 24;
> +		data |= frame->data[5] << 16;
> +		data |= frame->data[6] << 8;
> +		data |= frame->data[7];
> +		writel(data, &regs->cantxfg[FLEXCAN_TX_BUF_ID].data[1]);
> +	}

Ditto.

> +	writel(can_id, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_id);
> +	writel(ctrl, &regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
> +
> +	kfree_skb(skb);
> +
> +	stats->tx_bytes += frame->can_dlc;
> +
> +	return NETDEV_TX_OK;
> +}
> +
> +
> +static void flexcan_poll_err_frame(struct net_device *dev,
> +				   struct can_frame *cf, u32 reg_esr)
> +{
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	int error_warning = 0, rx_errors = 0, tx_errors = 0;
> +
> +	if (reg_esr & FLEXCAN_ESR_BIT1_ERR) {
> +		rx_errors = 1;
> +		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +		cf->data[2] |= CAN_ERR_PROT_BIT1;
> +	}
> +
> +	if (reg_esr & FLEXCAN_ESR_BIT0_ERR) {
> +		rx_errors = 1;
> +		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +		cf->data[2] |= CAN_ERR_PROT_BIT0;
> +	}
> +
> +	if (reg_esr & FLEXCAN_ESR_FRM_ERR) {
> +		rx_errors = 1;
> +		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +		cf->data[2] |= CAN_ERR_PROT_FORM;
> +	}
> +
> +	if (reg_esr & FLEXCAN_ESR_STF_ERR) {
> +		rx_errors = 1;
> +		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +		cf->data[2] |= CAN_ERR_PROT_STUFF;
> +	}
> +
> +
> +	if (reg_esr & FLEXCAN_ESR_ACK_ERR) {
> +		tx_errors = 1;
> +		cf->can_id |= CAN_ERR_ACK;

This is a bus-error as well. Therefore I think it should be:

	if (reg_esr & FLEXCAN_ESR_ACK_ERR) {
		tx_errors = 1;
		cf->can_id |= CAN_ERR_ACK;
		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
		cf->data[3] |= CAN_ERR_PROT_LOC_ACK; /* ACK slot */
	}

I need to check what CAN_ERR_ACK is intended for. Then, cf->can_id could
be preset with "CAN_ERR_PROT | CAN_ERR_BUSERROR" at the beginning.

> +	}
> +
> +	if (error_warning)
> +		priv->can.can_stats.error_warning++;

Hm, error_warning is always 0 !?

> +	if (rx_errors)
> +		dev->stats.rx_errors++;
> +	if (tx_errors)
> +		dev->stats.tx_errors++;
> +
> +}
> +
> +static void flexcan_poll_err(struct net_device *dev, u32 reg_esr)
> +{
> +	struct sk_buff *skb;
> +	struct can_frame *cf;
> +
> +	skb = alloc_can_err_skb(dev, &cf);
> +	if (unlikely(!skb))
> +		return;
> +
> +	flexcan_poll_err_frame(dev, cf, reg_esr);
> +	netif_receive_skb(skb);
> +
> +	dev->stats.rx_packets++;
> +	dev->stats.rx_bytes += cf->can_dlc;
> +}
> +
> +static void flexcan_read_fifo(const struct net_device *dev, struct can_frame *cf)
> +{
> +	const struct flexcan_priv *priv = netdev_priv(dev);
> +	struct flexcan_regs __iomem *regs = priv->base;
> +	struct flexcan_mb __iomem *mb = &regs->cantxfg[0];
> +	u32 reg_ctrl, reg_id;
> +
> +	reg_ctrl = readl(&mb->can_ctrl);
> +	reg_id = readl(&mb->can_id);
> +	if (reg_ctrl & FLEXCAN_MB_CNT_IDE)
> +		cf->can_id = ((reg_id >> 0) & CAN_EFF_MASK) | CAN_EFF_FLAG;
> +	else
> +		cf->can_id = (reg_id >> 18) & CAN_SFF_MASK;
> +
> +	if (reg_ctrl & FLEXCAN_MB_CNT_RTR)
> +		cf->can_id |= CAN_RTR_FLAG;
> +	cf->can_dlc = get_can_dlc((reg_ctrl >> 16) & 0xf);
> +
> +	*(__be32 *)(cf->data + 0) = cpu_to_be32(readl(&mb->data[0]));
> +	*(__be32 *)(cf->data + 4) = cpu_to_be32(readl(&mb->data[1]));
> +
> +	/* mark as read */
> +	writel(FLEXCAN_IFLAG_RX_FIFO_AVAILABLE, &regs->iflag1);
> +	readl(&regs->timer);
> +}
> +
> +static void flexcan_read_frame(struct net_device *dev)
> +{
> +	struct net_device_stats *stats = &dev->stats;
> +	struct can_frame *cf;
> +	struct sk_buff *skb;
> +
> +	skb = alloc_can_skb(dev, &cf);
> +	if (unlikely(!skb)) {
> +		stats->rx_dropped++;
> +		return;
> +	}
> +
> +	flexcan_read_fifo(dev, cf);
> +	netif_receive_skb(skb);
> +
> +	stats->rx_packets++;
> +	stats->rx_bytes += cf->can_dlc;
> +}
> +
> +static int flexcan_poll(struct napi_struct *napi, int quota)
> +{
> +	struct net_device *dev = napi->dev;
> +	const struct flexcan_priv *priv = netdev_priv(dev);
> +	struct flexcan_regs __iomem *regs = priv->base;
> +	u32 reg_iflag1, reg_esr;
> +	int work_done = 0;
> +
> +	reg_iflag1 = readl(&regs->iflag1);
> +
> +	/* first handle RX-FIFO */
> +	while (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE &&
> +	       work_done < quota) {
> +		flexcan_read_frame(dev);
> +
> +		work_done++;
> +		reg_iflag1 = readl(&regs->iflag1);
> +	}
> +
> +	/*
> +	 * The error bits are clear on read,
> +	 * so use saved value from irq handler.
> +	 */
> +	reg_esr = readl(&regs->esr) | priv->reg_esr;

Re-reading reg_esr may cause lost of state changes.

> +	if (work_done < quota) {
> +		flexcan_poll_err(dev, reg_esr);

An error frame is created here for each call of flexcan_poll(), not only
in case of errors.

> +		work_done++;
> +	}
> +
> +	if (work_done < quota) {
> +		napi_complete(napi);
> +		/* enable IRQs */
> +		writel(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
> +		writel(priv->reg_ctrl_default, &regs->ctrl);
> +	}
> +
> +	return work_done;
> +}
> +
> +static void flexcan_irq_err_state(struct net_device *dev,
> +				  struct can_frame *cf, enum can_state new_state)
> +{
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	struct can_berr_counter bec;
> +
> +	flexcan_get_berr_counter(dev, &bec);
> +
> +	switch (priv->can.state) {
> +	case CAN_STATE_ERROR_ACTIVE:
> +		/*
> +		 * from: ERROR_ACTIVE
> +		 * to  : ERROR_WARNING, ERROR_PASSIVE, BUS_OFF
> +		 * =>  : there was a warning int
> +		 */
> +		if (new_state >= CAN_STATE_ERROR_WARNING &&
> +		    new_state <= CAN_STATE_BUS_OFF) {
> +			dev_dbg(dev->dev.parent, "Error Warning IRQ\n");
> +			priv->can.can_stats.error_warning++;
> +
> +			cf->can_id |= CAN_ERR_CRTL;
> +			cf->data[1] = (bec.txerr > bec.rxerr) ?
> +				CAN_ERR_CRTL_TX_WARNING :
> +				CAN_ERR_CRTL_RX_WARNING;
> +		}
> +	case CAN_STATE_ERROR_WARNING:	/* fallthrough */
> +		/*
> +		 * from: ERROR_ACTIVE, ERROR_WARNING
> +		 * to  : ERROR_PASSIVE, BUS_OFF
> +		 * =>  : error passive int
> +		 */
> +		if (new_state >= CAN_STATE_ERROR_PASSIVE &&
> +		    new_state <= CAN_STATE_BUS_OFF) {
> +			dev_dbg(dev->dev.parent, "Error Passive IRQ\n");
> +			priv->can.can_stats.error_passive++;
> +
> +			cf->can_id |= CAN_ERR_CRTL;
> +			cf->data[1] = (bec.txerr > bec.rxerr) ?
> +				CAN_ERR_CRTL_TX_PASSIVE :
> +				CAN_ERR_CRTL_RX_PASSIVE;
> +		}
> +		break;
> +	case CAN_STATE_BUS_OFF:
> +		dev_err(dev->dev.parent,
> +			"BUG! hardware recovered automatically from BUS_OFF\n");
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	/* process state changes depending on the new state */
> +	switch (new_state) {
> +	case CAN_STATE_BUS_OFF:
> +		cf->can_id |= CAN_ERR_BUSOFF;
> +		can_bus_off(dev);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +static void flexcan_irq_err(struct net_device *dev)
> +{
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	struct flexcan_regs __iomem *regs = priv->base;
> +	struct sk_buff *skb;
> +	struct can_frame *cf;
> +	enum can_state new_state;
> +	u32 reg_esr;
> +	int flt;
> +
> +	reg_esr = readl(&regs->esr);

As discussed, reg_esr is re-read here. Should probably be passed via
function argument.

> +	writel(reg_esr, &regs->esr);
> +
> +	flt = reg_esr & FLEXCAN_ESR_FLT_CONF_MASK;
> +	if (likely(flt == FLEXCAN_ESR_FLT_CONF_ACTIVE)) {
> +		if (likely(!(reg_esr & (FLEXCAN_ESR_TX_WRN |
> +					FLEXCAN_ESR_RX_WRN))))
> +			new_state = CAN_STATE_ERROR_ACTIVE;
> +		else
> +			new_state = CAN_STATE_ERROR_WARNING;
> +	} else if (unlikely(flt == FLEXCAN_ESR_FLT_CONF_PASSIVE))
> +		new_state = CAN_STATE_ERROR_PASSIVE;
> +	else
> +		new_state = CAN_STATE_BUS_OFF;
> +
> +	/* state hasn't changed */
> +	if (likely(new_state == priv->can.state))
> +		return;
> +
> +	skb = alloc_can_err_skb(dev, &cf);
> +	if (unlikely(!skb))
> +		return;
> +
> +	flexcan_irq_err_state(dev, cf, new_state);
> +	netif_rx(skb);
> +
> +	dev->stats.rx_packets++;
> +	dev->stats.rx_bytes += cf->can_dlc;
> +
> +	priv->can.state = new_state;
> +}
> +
> +static irqreturn_t flexcan_irq(int irq, void *dev_id)
> +{
> +	struct net_device *dev = dev_id;
> +	struct net_device_stats *stats = &dev->stats;
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	struct flexcan_regs __iomem *regs = priv->base;
> +	u32 reg_iflag1, reg_esr;
> +
> +	reg_iflag1 = readl(&regs->iflag1);
> +	reg_esr = readl(&regs->esr);
> +
> +	/* receive or error interrupt -> napi */
> +	if ((reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_AVAILABLE) ||
> +	    (reg_esr & FLEXCAN_ESR_ERR_FRAME)) {
> +		/*
> +		 * The error bits are cleared on read,
> +		 * save for later use.
> +		 */
> +		priv->reg_esr = reg_esr;
> +		writel(FLEXCAN_IFLAG_DEFAULT & ~FLEXCAN_IFLAG_RX_FIFO_AVAILABLE,
> +		       &regs->imask1);
> +		writel(priv->reg_ctrl_default & ~FLEXCAN_CTRL_ERR_MSK,
> +		       &regs->ctrl);
> +		napi_schedule(&priv->napi);
> +	}
> +
> +	/* FIFO overflow */
> +	if (reg_iflag1 & FLEXCAN_IFLAG_RX_FIFO_OVERFLOW) {
> +		writel(FLEXCAN_IFLAG_RX_FIFO_OVERFLOW, &regs->iflag1);
> +		dev->stats.rx_over_errors++;
> +		dev->stats.rx_errors++;
> +	}
> +
> +	/* transmission complete interrupt */
> +	if (reg_iflag1 & (1 << FLEXCAN_TX_BUF_ID)) {
> +		stats->tx_packets++;

Where is stats->tx_bytes incremented?

> +		writel((1 << FLEXCAN_TX_BUF_ID), &regs->iflag1);
> +		netif_wake_queue(dev);
> +	}
> +
> +	/* handle state changes */
> +	flexcan_irq_err(dev);

This does create and send an error message for *each* interrupt, even
for non-error interrupts (RX and TX).

Also, state changes are handled in the hard-irq context, while the
errors are handle in the soft-irq context. This looks tricky and error
prune to me as both are derived from the esr register. State changes and
errors might not be realized in the correct order, etc. It might be
saver to handle both in the poll routine by a common function.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void flexcan_set_bittiming(struct net_device *dev)
> +{
> +	const struct flexcan_priv *priv = netdev_priv(dev);
> +	const struct can_bittiming *bt = &priv->can.bittiming;
> +	struct flexcan_regs __iomem *regs = priv->base;
> +	u32 reg;
> +
> +	reg = readl(&regs->ctrl);
> +	reg &= ~(FLEXCAN_CTRL_PRESDIV(0xff) |
> +		 FLEXCAN_CTRL_RJW(0x3) |
> +		 FLEXCAN_CTRL_PSEG1(0x7) |
> +		 FLEXCAN_CTRL_PSEG2(0x7) |
> +		 FLEXCAN_CTRL_PROPSEG(0x7) |
> +		 FLEXCAN_CTRL_LPB |
> +		 FLEXCAN_CTRL_SMP |
> +		 FLEXCAN_CTRL_LOM);
> +
> +	reg |= FLEXCAN_CTRL_PRESDIV(bt->brp - 1) |
> +		FLEXCAN_CTRL_PSEG1(bt->phase_seg1 - 1) |
> +		FLEXCAN_CTRL_PSEG2(bt->phase_seg2 - 1) |
> +		FLEXCAN_CTRL_RJW(bt->sjw - 1) |
> +		FLEXCAN_CTRL_PROPSEG(bt->prop_seg - 1);
> +
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> +		reg |= FLEXCAN_CTRL_LPB;
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> +		reg |= FLEXCAN_CTRL_LOM;
> +	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> +		reg |= FLEXCAN_CTRL_SMP;
> +
> +	dev_info(dev->dev.parent, "writing ctrl=0x%08x\n", reg);
> +	writel(reg, &regs->ctrl);
> +
> +	/* print chip status */
> +	dev_dbg(dev->dev.parent, "%s: mcr=0x%08x ctrl=0x%08x\n", __func__,
> +		readl(&regs->mcr), readl(&regs->ctrl));

This seems especially useful for development. Please check the other
dev_dbg's as well.

> +}
> +
> +/*
> + * flexcan_chip_start
> + *
> + * this functions is entered with clocks enabled
> + *
> + */
> +static int flexcan_chip_start(struct net_device *dev)
> +{
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	struct flexcan_regs __iomem *regs = priv->base;
> +	unsigned int i;
> +	int err;
> +	u32 reg_mcr, reg_ctrl;
> +
> +	/* enable module */
> +	flexcan_chip_enable(priv);
> +
> +	/* soft reset */
> +	writel(FLEXCAN_MCR_SOFTRST, &regs->mcr);
> +	udelay(10);
> +
> +	reg_mcr = readl(&regs->mcr);
> +	if (reg_mcr & FLEXCAN_MCR_SOFTRST) {
> +		dev_err(dev->dev.parent,
> +			"Failed to softreset can module (mcr=0x%08x)\n", reg_mcr);
> +		err = -ENODEV;
> +		goto out;
> +	}
> +
> +	flexcan_set_bittiming(dev);
> +
> +	/*
> +	 * MCR
> +	 *
> +	 * enable freeze
> +	 * enable fifo
> +	 * halt now
> +	 * only supervisor access
> +	 * enable warning int
> +	 * choose format C
> +	 *
> +	 */
> +	reg_mcr = readl(&regs->mcr);
> +	reg_mcr |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_FEN | FLEXCAN_MCR_HALT |
> +		FLEXCAN_MCR_SUPV | FLEXCAN_MCR_WRN_EN |
> +		FLEXCAN_MCR_IDAM_C;
> +	dev_dbg(dev->dev.parent, "%s: writing mcr=0x%08x", __func__, reg_mcr);
> +	writel(reg_mcr, &regs->mcr);
> +
> +	/*
> +	 * CTRL
> +	 *
> +	 * enable bus off interrupt
> +	 * disable auto busoff recovery
> +	 * enable tx and rx warning interrupt
> +	 * transmit lowest buffer first
> +	 */
> +	reg_ctrl = readl(&regs->ctrl);
> +	reg_ctrl |= FLEXCAN_CTRL_BOFF_MSK |FLEXCAN_CTRL_BOFF_REC |
> +		FLEXCAN_CTRL_TWRN_MSK | FLEXCAN_CTRL_RWRN_MSK |
> +		FLEXCAN_CTRL_LBUF;
> +	/*
> +	 * TODO: for now turn on the error interrupt, otherwise we
> +	 * don't get any warning or bus passive interrupts.
> +	 */
> +	reg_ctrl |= FLEXCAN_CTRL_ERR_MSK;
> +
> +	/* save for later use */
> +	priv->reg_ctrl_default = reg_ctrl;
> +	dev_dbg(dev->dev.parent, "%s: writing ctrl=0x%08x", __func__, reg_ctrl);
> +	writel(reg_ctrl, &regs->ctrl);
> +
> +	for (i = 0; i < ARRAY_SIZE(regs->cantxfg); i++) {
> +		writel(0, &regs->cantxfg[i].can_ctrl);
> +		writel(0, &regs->cantxfg[i].can_id);
> +		writel(0, &regs->cantxfg[i].data[0]);
> +		writel(0, &regs->cantxfg[i].data[1]);
> +
> +		/* put MB into rx queue */
> +		writel(FLEXCAN_MB_CNT_CODE(0x4), &regs->cantxfg[i].can_ctrl);
> +	}
> +
> +	/* acceptance mask/acceptance code (accept everything) */
> +	writel(0x0, &regs->rxgmask);
> +	writel(0x0, &regs->rx14mask);
> +	writel(0x0, &regs->rx15mask);
> +
> +	flexcan_transceiver_switch(priv, 1);
> +
> +	/* synchronize with the can bus */
> +	reg_mcr = readl(&regs->mcr);
> +	reg_mcr &= ~FLEXCAN_MCR_HALT;
> +	writel(reg_mcr, &regs->mcr);
> +
> +	priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +
> +	/* enable FIFO interrupts */
> +	writel(FLEXCAN_IFLAG_DEFAULT, &regs->imask1);
> +
> +	/* print chip status */
> +	dev_dbg(dev->dev.parent, "%s: mcr=0x%08x ctrl=0x%08x\n", __func__,
> +		readl(&regs->mcr), readl(&regs->ctrl));
> +
> +	return 0;
> +
> + out:
> +	flexcan_chip_disable(priv);
> +	return err;
> +}
> +
> +/*
> + * flexcan_chip_stop
> + *
> + * this functions is entered with clocks enabled
> + *
> + */
> +static void flexcan_chip_stop(struct net_device *dev)
> +{
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	struct flexcan_regs __iomem *regs = priv->base;
> +	u32 reg;
> +
> +	/* Disable all interrupts */
> +	writel(0, &regs->imask1);
> +
> +	/* Disable + halt module */
> +	reg = readl(&regs->mcr);
> +	reg |= FLEXCAN_MCR_MDIS | FLEXCAN_MCR_HALT;
> +	writel(reg, &regs->mcr);
> +
> +	flexcan_transceiver_switch(priv, 0);
> +	priv->can.state = CAN_STATE_STOPPED;
> +
> +	return;
> +}
> +
> +static int flexcan_open(struct net_device *dev)
> +{
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	int err;
> +
> +	clk_enable(priv->clk);
> +
> +	err = open_candev(dev);
> +	if (err)
> +		goto out;
> +
> +	err = request_irq(dev->irq, flexcan_irq, IRQF_SHARED, dev->name, dev);
> +	if (err)
> +		goto out_close;
> +
> +	/* start chip and queuing */
> +	err = flexcan_chip_start(dev);
> +	if (err)
> +		goto out_close;
> +	napi_enable(&priv->napi);
> +	netif_start_queue(dev);
> +
> +	return 0;
> +
> + out_close:
> +	close_candev(dev);
> + out:
> +	clk_disable(priv->clk);
> +
> +	return err;
> +}
> +
> +static int flexcan_close(struct net_device *dev)
> +{
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +
> +	netif_stop_queue(dev);
> +	napi_disable(&priv->napi);
> +	flexcan_chip_stop(dev);
> +
> +	free_irq(dev->irq, dev);
> +	clk_disable(priv->clk);
> +
> +	close_candev(dev);
> +
> +	return 0;
> +}
> +
> +static int flexcan_set_mode(struct net_device *dev, enum can_mode mode)
> +{
> +	int err;
> +
> +	switch (mode) {
> +	case CAN_MODE_START:
> +		err = flexcan_chip_start(dev);
> +		if (err)
> +			return err;
> +
> +		netif_wake_queue(dev);
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct net_device_ops flexcan_netdev_ops = {
> +	.ndo_open	= flexcan_open,
> +	.ndo_stop	= flexcan_close,
> +	.ndo_start_xmit	= flexcan_start_xmit,
> +};
> +
> +static int __devinit register_flexcandev(struct net_device *dev)
> +{
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	struct flexcan_regs __iomem *regs = priv->base;
> +	u32 reg, err;
> +
> +	clk_enable(priv->clk);
> +
> +	/* select "bus clock", chip must be disabled */
> +	flexcan_chip_disable(priv);
> +	reg = readl(&regs->ctrl);
> +	reg |= FLEXCAN_CTRL_CLK_SRC;
> +	writel(reg, &regs->ctrl);
> +
> +	flexcan_chip_enable(priv);
> +
> +	/* set freeze, halt and activate FIFO, restrict register access */
> +	reg = readl(&regs->mcr);
> +	reg |= FLEXCAN_MCR_FRZ | FLEXCAN_MCR_HALT |
> +		FLEXCAN_MCR_FEN | FLEXCAN_MCR_SUPV;
> +	writel(reg, &regs->mcr);
> +
> +	/*
> +	 * Currently we only support newer versions of this core
> +	 * featuring a RX FIFO. Older cores found on some Coldfire
> +	 * derivates are not yet supported.
> +	 */
> +	reg = readl(&regs->mcr);
> +	if (!(reg & FLEXCAN_MCR_FEN)) {
> +		dev_err(dev->dev.parent,
> +			"Could not enable RX FIFO, unsupported core\n");
> +		err = -ENODEV;
> +		goto out;
> +	}
> +
> +	err = register_candev(dev);
> +
> + out:
> +	/* disable core and turn off clocks */
> +	flexcan_chip_disable(priv);
> +	clk_disable(priv->clk);
> +
> +	return err;
> +}
> +
> +static void __devexit unregister_flexcandev(struct net_device *dev)
> +{
> +	unregister_candev(dev);
> +}
> +
> +static int __devinit flexcan_probe(struct platform_device *pdev)
> +{
> +	struct net_device *dev;
> +	struct flexcan_priv *priv;
> +	struct resource *mem;
> +	struct clk *clk;
> +	void __iomem *base;
> +	resource_size_t mem_size;
> +	int err, irq;
> +
> +	clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk)) {
> +		dev_err(&pdev->dev, "no clock defined\n");
> +		err = PTR_ERR(clk);
> +		goto failed_clock;
> +	}
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	irq = platform_get_irq(pdev, 0);
> +	if (!mem || irq <= 0) {
> +		err = -ENODEV;
> +		goto failed_get;
> +	}
> +
> +	mem_size = resource_size(mem);
> +	if (!request_mem_region(mem->start, mem_size, pdev->name)) {
> +		err = -EBUSY;
> +		goto failed_req;
> +	}
> +
> +	base = ioremap(mem->start, mem_size);
> +	if (!base) {
> +		err = -ENOMEM;
> +		goto failed_map;
> +	}
> +
> +	dev = alloc_candev(sizeof(struct flexcan_priv), 0);
> +	if (!dev) {
> +		err = -ENOMEM;
> +		goto failed_alloc;
> +	}
> +
> +	dev->netdev_ops = &flexcan_netdev_ops;
> +	dev->irq = irq;
> +	dev->flags |= IFF_ECHO; /* we support local echo in hardware */
> +
> +	priv = netdev_priv(dev);
> +	priv->can.clock.freq = clk_get_rate(clk);
> +	priv->can.bittiming_const = &flexcan_bittiming_const;
> +	priv->can.do_set_mode = flexcan_set_mode;
> +	priv->can.do_get_berr_counter = flexcan_get_berr_counter;
> +	priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
> +		CAN_CTRLMODE_LISTENONLY	| CAN_CTRLMODE_3_SAMPLES;
> +	priv->base = base;
> +	priv->dev = dev;
> +	priv->clk = clk;
> +	priv->pdata = pdev->dev.platform_data;
> +
> +	netif_napi_add(dev, &priv->napi, flexcan_poll, FLEXCAN_NAPI_WEIGHT);
> +
> +	dev_set_drvdata(&pdev->dev, dev);
> +	SET_NETDEV_DEV(dev, &pdev->dev);
> +
> +	err = register_flexcandev(dev);
> +	if (err) {
> +		dev_err(&pdev->dev, "registering netdev failed\n");
> +		goto failed_register;
> +	}
> +
> +	dev_info(&pdev->dev, "device registered (reg_base=%p, irq=%d)\n",
> +		 priv->base, dev->irq);
> +
> +	return 0;
> +
> + failed_register:
> +	free_candev(dev);
> + failed_alloc:
> +	iounmap(base);
> + failed_map:
> +	release_mem_region(mem->start, mem_size);
> + failed_req:
> +	clk_put(clk);
> + failed_get:
> + failed_clock:
> +	return err;
> +}
> +
> +static int __devexit flexcan_remove(struct platform_device *pdev)
> +{
> +	struct net_device *dev = platform_get_drvdata(pdev);
> +	struct flexcan_priv *priv = netdev_priv(dev);
> +	struct resource *mem;
> +
> +	unregister_flexcandev(dev);
> +	platform_set_drvdata(pdev, NULL);
> +	free_candev(dev);
> +	iounmap(priv->base);
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	release_mem_region(mem->start, resource_size(mem));
> +
> +	clk_put(priv->clk);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver flexcan_driver = {
> +	.driver.name = DRV_NAME,
> +	.probe = flexcan_probe,
> +	.remove = __devexit_p(flexcan_remove),
> +};
> +
> +static int __init flexcan_init(void)
> +{
> +	pr_info("%s netdevice driver\n", DRV_NAME);
> +	return platform_driver_register(&flexcan_driver);
> +}
> +
> +static void __exit flexcan_exit(void)
> +{
> +	platform_driver_unregister(&flexcan_driver);
> +	pr_info("%s: driver removed\n", DRV_NAME);
> +}
> +
> +module_init(flexcan_init);
> +module_exit(flexcan_exit);
> +
> +MODULE_AUTHOR("Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>, "
> +	      "Marc Kleine-Budde <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("CAN port driver for flexcan based chip");
> diff --git a/include/linux/can/platform/flexcan.h b/include/linux/can/platform/flexcan.h
> new file mode 100644
> index 0000000..72b713a
> --- /dev/null
> +++ b/include/linux/can/platform/flexcan.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright (C) 2010 Marc Kleine-Budde <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> + *
> + * This file is released under the GPLv2
> + *
> + */
> +
> +#ifndef __CAN_PLATFORM_FLEXCAN_H
> +#define __CAN_PLATFORM_FLEXCAN_H
> +
> +/**
> + * struct flexcan_platform_data - flex CAN controller platform data
> + * @transceiver_enable:         - called to power on/off the transceiver
> + *
> + */
> +struct flexcan_platform_data {
> +	void (*transceiver_switch)(int enable);
> +};
> +
> +#endif /* __CAN_PLATFORM_FLEXCAN_H */

Wolfgang.

^ permalink raw reply
* [PATCH 05/25] atm: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)
From: Peter Huewe @ 2010-07-15 18:41 UTC (permalink / raw)
  To: Kernel Janitors
  Cc: Chas Williams, David S. Miller, Eric Dumazet, Tejun Heo,
	linux-atm-general, netdev, linux-kernel

From: Peter Huewe <peterhuewe@gmx.de>

This patch converts pci_table entries, where .subvendor=PCI_ANY_ID and
.subdevice=PCI_ANY_ID, .class=0 and .class_mask=0, to use the
PCI_VDEVICE macro, and thus improves readability.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/atm/eni.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/atm/eni.c b/drivers/atm/eni.c
index 90a5a7c..80f9f36 100644
--- a/drivers/atm/eni.c
+++ b/drivers/atm/eni.c
@@ -2269,10 +2269,8 @@ out0:
 
 
 static struct pci_device_id eni_pci_tbl[] = {
-	{ PCI_VENDOR_ID_EF, PCI_DEVICE_ID_EF_ATM_FPGA, PCI_ANY_ID, PCI_ANY_ID,
-	  0, 0, 0 /* FPGA */ },
-	{ PCI_VENDOR_ID_EF, PCI_DEVICE_ID_EF_ATM_ASIC, PCI_ANY_ID, PCI_ANY_ID,
-	  0, 0, 1 /* ASIC */ },
+	{ PCI_VDEVICE(EF, PCI_DEVICE_ID_EF_ATM_FPGA), 0 /* FPGA */ },
+	{ PCI_VDEVICE(EF, PCI_DEVICE_ID_EF_ATM_ASIC), 1 /* ASIC */ },
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci,eni_pci_tbl);
-- 
1.7.1


^ permalink raw reply related
* [PATCH 06/25] atm: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)
From: Peter Huewe @ 2010-07-15 18:42 UTC (permalink / raw)
  To: Kernel Janitors
  Cc: Chas Williams, David S. Miller, Tejun Heo, linux-atm-general,
	netdev, linux-kernel

From: Peter Huewe <peterhuewe@gmx.de>

This patch converts pci_table entries, where .subvendor=PCI_ANY_ID and
.subdevice=PCI_ANY_ID, .class=0 and .class_mask=0, to use the
PCI_VDEVICE macro, and thus improves readability.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/atm/firestream.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/atm/firestream.c b/drivers/atm/firestream.c
index 6e600af..8717809 100644
--- a/drivers/atm/firestream.c
+++ b/drivers/atm/firestream.c
@@ -2027,10 +2027,8 @@ static void __devexit firestream_remove_one (struct pci_dev *pdev)
 }
 
 static struct pci_device_id firestream_pci_tbl[] = {
-	{ PCI_VENDOR_ID_FUJITSU_ME, PCI_DEVICE_ID_FUJITSU_FS50, 
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, FS_IS50},
-	{ PCI_VENDOR_ID_FUJITSU_ME, PCI_DEVICE_ID_FUJITSU_FS155, 
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, FS_IS155},
+	{ PCI_VDEVICE(FUJITSU_ME, PCI_DEVICE_ID_FUJITSU_FS50), FS_IS50},
+	{ PCI_VDEVICE(FUJITSU_ME, PCI_DEVICE_ID_FUJITSU_FS155), FS_IS155},
 	{ 0, }
 };
 
-- 
1.7.1


^ permalink raw reply related
* [PATCH 07/25] atm: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)
From: Peter Huewe @ 2010-07-15 18:44 UTC (permalink / raw)
  To: Kernel Janitors
  Cc: Chas Williams, David S. Miller, Roel Kluin, Eric Dumazet,
	Tejun Heo, linux-atm-general, netdev, linux-kernel

From: Peter Huewe <peterhuewe@gmx.de>

This patch converts pci_table entries, where .subvendor=PCI_ANY_ID and
.subdevice=PCI_ANY_ID, .class=0 and .class_mask=0, to use the
PCI_VDEVICE macro, and thus improves readability.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/atm/he.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/atm/he.c b/drivers/atm/he.c
index fa4d600..801e8b6 100644
--- a/drivers/atm/he.c
+++ b/drivers/atm/he.c
@@ -2870,8 +2870,7 @@ module_param(sdh, bool, 0);
 MODULE_PARM_DESC(sdh, "use SDH framing (default 0)");
 
 static struct pci_device_id he_pci_tbl[] = {
-	{ PCI_VENDOR_ID_FORE, PCI_DEVICE_ID_FORE_HE, PCI_ANY_ID, PCI_ANY_ID,
-	  0, 0, 0 },
+	{ PCI_VDEVICE(FORE, PCI_DEVICE_ID_FORE_HE), 0 },
 	{ 0, }
 };
 
-- 
1.7.1

^ permalink raw reply related
* [PATCH 3/8] drivers: irda: fix sign bug
From: Kulikov Vasiliy @ 2010-07-15 18:44 UTC (permalink / raw)
  To: kernel-janitors; +Cc: Samuel Ortiz, David S. Miller, Kuninori Morimoto, netdev

platform_get_irq_byname() can return negative results, it is not seen to
unsigned irq. Make it signed.

Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
---
 drivers/net/irda/sh_irda.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/irda/sh_irda.c b/drivers/net/irda/sh_irda.c
index 9a828b0..edd5666 100644
--- a/drivers/net/irda/sh_irda.c
+++ b/drivers/net/irda/sh_irda.c
@@ -749,7 +749,7 @@ static int __devinit sh_irda_probe(struct platform_device *pdev)
 	struct sh_irda_self *self;
 	struct resource *res;
 	char clk_name[8];
-	unsigned int irq;
+	int irq;
 	int err = -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
1.7.0.4


^ permalink raw reply related
* [PATCH 08/25] atm: Convert pci_table entries to PCI_VDEVICE (if PCI_ANY_ID is used)
From: Peter Huewe @ 2010-07-15 18:45 UTC (permalink / raw)
  To: Kernel Janitors
  Cc: Chas Williams, David S. Miller, H Hartley Sweeten, Tejun Heo,
	linux-atm-general, netdev, linux-kernel

From: Peter Huewe <peterhuewe@gmx.de>

This patch converts pci_table entries, where .subvendor=PCI_ANY_ID and
.subdevice=PCI_ANY_ID, .class=0 and .class_mask=0, to use the
PCI_VDEVICE macro, and thus improves readability.

Signed-off-by: Peter Huewe <peterhuewe@gmx.de>
---
 drivers/atm/idt77252.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/atm/idt77252.c b/drivers/atm/idt77252.c
index 558ad18..1679cbf 100644
--- a/drivers/atm/idt77252.c
+++ b/drivers/atm/idt77252.c
@@ -3779,8 +3779,7 @@ err_out_disable_pdev:
 
 static struct pci_device_id idt77252_pci_tbl[] =
 {
-	{ PCI_VENDOR_ID_IDT, PCI_DEVICE_ID_IDT_IDT77252,
-	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
+	{ PCI_VDEVICE(IDT, PCI_DEVICE_ID_IDT_IDT77252), 0 },
 	{ 0, }
 };
 
-- 
1.7.1

^ permalink raw reply related
* [PATCH 4/8] drivers: irda: fix sign bug
From: Kulikov Vasiliy @ 2010-07-15 18:45 UTC (permalink / raw)
  To: kernel-janitors
  Cc: Samuel Ortiz, David S. Miller, Kuninori Morimoto, Tejun Heo,
	netdev

platform_get_irq_byname() can return negative results, it is not seen to
unsigned irq. Make it signed.

Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
---
 drivers/net/irda/sh_sir.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/irda/sh_sir.c b/drivers/net/irda/sh_sir.c
index 5c5f99d..00b38bc 100644
--- a/drivers/net/irda/sh_sir.c
+++ b/drivers/net/irda/sh_sir.c
@@ -709,7 +709,7 @@ static int __devinit sh_sir_probe(struct platform_device *pdev)
 	struct sh_sir_self *self;
 	struct resource *res;
 	char clk_name[8];
-	unsigned int irq;
+	int irq;
 	int err = -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
1.7.0.4


^ permalink raw reply related
* [PATCH 5/8] drivers: ixgbevf: fix unsigned underflow
From: Kulikov Vasiliy @ 2010-07-15 18:45 UTC (permalink / raw)
  To: kernel-janitors
  Cc: David S. Miller, Jeff Kirsher, Greg Rose, Eric Dumazet,
	Joe Perches, netdev

'count' is unsigned. It is initialized to zero, then it can be increased
multiple times, and finally it is used in such a way:

   >>>> count--;
   |
   |    /* clear timestamp and dma mappings for remaining portion of packet */
   |    while (count >= 0) {
   |            count--;
   |            ...
   ^
If count is zero here (so, it was never increased), we would have a very
long loop :)

Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
---
 drivers/net/ixgbevf/ixgbevf_main.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ixgbevf/ixgbevf_main.c b/drivers/net/ixgbevf/ixgbevf_main.c
index 73f1e75..af49135 100644
--- a/drivers/net/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ixgbevf/ixgbevf_main.c
@@ -2935,7 +2935,8 @@ static int ixgbevf_tx_map(struct ixgbevf_adapter *adapter,
 	struct ixgbevf_tx_buffer *tx_buffer_info;
 	unsigned int len;
 	unsigned int total = skb->len;
-	unsigned int offset = 0, size, count = 0;
+	unsigned int offset = 0, size;
+	int count = 0;
 	unsigned int nr_frags = skb_shinfo(skb)->nr_frags;
 	unsigned int f;
 	int i;
-- 
1.7.0.4


^ permalink raw reply related
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox