[PATCH 1/1] net: pegasus: remove dead coding

[PATCH 1/1] net: pegasus: remove dead coding

[Heinrich Schuchardt]

(!count || count < 4) is always true.
So let's remove the coding which is dead at least since 2005.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 drivers/net/usb/pegasus.c | 53 -----------------------------------------------
 1 file changed, 53 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 36cd7f0..1903d2e 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -470,61 +470,8 @@ static void read_bulk_callback(struct urb *urb)
 		return;
 	default:
 		netif_dbg(pegasus, rx_err, net, "RX status %d\n", status);
-		goto goon;
 	}
 
-	if (!count || count < 4)
-		goto goon;
-
-	rx_status = buf[count - 2];
-	if (rx_status & 0x1e) {
-		netif_dbg(pegasus, rx_err, net,
-			  "RX packet error %x\n", rx_status);
-		pegasus->stats.rx_errors++;
-		if (rx_status & 0x06)	/* long or runt	*/
-			pegasus->stats.rx_length_errors++;
-		if (rx_status & 0x08)
-			pegasus->stats.rx_crc_errors++;
-		if (rx_status & 0x10)	/* extra bits	*/
-			pegasus->stats.rx_frame_errors++;
-		goto goon;
-	}
-	if (pegasus->chip == 0x8513) {
-		pkt_len = le32_to_cpu(*(__le32 *)urb->transfer_buffer);
-		pkt_len &= 0x0fff;
-		pegasus->rx_skb->data += 2;
-	} else {
-		pkt_len = buf[count - 3] << 8;
-		pkt_len += buf[count - 4];
-		pkt_len &= 0xfff;
-		pkt_len -= 4;
-	}
-
-	/*
-	 * If the packet is unreasonably long, quietly drop it rather than
-	 * kernel panicing by calling skb_put.
-	 */
-	if (pkt_len > PEGASUS_MTU)
-		goto goon;
-
-	/*
-	 * at this point we are sure pegasus->rx_skb != NULL
-	 * so we go ahead and pass up the packet.
-	 */
-	skb_put(pegasus->rx_skb, pkt_len);
-	pegasus->rx_skb->protocol = eth_type_trans(pegasus->rx_skb, net);
-	netif_rx(pegasus->rx_skb);
-	pegasus->stats.rx_packets++;
-	pegasus->stats.rx_bytes += pkt_len;
-
-	if (pegasus->flags & PEGASUS_UNPLUG)
-		return;
-
-	pegasus->rx_skb = __netdev_alloc_skb_ip_align(pegasus->net, PEGASUS_MTU,
-						      GFP_ATOMIC);
-
-	if (pegasus->rx_skb == NULL)
-		goto tl_sched;
 goon:
 	usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
 			  usb_rcvbulkpipe(pegasus->usb, 1),
-- 
2.1.4

[PATCH 1/1] net: pegasus: remove dead coding

[Heinrich Schuchardt]

(!count || count < 4) is always true.
So let's remove the coding which is dead at least since 2005.

Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/net/usb/pegasus.c | 53 -----------------------------------------------
 1 file changed, 53 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 36cd7f0..1903d2e 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -470,61 +470,8 @@ static void read_bulk_callback(struct urb *urb)
 		return;
 	default:
 		netif_dbg(pegasus, rx_err, net, "RX status %d\n", status);
-		goto goon;
 	}
 
-	if (!count || count < 4)
-		goto goon;
-
-	rx_status = buf[count - 2];
-	if (rx_status & 0x1e) {
-		netif_dbg(pegasus, rx_err, net,
-			  "RX packet error %x\n", rx_status);
-		pegasus->stats.rx_errors++;
-		if (rx_status & 0x06)	/* long or runt	*/
-			pegasus->stats.rx_length_errors++;
-		if (rx_status & 0x08)
-			pegasus->stats.rx_crc_errors++;
-		if (rx_status & 0x10)	/* extra bits	*/
-			pegasus->stats.rx_frame_errors++;
-		goto goon;
-	}
-	if (pegasus->chip == 0x8513) {
-		pkt_len = le32_to_cpu(*(__le32 *)urb->transfer_buffer);
-		pkt_len &= 0x0fff;
-		pegasus->rx_skb->data += 2;
-	} else {
-		pkt_len = buf[count - 3] << 8;
-		pkt_len += buf[count - 4];
-		pkt_len &= 0xfff;
-		pkt_len -= 4;
-	}
-
-	/*
-	 * If the packet is unreasonably long, quietly drop it rather than
-	 * kernel panicing by calling skb_put.
-	 */
-	if (pkt_len > PEGASUS_MTU)
-		goto goon;
-
-	/*
-	 * at this point we are sure pegasus->rx_skb != NULL
-	 * so we go ahead and pass up the packet.
-	 */
-	skb_put(pegasus->rx_skb, pkt_len);
-	pegasus->rx_skb->protocol = eth_type_trans(pegasus->rx_skb, net);
-	netif_rx(pegasus->rx_skb);
-	pegasus->stats.rx_packets++;
-	pegasus->stats.rx_bytes += pkt_len;
-
-	if (pegasus->flags & PEGASUS_UNPLUG)
-		return;
-
-	pegasus->rx_skb = __netdev_alloc_skb_ip_align(pegasus->net, PEGASUS_MTU,
-						      GFP_ATOMIC);

Re: [PATCH 1/1] net: pegasus: remove dead coding

[Petko Manolov]

On 16-05-18 02:13:30, Heinrich Schuchardt wrote:
> (!count || count < 4) is always true.
> So let's remove the coding which is dead at least since 2005.

You may want to reconsider the above statement.  Just assume that 'count' is 
typically between 56 and 1514 bytes.


		Petko


> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/net/usb/pegasus.c | 53 -----------------------------------------------
>  1 file changed, 53 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 36cd7f0..1903d2e 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -470,61 +470,8 @@ static void read_bulk_callback(struct urb *urb)
>  		return;
>  	default:
>  		netif_dbg(pegasus, rx_err, net, "RX status %d\n", status);
> -		goto goon;
>  	}
>  
> -	if (!count || count < 4)
> -		goto goon;
> -
> -	rx_status = buf[count - 2];
> -	if (rx_status & 0x1e) {
> -		netif_dbg(pegasus, rx_err, net,
> -			  "RX packet error %x\n", rx_status);
> -		pegasus->stats.rx_errors++;
> -		if (rx_status & 0x06)	/* long or runt	*/
> -			pegasus->stats.rx_length_errors++;
> -		if (rx_status & 0x08)
> -			pegasus->stats.rx_crc_errors++;
> -		if (rx_status & 0x10)	/* extra bits	*/
> -			pegasus->stats.rx_frame_errors++;
> -		goto goon;
> -	}
> -	if (pegasus->chip == 0x8513) {
> -		pkt_len = le32_to_cpu(*(__le32 *)urb->transfer_buffer);
> -		pkt_len &= 0x0fff;
> -		pegasus->rx_skb->data += 2;
> -	} else {
> -		pkt_len = buf[count - 3] << 8;
> -		pkt_len += buf[count - 4];
> -		pkt_len &= 0xfff;
> -		pkt_len -= 4;
> -	}
> -
> -	/*
> -	 * If the packet is unreasonably long, quietly drop it rather than
> -	 * kernel panicing by calling skb_put.
> -	 */
> -	if (pkt_len > PEGASUS_MTU)
> -		goto goon;
> -
> -	/*
> -	 * at this point we are sure pegasus->rx_skb != NULL
> -	 * so we go ahead and pass up the packet.
> -	 */
> -	skb_put(pegasus->rx_skb, pkt_len);
> -	pegasus->rx_skb->protocol = eth_type_trans(pegasus->rx_skb, net);
> -	netif_rx(pegasus->rx_skb);
> -	pegasus->stats.rx_packets++;
> -	pegasus->stats.rx_bytes += pkt_len;
> -
> -	if (pegasus->flags & PEGASUS_UNPLUG)
> -		return;
> -
> -	pegasus->rx_skb = __netdev_alloc_skb_ip_align(pegasus->net, PEGASUS_MTU,
> -						      GFP_ATOMIC);
> -
> -	if (pegasus->rx_skb == NULL)
> -		goto tl_sched;
>  goon:
>  	usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
>  			  usb_rcvbulkpipe(pegasus->usb, 1),
> -- 
> 2.1.4
> 
> 

Re: [1/1] net: pegasus: remove dead coding

[Guenter Roeck]

On Wed, May 18, 2016 at 02:13:30AM +0200, Heinrich Schuchardt wrote:
> (!count || count < 4) is always true.

Even if count >= 4 ?

Guenter

> So let's remove the coding which is dead at least since 2005.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> ---
> drivers/net/usb/pegasus.c | 53 -----------------------------------------------
>  1 file changed, 53 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 36cd7f0..1903d2e 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -470,61 +470,8 @@ static void read_bulk_callback(struct urb *urb)
>  		return;
>  	default:
>  		netif_dbg(pegasus, rx_err, net, "RX status %d\n", status);
> -		goto goon;
>  	}
>  
> -	if (!count || count < 4)
> -		goto goon;
> -
> -	rx_status = buf[count - 2];
> -	if (rx_status & 0x1e) {
> -		netif_dbg(pegasus, rx_err, net,
> -			  "RX packet error %x\n", rx_status);
> -		pegasus->stats.rx_errors++;
> -		if (rx_status & 0x06)	/* long or runt	*/
> -			pegasus->stats.rx_length_errors++;
> -		if (rx_status & 0x08)
> -			pegasus->stats.rx_crc_errors++;
> -		if (rx_status & 0x10)	/* extra bits	*/
> -			pegasus->stats.rx_frame_errors++;
> -		goto goon;
> -	}
> -	if (pegasus->chip == 0x8513) {
> -		pkt_len = le32_to_cpu(*(__le32 *)urb->transfer_buffer);
> -		pkt_len &= 0x0fff;
> -		pegasus->rx_skb->data += 2;
> -	} else {
> -		pkt_len = buf[count - 3] << 8;
> -		pkt_len += buf[count - 4];
> -		pkt_len &= 0xfff;
> -		pkt_len -= 4;
> -	}
> -
> -	/*
> -	 * If the packet is unreasonably long, quietly drop it rather than
> -	 * kernel panicing by calling skb_put.
> -	 */
> -	if (pkt_len > PEGASUS_MTU)
> -		goto goon;
> -
> -	/*
> -	 * at this point we are sure pegasus->rx_skb != NULL
> -	 * so we go ahead and pass up the packet.
> -	 */
> -	skb_put(pegasus->rx_skb, pkt_len);
> -	pegasus->rx_skb->protocol = eth_type_trans(pegasus->rx_skb, net);
> -	netif_rx(pegasus->rx_skb);
> -	pegasus->stats.rx_packets++;
> -	pegasus->stats.rx_bytes += pkt_len;
> -
> -	if (pegasus->flags & PEGASUS_UNPLUG)
> -		return;
> -
> -	pegasus->rx_skb = __netdev_alloc_skb_ip_align(pegasus->net, PEGASUS_MTU,
> -						      GFP_ATOMIC);
> -
> -	if (pegasus->rx_skb == NULL)
> -		goto tl_sched;
>  goon:
>  	usb_fill_bulk_urb(pegasus->rx_urb, pegasus->usb,
>  			  usb_rcvbulkpipe(pegasus->usb, 1),

Re: [1/1] net: pegasus: remove dead coding

[Oliver Neukum]

On Tue, 2016-05-17 at 23:30 -0700, Guenter Roeck wrote:
> On Wed, May 18, 2016 at 02:13:30AM +0200, Heinrich Schuchardt wrote:
> > (!count || count < 4) is always true.
> 
> Even if count >= 4 ?

The check for !count is redundant, though. Gcc, however,
will surely simplify the expression.

	Regards
		Oliver

Re: [1/1] net: pegasus: remove dead coding

[Petko Manolov]

On 16-05-18 09:15:40, Oliver Neukum wrote:
> On Tue, 2016-05-17 at 23:30 -0700, Guenter Roeck wrote:
> > On Wed, May 18, 2016 at 02:13:30AM +0200, Heinrich Schuchardt wrote:
> > > (!count || count < 4) is always true.
> > 
> > Even if count >= 4 ?
> 
> The check for !count is redundant, though. Gcc, however,
> will surely simplify the expression.

Yep, gcc-6 generates this code:

	...
	cmp	$0x3,%edx
	jle	b9 <read_bulk_callback+0x69>
	...

Which does not invalidate your statement that "!count" is redundant. :)


		Petko

[PATCH 1/1] net: pegasus: simplify logical constraint

[Heinrich Schuchardt]

If !count is true, count < 4 is also true.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 drivers/net/usb/pegasus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 36cd7f0..9bbe0161 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -473,7 +473,7 @@ static void read_bulk_callback(struct urb *urb)
 		goto goon;
 	}
 
-	if (!count || count < 4)
+	if (count < 4)
 		goto goon;
 
 	rx_status = buf[count - 2];
-- 
2.1.4

Re: [PATCH 1/1] net: pegasus: simplify logical constraint

[Petko Manolov]

On 16-05-18 20:40:51, Heinrich Schuchardt wrote:
> If !count is true, count < 4 is also true.

Yep, you're right.  However, gcc optimizes away the first condition.  What you 
really got me to think about is whether 4 is the right number.  I guess i shall 
refer to the HW documentation.


		Petko


> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/net/usb/pegasus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 36cd7f0..9bbe0161 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -473,7 +473,7 @@ static void read_bulk_callback(struct urb *urb)
>  		goto goon;
>  	}
>  
> -	if (!count || count < 4)
> +	if (count < 4)
>  		goto goon;
>  
>  	rx_status = buf[count - 2];
> -- 
> 2.1.4
> 
> 

Re: [PATCH 1/1] net: pegasus: remove dead coding

[David Miller]

From: Heinrich Schuchardt <xypron.glpk@gmx.de>
Date: Wed, 18 May 2016 02:13:30 +0200

> (!count || count < 4) is always true.
> So let's remove the coding which is dead at least since 2005.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Applied.

Re: [PATCH 1/1] net: pegasus: remove dead coding

[Petko Manolov]

On 16-05-19 11:35:42, David Miller wrote:
> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Date: Wed, 18 May 2016 02:13:30 +0200
> 
> > (!count || count < 4) is always true.
> > So let's remove the coding which is dead at least since 2005.
> > 
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> Applied.

David, the patch you applied is broken.  It seems that you didn't follow the 
discussion from the past couple of days.  Please revert it.


		Petko

Re: [PATCH 1/1] net: pegasus: remove dead coding

[David Miller]

From: Petko Manolov <petkan@mip-labs.com>
Date: Fri, 20 May 2016 10:33:47 +0300

> On 16-05-19 11:35:42, David Miller wrote:
>> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> Date: Wed, 18 May 2016 02:13:30 +0200
>> 
>> > (!count || count < 4) is always true.
>> > So let's remove the coding which is dead at least since 2005.
>> > 
>> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> 
>> Applied.
> 
> David, the patch you applied is broken.  It seems that you didn't follow the 
> discussion from the past couple of days.  Please revert it.

I did.

Re: [PATCH 1/1] net: pegasus: simplify logical constraint

[David Miller]

From: Heinrich Schuchardt <xypron.glpk@gmx.de>
Date: Wed, 18 May 2016 20:40:51 +0200

> If !count is true, count < 4 is also true.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Applied.