* [PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing
@ 2015-04-10 23:03 Jonathan Toppins
2015-04-10 23:03 ` [PATCH v1 net-next 2/3] igb: move initialization of link properties before igb_sw_init Jonathan Toppins
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jonathan Toppins @ 2015-04-10 23:03 UTC (permalink / raw)
To: jeffrey.t.kirsher
Cc: jesse.brandeburg, shannon.nelson, carolyn.wyborny,
donald.c.skidmore, matthew.vick, john.ronciak, mitch.a.williams,
intel-wired-lan, netdev, gospo, shm
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v1 net-next 2/3] igb: move initialization of link properties before igb_sw_init
2015-04-10 23:03 [PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing Jonathan Toppins
@ 2015-04-10 23:03 ` Jonathan Toppins
2015-04-28 3:20 ` Brown, Aaron F
2015-04-10 23:03 ` [PATCH v1 net-next 3/3] igb: add PHY support for Broadcom 54616 Jonathan Toppins
2015-04-27 15:42 ` [PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing Jonathan Toppins
2 siblings, 1 reply; 7+ messages in thread
From: Jonathan Toppins @ 2015-04-10 23:03 UTC (permalink / raw)
To: jeffrey.t.kirsher
Cc: jesse.brandeburg, shannon.nelson, carolyn.wyborny,
donald.c.skidmore, matthew.vick, john.ronciak, mitch.a.williams,
intel-wired-lan, netdev, gospo, shm
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v1 net-next 3/3] igb: add PHY support for Broadcom 54616
2015-04-10 23:03 [PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing Jonathan Toppins
2015-04-10 23:03 ` [PATCH v1 net-next 2/3] igb: move initialization of link properties before igb_sw_init Jonathan Toppins
@ 2015-04-10 23:03 ` Jonathan Toppins
2015-04-28 3:22 ` Brown, Aaron F
2015-04-27 15:42 ` [PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing Jonathan Toppins
2 siblings, 1 reply; 7+ messages in thread
From: Jonathan Toppins @ 2015-04-10 23:03 UTC (permalink / raw)
To: jeffrey.t.kirsher
Cc: jesse.brandeburg, shannon.nelson, carolyn.wyborny,
donald.c.skidmore, matthew.vick, john.ronciak, mitch.a.williams,
intel-wired-lan, netdev, gospo, shm, Alan Liebthal
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing
2015-04-10 23:03 [PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing Jonathan Toppins
2015-04-10 23:03 ` [PATCH v1 net-next 2/3] igb: move initialization of link properties before igb_sw_init Jonathan Toppins
2015-04-10 23:03 ` [PATCH v1 net-next 3/3] igb: add PHY support for Broadcom 54616 Jonathan Toppins
@ 2015-04-27 15:42 ` Jonathan Toppins
2015-04-28 3:17 ` Brown, Aaron F
2 siblings, 1 reply; 7+ messages in thread
From: Jonathan Toppins @ 2015-04-27 15:42 UTC (permalink / raw)
To: jeffrey.t.kirsher
Cc: jesse.brandeburg, shannon.nelson, carolyn.wyborny,
donald.c.skidmore, matthew.vick, john.ronciak, mitch.a.williams,
intel-wired-lan, netdev, gospo, shm
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing
2015-04-27 15:42 ` [PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing Jonathan Toppins
@ 2015-04-28 3:17 ` Brown, Aaron F
0 siblings, 0 replies; 7+ messages in thread
From: Brown, Aaron F @ 2015-04-28 3:17 UTC (permalink / raw)
To: Jonathan Toppins, Kirsher, Jeffrey T
Cc: Brandeburg, Jesse, Nelson, Shannon, Wyborny, Carolyn, Skidmore,
Donald C, Vick, Matthew, Ronciak, John, Williams, Mitch A,
intel-wired-lan, netdev, gospo, shm
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v1 net-next 2/3] igb: move initialization of link properties before igb_sw_init
2015-04-10 23:03 ` [PATCH v1 net-next 2/3] igb: move initialization of link properties before igb_sw_init Jonathan Toppins
@ 2015-04-28 3:20 ` Brown, Aaron F
0 siblings, 0 replies; 7+ messages in thread
From: Brown, Aaron F @ 2015-04-28 3:20 UTC (permalink / raw)
To: Jonathan Toppins, Kirsher, Jeffrey T
Cc: Brandeburg, Jesse, Nelson, Shannon, Wyborny, Carolyn, Skidmore,
Donald C, Vick, Matthew, Ronciak, John, Williams, Mitch A,
intel-wired-lan, netdev, gospo, shm
> 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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v1 net-next 3/3] igb: add PHY support for Broadcom 54616
2015-04-10 23:03 ` [PATCH v1 net-next 3/3] igb: add PHY support for Broadcom 54616 Jonathan Toppins
@ 2015-04-28 3:22 ` Brown, Aaron F
0 siblings, 0 replies; 7+ messages in thread
From: Brown, Aaron F @ 2015-04-28 3:22 UTC (permalink / raw)
To: Jonathan Toppins, Kirsher, Jeffrey T
Cc: Brandeburg, Jesse, Nelson, Shannon, Wyborny, Carolyn, Skidmore,
Donald C, Vick, Matthew, Ronciak, John, Williams, Mitch A,
intel-wired-lan, netdev, gospo, shm, Alan Liebthal
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-04-28 3:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-10 23:03 [PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing Jonathan Toppins
2015-04-10 23:03 ` [PATCH v1 net-next 2/3] igb: move initialization of link properties before igb_sw_init Jonathan Toppins
2015-04-28 3:20 ` Brown, Aaron F
2015-04-10 23:03 ` [PATCH v1 net-next 3/3] igb: add PHY support for Broadcom 54616 Jonathan Toppins
2015-04-28 3:22 ` Brown, Aaron F
2015-04-27 15:42 ` [PATCH v1 net-next 1/3] igb: set driver data for device entries that were missing Jonathan Toppins
2015-04-28 3:17 ` Brown, Aaron F
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).