[Intel-wired-lan] [next-queue 0/8] Trivial fix-ups for Intel wired LAN drivers 09/10/2015

[Intel-wired-lan] [next-queue 0/8] Trivial fix-ups for Intel wired LAN drivers 09/10/2015

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

This series contains trivial fix-ups and cleanups for Intel wired LAN
drivers as of 09/10/2015:

1) Two trivial fix-ups for local object initialization and duplicate
   exit path.

2) Two cleanups for local variable and a keyword.

3) Four kernel-doc fix-ups.

Jean Sacren (8):
  e1000: clean up the useless 'continue' statement
  e1000: fix a typo in the comment
  e1000e: clean up the local variable
  i40e: fix kernel-doc argument name
  ixgbe: fix multiple kernel-doc errors
  i40e: declare rather than initialize int object
  e1000: fix kernel-doc argument being missing
  e1000: get rid of duplicate exit path

 drivers/net/ethernet/intel/e1000/e1000_hw.c   | 8 ++++----
 drivers/net/ethernet/intel/e1000/e1000_main.c | 7 ++++---
 drivers/net/ethernet/intel/e1000e/netdev.c    | 5 +----
 drivers/net/ethernet/intel/i40e/i40e_main.c   | 4 ++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 9 +++------
 5 files changed, 14 insertions(+), 19 deletions(-)

[Intel-wired-lan] [next-queue 1/8] e1000: clean up the useless 'continue' statement

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

From: Jean Sacren <sakiwit@gmail.com>

We can delete the 'continue' statement as it is in the end of the block
and doesn't do any good. To make it distinctive, put an empty line above
the checking block.

Additionally add a pair of braces in the 'else' branch.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
 drivers/net/ethernet/intel/e1000/e1000_main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 74dc15055971..47397d7b97df 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1199,12 +1199,13 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		for (i = 0; i < 32; i++) {
 			hw->phy_addr = i;
 			e1000_read_phy_reg(hw, PHY_ID2, &tmp);
+
 			if (tmp == 0 || tmp == 0xFF) {
 				if (i == 31)
 					goto err_eeprom;
-				continue;
-			} else
+			} else {
 				break;
+			}
 		}
 	}
 

[Intel-wired-lan] [next-queue 2/8] e1000: fix a typo in the comment

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

From: Jean Sacren <sakiwit@gmail.com>

Use 'That' to replace 'The' so that the comment would make sense.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
 drivers/net/ethernet/intel/e1000/e1000_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 47397d7b97df..7a7b14c5d524 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1264,7 +1264,7 @@ err_pci_reg:
  * @pdev: PCI device information struct
  *
  * e1000_remove is called by the PCI subsystem to alert the driver
- * that it should release a PCI device.  The could be caused by a
+ * that it should release a PCI device. That could be caused by a
  * Hot-Plug event, or because the driver is going to be removed from
  * memory.
  **/

[Intel-wired-lan] [next-queue 3/8] e1000e: clean up the local variable

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

From: Jean Sacren <sakiwit@gmail.com>

The local variable 'ret' doesn't serve much purpose so we might as well
clean it up.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 7be339eb9927..a501e7744597 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -7504,14 +7504,11 @@ static struct pci_driver e1000_driver = {
  **/
 static int __init e1000_init_module(void)
 {
-	int ret;
-
 	pr_info("Intel(R) PRO/1000 Network Driver - %s\n",
 		e1000e_driver_version);
 	pr_info("Copyright(c) 1999 - 2015 Intel Corporation.\n");
-	ret = pci_register_driver(&e1000_driver);
 
-	return ret;
+	return pci_register_driver(&e1000_driver);
 }
 module_init(e1000_init_module);
 

[Intel-wired-lan] [next-queue 4/8] i40e: fix kernel-doc argument name

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

From: Jean Sacren <sakiwit@gmail.com>

The second argument name in the kernel-doc argument list for
i40e_features_check() was slightly off. Fix it for the kernel doc.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index a702b810538e..23e0a4b007a2 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -8498,7 +8498,7 @@ static int i40e_ndo_bridge_getlink(struct sk_buff *skb, u32 pid, u32 seq,
 /**
  * i40e_features_check - Validate encapsulated packet conforms to limits
  * @skb: skb buff
- * @netdev: This physical port's netdev
+ * @dev: This physical port's netdev
  * @features: Offload features that the stack believes apply
  **/
 static netdev_features_t i40e_features_check(struct sk_buff *skb,

[Intel-wired-lan] [next-queue 5/8] ixgbe: fix multiple kernel-doc errors

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

From: Jean Sacren <sakiwit@gmail.com>

The commit dfaf891dd3e1 ("ixgbe: Refactor the RSS configuration code")
introduced a few kernel-doc errors:

1) The function name is missing;
2) The format is wrong;
3) The short description is redundant.

Fix all the above for the correct execution of the kernel doc.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 49be9d21d6b5..3489c35ead2c 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3311,8 +3311,7 @@ static void ixgbe_configure_srrctl(struct ixgbe_adapter *adapter,
 }
 
 /**
- * Return a number of entries in the RSS indirection table
- *
+ * ixgbe_rss_indir_tbl_entries - Return RSS indirection table entries
  * @adapter: device handle
  *
  *  - 82598/82599/X540:     128
@@ -3330,8 +3329,7 @@ u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
 }
 
 /**
- * Write the RETA table to HW
- *
+ * ixgbe_store_reta - Write the RETA table to HW
  * @adapter: device handle
  *
  * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
@@ -3370,8 +3368,7 @@ void ixgbe_store_reta(struct ixgbe_adapter *adapter)
 }
 
 /**
- * Write the RETA table to HW (for x550 devices in SRIOV mode)
- *
+ * ixgbe_store_vfreta - Write the RETA table to HW (x550 devices in SRIOV mode)
  * @adapter: device handle
  *
  * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.

[Intel-wired-lan] [next-queue 6/8] i40e: declare rather than initialize int object

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

From: Jean Sacren <sakiwit@gmail.com>

'err' would be overwritten immediately, so we should declare it only
rather than initialize it to zero.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 23e0a4b007a2..91224b38cbd3 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -10140,7 +10140,7 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	static u16 pfs_found;
 	u16 wol_nvm_bits;
 	u16 link_status;
-	int err = 0;
+	int err;
 	u32 len;
 	u32 i;
 

[Intel-wired-lan] [next-queue 7/8] e1000: fix kernel-doc argument being missing

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

From: Jean Sacren <sakiwit@gmail.com>

Due to historical reason, 'phy_data' has never been included in the
kernel doc. Fix it so that the requirement could be fulfilled.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
 drivers/net/ethernet/intel/e1000/e1000_hw.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c b/drivers/net/ethernet/intel/e1000/e1000_hw.c
index 2523e301c26c..a67a2c4499eb 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_hw.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c
@@ -2808,6 +2808,7 @@ static u16 e1000_shift_in_mdi_bits(struct e1000_hw *hw)
  * e1000_read_phy_reg - read a phy register
  * @hw: Struct containing variables accessed by shared code
  * @reg_addr: address of the PHY register to read
+ * @phy_data: pointer to the value on the PHY register
  *
  * Reads the value from a PHY register, if the value is on a specific non zero
  * page, sets the page first.

[Intel-wired-lan] [next-queue 8/8] e1000: get rid of duplicate exit path

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

From: Jean Sacren <sakiwit@gmail.com>

By using goto statement, we can achieve sharing the same exit path so
that code duplication could be minimized.

Signed-off-by: Jean Sacren <sakiwit@gmail.com>
---
 drivers/net/ethernet/intel/e1000/e1000_hw.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c b/drivers/net/ethernet/intel/e1000/e1000_hw.c
index a67a2c4499eb..737d5c79256b 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_hw.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c
@@ -2824,14 +2824,13 @@ s32 e1000_read_phy_reg(struct e1000_hw *hw, u32 reg_addr, u16 *phy_data)
 	    (reg_addr > MAX_PHY_MULTI_PAGE_REG)) {
 		ret_val = e1000_write_phy_reg_ex(hw, IGP01E1000_PHY_PAGE_SELECT,
 						 (u16) reg_addr);
-		if (ret_val) {
-			spin_unlock_irqrestore(&e1000_phy_lock, flags);
-			return ret_val;
-		}
+		if (ret_val)
+			goto out;
 	}
 
 	ret_val = e1000_read_phy_reg_ex(hw, MAX_PHY_REG_ADDRESS & reg_addr,
 					phy_data);
+out:
 	spin_unlock_irqrestore(&e1000_phy_lock, flags);
 
 	return ret_val;

[Intel-wired-lan] [next-queue 1/8] e1000: clean up the useless 'continue' statement

[Rustad, Mark D]

> On Sep 13, 2015, at 11:27 PM, J?an Sacren <sakiwit@gmail.com> wrote:
> 
> We can delete the 'continue' statement as it is in the end of the block
> and doesn't do any good. To make it distinctive, put an empty line above
> the checking block.
> 
> Additionally add a pair of braces in the 'else' branch.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> ---
> drivers/net/ethernet/intel/e1000/e1000_main.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
> index 74dc15055971..47397d7b97df 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> @@ -1199,12 +1199,13 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> 		for (i = 0; i < 32; i++) {
> 			hw->phy_addr = i;
> 			e1000_read_phy_reg(hw, PHY_ID2, &tmp);
> +
> 			if (tmp == 0 || tmp == 0xFF) {
> 				if (i == 31)
> 					goto err_eeprom;
> -				continue;
> -			} else
> +			} else {
> 				break;
> +			}
> 		}
> 	}

I think I would prefer leaving the continue, deleting the else and undenting the break one level. Since the first portion of the original if statement will never "fall through", the else is really the redundant part.

--
Mark Rustad, Networking Division, Intel Corporation

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150914/c27940eb/attachment.asc>

[Intel-wired-lan] [next-queue 1/8] e1000: clean up the useless 'continue' statement

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

From: "Rustad, Mark D" <mark.d.rustad@intel.com>
Date: Mon, 14 Sep 2015 16:48:48 +0000
>
> > On Sep 13, 2015, at 11:27 PM, J?an Sacren <sakiwit@gmail.com> wrote:
> > 
> > We can delete the 'continue' statement as it is in the end of the block
> > and doesn't do any good. To make it distinctive, put an empty line above
> > the checking block.
> > 
> > Additionally add a pair of braces in the 'else' branch.
> > 
> > Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> > ---
> > drivers/net/ethernet/intel/e1000/e1000_main.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
> > index 74dc15055971..47397d7b97df 100644
> > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> > @@ -1199,12 +1199,13 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > 		for (i = 0; i < 32; i++) {
> > 			hw->phy_addr = i;
> > 			e1000_read_phy_reg(hw, PHY_ID2, &tmp);
> > +
> > 			if (tmp == 0 || tmp == 0xFF) {
> > 				if (i == 31)
> > 					goto err_eeprom;
> > -				continue;
> > -			} else
> > +			} else {
> > 				break;
> > +			}
> > 		}
> > 	}
> 
> I think I would prefer leaving the continue, deleting the else and
> undenting the break one level. Since the first portion of the original
> if statement will never "fall through", the else is really the
> redundant part.

Thank you for reviewing.

The goto statement might be executed if and only if at the last loop out
of a total of 32. I think we need to rewrite the checking logic here:

			...
			if (tmp != 0 && tmp != 0xFF)
				break;

			if (i == 31)
				goto err_eeprom;
			...

What do you think?

[Intel-wired-lan] [next-queue 1/8] e1000: clean up the useless 'continue' statement

[Rustad, Mark D]

> On Sep 14, 2015, at 10:35 PM, J?an Sacren <sakiwit@gmail.com> wrote:
> 
> From: "Rustad, Mark D" <mark.d.rustad@intel.com>
> Date: Mon, 14 Sep 2015 16:48:48 +0000
>> 
>>> On Sep 13, 2015, at 11:27 PM, J?an Sacren <sakiwit@gmail.com> wrote:
>>> 
>>> We can delete the 'continue' statement as it is in the end of the block
>>> and doesn't do any good. To make it distinctive, put an empty line above
>>> the checking block.
>>> 
>>> Additionally add a pair of braces in the 'else' branch.
>>> 
>>> Signed-off-by: Jean Sacren <sakiwit@gmail.com>
>>> ---
>>> drivers/net/ethernet/intel/e1000/e1000_main.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
>>> index 74dc15055971..47397d7b97df 100644
>>> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
>>> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
>>> @@ -1199,12 +1199,13 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>> 		for (i = 0; i < 32; i++) {
>>> 			hw->phy_addr = i;
>>> 			e1000_read_phy_reg(hw, PHY_ID2, &tmp);
>>> +
>>> 			if (tmp == 0 || tmp == 0xFF) {
>>> 				if (i == 31)
>>> 					goto err_eeprom;
>>> -				continue;
>>> -			} else
>>> +			} else {
>>> 				break;
>>> +			}
>>> 		}
>>> 	}
>> 
>> I think I would prefer leaving the continue, deleting the else and
>> undenting the break one level. Since the first portion of the original
>> if statement will never "fall through", the else is really the
>> redundant part.
> 
> Thank you for reviewing.
> 
> The goto statement might be executed if and only if at the last loop out
> of a total of 32. I think we need to rewrite the checking logic here:
> 
> 			...
> 			if (tmp != 0 && tmp != 0xFF)
> 				break;
> 
> 			if (i == 31)
> 				goto err_eeprom;
> 			...
> 
> What do you think?

This is ok too, but this makes it look to me like the if and goto should really be outside the loop, so after the loop one would have:

		if (i == 32)
			goto err_eeprom;

I think I like that best of all.

--
Mark Rustad, Networking Division, Intel Corporation

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150915/d658b5b5/attachment-0001.asc>

[Intel-wired-lan] [next-queue 1/8] e1000: clean up the useless 'continue' statement

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

From: "Rustad, Mark D" <mark.d.rustad@intel.com>
Date: Tue, 15 Sep 2015 18:24:39 +0000
>
> > On Sep 14, 2015, at 10:35 PM, J?an Sacren <sakiwit@gmail.com> wrote:

[...]

> > The goto statement might be executed if and only if at the last loop out
> > of a total of 32. I think we need to rewrite the checking logic here:
> > 
> > 			...
> > 			if (tmp != 0 && tmp != 0xFF)
> > 				break;
> > 
> > 			if (i == 31)
> > 				goto err_eeprom;
> > 			...
> > 
> > What do you think?
> 
> This is ok too, but this makes it look to me like the if and goto
> should really be outside the loop, so after the loop one would have:
> 
> 		if (i == 32)
> 			goto err_eeprom;

I think moving the above check out of the loop is a good idea, yet it
still retains the same issue as doing the unnecessary checking.

How about the following patch:

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 74dc15055971..4c3ca7c9ce49 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1199,15 +1199,14 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		for (i = 0; i < 32; i++) {
 			hw->phy_addr = i;
 			e1000_read_phy_reg(hw, PHY_ID2, &tmp);
-			if (tmp == 0 || tmp == 0xFF) {
-				if (i == 31)
-					goto err_eeprom;
-				continue;
-			} else
-				break;
+
+			if (tmp != 0 && tmp != 0xFF)
+				goto no_err;
 		}
-	}
 
+		goto err_eeprom;
+	}
+no_err:
 	/* reset the hardware with the new settings */
 	e1000_reset(adapter);
 

[Intel-wired-lan] [next-queue 1/8] e1000: clean up the useless 'continue' statement

[Alexander Duyck]

On 09/16/2015 08:34 AM, J?an Sacren wrote:
> From: "Rustad, Mark D" <mark.d.rustad@intel.com>
> Date: Tue, 15 Sep 2015 18:24:39 +0000
>>
>>> On Sep 14, 2015, at 10:35 PM, J?an Sacren <sakiwit@gmail.com> wrote:
>
> [...]
>
>>> The goto statement might be executed if and only if at the last loop out
>>> of a total of 32. I think we need to rewrite the checking logic here:
>>>
>>> 			...
>>> 			if (tmp != 0 && tmp != 0xFF)
>>> 				break;
>>>
>>> 			if (i == 31)
>>> 				goto err_eeprom;
>>> 			...
>>>
>>> What do you think?
>>
>> This is ok too, but this makes it look to me like the if and goto
>> should really be outside the loop, so after the loop one would have:
>>
>> 		if (i == 32)
>> 			goto err_eeprom;
>
> I think moving the above check out of the loop is a good idea, yet it
> still retains the same issue as doing the unnecessary checking.
>
> How about the following patch:
>
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
> index 74dc15055971..4c3ca7c9ce49 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> @@ -1199,15 +1199,14 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   		for (i = 0; i < 32; i++) {
>   			hw->phy_addr = i;
>   			e1000_read_phy_reg(hw, PHY_ID2, &tmp);
> -			if (tmp == 0 || tmp == 0xFF) {
> -				if (i == 31)
> -					goto err_eeprom;
> -				continue;
> -			} else
> -				break;
> +
> +			if (tmp != 0 && tmp != 0xFF)
> +				goto no_err;
>   		}
> -	}
>
> +		goto err_eeprom;
> +	}
> +no_err:
>   	/* reset the hardware with the new settings */
>   	e1000_reset(adapter);
>

I kind of like Mark's idea better, though I would make one change.  I 
would make it so that you pull the check for i outside the loop so you 
have something like:
		for (i = 0; i < 32; i++) {
			hw->phy_addr = i;
			e1000_read_phy_reg(hw, PHY_ID2, &tmp);

			if (tmp != 0 && tmp != 0xFF)
				break;
		}

		if (i >= 32)
			goto no_err;

This way you should only have to ever do a comparison of i once per 
loop, and hopefully the compiler is smart enough to realize that i >= 32 
is the exit condition for the loop and will place the jump to that label 
accordingly.

As a side note I am curious if this code is even correct.  I see tmp is 
a u16, the declaration for which could be moved down into the if 
statement if I am not mistaken, and I am curious as to why we are 
comparing it to 0xFF instead of 0xFFFF.  I suspect that the 0xFF check 
might not be adding any value, but I could be wrong.

- Alex

[Intel-wired-lan] [next-queue 1/8] e1000: clean up the useless 'continue' statement

[Rustad, Mark D]

> On Sep 16, 2015, at 9:38 AM, Alexander Duyck <alexander.duyck@gmail.com> wrote:
> 
> I kind of like Mark's idea better, though I would make one change.  I would make it so that you pull the check for i outside the loop so you have something like:
> 		for (i = 0; i < 32; i++) {
> 			hw->phy_addr = i;
> 			e1000_read_phy_reg(hw, PHY_ID2, &tmp);
> 
> 			if (tmp != 0 && tmp != 0xFF)
> 				break;
> 		}
> 
> 		if (i >= 32)
> 			goto no_err;

Shouldn't that goto target be err_eeprom?

> This way you should only have to ever do a comparison of i once per loop, and hopefully the compiler is smart enough to realize that i >= 32 is the exit condition for the loop and will place the jump to that label accordingly.

Yes, obviously I prefer the above form, with the corrected goto target, since that is what I described. The only difference being >= vs = and thought shouldn't matter. With >= being the logical opposite of the loop end test, it is probably the better choice.

> As a side note I am curious if this code is even correct.  I see tmp is a u16, the declaration for which could be moved down into the if statement if I am not mistaken, and I am curious as to why we are comparing it to 0xFF instead of 0xFFFF.  I suspect that the 0xFF check might not be adding any value, but I could be wrong.

Yow! That is a really good question that I don't have an immediate answer to, having no experience with the internals of the e1000 driver and the devices it supports. Some research is required to answer that question.

--
Mark Rustad, Networking Division, Intel Corporation

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150916/32366eb1/attachment-0001.asc>

[Intel-wired-lan] [next-queue 1/8] e1000: clean up the useless 'continue' statement

[Alexander Duyck]

On 09/16/2015 11:27 AM, Rustad, Mark D wrote:
>> On Sep 16, 2015, at 9:38 AM, Alexander Duyck <alexander.duyck@gmail.com> wrote:
>>
>> I kind of like Mark's idea better, though I would make one change.  I would make it so that you pull the check for i outside the loop so you have something like:
>> 		for (i = 0; i < 32; i++) {
>> 			hw->phy_addr = i;
>> 			e1000_read_phy_reg(hw, PHY_ID2, &tmp);
>>
>> 			if (tmp != 0 && tmp != 0xFF)
>> 				break;
>> 		}
>>
>> 		if (i >= 32)
>> 			goto no_err;
> Shouldn't that goto target be err_eeprom?

You're right.  My goof there.

>> This way you should only have to ever do a comparison of i once per loop, and hopefully the compiler is smart enough to realize that i >= 32 is the exit condition for the loop and will place the jump to that label accordingly.
> Yes, obviously I prefer the above form, with the corrected goto target, since that is what I described. The only difference being >= vs = and thought shouldn't matter. With >= being the logical opposite of the loop end test, it is probably the better choice.

I originally wrote it as == but the >= is the more explicit. Normally 
the loop would probably do a cmp followed by a jb to repeat the loop.

>> As a side note I am curious if this code is even correct.  I see tmp is a u16, the declaration for which could be moved down into the if statement if I am not mistaken, and I am curious as to why we are comparing it to 0xFF instead of 0xFFFF.  I suspect that the 0xFF check might not be adding any value, but I could be wrong.
> Yow! That is a really good question that I don't have an immediate answer to, having no experience with the internals of the e1000 driver and the devices it supports. Some research is required to answer that question.

I have a very strong suspicion that the 0xFF value is an error, but I 
don't see the documentation for the ce4100 anywhere to be had on where 
they came up with that value.  If it is supposed to be the result of a 
register read failure I believe the result would already be 0 since 
there is a flag in the register that indicates a read status failue and 
if it is set the value isn't stored.

- Alex

[Intel-wired-lan] [next-queue 1/8] e1000: clean up the useless 'continue' statement

[Rustad, Mark D]

> On Sep 16, 2015, at 2:09 PM, Alexander Duyck <alexander.duyck@gmail.com> wrote:
> 
> I have a very strong suspicion that the 0xFF value is an error, but I don't see the documentation for the ce4100 anywhere to be had on where they came up with that value.  If it is supposed to be the result of a register read failure I believe the result would already be 0 since there is a flag in the register that indicates a read status failue and if it is set the value isn't stored.

I think that it is likely that Alex is right, but the code cleanup should be separated from a functional change in any case, so for this patch, please leave it as 0xFF for now. I have asked someone more familiar with it to look further. Thanks.

--
Mark Rustad, Networking Division, Intel Corporation

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 841 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20150916/a1c6c6c8/attachment.asc>

[Intel-wired-lan] [next-queue 4/8] i40e: fix kernel-doc argument name

[Bowers, AndrewX]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On
> Behalf Of Jean Sacren
> Sent: Sunday, September 13, 2015 11:28 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [next-queue 4/8] i40e: fix kernel-doc argument
> name
> 
> From: Jean Sacren <sakiwit@gmail.com>
> 
> The second argument name in the kernel-doc argument list for
> i40e_features_check() was slightly off. Fix it for the kernel doc.
> 
> Signed-off-by: Jean Sacren <sakiwit@gmail.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Code changes present in tree