RE: [net-next 04/10] ixgbe: fix link test when connected to 1Gbps link partner

-----Original Message-----
From: Sergei Shtylyov
[mailto:sergei.shtylyov@cogentembedded.com]
Sent: Wednesday, August 28, 2013 11:28 AM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Tantilov, Emil S;
netdev@vger.kernel.org; gospo@redhat.com;
sassmann@redhat.com
Subject: Re: [net-next 04/10] ixgbe: fix link test when
connected to 1Gbps link partner

Hello.

On 08/28/2013 02:33 PM, Jeff Kirsher wrote:

quoted

From: Emil Tantilov <redacted>

quoted

This patch is a partial reverse of:
commit dfcc4615f09c33454bc553567f7c7506cae60cb9

   Please also specify that commit's summary line in
parens.

commit dfcc4615f09c33454bc553567f7c7506cae60cb9
Author: Jacob Keller [off-list ref]
Date:   Thu Nov 8 07:07:08 2012 +0000

    ixgbe: ethtool ixgbe_diag_test cleanup

quoted

Specifically forcing the laser before the link check can

lead to

quoted

inconsistent results because it does not guarantee that

the link will be

quoted

negotiated correctly. Such is the case when dual speed

SFP+ module is

quoted

connected to a gigabit link partner.

quoted

Signed-off-by: Emil Tantilov <redacted>
Tested-by: Phil Schmitt <redacted>
Signed-off-by: Jeff Kirsher <redacted>
---
  drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 26

++++++++----------------

quoted

  1 file changed, 9 insertions(+), 17 deletions(-)

quoted

diff --git

a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c

quoted

index db0dbf6..57465d8 100644

--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c

@@ -1885,11 +1885,11 @@ static void ixgbe_diag_test(struct

net_device *netdev,

quoted

                              struct ethtool_test

*eth_test, u64 *data)

quoted

  {
  	struct ixgbe_adapter *adapter = netdev_priv(netdev);
-	struct ixgbe_hw *hw = &adapter->hw;
  	bool if_running = netif_running(netdev);

  	set_bit(__IXGBE_TESTING, &adapter->state);
  	if (eth_test->flags == ETH_TEST_FL_OFFLINE) {
+		struct ixgbe_hw *hw = &adapter->hw;

   Empty line wouldn't here, after declaration. At least in
the code above you have it.

It's probably a good idea to make it more readable. Assuming you meant adding an empty line after the declaration.

quoted

  		if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) {
  			int i;
  			for (i = 0; i < adapter->num_vfs; i++) {

WBR, Sergei

If the lack of commit description and the empty line after the *hw declaration is too big of a concern I can resubmit the patch.

Thanks,
Emil