[PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing

[PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing

[Jonathan Toppins]

Three of the supported PCI device entries neglected to set driver data
to "board_82575". This is not a problem until a new board type is
defined, so avoid the potential problem all together.

Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 8457d03..bb467bb 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -73,9 +73,9 @@ static const struct e1000_info *igb_info_tbl[] = {
 };
 
 static const struct pci_device_id igb_pci_tbl[] = {
-	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I354_BACKPLANE_1GBPS) },
-	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I354_SGMII) },
-	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I354_BACKPLANE_2_5GBPS) },
+	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I354_BACKPLANE_1GBPS), board_82575 },
+	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I354_SGMII), board_82575 },
+	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I354_BACKPLANE_2_5GBPS), board_82575 },
 	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I211_COPPER), board_82575 },
 	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I210_COPPER), board_82575 },
 	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I210_FIBER), board_82575 },
-- 
1.7.10.4

[PATCH v1 net-next 2/3] igb: move initialization of link properties before igb_sw_init

[Jonathan Toppins]

This is required otherwise the driver may experience a NULL ptr
dereference if CONFIG_PCI_IOV is set to yes.

Since the code can follow the flow on init (driver insmod):
	hw->mac.autoneg = false; (this is not set it is its default)
	igb_probe()
	+- igb_sw_init()
	    +- igb_probe_vfs()
	        +- igb_pci_enable_sriov()
	            +- igb_sriov_reinit()
	               +- igb_reset()
	                  trimmed path is the same see call path for
	                  igb_reset below
	+- hw->mac.autoneg = true;
	+- igb_reset()

	/* igb_reset() call chain */
	igb_reset()
	+- hw->mac.ops.init_hw() == igb_init_hw_82575
	   +- igb_setup_link()
	      +- hw->mac.ops.setup_physical_interface() ==
	           igb_setup_copper_link_82575()
	         +- igb_setup_copper_link()
	            +- possible NULL dereference

Pseudo code from igb_setup_copper_link():
	if (hw->mac.autoneg) {
	    /* setup link */
	} else {
	    hw->phy.ops.force_speed_duplex(hw);  // <-- NULL deref here
	}

Since the way the current code is designed the driver will call
igb_setup_copper_link twice if SRIOV is configured to be enabled. The
call will occur once with autoneg == false and the second
time autoneg == true. Since the decision to call the function pointer
(hw->phy.ops.force_speed_duplex) is predicated on autoneg being set
correctly move the setting of these parameters to as early as possible
in the probe function.

Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index bb467bb..720b785 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2328,6 +2328,14 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (err)
 		goto err_sw_init;
 
+	/* Initialize link properties that are user-changeable */
+	adapter->fc_autoneg = true;
+	hw->mac.autoneg = true;
+	hw->phy.autoneg_advertised = 0x2f;
+
+	hw->fc.requested_mode = e1000_fc_default;
+	hw->fc.current_mode = e1000_fc_default;
+
 	/* setup the private structure */
 	err = igb_sw_init(adapter);
 	if (err)
@@ -2449,14 +2457,6 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	INIT_WORK(&adapter->reset_task, igb_reset_task);
 	INIT_WORK(&adapter->watchdog_task, igb_watchdog_task);
 
-	/* Initialize link properties that are user-changeable */
-	adapter->fc_autoneg = true;
-	hw->mac.autoneg = true;
-	hw->phy.autoneg_advertised = 0x2f;
-
-	hw->fc.requested_mode = e1000_fc_default;
-	hw->fc.current_mode = e1000_fc_default;
-
 	igb_validate_mdi_setting(hw);
 
 	/* By default, support wake on port A */
-- 
1.7.10.4

RE: [PATCH v1 net-next 2/3] igb: move initialization of link properties before igb_sw_init

[Brown, Aaron F]

> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Jonathan Toppins
> Sent: Friday, April 10, 2015 4:04 PM
> To: Kirsher, Jeffrey T
> Cc: Brandeburg, Jesse; Nelson, Shannon; Wyborny, Carolyn; Skidmore, Donald
> C; Vick, Matthew; Ronciak, John; Williams, Mitch A; intel-wired-
> lan@lists.osuosl.org; netdev@vger.kernel.org; gospo@cumulusnetworks.com;
> shm@cumulusnetworks.com
> Subject: [PATCH v1 net-next 2/3] igb: move initialization of link
> properties before igb_sw_init
> 
> This is required otherwise the driver may experience a NULL ptr
> dereference if CONFIG_PCI_IOV is set to yes.
> 
> Since the code can follow the flow on init (driver insmod):
> 	hw->mac.autoneg = false; (this is not set it is its default)
> 	igb_probe()
> 	+- igb_sw_init()
> 	    +- igb_probe_vfs()
> 	        +- igb_pci_enable_sriov()
> 	            +- igb_sriov_reinit()
> 	               +- igb_reset()
> 	                  trimmed path is the same see call path for
> 	                  igb_reset below
> 	+- hw->mac.autoneg = true;
> 	+- igb_reset()
> 
> 	/* igb_reset() call chain */
> 	igb_reset()
> 	+- hw->mac.ops.init_hw() == igb_init_hw_82575
> 	   +- igb_setup_link()
> 	      +- hw->mac.ops.setup_physical_interface() ==
> 	           igb_setup_copper_link_82575()
> 	         +- igb_setup_copper_link()
> 	            +- possible NULL dereference
> 
> Pseudo code from igb_setup_copper_link():
> 	if (hw->mac.autoneg) {
> 	    /* setup link */
> 	} else {
> 	    hw->phy.ops.force_speed_duplex(hw);  // <-- NULL deref here
> 	}
> 
> Since the way the current code is designed the driver will call
> igb_setup_copper_link twice if SRIOV is configured to be enabled. The
> call will occur once with autoneg == false and the second
> time autoneg == true. Since the decision to call the function pointer
> (hw->phy.ops.force_speed_duplex) is predicated on autoneg being set
> correctly move the setting of these parameters to as early as possible
> in the probe function.
> 
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[PATCH v1 net-next 3/3] igb: add PHY support for Broadcom 54616

[Jonathan Toppins]

From: Alan Liebthal <alanl@cumulusnetworks.com>

The Celestica Redstone-XP Ethernet management port uses a Broadcom 54616
chip for the PHY layer. This adds support for this PHY to the Intel igb
e1000 driver.

Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
---
 drivers/net/ethernet/intel/igb/e1000_82575.c   |    5 +++++
 drivers/net/ethernet/intel/igb/e1000_defines.h |    1 +
 drivers/net/ethernet/intel/igb/e1000_hw.h      |    1 +
 3 files changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
index 0f69ef8..2dcc808 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.c
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
@@ -286,6 +286,9 @@ static s32 igb_init_phy_params_82575(struct e1000_hw *hw)
 		phy->ops.set_d3_lplu_state = igb_set_d3_lplu_state_82580;
 		phy->ops.force_speed_duplex = igb_phy_force_speed_duplex_m88;
 		break;
+	case BCM54616_E_PHY_ID:
+		phy->type = e1000_phy_bcm54616;
+		break;
 	default:
 		ret_val = -E1000_ERR_PHY;
 		goto out;
@@ -1593,6 +1596,8 @@ static s32 igb_setup_copper_link_82575(struct e1000_hw *hw)
 	case e1000_phy_82580:
 		ret_val = igb_copper_link_setup_82580(hw);
 		break;
+	case e1000_phy_bcm54616:
+		break;
 	default:
 		ret_val = -E1000_ERR_PHY;
 		break;
diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
index 217f813..50d51e4 100644
--- a/drivers/net/ethernet/intel/igb/e1000_defines.h
+++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
@@ -860,6 +860,7 @@
 #define M88_VENDOR           0x0141
 #define I210_I_PHY_ID        0x01410C00
 #define M88E1543_E_PHY_ID    0x01410EA0
+#define BCM54616_E_PHY_ID    0x03625D10
 
 /* M88E1000 Specific Registers */
 #define M88E1000_PHY_SPEC_CTRL     0x10  /* PHY Specific Control Register */
diff --git a/drivers/net/ethernet/intel/igb/e1000_hw.h b/drivers/net/ethernet/intel/igb/e1000_hw.h
index 2003b37..d82c96b 100644
--- a/drivers/net/ethernet/intel/igb/e1000_hw.h
+++ b/drivers/net/ethernet/intel/igb/e1000_hw.h
@@ -128,6 +128,7 @@ enum e1000_phy_type {
 	e1000_phy_ife,
 	e1000_phy_82580,
 	e1000_phy_i210,
+	e1000_phy_bcm54616,
 };
 
 enum e1000_bus_type {
-- 
1.7.10.4

RE: [PATCH v1 net-next 3/3] igb: add PHY support for Broadcom 54616

[Brown, Aaron F]

> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Jonathan Toppins
> Sent: Friday, April 10, 2015 4:04 PM
> To: Kirsher, Jeffrey T
> Cc: Brandeburg, Jesse; Nelson, Shannon; Wyborny, Carolyn; Skidmore, Donald
> C; Vick, Matthew; Ronciak, John; Williams, Mitch A; intel-wired-
> lan@lists.osuosl.org; netdev@vger.kernel.org; gospo@cumulusnetworks.com;
> shm@cumulusnetworks.com; Alan Liebthal
> Subject: [PATCH v1 net-next 3/3] igb: add PHY support for Broadcom 54616
> 
> From: Alan Liebthal <alanl@cumulusnetworks.com>
> 
> The Celestica Redstone-XP Ethernet management port uses a Broadcom 54616
> chip for the PHY layer. This adds support for this PHY to the Intel igb
> e1000 driver.
> 
> Signed-off-by: Alan Liebthal <alanl@cumulusnetworks.com>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
>  drivers/net/ethernet/intel/igb/e1000_82575.c   |    5 +++++
>  drivers/net/ethernet/intel/igb/e1000_defines.h |    1 +
>  drivers/net/ethernet/intel/igb/e1000_hw.h      |    1 +
>  3 files changed, 7 insertions(+)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>

Again, just regression, I do not have this phy.

Re: [PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing

[Jonathan Toppins]

On 4/10/15 7:03 PM, Jonathan Toppins wrote:
> Three of the supported PCI device entries neglected to set driver data
> to "board_82575". This is not a problem until a new board type is
> defined, so avoid the potential problem all together.
>
> Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> ---
>   drivers/net/ethernet/intel/igb/igb_main.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 8457d03..bb467bb 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -73,9 +73,9 @@ static const struct e1000_info *igb_info_tbl[] = {
>   };
>
>   static const struct pci_device_id igb_pci_tbl[] = {
> -	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I354_BACKPLANE_1GBPS) },
> -	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I354_SGMII) },
> -	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I354_BACKPLANE_2_5GBPS) },
> +	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I354_BACKPLANE_1GBPS), board_82575 },
> +	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I354_SGMII), board_82575 },
> +	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I354_BACKPLANE_2_5GBPS), board_82575 },
>   	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I211_COPPER), board_82575 },
>   	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I210_COPPER), board_82575 },
>   	{ PCI_VDEVICE(INTEL, E1000_DEV_ID_I210_FIBER), board_82575 },
>


Simple keepalive to make sure these have not been forgotten, the 
patchworks entries are still in "new" state.

http://patchwork.ozlabs.org/patch/460268/
http://patchwork.ozlabs.org/patch/460270/
http://patchwork.ozlabs.org/patch/460269/

Regards,
-Jon

RE: [PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing

[Brown, Aaron F]

> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of Jonathan Toppins
> Sent: Monday, April 27, 2015 8:42 AM
> To: Kirsher, Jeffrey T
> Cc: Brandeburg, Jesse; Nelson, Shannon; Wyborny, Carolyn; Skidmore, Donald
> C; Vick, Matthew; Ronciak, John; Williams, Mitch A; intel-wired-
> lan@lists.osuosl.org; netdev@vger.kernel.org; gospo@cumulusnetworks.com;
> shm@cumulusnetworks.com
> Subject: Re: [PATCH v1 net-next 1/3] igb: set driver data for device
> entries that were missing
> 
> On 4/10/15 7:03 PM, Jonathan Toppins wrote:
> > Three of the supported PCI device entries neglected to set driver data
> > to "board_82575". This is not a problem until a new board type is
> > defined, so avoid the potential problem all together.
> >
> > Signed-off-by: Jonathan Toppins <jtoppins@cumulusnetworks.com>
> > ---
> >   drivers/net/ethernet/intel/igb/igb_main.c |    6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)

Tested-by: Aaron Brown <aaron.f.brown@intel.com>

Well, regression tested that is as I don't have any of the potential new boards.