[net-2.6 PATCH 1/3] ixgbe: Fix compiler warning about variable being used uninitialized

[net-2.6 PATCH 1/3] ixgbe: Fix compiler warning about variable being used uninitialized

[Jeff Kirsher]

From: PJ Waskiewicz <peter.p.waskiewicz.jr@intel.com>

tc is still throwing a warning that is could be used
uninitialized.  This fixes it.

Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/ixgbe/ixgbe_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index 35ea8c9..d561fdf 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -258,7 +258,7 @@ static inline bool ixgbe_tx_is_paused(struct ixgbe_adapter *adapter,
 
 #ifdef CONFIG_IXGBE_DCB
 	if (adapter->flags & IXGBE_FLAG_DCB_ENABLED) {
-		int tc;
+		int tc = 0;
 		int reg_idx = tx_ring->reg_idx;
 		int dcb_i = adapter->ring_feature[RING_F_DCB].indices;
 

[net-2.6 PATCH 2/3] ixgbe: Fix tx_restart_queue/non_eop_desc statistics counters

[Jeff Kirsher]

From: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com>

Fix tx_restart_queue & non_eop_desc stats counters, counters were not
getting incremented as expected with the traffic

Signed-off-by:  Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com>
Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/ixgbe/ixgbe_main.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c
index d561fdf..65eec2d 100644
--- a/drivers/net/ixgbe/ixgbe_main.c
+++ b/drivers/net/ixgbe/ixgbe_main.c
@@ -4511,6 +4511,7 @@ void ixgbe_update_stats(struct ixgbe_adapter *adapter)
 	struct ixgbe_hw *hw = &adapter->hw;
 	u64 total_mpc = 0;
 	u32 i, missed_rx = 0, mpc, bprc, lxon, lxoff, xon_off_tot;
+	u64 non_eop_descs = 0, restart_queue = 0;
 
 	if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) {
 		u64 rsc_count = 0;
@@ -4528,10 +4529,12 @@ void ixgbe_update_stats(struct ixgbe_adapter *adapter)
 
 	/* gather some stats to the adapter struct that are per queue */
 	for (i = 0; i < adapter->num_tx_queues; i++)
-		adapter->restart_queue += adapter->tx_ring[i].restart_queue;
+		restart_queue += adapter->tx_ring[i]->restart_queue;
+	adapter->restart_queue = restart_queue;
 
 	for (i = 0; i < adapter->num_rx_queues; i++)
-		adapter->non_eop_descs += adapter->tx_ring[i].non_eop_descs;
+		non_eop_descs += adapter->rx_ring[i]->non_eop_descs;
+	adapter->non_eop_descs = non_eop_descs;
 
 	adapter->stats.crcerrs += IXGBE_READ_REG(hw, IXGBE_CRCERRS);
 	for (i = 0; i < 8; i++) {

Re: [net-2.6 PATCH 2/3] ixgbe: Fix tx_restart_queue/non_eop_desc statistics counters

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 11 Dec 2009 23:51:41 -0800

> @@ -4528,10 +4529,12 @@ void ixgbe_update_stats(struct ixgbe_adapter *adapter)
>  
>  	/* gather some stats to the adapter struct that are per queue */
>  	for (i = 0; i < adapter->num_tx_queues; i++)
> -		adapter->restart_queue += adapter->tx_ring[i].restart_queue;
> +		restart_queue += adapter->tx_ring[i]->restart_queue;
> +	adapter->restart_queue = restart_queue;

I don't see how these two versions of the code can behave
differently unless there is very broken locking on the
adapter->* statistic counters.

I'm not applying this without at least a better commit
log message.

Re: [net-2.6 PATCH 2/3] ixgbe: Fix tx_restart_queue/non_eop_desc statistics counters

[Peter P Waskiewicz Jr]

On Sun, 2009-12-13 at 19:18 -0800, David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Fri, 11 Dec 2009 23:51:41 -0800
> 
> > @@ -4528,10 +4529,12 @@ void ixgbe_update_stats(struct ixgbe_adapter *adapter)
> >  
> >  	/* gather some stats to the adapter struct that are per queue */
> >  	for (i = 0; i < adapter->num_tx_queues; i++)
> > -		adapter->restart_queue += adapter->tx_ring[i].restart_queue;
> > +		restart_queue += adapter->tx_ring[i]->restart_queue;
> > +	adapter->restart_queue = restart_queue;
> 
> I don't see how these two versions of the code can behave
> differently unless there is very broken locking on the
> adapter->* statistic counters.

The problem is the adapter->restart_queue is being double-counted.  If
we accumulate from each ring's restart_queue, which are cumulative, then
we don't want to add it to the previous update_stats() values in
adapter->restart_queue.

> I'm not applying this without at least a better commit
> log message.

We can fix the commit log message to better explain what the problem is.

Thanks,
-PJ

Re: [net-2.6 PATCH 2/3] ixgbe: Fix tx_restart_queue/non_eop_desc statistics counters

[David Miller]

From: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Date: Sun, 13 Dec 2009 20:57:05 -0800

> On Sun, 2009-12-13 at 19:18 -0800, David Miller wrote:
>> I don't see how these two versions of the code can behave
>> differently unless there is very broken locking on the
>> adapter->* statistic counters.
> 
> The problem is the adapter->restart_queue is being double-counted.  If
> we accumulate from each ring's restart_queue, which are cumulative, then
> we don't want to add it to the previous update_stats() values in
> adapter->restart_queue.

Now I understand, thanks.

>> I'm not applying this without at least a better commit
>> log message.
> 
> We can fix the commit log message to better explain what the problem is.

Thanks PJ.

[net-2.6 PATCH 3/3] Fix 82598 premature copper PHY link indicatation

[Jeff Kirsher]

From: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com>

Fix 82598 copper link issue, where the phy prematurely indicates link
before it is ready to process packets. The new function looks for phy
link and indicates that, when it is available. If phy is not ready
within few seconds of MAC indicating link, the function will return
failure which translates to link down indication

Signed-off-by:  Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com>
Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/ixgbe/ixgbe_82598.c |   38 ++++++++++++++++++++++++++++++++++++++
 drivers/net/ixgbe/ixgbe_type.h  |    2 ++
 2 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ixgbe/ixgbe_82598.c b/drivers/net/ixgbe/ixgbe_82598.c
index e2d5343..1cc91f8 100644
--- a/drivers/net/ixgbe/ixgbe_82598.c
+++ b/drivers/net/ixgbe/ixgbe_82598.c
@@ -510,6 +510,40 @@ static s32 ixgbe_start_mac_link_82598(struct ixgbe_hw *hw,
 }
 
 /**
+ *  ixgbe_validate_link_ready - Function looks for phy link
+ *  @hw: pointer to hardware structure
+ *
+ *  Function indicates success when phy link is available. If phy is not ready
+ *  within 5 seconds of MAC indicating link, the function returns error.
+ **/
+static s32 ixgbe_validate_link_ready(struct ixgbe_hw *hw)
+{
+	u32 timeout;
+	u16 an_reg;
+
+	if (hw->device_id != IXGBE_DEV_ID_82598AT2)
+		return 0;
+
+	for (timeout = 0;
+	     timeout < IXGBE_VALIDATE_LINK_READY_TIMEOUT; timeout++) {
+		hw->phy.ops.read_reg(hw, MDIO_STAT1, MDIO_MMD_AN, &an_reg);
+
+		if ((an_reg & MDIO_AN_STAT1_COMPLETE) &&
+		    (an_reg & MDIO_STAT1_LSTATUS))
+			break;
+
+		mdelay(100);
+	}
+
+	if (timeout == IXGBE_VALIDATE_LINK_READY_TIMEOUT) {
+		hw_dbg(hw, "Link was indicated but link is down\n");
+		return IXGBE_ERR_LINK_SETUP;
+	}
+
+	return 0;
+}
+
+/**
  *  ixgbe_check_mac_link_82598 - Get link/speed status
  *  @hw: pointer to hardware structure
  *  @speed: pointer to link speed
@@ -589,6 +623,10 @@ static s32 ixgbe_check_mac_link_82598(struct ixgbe_hw *hw,
 	else
 		*speed = IXGBE_LINK_SPEED_1GB_FULL;
 
+	if ((hw->device_id == IXGBE_DEV_ID_82598AT2) && (*link_up == true) &&
+	    (ixgbe_validate_link_ready(hw) != 0))
+		*link_up = false;
+
 	/* if link is down, zero out the current_mode */
 	if (*link_up == false) {
 		hw->fc.current_mode = ixgbe_fc_none;
diff --git a/drivers/net/ixgbe/ixgbe_type.h b/drivers/net/ixgbe/ixgbe_type.h
index f3e8d52..84650c6 100644
--- a/drivers/net/ixgbe/ixgbe_type.h
+++ b/drivers/net/ixgbe/ixgbe_type.h
@@ -841,6 +841,8 @@
 #define IXGBE_MPVC      0x04318
 #define IXGBE_SGMIIC    0x04314
 
+#define IXGBE_VALIDATE_LINK_READY_TIMEOUT 50
+
 /* Omer CORECTL */
 #define IXGBE_CORECTL           0x014F00
 /* BARCTRL */

Re: [net-2.6 PATCH 3/3] Fix 82598 premature copper PHY link indicatation

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 11 Dec 2009 23:52:01 -0800

> +	for (timeout = 0;
> +	     timeout < IXGBE_VALIDATE_LINK_READY_TIMEOUT; timeout++) {
> +		hw->phy.ops.read_reg(hw, MDIO_STAT1, MDIO_MMD_AN, &an_reg);
> +
> +		if ((an_reg & MDIO_AN_STAT1_COMPLETE) &&
> +		    (an_reg & MDIO_STAT1_LSTATUS))
> +			break;
> +
> +		mdelay(100);
> +	}

An up to 5000 msec non-preemptable spin loop.

If this used "msleep()" that would make it OK.

But as it is, sorry, no way.

Re: [net-2.6 PATCH 3/3] Fix 82598 premature copper PHY link indicatation

[Malli]

Thanks Dave. I'll respin the patch, didn't mean to wait in non-preemptive loop.

On Sun, Dec 13, 2009 at 7:18 PM, David Miller <davem@davemloft.net> wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Fri, 11 Dec 2009 23:52:01 -0800
>
>> +     for (timeout = 0;
>> +          timeout < IXGBE_VALIDATE_LINK_READY_TIMEOUT; timeout++) {
>> +             hw->phy.ops.read_reg(hw, MDIO_STAT1, MDIO_MMD_AN, &an_reg);
>> +
>> +             if ((an_reg & MDIO_AN_STAT1_COMPLETE) &&
>> +                 (an_reg & MDIO_STAT1_LSTATUS))
>> +                     break;
>> +
>> +             mdelay(100);
>> +     }
>
> An up to 5000 msec non-preemptable spin loop.
>
> If this used "msleep()" that would make it OK.
>
> But as it is, sorry, no way.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [net-2.6 PATCH 1/3] ixgbe: Fix compiler warning about variable being used uninitialized

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 11 Dec 2009 23:51:23 -0800

> From: PJ Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> 
> tc is still throwing a warning that is could be used
> uninitialized.  This fixes it.
> 
> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

It's warning because it absolutely can be used uninitialized.

The two code blocks only handle two chip variants.  Why not
make what's happening here explicit by using a switch
statement and having a 'default' case?

Thanks.

Re: [net-2.6 PATCH 1/3] ixgbe: Fix compiler warning about variable being used uninitialized

[Peter P Waskiewicz Jr]

On Sun, 2009-12-13 at 19:20 -0800, David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Fri, 11 Dec 2009 23:51:23 -0800
> 
> > From: PJ Waskiewicz <peter.p.waskiewicz.jr@intel.com>
> > 
> > tc is still throwing a warning that is could be used
> > uninitialized.  This fixes it.
> > 
> > Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> 
> It's warning because it absolutely can be used uninitialized.
> 
> The two code blocks only handle two chip variants.  Why not
> make what's happening here explicit by using a switch
> statement and having a 'default' case?

That I can do, and makes more sense.  I'll respin the patch.

Thanks Dave,
-PJ