Skip to content

Commit ade6fde

Browse files
vlifshtsanguy11
vlifshts
authored and
anguy11
committed
igc: remove autoneg parameter from igc_mac_info
Since the igc driver doesn't support forced speed configuration and its current related hardware doesn't support it either, there is no use of the mac.autoneg parameter. Moreover, in one case this usage might result in a NULL pointer dereference due to an uninitialized function pointer, phy.ops.force_speed_duplex. Therefore, remove this parameter from the igc code. Signed-off-by: Vitaly Lifshits <vitaly.lifshits@intel.com> Tested-by: Mor Bar-Gabay <morx.bar.gabay@intel.com> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
1 parent 4b2c75f commit ade6fde

File tree

6 files changed

+163
-195
lines changed

6 files changed

+163
-195
lines changed

‎drivers/net/ethernet/intel/igc/igc_diag.c‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -173,8 +173,7 @@ bool igc_link_test(struct igc_adapter *adapter, u64 *data)
173173
*data = 0;
174174

175175
/* add delay to give enough time for autonegotioation to finish */
176-
if (adapter->hw.mac.autoneg)
177-
ssleep(5);
176+
ssleep(5);
178177

179178
link_up = igc_has_link(adapter);
180179
if (!link_up) {

‎drivers/net/ethernet/intel/igc/igc_ethtool.c‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1821,11 +1821,8 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
18211821
ethtool_link_ksettings_add_link_mode(cmd, advertising, 2500baseT_Full);
18221822

18231823
/* set autoneg settings */
1824-
if (hw->mac.autoneg == 1) {
1825-
ethtool_link_ksettings_add_link_mode(cmd, supported, Autoneg);
1826-
ethtool_link_ksettings_add_link_mode(cmd, advertising,
1827-
Autoneg);
1828-
}
1824+
ethtool_link_ksettings_add_link_mode(cmd, supported, Autoneg);
1825+
ethtool_link_ksettings_add_link_mode(cmd, advertising, Autoneg);
18291826

18301827
/* Set pause flow control settings */
18311828
ethtool_link_ksettings_add_link_mode(cmd, supported, Pause);
@@ -1878,10 +1875,7 @@ static int igc_ethtool_get_link_ksettings(struct net_device *netdev,
18781875
cmd->base.duplex = DUPLEX_UNKNOWN;
18791876
}
18801877
cmd->base.speed = speed;
1881-
if (hw->mac.autoneg)
1882-
cmd->base.autoneg = AUTONEG_ENABLE;
1883-
else
1884-
cmd->base.autoneg = AUTONEG_DISABLE;
1878+
cmd->base.autoneg = AUTONEG_ENABLE;
18851879

18861880
/* MDI-X => 2; MDI =>1; Invalid =>0 */
18871881
if (hw->phy.media_type == igc_media_type_copper)
@@ -1955,7 +1949,6 @@ igc_ethtool_set_link_ksettings(struct net_device *netdev,
19551949
advertised |= ADVERTISE_10_HALF;
19561950

19571951
if (cmd->base.autoneg == AUTONEG_ENABLE) {
1958-
hw->mac.autoneg = 1;
19591952
hw->phy.autoneg_advertised = advertised;
19601953
if (adapter->fc_autoneg)
19611954
hw->fc.requested_mode = igc_fc_default;

‎drivers/net/ethernet/intel/igc/igc_hw.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,6 @@ struct igc_mac_info {
9292
bool asf_firmware_present;
9393
bool arc_subsystem_valid;
9494

95-
bool autoneg;
9695
bool autoneg_failed;
9796
bool get_link_status;
9897
};

‎drivers/net/ethernet/intel/igc/igc_mac.c‎

Lines changed: 153 additions & 163 deletions
Original file line numberDiff line numberDiff line change
@@ -386,14 +386,6 @@ s32 igc_check_for_copper_link(struct igc_hw *hw)
386386
*/
387387
igc_check_downshift(hw);
388388

389-
/* If we are forcing speed/duplex, then we simply return since
390-
* we have already determined whether we have link or not.
391-
*/
392-
if (!mac->autoneg) {
393-
ret_val = -IGC_ERR_CONFIG;
394-
goto out;
395-
}
396-
397389
/* Auto-Neg is enabled. Auto Speed Detection takes care
398390
* of MAC speed/duplex configuration. So we only need to
399391
* configure Collision Distance in the MAC.
@@ -468,173 +460,171 @@ s32 igc_config_fc_after_link_up(struct igc_hw *hw)
468460
goto out;
469461
}
470462

471-
/* Check for the case where we have copper media and auto-neg is
472-
* enabled. In this case, we need to check and see if Auto-Neg
473-
* has completed, and if so, how the PHY and link partner has
474-
* flow control configured.
463+
/* In auto-neg, we need to check and see if Auto-Neg has completed,
464+
* and if so, how the PHY and link partner has flow control
465+
* configured.
475466
*/
476-
if (mac->autoneg) {
477-
/* Read the MII Status Register and check to see if AutoNeg
478-
* has completed. We read this twice because this reg has
479-
* some "sticky" (latched) bits.
480-
*/
481-
ret_val = hw->phy.ops.read_reg(hw, PHY_STATUS,
482-
&mii_status_reg);
483-
if (ret_val)
484-
goto out;
485-
ret_val = hw->phy.ops.read_reg(hw, PHY_STATUS,
486-
&mii_status_reg);
487-
if (ret_val)
488-
goto out;
489467

490-
if (!(mii_status_reg & MII_SR_AUTONEG_COMPLETE)) {
491-
hw_dbg("Copper PHY and Auto Neg has not completed.\n");
492-
goto out;
493-
}
468+
/* Read the MII Status Register and check to see if AutoNeg
469+
* has completed. We read this twice because this reg has
470+
* some "sticky" (latched) bits.
471+
*/
472+
ret_val = hw->phy.ops.read_reg(hw, PHY_STATUS,
473+
&mii_status_reg);
474+
if (ret_val)
475+
goto out;
476+
ret_val = hw->phy.ops.read_reg(hw, PHY_STATUS,
477+
&mii_status_reg);
478+
if (ret_val)
479+
goto out;
494480

495-
/* The AutoNeg process has completed, so we now need to
496-
* read both the Auto Negotiation Advertisement
497-
* Register (Address 4) and the Auto_Negotiation Base
498-
* Page Ability Register (Address 5) to determine how
499-
* flow control was negotiated.
500-
*/
501-
ret_val = hw->phy.ops.read_reg(hw, PHY_AUTONEG_ADV,
502-
&mii_nway_adv_reg);
503-
if (ret_val)
504-
goto out;
505-
ret_val = hw->phy.ops.read_reg(hw, PHY_LP_ABILITY,
506-
&mii_nway_lp_ability_reg);
507-
if (ret_val)
508-
goto out;
509-
/* Two bits in the Auto Negotiation Advertisement Register
510-
* (Address 4) and two bits in the Auto Negotiation Base
511-
* Page Ability Register (Address 5) determine flow control
512-
* for both the PHY and the link partner. The following
513-
* table, taken out of the IEEE 802.3ab/D6.0 dated March 25,
514-
* 1999, describes these PAUSE resolution bits and how flow
515-
* control is determined based upon these settings.
516-
* NOTE: DC = Don't Care
517-
*
518-
* LOCAL DEVICE | LINK PARTNER
519-
* PAUSE | ASM_DIR | PAUSE | ASM_DIR | NIC Resolution
520-
*-------|---------|-------|---------|--------------------
521-
* 0 | 0 | DC | DC | igc_fc_none
522-
* 0 | 1 | 0 | DC | igc_fc_none
523-
* 0 | 1 | 1 | 0 | igc_fc_none
524-
* 0 | 1 | 1 | 1 | igc_fc_tx_pause
525-
* 1 | 0 | 0 | DC | igc_fc_none
526-
* 1 | DC | 1 | DC | igc_fc_full
527-
* 1 | 1 | 0 | 0 | igc_fc_none
528-
* 1 | 1 | 0 | 1 | igc_fc_rx_pause
529-
*
530-
* Are both PAUSE bits set to 1? If so, this implies
531-
* Symmetric Flow Control is enabled at both ends. The
532-
* ASM_DIR bits are irrelevant per the spec.
533-
*
534-
* For Symmetric Flow Control:
535-
*
536-
* LOCAL DEVICE | LINK PARTNER
537-
* PAUSE | ASM_DIR | PAUSE | ASM_DIR | Result
538-
*-------|---------|-------|---------|--------------------
539-
* 1 | DC | 1 | DC | IGC_fc_full
540-
*
541-
*/
542-
if ((mii_nway_adv_reg & NWAY_AR_PAUSE) &&
543-
(mii_nway_lp_ability_reg & NWAY_LPAR_PAUSE)) {
544-
/* Now we need to check if the user selected RX ONLY
545-
* of pause frames. In this case, we had to advertise
546-
* FULL flow control because we could not advertise RX
547-
* ONLY. Hence, we must now check to see if we need to
548-
* turn OFF the TRANSMISSION of PAUSE frames.
549-
*/
550-
if (hw->fc.requested_mode == igc_fc_full) {
551-
hw->fc.current_mode = igc_fc_full;
552-
hw_dbg("Flow Control = FULL.\n");
553-
} else {
554-
hw->fc.current_mode = igc_fc_rx_pause;
555-
hw_dbg("Flow Control = RX PAUSE frames only.\n");
556-
}
557-
}
481+
if (!(mii_status_reg & MII_SR_AUTONEG_COMPLETE)) {
482+
hw_dbg("Copper PHY and Auto Neg has not completed.\n");
483+
goto out;
484+
}
558485

559-
/* For receiving PAUSE frames ONLY.
560-
*
561-
* LOCAL DEVICE | LINK PARTNER
562-
* PAUSE | ASM_DIR | PAUSE | ASM_DIR | Result
563-
*-------|---------|-------|---------|--------------------
564-
* 0 | 1 | 1 | 1 | igc_fc_tx_pause
565-
*/
566-
else if (!(mii_nway_adv_reg & NWAY_AR_PAUSE) &&
567-
(mii_nway_adv_reg & NWAY_AR_ASM_DIR) &&
568-
(mii_nway_lp_ability_reg & NWAY_LPAR_PAUSE) &&
569-
(mii_nway_lp_ability_reg & NWAY_LPAR_ASM_DIR)) {
570-
hw->fc.current_mode = igc_fc_tx_pause;
571-
hw_dbg("Flow Control = TX PAUSE frames only.\n");
572-
}
573-
/* For transmitting PAUSE frames ONLY.
574-
*
575-
* LOCAL DEVICE | LINK PARTNER
576-
* PAUSE | ASM_DIR | PAUSE | ASM_DIR | Result
577-
*-------|---------|-------|---------|--------------------
578-
* 1 | 1 | 0 | 1 | igc_fc_rx_pause
579-
*/
580-
else if ((mii_nway_adv_reg & NWAY_AR_PAUSE) &&
581-
(mii_nway_adv_reg & NWAY_AR_ASM_DIR) &&
582-
!(mii_nway_lp_ability_reg & NWAY_LPAR_PAUSE) &&
583-
(mii_nway_lp_ability_reg & NWAY_LPAR_ASM_DIR)) {
584-
hw->fc.current_mode = igc_fc_rx_pause;
585-
hw_dbg("Flow Control = RX PAUSE frames only.\n");
586-
}
587-
/* Per the IEEE spec, at this point flow control should be
588-
* disabled. However, we want to consider that we could
589-
* be connected to a legacy switch that doesn't advertise
590-
* desired flow control, but can be forced on the link
591-
* partner. So if we advertised no flow control, that is
592-
* what we will resolve to. If we advertised some kind of
593-
* receive capability (Rx Pause Only or Full Flow Control)
594-
* and the link partner advertised none, we will configure
595-
* ourselves to enable Rx Flow Control only. We can do
596-
* this safely for two reasons: If the link partner really
597-
* didn't want flow control enabled, and we enable Rx, no
598-
* harm done since we won't be receiving any PAUSE frames
599-
* anyway. If the intent on the link partner was to have
600-
* flow control enabled, then by us enabling RX only, we
601-
* can at least receive pause frames and process them.
602-
* This is a good idea because in most cases, since we are
603-
* predominantly a server NIC, more times than not we will
604-
* be asked to delay transmission of packets than asking
605-
* our link partner to pause transmission of frames.
486+
/* The AutoNeg process has completed, so we now need to
487+
* read both the Auto Negotiation Advertisement
488+
* Register (Address 4) and the Auto_Negotiation Base
489+
* Page Ability Register (Address 5) to determine how
490+
* flow control was negotiated.
491+
*/
492+
ret_val = hw->phy.ops.read_reg(hw, PHY_AUTONEG_ADV,
493+
&mii_nway_adv_reg);
494+
if (ret_val)
495+
goto out;
496+
ret_val = hw->phy.ops.read_reg(hw, PHY_LP_ABILITY,
497+
&mii_nway_lp_ability_reg);
498+
if (ret_val)
499+
goto out;
500+
/* Two bits in the Auto Negotiation Advertisement Register
501+
* (Address 4) and two bits in the Auto Negotiation Base
502+
* Page Ability Register (Address 5) determine flow control
503+
* for both the PHY and the link partner. The following
504+
* table, taken out of the IEEE 802.3ab/D6.0 dated March 25,
505+
* 1999, describes these PAUSE resolution bits and how flow
506+
* control is determined based upon these settings.
507+
* NOTE: DC = Don't Care
508+
*
509+
* LOCAL DEVICE | LINK PARTNER
510+
* PAUSE | ASM_DIR | PAUSE | ASM_DIR | NIC Resolution
511+
*-------|---------|-------|---------|--------------------
512+
* 0 | 0 | DC | DC | igc_fc_none
513+
* 0 | 1 | 0 | DC | igc_fc_none
514+
* 0 | 1 | 1 | 0 | igc_fc_none
515+
* 0 | 1 | 1 | 1 | igc_fc_tx_pause
516+
* 1 | 0 | 0 | DC | igc_fc_none
517+
* 1 | DC | 1 | DC | igc_fc_full
518+
* 1 | 1 | 0 | 0 | igc_fc_none
519+
* 1 | 1 | 0 | 1 | igc_fc_rx_pause
520+
*
521+
* Are both PAUSE bits set to 1? If so, this implies
522+
* Symmetric Flow Control is enabled at both ends. The
523+
* ASM_DIR bits are irrelevant per the spec.
524+
*
525+
* For Symmetric Flow Control:
526+
*
527+
* LOCAL DEVICE | LINK PARTNER
528+
* PAUSE | ASM_DIR | PAUSE | ASM_DIR | Result
529+
*-------|---------|-------|---------|--------------------
530+
* 1 | DC | 1 | DC | IGC_fc_full
531+
*
532+
*/
533+
if ((mii_nway_adv_reg & NWAY_AR_PAUSE) &&
534+
(mii_nway_lp_ability_reg & NWAY_LPAR_PAUSE)) {
535+
/* Now we need to check if the user selected RX ONLY
536+
* of pause frames. In this case, we had to advertise
537+
* FULL flow control because we could not advertise RX
538+
* ONLY. Hence, we must now check to see if we need to
539+
* turn OFF the TRANSMISSION of PAUSE frames.
606540
*/
607-
else if ((hw->fc.requested_mode == igc_fc_none) ||
608-
(hw->fc.requested_mode == igc_fc_tx_pause) ||
609-
(hw->fc.strict_ieee)) {
610-
hw->fc.current_mode = igc_fc_none;
611-
hw_dbg("Flow Control = NONE.\n");
541+
if (hw->fc.requested_mode == igc_fc_full) {
542+
hw->fc.current_mode = igc_fc_full;
543+
hw_dbg("Flow Control = FULL.\n");
612544
} else {
613545
hw->fc.current_mode = igc_fc_rx_pause;
614546
hw_dbg("Flow Control = RX PAUSE frames only.\n");
615547
}
548+
}
616549

617-
/* Now we need to do one last check... If we auto-
618-
* negotiated to HALF DUPLEX, flow control should not be
619-
* enabled per IEEE 802.3 spec.
620-
*/
621-
ret_val = hw->mac.ops.get_speed_and_duplex(hw, &speed, &duplex);
622-
if (ret_val) {
623-
hw_dbg("Error getting link speed and duplex\n");
624-
goto out;
625-
}
550+
/* For receiving PAUSE frames ONLY.
551+
*
552+
* LOCAL DEVICE | LINK PARTNER
553+
* PAUSE | ASM_DIR | PAUSE | ASM_DIR | Result
554+
*-------|---------|-------|---------|--------------------
555+
* 0 | 1 | 1 | 1 | igc_fc_tx_pause
556+
*/
557+
else if (!(mii_nway_adv_reg & NWAY_AR_PAUSE) &&
558+
(mii_nway_adv_reg & NWAY_AR_ASM_DIR) &&
559+
(mii_nway_lp_ability_reg & NWAY_LPAR_PAUSE) &&
560+
(mii_nway_lp_ability_reg & NWAY_LPAR_ASM_DIR)) {
561+
hw->fc.current_mode = igc_fc_tx_pause;
562+
hw_dbg("Flow Control = TX PAUSE frames only.\n");
563+
}
564+
/* For transmitting PAUSE frames ONLY.
565+
*
566+
* LOCAL DEVICE | LINK PARTNER
567+
* PAUSE | ASM_DIR | PAUSE | ASM_DIR | Result
568+
*-------|---------|-------|---------|--------------------
569+
* 1 | 1 | 0 | 1 | igc_fc_rx_pause
570+
*/
571+
else if ((mii_nway_adv_reg & NWAY_AR_PAUSE) &&
572+
(mii_nway_adv_reg & NWAY_AR_ASM_DIR) &&
573+
!(mii_nway_lp_ability_reg & NWAY_LPAR_PAUSE) &&
574+
(mii_nway_lp_ability_reg & NWAY_LPAR_ASM_DIR)) {
575+
hw->fc.current_mode = igc_fc_rx_pause;
576+
hw_dbg("Flow Control = RX PAUSE frames only.\n");
577+
}
578+
/* Per the IEEE spec, at this point flow control should be
579+
* disabled. However, we want to consider that we could
580+
* be connected to a legacy switch that doesn't advertise
581+
* desired flow control, but can be forced on the link
582+
* partner. So if we advertised no flow control, that is
583+
* what we will resolve to. If we advertised some kind of
584+
* receive capability (Rx Pause Only or Full Flow Control)
585+
* and the link partner advertised none, we will configure
586+
* ourselves to enable Rx Flow Control only. We can do
587+
* this safely for two reasons: If the link partner really
588+
* didn't want flow control enabled, and we enable Rx, no
589+
* harm done since we won't be receiving any PAUSE frames
590+
* anyway. If the intent on the link partner was to have
591+
* flow control enabled, then by us enabling RX only, we
592+
* can at least receive pause frames and process them.
593+
* This is a good idea because in most cases, since we are
594+
* predominantly a server NIC, more times than not we will
595+
* be asked to delay transmission of packets than asking
596+
* our link partner to pause transmission of frames.
597+
*/
598+
else if ((hw->fc.requested_mode == igc_fc_none) ||
599+
(hw->fc.requested_mode == igc_fc_tx_pause) ||
600+
(hw->fc.strict_ieee)) {
601+
hw->fc.current_mode = igc_fc_none;
602+
hw_dbg("Flow Control = NONE.\n");
603+
} else {
604+
hw->fc.current_mode = igc_fc_rx_pause;
605+
hw_dbg("Flow Control = RX PAUSE frames only.\n");
606+
}
626607

627-
if (duplex == HALF_DUPLEX)
628-
hw->fc.current_mode = igc_fc_none;
608+
/* Now we need to do one last check... If we auto-
609+
* negotiated to HALF DUPLEX, flow control should not be
610+
* enabled per IEEE 802.3 spec.
611+
*/
612+
ret_val = hw->mac.ops.get_speed_and_duplex(hw, &speed, &duplex);
613+
if (ret_val) {
614+
hw_dbg("Error getting link speed and duplex\n");
615+
goto out;
616+
}
629617

630-
/* Now we call a subroutine to actually force the MAC
631-
* controller to use the correct flow control settings.
632-
*/
633-
ret_val = igc_force_mac_fc(hw);
634-
if (ret_val) {
635-
hw_dbg("Error forcing flow control settings\n");
636-
goto out;
637-
}
618+
if (duplex == HALF_DUPLEX)
619+
hw->fc.current_mode = igc_fc_none;
620+
621+
/* Now we call a subroutine to actually force the MAC
622+
* controller to use the correct flow control settings.
623+
*/
624+
ret_val = igc_force_mac_fc(hw);
625+
if (ret_val) {
626+
hw_dbg("Error forcing flow control settings\n");
627+
goto out;
638628
}
639629

640630
out:

0 commit comments

Comments
 (0)