[PATCH] 3c59x: sparse warning fix

[PATCH] 3c59x: sparse warning fix

[Stephen Hemminger]

--- a/drivers/net/3c59x.c	2007-09-05 15:15:16.000000000 +0100
+++ b/drivers/net/3c59x.c	2007-09-05 15:16:29.000000000 +0100
@@ -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. */

Re: [PATCH] 3c59x: sparse warning fix

[Joe Perches]

On Wed, 2007-09-05 at 15:23 +0100, Stephen Hemminger wrote:
> --- a/drivers/net/3c59x.c	2007-09-05 15:15:16.000000000 +0100
> +++ b/drivers/net/3c59x.c	2007-09-05 15:16:29.000000000 +0100
> @@ -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. */

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)
		}

Re: [PATCH] 3c59x: sparse warning fix

[Francois Romieu]

Joe Perches <joe@perches.com> :
[...]
> 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.

-- 
Ueimor

Re: [PATCH] 3c59x: sparse warning fix

[Stephen Hemminger]

On Wed, 05 Sep 2007 09:57:05 -0700
Joe Perches <joe@perches.com> wrote:

> On Wed, 2007-09-05 at 15:23 +0100, Stephen Hemminger wrote:
> > --- a/drivers/net/3c59x.c	2007-09-05 15:15:16.000000000 +0100
> > +++ b/drivers/net/3c59x.c	2007-09-05 15:16:29.000000000 +0100
> > @@ -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. */
> 
> 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)
> 		}

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.

Re: [PATCH] 3c59x: sparse warning fix

[Steffen Klassert]

On Wed, Sep 05, 2007 at 03:23:59PM +0100, Stephen Hemminger wrote:
> --- a/drivers/net/3c59x.c	2007-09-05 15:15:16.000000000 +0100
> +++ b/drivers/net/3c59x.c	2007-09-05 15:16:29.000000000 +0100
> @@ -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. */
> -

Thanks,

Acked-by: Steffen Klassert <klassert@mathematik.tu-chemnitz.de>