Re: [dpdk-dev] [PATCH] Fix misleading indentation in ethtool

["De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>]

Hi Pengyu,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Pengyu Ma
> Sent: Friday, July 01, 2016 9:53 AM
> To: thomas.monjalon@6wind.com; dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] Fix misleading indentation in ethtool

You need to add the library that you are fixing in the title:
i.e. kni: fix misleading identation in ethtool


> 
> gcc complains about:
> build/lib/librte_eal/linuxapp/kni/e1000_phy.c:3303:2:
> error: this 'if' clause does not guard... [-Werror=misleading-indentation]

Could you tell me which gcc version you are using?

> 
> Code indentation is misleadingly indented as whether
> the following content is guarded by if or not.
> With the reference of the context, add the curly braces.
> 
> Remove unused const variables too.

You are fixing part of a library, so you should add a fixes line here.
Please refer to the following document to know how to do it:
http://dpdk.readthedocs.io/en/v16.04/contributing/patches.html

> 
> Signed-off-by: Pengyu Ma <pengyu.ma@windriver.com>
> ---
>  lib/librte_eal/linuxapp/kni/ethtool/igb/e1000_phy.c     | 9 ++++++---
>  lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_82599.c | 8 +++++---
>  lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_main.c  | 5 +++++
>  3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/kni/ethtool/igb/e1000_phy.c
> b/lib/librte_eal/linuxapp/kni/ethtool/igb/e1000_phy.c
> index df22470..120e57b 100644
> --- a/lib/librte_eal/linuxapp/kni/ethtool/igb/e1000_phy.c
> +++ b/lib/librte_eal/linuxapp/kni/ethtool/igb/e1000_phy.c
> @@ -3300,12 +3300,13 @@ s32 e1000_read_phy_reg_mphy(struct e1000_hw
> *hw, u32 address, u32 *data)
>  	*data = E1000_READ_REG(hw, E1000_MPHY_DATA);
> 
>  	/* Disable access to mPHY if it was originally disabled */
> -	if (locked)
> +	if (locked) {
>  		ready = e1000_is_mphy_ready(hw);
>  		if (!ready)
>  			return -E1000_ERR_PHY;
>  		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
>  				E1000_MPHY_DIS_ACCESS);
> +	}

You are actually fixing the code flow, right?
So, not sure if you should modify the title and body of the patch.

> 
>  	return E1000_SUCCESS;
>  }
> @@ -3365,12 +3366,14 @@ s32 e1000_write_phy_reg_mphy(struct
> e1000_hw *hw, u32 address, u32 data,
>  	E1000_WRITE_REG(hw, E1000_MPHY_DATA, data);
> 
>  	/* Disable access to mPHY if it was originally disabled */
> -	if (locked)
> +	if (locked) {
>  		ready = e1000_is_mphy_ready(hw);
> -		if (!ready)
> +		if (!ready) {
>  			return -E1000_ERR_PHY;
> +		}

Not needed it, but harmless :)

>  		E1000_WRITE_REG(hw, E1000_MPHY_ADDR_CTRL,
>  				E1000_MPHY_DIS_ACCESS);
> +	}
> 
>  	return E1000_SUCCESS;
>  }
> diff --git a/lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_82599.c
> b/lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_82599.c
> index 017dfe1..71cbf1a 100644
> --- a/lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_82599.c
> +++ b/lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_82599.c
> @@ -867,14 +867,16 @@ s32 ixgbe_setup_mac_link_82599(struct ixgbe_hw
> *hw,
>  	    link_mode == IXGBE_AUTOC_LMS_KX4_KX_KR_SGMII) {
>  		/* Set KX4/KX/KR support according to speed requested */
>  		autoc &= ~(IXGBE_AUTOC_KX4_KX_SUPP_MASK |
> IXGBE_AUTOC_KR_SUPP);
> -		if (speed & IXGBE_LINK_SPEED_10GB_FULL)
> +		if (speed & IXGBE_LINK_SPEED_10GB_FULL) {
>  			if (orig_autoc & IXGBE_AUTOC_KX4_SUPP)
>  				autoc |= IXGBE_AUTOC_KX4_SUPP;
>  			if ((orig_autoc & IXGBE_AUTOC_KR_SUPP) &&
> -			    (hw->phy.smart_speed_active == false))
> +				(hw->phy.smart_speed_active == false))
>  				autoc |= IXGBE_AUTOC_KR_SUPP;
> -		if (speed & IXGBE_LINK_SPEED_1GB_FULL)
> +		}
> +		if (speed & IXGBE_LINK_SPEED_1GB_FULL) {
>  			autoc |= IXGBE_AUTOC_KX_SUPP;
> +		}
>  	} else if ((pma_pmd_1g == IXGBE_AUTOC_1G_SFI) &&
>  		   (link_mode == IXGBE_AUTOC_LMS_1G_LINK_NO_AN ||
>  		    link_mode == IXGBE_AUTOC_LMS_1G_AN)) {
> diff --git a/lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_main.c
> b/lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_main.c
> index 8c1d2fe..1e9f9d1 100644
> --- a/lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_main.c
> +++ b/lib/librte_eal/linuxapp/kni/ethtool/ixgbe/ixgbe_main.c
> @@ -59,8 +59,11 @@
>  #undef CONFIG_DCA_MODULE
> 
>  char ixgbe_driver_name[] = "ixgbe";
> +/*
>  static const char ixgbe_driver_string[] =
>  			      "Intel(R) 10 Gigabit PCI Express Network Driver";
> +*/

This line and the one below that you are commenting out have been removed in this patch
(http://dpdk.org/dev/patchwork/patch/14276/) and it has been applied, so no need to touch this file :)

Make sure that you rebase your patch against head before submitting a v2, to avoid this issue.

> +
>  #define DRV_HW_PERF
> 
>  #ifndef CONFIG_IXGBE_NAPI
> @@ -79,8 +82,10 @@ static const char ixgbe_driver_string[] =
>  #define DRV_VERSION	__stringify(MAJ) "." __stringify(MIN) "." \
>  			__stringify(BUILD) DRIVERNAPI DRV_HW_PERF FPGA
> VMDQ_TAG
>  const char ixgbe_driver_version[] = DRV_VERSION;
> +/*
>  static const char ixgbe_copyright[] =
>  				"Copyright (c) 1999-2012 Intel Corporation.";
> +*/
> 
>  /* ixgbe_pci_tbl - PCI Device ID Table
>   *
> --
> 2.7.4