Re: [dpdk-dev] [PATCH v2 02/18] ixgbe: Clean up IXGBE base codes

[Neil Horman <nhorman@tuxdriver.com>]

On Mon, Sep 29, 2014 at 03:16:10PM +0800, Ouyang Changchun wrote:
> This patch cleans up some IXGBE base codes, such as remove unnecessary return statement,
> and reduce goto statement etc.
> 
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> ---
>  lib/librte_pmd_ixgbe/ixgbe/ixgbe_82598.c     | 2 --
>  lib/librte_pmd_ixgbe/ixgbe/ixgbe_82599.c     | 7 +++----
>  lib/librte_pmd_ixgbe/ixgbe/ixgbe_common.c    | 2 +-
>  lib/librte_pmd_ixgbe/ixgbe/ixgbe_common.h    | 2 --
>  lib/librte_pmd_ixgbe/ixgbe/ixgbe_dcb_82598.c | 2 ++
>  lib/librte_pmd_ixgbe/ixgbe/ixgbe_dcb_82599.c | 1 +
>  lib/librte_pmd_ixgbe/ixgbe/ixgbe_phy.c       | 3 ++-
>  7 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_82598.c b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_82598.c
> index ee2217d..c8ce893 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_82598.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_82598.c
> @@ -1417,8 +1417,6 @@ STATIC void ixgbe_set_rxpba_82598(struct ixgbe_hw *hw, int num_pb,
>  	/* Setup Tx packet buffer sizes */
>  	for (i = 0; i < IXGBE_MAX_PACKET_BUFFERS; i++)
>  		IXGBE_WRITE_REG(hw, IXGBE_TXPBSIZE(i), IXGBE_TXPBSIZE_40KB);
> -
> -	return;
>  }
>  
>  /**
> diff --git a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_82599.c b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_82599.c
> index 835331b..046a35e 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe/ixgbe_82599.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe/ixgbe_82599.c
> @@ -2103,7 +2103,7 @@ s32 ixgbe_identify_phy_82599(struct ixgbe_hw *hw)
>  	if (status != IXGBE_SUCCESS) {
>  		/* 82599 10GBASE-T requires an external PHY */
>  		if (hw->mac.ops.get_media_type(hw) == ixgbe_media_type_copper)
> -			goto out;
> +			return status;
>  		else
>  			status = ixgbe_identify_module_generic(hw);
>  	}
> @@ -2111,14 +2111,13 @@ s32 ixgbe_identify_phy_82599(struct ixgbe_hw *hw)
>  	/* Set PHY type none if no PHY detected */
>  	if (hw->phy.type == ixgbe_phy_unknown) {
>  		hw->phy.type = ixgbe_phy_none;
> -		status = IXGBE_SUCCESS;
> +		return IXGBE_SUCCESS;
>  	}
>  
>  	/* Return error if SFP module has been detected but is not supported */
>  	if (hw->phy.type == ixgbe_phy_sfp_unsupported)
> -		status = IXGBE_ERR_SFP_NOT_SUPPORTED;
> +		return IXGBE_ERR_SFP_NOT_SUPPORTED;
>  
> -out:
>  	return status;
>  }
>  
How is this a cleanup?  I understand that you've removed a set of goto
statements from this function, and, while I don't think gotos are a problem, its
fine that you did.  But there are literally dozens of other goto statements that
could be cleaned up in a simmilar fashion that you ignored in this patch.  Why
just this one location?

Also, isn't this code lifted directly from the linux ixgbe driver?  Wouldn't it
be prudent to just keep it in line with that driver as much as possible?

Neil