[Intel-wired-lan] [next-queue v2 1/6] i40evf: add missing kernel-doc argument

[Intel-wired-lan] [next-queue v2 1/6] i40evf: add missing kernel-doc argument

[=?unknown-8bit?q?J=CE=B5an?= Sacren]

From: Jean Sacren <sakiwit@gmail.com>

@flush has been missing since the inception of i40evf_irq_enable(). Add
it for the kernel doc.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
v2: For the whole series, drop the first two due to the simple mistake.
    
    For this patch, no changes except for the serial number.

 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
index 186914fe64bc..3b638f68b249 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
@@ -282,6 +282,7 @@ static void i40evf_fire_sw_int(struct i40evf_adapter *adapter, u32 mask)
 /**
  * i40evf_irq_enable - Enable default interrupt generation settings
  * @adapter: board private structure
+ * @flush: boolean value whether to run rd32()
  **/
 void i40evf_irq_enable(struct i40evf_adapter *adapter, bool flush)
 {

[Intel-wired-lan] [next-queue v2 2/6] i40e: add missing kernel-doc argument

[=?unknown-8bit?q?J=CE=B5an?= Sacren]

From: Jean Sacren <sakiwit@gmail.com>

The following kernell-doc arguments for their respective functions are
missing:

1) @cd_type_cmd_tso_mss for i40e_tso();
2) @cd_type_cmd_tso_mss for i40e_tsyn();
3) @tx_ring for i40e_tx_enable_csum().

Add them all for the kernel-doc requirement.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
v2: No changes except for the serial number.

 drivers/net/ethernet/intel/i40e/i40e_txrx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index c9f7caf1d920..8db8b9b6cdcd 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2187,6 +2187,7 @@ out:
  * @tx_ring:  ptr to the ring to send
  * @skb:      ptr to the skb we're sending
  * @hdr_len:  ptr to the size of the packet header
+ * @cd_type_cmd_tso_mss: ptr to u64 object
  * @cd_tunneling: ptr to context descriptor bits
  *
  * Returns 0 if no TSO can happen, 1 if tso is going, or error
@@ -2246,6 +2247,7 @@ static int i40e_tso(struct i40e_ring *tx_ring, struct sk_buff *skb,
  * @tx_ring:  ptr to the ring to send
  * @skb:      ptr to the skb we're sending
  * @tx_flags: the collected send information
+ * @cd_type_cmd_tso_mss: ptr to u64 object
  *
  * Returns 0 if no Tx timestamp can happen and 1 if the timestamp will happen
  **/
@@ -2288,6 +2290,7 @@ static int i40e_tsyn(struct i40e_ring *tx_ring, struct sk_buff *skb,
  * @tx_flags: pointer to Tx flags currently set
  * @td_cmd: Tx descriptor command bits to set
  * @td_offset: Tx descriptor header offsets to set
+ * @tx_ring: Tx descriptor ring
  * @cd_tunneling: ptr to context desc bits
  **/
 static void i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,

[Intel-wired-lan] [next-queue v2 3/6] i40e: clean up the entire function exit path

[=?unknown-8bit?q?J=CE=B5an?= Sacren]

From: Jean Sacren <sakiwit@gmail.com>

In i40e_tsyn(), by using a label of 'no_timestamp', we will be able to
clearly denote a slew of circumstances that return with 0, which is much
better as a default exit than the one that returns with 1.

With the above change, in the last check, we will be able to spare the
'else' branch. Further, the whole branch for returning 1 shall be put
together for clarity.

Add a comment next to returning 1 in comparison to no_timestamp label.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
v2: No changes except for the serial number.

 drivers/net/ethernet/intel/i40e/i40e_txrx.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index 8db8b9b6cdcd..6cd091877731 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -2257,31 +2257,31 @@ static int i40e_tsyn(struct i40e_ring *tx_ring, struct sk_buff *skb,
 	struct i40e_pf *pf;
 
 	if (likely(!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)))
-		return 0;
+		goto no_timestamp;
 
 	/* Tx timestamps cannot be sampled when doing TSO */
 	if (tx_flags & I40E_TX_FLAGS_TSO)
-		return 0;
+		goto no_timestamp;
 
 	/* only timestamp the outbound packet if the user has requested it and
 	 * we are not already transmitting a packet to be timestamped
 	 */
 	pf = i40e_netdev_to_pf(tx_ring->netdev);
 	if (!(pf->flags & I40E_FLAG_PTP))
-		return 0;
+		goto no_timestamp;
 
 	if (pf->ptp_tx &&
 	    !test_and_set_bit_lock(__I40E_PTP_TX_IN_PROGRESS, &pf->state)) {
 		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
 		pf->ptp_tx_skb = skb_get(skb);
-	} else {
-		return 0;
-	}
+		*cd_type_cmd_tso_mss |= (u64)I40E_TX_CTX_DESC_TSYN <<
+					I40E_TXD_CTX_QW1_CMD_SHIFT;
 
-	*cd_type_cmd_tso_mss |= (u64)I40E_TX_CTX_DESC_TSYN <<
-				I40E_TXD_CTX_QW1_CMD_SHIFT;
+		return 1; /* Tx timestamp will happen */
+	}
 
-	return 1;
+no_timestamp:
+	return 0;
 }
 
 /**

[Intel-wired-lan] [next-queue v2 4/6] i40evf: clean up local variable initialization

[=?unknown-8bit?q?J=CE=B5an?= Sacren]

From: Jean Sacren <sakiwit@gmail.com>

In i40evf_msix_aq(), the first two lines of rd32() are mainly to clear
the registers. If we initialize 'val' at this point, it will be
overwritten immediately. We shall simply discard the return value here.

When we initialize 'val', we might as well include the mask in one step.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
v2: No changes except for the serial number.

 drivers/net/ethernet/intel/i40evf/i40evf_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
index 3b638f68b249..7d6d30c53060 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
@@ -308,11 +308,11 @@ static irqreturn_t i40evf_msix_aq(int irq, void *data)
 	u32 val;
 
 	/* handle non-queue interrupts, these reads clear the registers */
-	val = rd32(hw, I40E_VFINT_ICR01);
-	val = rd32(hw, I40E_VFINT_ICR0_ENA1);
+	rd32(hw, I40E_VFINT_ICR01);
+	rd32(hw, I40E_VFINT_ICR0_ENA1);
 
-	val = rd32(hw, I40E_VFINT_DYN_CTL01);
-	val = val | I40E_VFINT_DYN_CTL01_CLEARPBA_MASK;
+	val = rd32(hw, I40E_VFINT_DYN_CTL01) |
+	      I40E_VFINT_DYN_CTL01_CLEARPBA_MASK;
 	wr32(hw, I40E_VFINT_DYN_CTL01, val);
 
 	/* schedule work on the private workqueue */

[Intel-wired-lan] [next-queue v2 5/6] i40e: clean up local variable initialization

[=?unknown-8bit?q?J=CE=B5an?= Sacren]

From: Jean Sacren <sakiwit@gmail.com>

In both i40e_calc_nvm_checksum() and i40e_update_nvm_checksum(), the
local variables designated by 'ret_code' are overwritten immediately. As
such, they should merely be declared.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
v2: No changes except for the serial number.

 drivers/net/ethernet/intel/i40e/i40e_nvm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_nvm.c b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
index 58e384a372a0..1715358a8a6c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_nvm.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
@@ -484,7 +484,7 @@ static i40e_status i40e_write_nvm_aq(struct i40e_hw *hw, u8 module_pointer,
 static i40e_status i40e_calc_nvm_checksum(struct i40e_hw *hw,
 						    u16 *checksum)
 {
-	i40e_status ret_code = 0;
+	i40e_status ret_code;
 	struct i40e_virt_mem vmem;
 	u16 pcie_alt_module = 0;
 	u16 checksum_local = 0;
@@ -564,7 +564,7 @@ i40e_calc_nvm_checksum_exit:
  **/
 i40e_status i40e_update_nvm_checksum(struct i40e_hw *hw)
 {
-	i40e_status ret_code = 0;
+	i40e_status ret_code;
 	u16 checksum;
 	__le16 le_sum;
 

[Intel-wired-lan] [next-queue v2 6/6] i40e: fix unconditional execution of cpu_to_le16()

[=?unknown-8bit?q?J=CE=B5an?= Sacren]

From: Jean Sacren <sakiwit@gmail.com>

The commit 3092e5e4cc79 ("i40e: add little endian conversion for
checksum") fixed the checksum bug on big-endian architecture.

But we should not execute cpu_to_le16() unconditionally. Thus, put
cpu_to_le16() under certain condition.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
Cc: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
---
v2: Update patch serial number and add CC to Paul M Stillwell Jr.

 drivers/net/ethernet/intel/i40e/i40e_nvm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_nvm.c b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
index 1715358a8a6c..6100cdd9ad13 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_nvm.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
@@ -569,10 +569,11 @@ i40e_status i40e_update_nvm_checksum(struct i40e_hw *hw)
 	__le16 le_sum;
 
 	ret_code = i40e_calc_nvm_checksum(hw, &checksum);
-	le_sum = cpu_to_le16(checksum);
-	if (!ret_code)
+	if (!ret_code) {
+		le_sum = cpu_to_le16(checksum);
 		ret_code = i40e_write_nvm_aq(hw, 0x00, I40E_SR_SW_CHECKSUM_WORD,
 					     1, &le_sum, true);
+	}
 
 	return ret_code;
 }

[Intel-wired-lan] [next-queue v2 1/6] i40evf: add missing kernel-doc argument

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Jean Sacren
> Sent: Tuesday, October 13, 2015 12:06 AM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next-queue v2 1/6] i40evf: add missing kernel-doc
> argument
> 
> From: Jean Sacren <sakiwit@gmail.com>
> 
> @flush has been missing since the inception of i40evf_irq_enable(). Add it
> for the kernel doc.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> ---
> v2: For the whole series, drop the first two due to the simple mistake.
> 
>     For this patch, no changes except for the serial number.
> 
>  drivers/net/ethernet/intel/i40evf/i40evf_main.c | 1 +
>  1 file changed, 1 insertion(+)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Patch code changes correctly applied

[Intel-wired-lan] [next-queue v2 2/6] i40e: add missing kernel-doc argument

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Jean Sacren
> Sent: Tuesday, October 13, 2015 12:06 AM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next-queue v2 2/6] i40e: add missing kernel-doc
> argument
> 
> From: Jean Sacren <sakiwit@gmail.com>
> 
> The following kernell-doc arguments for their respective functions are
> missing:
> 
> 1) @cd_type_cmd_tso_mss for i40e_tso();
> 2) @cd_type_cmd_tso_mss for i40e_tsyn();
> 3) @tx_ring for i40e_tx_enable_csum().
> 
> Add them all for the kernel-doc requirement.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> ---
> v2: No changes except for the serial number.
> 
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c | 3 +++
>  1 file changed, 3 insertions(+)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Patch code changes correctly applied

[Intel-wired-lan] [next-queue v2 3/6] i40e: clean up the entire function exit path

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Jean Sacren
> Sent: Tuesday, October 13, 2015 12:06 AM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next-queue v2 3/6] i40e: clean up the entire
> function exit path
> 
> From: Jean Sacren <sakiwit@gmail.com>
> 
> In i40e_tsyn(), by using a label of 'no_timestamp', we will be able to clearly
> denote a slew of circumstances that return with 0, which is much better as a
> default exit than the one that returns with 1.
> 
> With the above change, in the last check, we will be able to spare the 'else'
> branch. Further, the whole branch for returning 1 shall be put together for
> clarity.
> 
> Add a comment next to returning 1 in comparison to no_timestamp label.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> ---
> v2: No changes except for the serial number.
> 
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Patch code changes correctly applied

[Intel-wired-lan] [next-queue v2 4/6] i40evf: clean up local variable initialization

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Jean Sacren
> Sent: Tuesday, October 13, 2015 12:07 AM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next-queue v2 4/6] i40evf: clean up local variable
> initialization
> 
> From: Jean Sacren <sakiwit@gmail.com>
> 
> In i40evf_msix_aq(), the first two lines of rd32() are mainly to clear the
> registers. If we initialize 'val' at this point, it will be overwritten immediately.
> We shall simply discard the return value here.
> 
> When we initialize 'val', we might as well include the mask in one step.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> ---
> v2: No changes except for the serial number.
> 
>  drivers/net/ethernet/intel/i40evf/i40evf_main.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Patch code changes correctly applied

[Intel-wired-lan] [next-queue v2 5/6] i40e: clean up local variable initialization

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Jean Sacren
> Sent: Tuesday, October 13, 2015 12:07 AM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next-queue v2 5/6] i40e: clean up local variable
> initialization
> 
> From: Jean Sacren <sakiwit@gmail.com>
> 
> In both i40e_calc_nvm_checksum() and i40e_update_nvm_checksum(), the
> local variables designated by 'ret_code' are overwritten immediately. As
> such, they should merely be declared.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> ---
> v2: No changes except for the serial number.
> 
>  drivers/net/ethernet/intel/i40e/i40e_nvm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Patch code changes correctly applied

[Intel-wired-lan] [next-queue v2 6/6] i40e: fix unconditional execution of cpu_to_le16()

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Jean Sacren
> Sent: Tuesday, October 13, 2015 12:07 AM
> To: intel-wired-lan at lists.osuosl.org
> Cc: Stillwell Jr, Paul M
> Subject: [Intel-wired-lan] [next-queue v2 6/6] i40e: fix unconditional
> execution of cpu_to_le16()
> 
> From: Jean Sacren <sakiwit@gmail.com>
> 
> The commit 3092e5e4cc79 ("i40e: add little endian conversion for
> checksum") fixed the checksum bug on big-endian architecture.
> 
> But we should not execute cpu_to_le16() unconditionally. Thus, put
> cpu_to_le16() under certain condition.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Cc: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> ---
> v2: Update patch serial number and add CC to Paul M Stillwell Jr.
> 
>  drivers/net/ethernet/intel/i40e/i40e_nvm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Patch code changes correctly applied

[Intel-wired-lan] [next-queue v2 1/6] i40evf: add missing kernel-doc argument

[Jesse Brandeburg]

On Tue, 13 Oct 2015 01:06:27 -0600
J?an Sacren <sakiwit@gmail.com> wrote:

> From: Jean Sacren <sakiwit@gmail.com>
> 
> @flush has been missing since the inception of i40evf_irq_enable(). Add
> it for the kernel doc.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>

Acked-by: Jesse Brandeburg <jesse.brandeburg@intel.com>

[Intel-wired-lan] [next-queue v2 2/6] i40e: add missing kernel-doc argument

[Jesse Brandeburg]

On Tue, 13 Oct 2015 01:06:28 -0600
J?an Sacren <sakiwit@gmail.com> wrote:

> From: Jean Sacren <sakiwit@gmail.com>
> 
> The following kernell-doc arguments for their respective functions are
> missing:
> 
> 1) @cd_type_cmd_tso_mss for i40e_tso();
> 2) @cd_type_cmd_tso_mss for i40e_tsyn();
> 3) @tx_ring for i40e_tx_enable_csum().
> 
> Add them all for the kernel-doc requirement.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> ---
> v2: No changes except for the serial number.

Acked-by: Jesse Brandeburg <jesse.brandeburg@intel.com>

[Intel-wired-lan] [next-queue v2 3/6] i40e: clean up the entire function exit path

[Jesse Brandeburg]

On Tue, 13 Oct 2015 01:06:29 -0600
J?an Sacren <sakiwit@gmail.com> wrote:

> From: Jean Sacren <sakiwit@gmail.com>
> 
> In i40e_tsyn(), by using a label of 'no_timestamp', we will be able to
> clearly denote a slew of circumstances that return with 0, which is much
> better as a default exit than the one that returns with 1.

What's the motivation for making this change?  Just code clean up? 

BTW thanks for the patches, I realize you spent a good amount of time
finding the issues/reviewing code.

> 
> With the above change, in the last check, we will be able to spare the
> 'else' branch. Further, the whole branch for returning 1 shall be put
> together for clarity.

Does sparing the "else branch" help anything?  I'm not quite sure what
the point of this change is beyond just code churn.  If it is deemed
better this way by someone else, then fine, and I'll ACK it, but the
change itself really seems unnecessary.

> 
> Add a comment next to returning 1 in comparison to no_timestamp label.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> ---
> v2: No changes except for the serial number.
> 
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index 8db8b9b6cdcd..6cd091877731 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -2257,31 +2257,31 @@ static int i40e_tsyn(struct i40e_ring *tx_ring, struct sk_buff *skb,
>  	struct i40e_pf *pf;
>  
>  	if (likely(!(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)))
> -		return 0;
> +		goto no_timestamp;
>  
>  	/* Tx timestamps cannot be sampled when doing TSO */
>  	if (tx_flags & I40E_TX_FLAGS_TSO)
> -		return 0;
> +		goto no_timestamp;
>  
>  	/* only timestamp the outbound packet if the user has requested it and
>  	 * we are not already transmitting a packet to be timestamped
>  	 */
>  	pf = i40e_netdev_to_pf(tx_ring->netdev);
>  	if (!(pf->flags & I40E_FLAG_PTP))
> -		return 0;
> +		goto no_timestamp;
>  
>  	if (pf->ptp_tx &&
>  	    !test_and_set_bit_lock(__I40E_PTP_TX_IN_PROGRESS, &pf->state)) {
>  		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
>  		pf->ptp_tx_skb = skb_get(skb);
> -	} else {
> -		return 0;
> -	}
> +		*cd_type_cmd_tso_mss |= (u64)I40E_TX_CTX_DESC_TSYN <<
> +					I40E_TXD_CTX_QW1_CMD_SHIFT;
>  
> -	*cd_type_cmd_tso_mss |= (u64)I40E_TX_CTX_DESC_TSYN <<
> -				I40E_TXD_CTX_QW1_CMD_SHIFT;
> +		return 1; /* Tx timestamp will happen */
> +	}
>  
> -	return 1;
> +no_timestamp:
> +	return 0;
>  }
>  
>  /**
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [next-queue v2 3/6] i40e: clean up the entire function exit path

[Tantilov, Emil S]

>-----Original Message-----
>From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
>Behalf Of Jesse Brandeburg
>Sent: Tuesday, October 13, 2015 9:59 AM
>To: J?an Sacren
>Cc: intel-wired-lan at lists.osuosl.org
>Subject: Re: [Intel-wired-lan] [next-queue v2 3/6] i40e: clean up the
>entire function exit path
>
>On Tue, 13 Oct 2015 01:06:29 -0600
>J?an Sacren <sakiwit@gmail.com> wrote:
>
>> From: Jean Sacren <sakiwit@gmail.com>
>>
>> In i40e_tsyn(), by using a label of 'no_timestamp', we will be able to
>> clearly denote a slew of circumstances that return with 0, which is much
>> better as a default exit than the one that returns with 1.
>
>What's the motivation for making this change?  Just code clean up?
>
>BTW thanks for the patches, I realize you spent a good amount of time
>finding the issues/reviewing code.
>
>>
>> With the above change, in the last check, we will be able to spare the
>> 'else' branch. Further, the whole branch for returning 1 shall be put
>> together for clarity.
>
>Does sparing the "else branch" help anything?  I'm not quite sure what
>the point of this change is beyond just code churn.  If it is deemed
>better this way by someone else, then fine, and I'll ACK it, but the
>change itself really seems unnecessary.

Actually this patch goes against guidelines of the coding style:
https://www.kernel.org/doc/Documentation/CodingStyle

Specifically the following paragraph:

The goto statement comes in handy when a function exits from multiple
locations and some common work such as cleanup has to be done.  If there is no
cleanup needed then just return directly.

So this is a clear NACK IMHO.

Thanks,
Emil