Skip to contentSkip to conversation

3c59x: sparse warning fix

PATCH In review Stephen Hemminger 4 participants 5 messages Sep 5, 2007 – Sep 13, 2007

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

PATCH 1/1 3c59x: sparse warning fix

drivers/net/3c59x.c +3 -2
@@ -1122,7 +1122,7 @@ static int __devinit vortex_probe1(struc
 					   + sizeof(struct boom_tx_desc) * TX_RING_SIZE,
 					   &vp->rx_ring_dma);
 	retval = -ENOMEM;
-	if (vp->rx_ring == 0)
+	if (!vp->rx_ring)
 		goto free_region;
 
 	vp->tx_ring = (struct boom_tx_desc *)(vp->rx_ring + RX_RING_SIZE);

@@ -2476,7 +2476,8 @@ boomerang_rx(struct net_device *dev)
 
 			/* Check if the packet is long enough to just accept without
 			   copying to a properly sized skbuff. */
-			if (pkt_len < rx_copybreak && (skb = dev_alloc_skb(pkt_len + 2)) != 0) {
+			if (pkt_len < rx_copybreak &&
+			    (skb = dev_alloc_skb(pkt_len + 2)) ) {
 				skb_reserve(skb, 2);	/* Align IP on 16 byte boundaries */
 				pci_dma_sync_single_for_cpu(VORTEX_PCI(vp), dma, PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
 				/* 'skb_put()' points to the start of sk_buff data area. */
#1 lore raw
Quoted 12 lines · Stephen Hemminger
drivers/net/3c59x.c +1 -1
@@ -1122,7 +1122,7 @@ static int __devinit vortex_probe1(struc
 					   + sizeof(struct boom_tx_desc) * TX_RING_SIZE,
 					   &vp->rx_ring_dma);
 	retval = -ENOMEM;
-	if (vp->rx_ring == 0)
+	if (!vp->rx_ring)
 		goto free_region;
 
 	vp->tx_ring = (struct boom_tx_desc *)(vp->rx_ring + RX_RING_SIZE);

drivers/net/3c59x.c boomerang_rx(struct net_device *dev) · line 2483

Quoted 9 lines
drivers/net/3c59x.c +2 -1
@@ -2476,7 +2476,8 @@ boomerang_rx(struct net_device *dev)
 
 			/* Check if the packet is long enough to just accept without
 			   copying to a properly sized skbuff. */
-			if (pkt_len < rx_copybreak && (skb = dev_alloc_skb(pkt_len + 2)) != 0) {
+			if (pkt_len < rx_copybreak &&
+			    (skb = dev_alloc_skb(pkt_len + 2)) ) {
 				skb_reserve(skb, 2);	/* Align IP on 16 byte boundaries */
 				pci_dma_sync_single_for_cpu(VORTEX_PCI(vp), dma, PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
 				/* 'skb_put()' points to the start of sk_buff data area. */

Shouldn't this be:

	if (pkt_len < rx_copybreak) {
		skb = dev_alloc_skb(pkt_len + 2);
		if (!skb) {
			bad_news! (like the refill rx ring buffers block)
		}
#2 lore raw

Joe Perches <[email protected]> :
[...]

Quoted 7 lines

Shouldn't this be:

	if (pkt_len < rx_copybreak) {
		skb = dev_alloc_skb(pkt_len + 2);
		if (!skb) {
			bad_news! (like the refill rx ring buffers block)
		}

It is not that bad a news: the rx_copybreak optimization is lost but it is still possible to push the data up the stack.

Signature · 1 line
Ueimor
author
#3 lore raw
In reply to Joe Perches
Quoted 25 lines · On Wed, 05 Sep 2007 09:57:05 -0700 Joe Perches <[email protected]> wrote:
Quoted 12 lines · Stephen Hemminger
drivers/net/3c59x.c +1 -1
@@ -1122,7 +1122,7 @@ static int __devinit vortex_probe1(struc
 					   + sizeof(struct boom_tx_desc) * TX_RING_SIZE,
 					   &vp->rx_ring_dma);
 	retval = -ENOMEM;
-	if (vp->rx_ring == 0)
+	if (!vp->rx_ring)
 		goto free_region;
 
 	vp->tx_ring = (struct boom_tx_desc *)(vp->rx_ring + RX_RING_SIZE);

drivers/net/3c59x.c boomerang_rx(struct net_device *dev) · line 2483

Quoted 9 lines
drivers/net/3c59x.c +2 -1
@@ -2476,7 +2476,8 @@ boomerang_rx(struct net_device *dev)
 
 			/* Check if the packet is long enough to just accept without
 			   copying to a properly sized skbuff. */
-			if (pkt_len < rx_copybreak && (skb = dev_alloc_skb(pkt_len + 2)) != 0) {
+			if (pkt_len < rx_copybreak &&
+			    (skb = dev_alloc_skb(pkt_len + 2)) ) {
 				skb_reserve(skb, 2);	/* Align IP on 16 byte boundaries */
 				pci_dma_sync_single_for_cpu(VORTEX_PCI(vp), dma, PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
 				/* 'skb_put()' points to the start of sk_buff data area. */

Shouldn't this be:

Quoted 5 lines
	if (pkt_len < rx_copybreak) {
		skb = dev_alloc_skb(pkt_len + 2);
		if (!skb) {
			bad_news! (like the refill rx ring buffers block)
		}

No, the existing code path does the right thing, and if skb can't be allocated it just reuses the existing one. Having a single error recovery path (for out of memory) is preferable to two paths.

Acked-by on v1 1/1
#4 lore raw
In reply to Stephen Hemminger
Quoted 12 lines · Stephen Hemminger
drivers/net/3c59x.c +1 -1
@@ -1122,7 +1122,7 @@ static int __devinit vortex_probe1(struc
 					   + sizeof(struct boom_tx_desc) * TX_RING_SIZE,
 					   &vp->rx_ring_dma);
 	retval = -ENOMEM;
-	if (vp->rx_ring == 0)
+	if (!vp->rx_ring)
 		goto free_region;
 
 	vp->tx_ring = (struct boom_tx_desc *)(vp->rx_ring + RX_RING_SIZE);

drivers/net/3c59x.c boomerang_rx(struct net_device *dev)

Quoted 10 lines
drivers/net/3c59x.c +2 -2
@@ -2476,7 +2476,8 @@ boomerang_rx(struct net_device *dev)
 
 			/* Check if the packet is long enough to just accept without
 			   copying to a properly sized skbuff. */
-			if (pkt_len < rx_copybreak && (skb = dev_alloc_skb(pkt_len + 2)) != 0) {
+			if (pkt_len < rx_copybreak &&
+			    (skb = dev_alloc_skb(pkt_len + 2)) ) {
 				skb_reserve(skb, 2);	/* Align IP on 16 byte boundaries */
 				pci_dma_sync_single_for_cpu(VORTEX_PCI(vp), dma, PKT_BUF_SZ, PCI_DMA_FROMDEVICE);
 				/* 'skb_put()' points to the start of sk_buff data area. */
-

Thanks,

Acked-by Steffen Klassert <[email protected]>