From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 7C4416893 for ; Mon, 29 Sep 2014 16:49:12 +0200 (CEST) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1XYcMX-0004PB-OJ; Mon, 29 Sep 2014 10:55:47 -0400 Date: Mon, 29 Sep 2014 10:55:41 -0400 From: Neil Horman To: Ouyang Changchun Message-ID: <20140929145541.GE26483@hmsreliant.think-freely.org> References: <1411974986-28137-1-git-send-email-changchun.ouyang@intel.com> <1411974986-28137-3-git-send-email-changchun.ouyang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1411974986-28137-3-git-send-email-changchun.ouyang@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v2 02/18] ixgbe: Clean up IXGBE base codes X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Sep 2014 14:49:12 -0000 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 > --- > 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