Re: [PATCH v3 2/5] staging: et131x: drop packet when error occurs in et131x_tx

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, Nov 21, 2013 at 09:19:21PM +0000, Mark Einon wrote:
> On Wed, Nov 20, 2013 at 03:55:27PM +0800, ZHAO Gang wrote:
> > As TODO file suggested, drop packet instead of return NETDEV_TX_BUSY
> > when tx failed.
> > 
> > et131x_tx calls function et131x_send_packets, I put the work of
> > et131x_send_packets directly into et131x_tx, and made some changes to
> > let the code more readable.
> > 
> > Signed-off-by: ZHAO Gang <gamerh2o@gmail.com>
> 
> Acked-by: Mark Einon <mark.einon@gmail.com>
> 
> > ---
> >  drivers/staging/et131x/et131x.c | 84 +++++++++++------------------------------
> >  1 file changed, 22 insertions(+), 62 deletions(-)
> > 
> > diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
> > index 836a4db..cda037a 100644
> > --- a/drivers/staging/et131x/et131x.c
> > +++ b/drivers/staging/et131x/et131x.c
> > @@ -3123,55 +3123,6 @@ static int send_packet(struct sk_buff *skb, struct et131x_adapter *adapter)
> >  	return 0;
> >  }
> >  
> > -/* et131x_send_packets - This function is called by the OS to send packets
> > - * @skb: the packet(s) to send
> > - * @netdev:device on which to TX the above packet(s)
> > - *
> > - * Return 0 in almost all cases; non-zero value in extreme hard failure only
> > - */
> > -static int et131x_send_packets(struct sk_buff *skb, struct net_device *netdev)
> > -{
> > -	int status = 0;
> > -	struct et131x_adapter *adapter = netdev_priv(netdev);
> > -
> > -	/* Send these packets
> > -	 *
> > -	 * NOTE: The Linux Tx entry point is only given one packet at a time
> > -	 * to Tx, so the PacketCount and it's array used makes no sense here
> > -	 */
> > -
> > -	/* TCB is not available */
> > -	if (adapter->tx_ring.used >= NUM_TCB) {
> > -		/* NOTE: If there's an error on send, no need to queue the
> > -		 * packet under Linux; if we just send an error up to the
> > -		 * netif layer, it will resend the skb to us.
> > -		 */
> > -		status = -ENOMEM;
> > -	} else {
> > -		/* We need to see if the link is up; if it's not, make the
> > -		 * netif layer think we're good and drop the packet
> > -		 */
> > -		if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
> > -					!netif_carrier_ok(netdev)) {
> > -			dev_kfree_skb_any(skb);
> > -			skb = NULL;
> > -
> > -			adapter->net_stats.tx_dropped++;
> > -		} else {
> > -			status = send_packet(skb, adapter);
> > -			if (status != 0 && status != -ENOMEM) {
> > -				/* On any other error, make netif think we're
> > -				 * OK and drop the packet
> > -				 */
> > -				dev_kfree_skb_any(skb);
> > -				skb = NULL;
> > -				adapter->net_stats.tx_dropped++;
> > -			}
> > -		}
> > -	}
> > -	return status;
> > -}
> > -
> >  /* free_send_packet - Recycle a struct tcb
> >   * @adapter: pointer to our adapter
> >   * @tcb: pointer to struct tcb
> > @@ -4537,12 +4488,9 @@ static void et131x_multicast(struct net_device *netdev)
> >  /* et131x_tx - The handler to tx a packet on the device
> >   * @skb: data to be Tx'd
> >   * @netdev: device on which data is to be Tx'd
> > - *
> > - * Returns 0 on success, errno on failure (as defined in errno.h)
> >   */
> > -static int et131x_tx(struct sk_buff *skb, struct net_device *netdev)
> > +static netdev_tx_t et131x_tx(struct sk_buff *skb, struct net_device *netdev)
> >  {
> > -	int status = 0;
> >  	struct et131x_adapter *adapter = netdev_priv(netdev);
> >  
> >  	/* stop the queue if it's getting full */
> > @@ -4553,17 +4501,29 @@ static int et131x_tx(struct sk_buff *skb, struct net_device *netdev)
> >  	/* Save the timestamp for the TX timeout watchdog */
> >  	netdev->trans_start = jiffies;
> >  
> > -	/* Call the device-specific data Tx routine */
> > -	status = et131x_send_packets(skb, netdev);
> > +	/* TCB is not available */
> > +	if (adapter->tx_ring.used >= NUM_TCB)
> > +		goto drop;
> >  
> > -	/* Check status and manage the netif queue if necessary */
> > -	if (status != 0) {
> > -		if (status == -ENOMEM)
> > -			status = NETDEV_TX_BUSY;
> > -		else
> > -			status = NETDEV_TX_OK;
> > +	/* We need to see if the link is up; if it's not, make the
> > +	 * netif layer think we're good and drop the packet
> > +	 */
> > +	if ((adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK) ||
> > +	    !netif_carrier_ok(netdev)) {
> > +		goto drop;
> > +	} else {
> > +		if (!send_packet(skb, adapter))
> > +			goto out;
> > +		/* On error (send_packet returns != 0), make netif
> > +		 * think we're OK and drop the packet
> > +		 */
> >  	}
> > -	return status;
> > +
> > +drop:
> > +	dev_kfree_skb_any(skb);
> > +	adapter->net_stats.tx_dropped++;
> > +out:
> > +	return NETDEV_TX_OK;
> >  }

This is a lot of spaghetti.  Just write it like this:


	if (adapter->tx_ring.used >= NUM_TCB)
		goto drop;
	if (adapter->flags & FMP_ADAPTER_FAIL_SEND_MASK)
		goto drop;
	if (!netif_carrier_ok(netdev))
		goto drop;
	status = send_packet(skb, adapter);
	if (status)
		goto drop;

	return NETDEV_TX_OK;

drop:
	dev_kfree_skb_any(skb);
	adapter->net_stats.tx_dropped++;

	/* We return success deliberately to make the netif layer happy */
	return NETDEV_TX_OK;

In the original code the success path was if condition false, then if
condition true, then follow a goto then return.  In the new code it is
straight line path to success with no nested if else statements.

regards,
dan carpenter