i40e: Look up MAC address in Open Firmware or IDPROM

PATCH Superseded Sowmini Varadhan 5 participants 11 messages Nov 4, 2015 – Dec 3, 2015

Subsystems (3)

Networking Network drivers Intel Ethernet

lore

Share & tools

lore

Sowmini Varadhan

author

2015-11-04 19:39 UTC #0 lore raw

PATCH v5 1/1 i40e: Look up MAC address in Open Firmware or IDPROM

This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC address in Open Firmware or IDPROM").

As with that fix, attempt to look up the MAC address in Open Firmware on systems that support it, and use IDPROM on SPARC if no OF address is found.

In the case of the i40e there is an assumption that the default mac address has already been set up as the primary mac filter on probe, so if this filter is obtained from the Open Firmware or IDPROM, an explicit write is needed via i40e_aq_mac_address_write() and i40e_aq_add_macvlan() invocation.

Reviewed-by Martin K. Petersen <[email protected]>

Signed-off-by Sowmini Varadhan <[email protected]>

---
v2, v3: Andy Shevchenko comments
v4: Shannon Nelson review: explicitly set up mac filters before register_netdev
v5: Shannon Nelson code style comments

 drivers/net/ethernet/intel/i40e/i40e_main.c |   84 ++++++++++++++++++++++++++-
 1 files changed, 83 insertions(+), 1 deletions(-)

drivers/net/ethernet/intel/i40e/i40e_main.c +83 -1

index b825f97..a3883cf 100644
@@ -24,6 +24,15 @@
  *
  ******************************************************************************/
 
+#include <linux/etherdevice.h>
+#include <linux/of_net.h>
+#include <linux/pci.h>
+
+#ifdef CONFIG_SPARC
+#include <asm/idprom.h>
+#include <asm/prom.h>
+#endif
+
 /* Local includes */
 #include "i40e.h"
 #include "i40e_diag.h"

@@ -9213,6 +9222,44 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
 }
 
 /**
+ * i40e_macaddr_init - explicitly write the mac address filters. This
+ * is needed when the macaddr has been obtained by other means than
+ * the default, e.g., from Open Firmware or IDPROM.
+ *
+ * @vsi: pointer to the vsi.
+ * @macaddr: the MAC address
+ *
+ * Returns 0 on success, negative on failure
+ **/
+static int i40e_macaddr_init(struct i40e_vsi *vsi, u8 *macaddr)
+{
+	int ret, aq_err;
+	struct i40e_aqc_add_macvlan_element_data element;
+
+	ret = i40e_aq_mac_address_write(&vsi->back->hw,
+					I40E_AQC_WRITE_TYPE_LAA_WOL,
+					macaddr, NULL);
+	if (ret) {
+		dev_info(&vsi->back->pdev->dev,
+			 "Addr change for VSI failed: %d\n", ret);
+		return -EADDRNOTAVAIL;
+	}
+
+	memset(&element, 0, sizeof(element));
+	ether_addr_copy(element.mac_addr, macaddr);
+	element.flags = cpu_to_le16(I40E_AQC_MACVLAN_ADD_PERFECT_MATCH);
+	ret = i40e_aq_add_macvlan(&vsi->back->hw, vsi->seid, &element, 1, NULL);
+	aq_err = vsi->back->hw.aq.asq_last_status;
+	if (aq_err != I40E_AQ_RC_OK) {
+		dev_info(&vsi->back->pdev->dev,
+			 "add filter failed err %s aq_err %s\n",
+			 i40e_stat_str(&vsi->back->hw, ret),
+			 i40e_aq_str(&vsi->back->hw, aq_err));
+	}
+	return ret;
+}
+
+/**
  * i40e_vsi_setup - Set up a VSI by a given type
  * @pf: board private structure
  * @type: VSI type

@@ -9341,6 +9388,9 @@ struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf, u8 type,
 		ret = i40e_config_netdev(vsi);
 		if (ret)
 			goto err_netdev;
+		ret = i40e_macaddr_init(vsi, pf->hw.mac.addr);
+		if (ret)
+			goto err_netdev;
 		ret = register_netdev(vsi->netdev);
 		if (ret)
 			goto err_netdev;

@@ -10163,6 +10213,36 @@ static void i40e_print_features(struct i40e_pf *pf)
 }
 
 /**
+ * i40e_get_platform_mac_addr - get mac address from Open Firmware
+ * or IDPROM if supported by the platform
+ *
+ * @pdev: PCI device information struct
+ * @mac_addr: the MAC address to be returned
+ *
+ * Look up the MAC address in Open Firmware  on systems that support it,
+ * and use IDPROM on SPARC if no OF address is found.
+ *
+ * Returns 0 on success, negative on failure
+ **/
+static int i40e_get_platform_mac_addr(struct pci_dev *pdev, u8 *mac_addr)
+{
+	struct device_node *dp = pci_device_to_OF_node(pdev);
+	const unsigned char *addr;
+
+	addr = of_get_mac_address(dp);
+	if (addr) {
+		ether_addr_copy(mac_addr, addr);
+		return 0;
+	}
+#ifdef CONFIG_SPARC
+	ether_addr_copy(mac_addr, idprom->id_ethaddr);
+	return 0;
+#else
+	return -EINVAL;
+#endif /* CONFIG_SPARC */
+}
+
+/**
  * i40e_probe - Device initialization routine
  * @pdev: PCI device information struct
  * @ent: entry in i40e_pci_tbl

@@ -10360,7 +10440,9 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		i40e_aq_stop_lldp(hw, true, NULL);
 	}
 
-	i40e_get_mac_addr(hw, hw->mac.addr);
+	err = i40e_get_platform_mac_addr(pdev, hw->mac.addr);
+	if (err)
+		i40e_get_mac_addr(hw, hw->mac.addr);
 	if (!is_valid_ether_addr(hw->mac.addr)) {
 		dev_info(&pdev->dev, "invalid MAC address %pM\n", hw->mac.addr);
 		err = -EIO;

Signature · 1 line

1.7.1

Andy Shevchenko

Reviewed-by on v5 1/1

2015-11-04 19:59 UTC #1 lore raw

Quoted 8 lines · Sowmini Varadhan

This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC address in Open Firmware or IDPROM").
As with that fix, attempt to look up the MAC address in Open Firmware on systems that support it, and use IDPROM on SPARC if no OF address is found.

Quoted 6 lines

In the case of the i40e there is an assumption that the default mac address has already been set up as the primary mac filter on probe, so if this filter is obtained from the Open Firmware or IDPROM, an explicit write is needed via i40e_aq_mac_address_write() and i40e_aq_add_macvlan() invocation.

Few comments (mostly stylish)
And take my

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 32 lines

Reviewed-by Martin K. Petersen <[email protected]>
Signed-off-by Sowmini Varadhan <[email protected]>
---
v2, v3: Andy Shevchenko comments
v4: Shannon Nelson review: explicitly set up mac filters before register_netdev
v5: Shannon Nelson code style comments
 drivers/net/ethernet/intel/i40e/i40e_main.c |   84 ++++++++++++++++++++++++++-
 1 files changed, 83 insertions(+), 1 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_main.c +9 -0
index b825f97..a3883cf 100644
@@ -24,6 +24,15 @@
  *
  ******************************************************************************/
 
+#include <linux/etherdevice.h>
+#include <linux/of_net.h>
+#include <linux/pci.h>
+
+#ifdef CONFIG_SPARC
+#include <asm/idprom.h>
+#include <asm/prom.h>
+#endif
+
 /* Local includes */
 #include "i40e.h"
 #include "i40e_diag.h"
@@ -9213,6 +9222,44 @@ static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
 }

drivers/net/ethernet/intel/i40e/i40e_main.c static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi) · line 9232

Quoted 10 lines

drivers/net/ethernet/intel/i40e/i40e_main.c +8 -0

 
 /**
+ * i40e_macaddr_init - explicitly write the mac address filters. This
+ * is needed when the macaddr has been obtained by other means than
+ * the default, e.g., from Open Firmware or IDPROM.
+ *
+ * @vsi: pointer to the vsi.
+ * @macaddr: the MAC address
+ *
+ * Returns 0 on success, negative on failure

Usually the structure of kernel doc is something like following

/**
* func - summary
* @paramx: desc
*
* Description:
* Long description in many lines and / or paragraphs *
* Returns:
* 0 on success or errno otherwise.
*/

drivers/net/ethernet/intel/i40e/i40e_main.c static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi) · line 9233

Quoted 1 line

drivers/net/ethernet/intel/i40e/i40e_main.c +1 -0
+ **/

No need two stars.

drivers/net/ethernet/intel/i40e/i40e_main.c static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi) · line 9237

Quoted 4 lines

drivers/net/ethernet/intel/i40e/i40e_main.c +4 -0

+static int i40e_macaddr_init(struct i40e_vsi *vsi, u8 *macaddr)
+{
+       int ret, aq_err;
+       struct i40e_aqc_add_macvlan_element_data element;

Usually

struct something whatever;
int ret;

looks better.

drivers/net/ethernet/intel/i40e/i40e_main.c static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi) · line 9244

Quoted 7 lines

drivers/net/ethernet/intel/i40e/i40e_main.c +7 -0

+
+       ret = i40e_aq_mac_address_write(&vsi->back->hw,
+                                       I40E_AQC_WRITE_TYPE_LAA_WOL,
+                                       macaddr, NULL);
+       if (ret) {
+               dev_info(&vsi->back->pdev->dev,
+                        "Addr change for VSI failed: %d\n", ret);

dev_err() or dev_warn() I would say.

drivers/net/ethernet/intel/i40e/i40e_main.c static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi) · line 9252

Quoted 8 lines

drivers/net/ethernet/intel/i40e/i40e_main.c +8 -0

+               return -EADDRNOTAVAIL;
+       }
+
+       memset(&element, 0, sizeof(element));
+       ether_addr_copy(element.mac_addr, macaddr);
+       element.flags = cpu_to_le16(I40E_AQC_MACVLAN_ADD_PERFECT_MATCH);
+       ret = i40e_aq_add_macvlan(&vsi->back->hw, vsi->seid, &element, 1, NULL);
+       aq_err = vsi->back->hw.aq.asq_last_status;

Do you really need a separate variable (aq_err)?

Quoted 28 lines

drivers/net/ethernet/intel/i40e/i40e_main.c +14 -0

+       if (aq_err != I40E_AQ_RC_OK) {
+               dev_info(&vsi->back->pdev->dev,
+                        "add filter failed err %s aq_err %s\n",
+                        i40e_stat_str(&vsi->back->hw, ret),
+                        i40e_aq_str(&vsi->back->hw, aq_err));
+       }
+       return ret;
+}
+
+/**
  * i40e_vsi_setup - Set up a VSI by a given type
  * @pf: board private structure
  * @type: VSI type

@@ -9341,6 +9388,9 @@ struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf, u8 type,
                ret = i40e_config_netdev(vsi);
                if (ret)
                        goto err_netdev;
+               ret = i40e_macaddr_init(vsi, pf->hw.mac.addr);
+               if (ret)
+                       goto err_netdev;
                ret = register_netdev(vsi->netdev);
                if (ret)
                        goto err_netdev;

@@ -10163,6 +10213,36 @@ static void i40e_print_features(struct i40e_pf *pf)
 }
 
 /**
+ * i40e_get_platform_mac_addr - get mac address from Open Firmware

drivers/net/ethernet/intel/i40e/i40e_main.c static void i40e_print_features(struct i40e_pf *pf) · line 10226

Quoted 10 lines

drivers/net/ethernet/intel/i40e/i40e_main.c +10 -0

+ * or IDPROM if supported by the platform
+ *
+ * @pdev: PCI device information struct
+ * @mac_addr: the MAC address to be returned
+ *
+ * Look up the MAC address in Open Firmware  on systems that support it,
+ * and use IDPROM on SPARC if no OF address is found.
+ *
+ * Returns 0 on success, negative on failure
+ **/

Same about kernel doc.

Quoted 36 lines

drivers/net/ethernet/intel/i40e/i40e_main.c +22 -2

+static int i40e_get_platform_mac_addr(struct pci_dev *pdev, u8 *mac_addr)
+{
+       struct device_node *dp = pci_device_to_OF_node(pdev);
+       const unsigned char *addr;
+
+       addr = of_get_mac_address(dp);
+       if (addr) {
+               ether_addr_copy(mac_addr, addr);
+               return 0;
+       }
+#ifdef CONFIG_SPARC
+       ether_addr_copy(mac_addr, idprom->id_ethaddr);
+       return 0;
+#else
+       return -EINVAL;
+#endif /* CONFIG_SPARC */
+}
+
+/**
  * i40e_probe - Device initialization routine
  * @pdev: PCI device information struct
  * @ent: entry in i40e_pci_tbl

@@ -10360,7 +10440,9 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
                i40e_aq_stop_lldp(hw, true, NULL);
        }
 
-       i40e_get_mac_addr(hw, hw->mac.addr);
+       err = i40e_get_platform_mac_addr(pdev, hw->mac.addr);
+       if (err)
+               i40e_get_mac_addr(hw, hw->mac.addr);
        if (!is_valid_ether_addr(hw->mac.addr)) {
                dev_info(&pdev->dev, "invalid MAC address %pM\n", hw->mac.addr);
                err = -EIO;
--

1.7.1

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Sowmini Varadhan

author

2015-11-04 20:06 UTC #2 lore raw

Quoted 15 lines · Andy Shevchenko

Usually the structure of kernel doc is something like following
/**
* func - summary
* @paramx: desc
*
* Description:
* Long description in many lines and / or paragraphs *
* Returns:
* 0 on success or errno otherwise.
*/

Quoted 3 lines

Quoted 1 line
+ **/
No need two stars.

I was actually following the exact comment style of the function just before i40e_macaddr_init, namely:;

/**
* i40e_vsi_setup - Set up a VSI by a given type
* @pf: board private structure
* @type: VSI type
* @uplink_seid: the switch element to link to
* @param1: usage depends upon VSI type. For VF types, indicates VF id *
* This allocates the sw VSI structure and its queue resources, then add a VSI
* to the identified VEB.
*
* Returns pointer to the successfully allocated and configure VSI sw struct on
* success, otherwise returns NULL on failure.
**/
struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf, u8 type,

                                u16 uplink_seid, u32 param1)

So I'm not sure we need to really bike-shed this one?

Quoted 6 lines

drivers/net/ethernet/intel/i40e/i40e_main.c static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi) · line 9244
Quoted 4 lines
drivers/net/ethernet/intel/i40e/i40e_main.c +4 -0
+                                       macaddr, NULL);
+       if (ret) {
+               dev_info(&vsi->back->pdev->dev,
+                        "Addr change for VSI failed: %d\n", ret);
dev_err() or dev_warn() I would say.

again, this was a cut/paste of code from i40e_set_mac() which does netdev_info.

Quoted 4 lines

drivers/net/ethernet/intel/i40e/i40e_main.c static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi) · line 9252
Quoted 2 lines
drivers/net/ethernet/intel/i40e/i40e_main.c +2 -0
+       ret = i40e_aq_add_macvlan(&vsi->back->hw, vsi->seid, &element, 1, NULL);
+       aq_err = vsi->back->hw.aq.asq_last_status;
Do you really need a separate variable (aq_err)?

That seems to be the convention used elsewhere, where ret is distinguished from aq_err, see i40e_sync_vsi_filters()

Quoted 9 lines

drivers/net/ethernet/intel/i40e/i40e_main.c static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi) · line 9259

Quoted 7 lines

drivers/net/ethernet/intel/i40e/i40e_main.c +7 -0

+       if (aq_err != I40E_AQ_RC_OK) {
+               dev_info(&vsi->back->pdev->dev,
+                        "add filter failed err %s aq_err %s\n",
+                        i40e_stat_str(&vsi->back->hw, ret),
+                        i40e_aq_str(&vsi->back->hw, aq_err));
+       }
+       return ret;

Same about kernel doc.

See earlier response.

--Sowmini

Andy Shevchenko

2015-11-04 21:31 UTC #3 lore raw

Quoted 3 lines · Sowmini Varadhan

Quoted 1 line · Andy Shevchenko
See earlier response.

So, if maintainer is okay I'm also okay with those and you may take my tag.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Nelson, Shannon

2015-11-04 22:53 UTC #4 lore raw

In reply to Andy Shevchenko

Quoted 8 lines

From: Andy Shevchenko [mailto:andy.shevchenko at gmail.com]
Sent: Wednesday, November 04, 2015 11:59 AM
Quoted 3 lines · Sowmini Varadhan
This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC address in Open Firmware or IDPROM").

[...]

Quoted 10 lines

drivers/net/ethernet/intel/i40e/i40e_main.c static struct i40e_vsi *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
Quoted 6 lines
drivers/net/ethernet/intel/i40e/i40e_main.c +6 -0
+       }
+
+       memset(&element, 0, sizeof(element));
+       ether_addr_copy(element.mac_addr, macaddr);
+       element.flags = cpu_to_le16(I40E_AQC_MACVLAN_ADD_PERFECT_MATCH);
+       ret = i40e_aq_add_macvlan(&vsi->back->hw, vsi->seid, &element,
1, NULL);
Quoted 1 line
+ aq_err = vsi->back->hw.aq.asq_last_status;
Do you really need a separate variable (aq_err)?

These are two separate error values that we're tracking - one from the communication between the driver and the firmware (aq_err) and one from the driver activity. Sometimes there may be an AQ error that we want to report, but it might not actually be a driver error. Alternatively, there are times when the AQ error needs to get interpreted different ways depending on which task the driver is performing. Lastly, the AQ error gives us more detail on whatever the transaction error may have been which gives us more useful debug info.

sln

Nelson, Shannon

2015-11-04 23:01 UTC #10 lore raw

In reply to Sowmini Varadhan

Quoted 53 lines

From: Sowmini Varadhan [mailto:sowmini.varadhan at oracle.com]
Sent: Wednesday, November 04, 2015 11:40 AM
This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC address in Open Firmware or IDPROM").
As with that fix, attempt to look up the MAC address in Open Firmware on systems that support it, and use IDPROM on SPARC if no OF address is found.
In the case of the i40e there is an assumption that the default mac address has already been set up as the primary mac filter on probe, so if this filter is obtained from the Open Firmware or IDPROM, an explicit write is needed via i40e_aq_mac_address_write() and i40e_aq_add_macvlan() invocation.
Reviewed-by Martin K. Petersen <[email protected]>
Signed-off-by Sowmini Varadhan <[email protected]>
---
v2, v3: Andy Shevchenko comments
v4: Shannon Nelson review: explicitly set up mac filters before register_netdev
v5: Shannon Nelson code style comments
 drivers/net/ethernet/intel/i40e/i40e_main.c |   84
++++++++++++++++++++++++++-
 1 files changed, 83 insertions(+), 1 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_main.c +9 -0
index b825f97..a3883cf 100644
@@ -24,6 +24,15 @@
  *
 
 **************************************************************************
 ****/
 
+#include <linux/etherdevice.h>
+#include <linux/of_net.h>
+#include <linux/pci.h>
+
+#ifdef CONFIG_SPARC
+#include <asm/idprom.h>
+#include <asm/prom.h>
+#endif
+
 /* Local includes */
#include "i40e.h"
#include "i40e_diag.h"

drivers/net/ethernet/intel/i40e/i40e_main.c static struct i40e_vsi · line 9228

Quoted 7 lines

drivers/net/ethernet/intel/i40e/i40e_main.c +3 -0

@@ -9213,6 +9222,44 @@ static struct i40e_vsi
 *i40e_vsi_reinit_setup(struct i40e_vsi *vsi)
 }
 
 /**
+ * i40e_macaddr_init - explicitly write the mac address filters. This
+ * is needed when the macaddr has been obtained by other means than
+ * the default, e.g., from Open Firmware or IDPROM.

Note that this should be a simple single line, function name and short summary; anything more detailed goes into a description after the variables.

[...]

drivers/net/ethernet/intel/i40e/i40e_main.c static void i40e_print_features(struct i40e_pf *pf) · line 10217

Quoted 4 lines

drivers/net/ethernet/intel/i40e/i40e_main.c +2 -0

 /**
+ * i40e_get_platform_mac_addr - get mac address from Open Firmware
+ * or IDPROM if supported by the platform

Again, single line.

Thanks for your work on this, Sowmini. If you can do a quick repost with these little function header comment bits tweaked, I'm willing to ACK this patch and I think we'll be ready for Jeff to include it into his tree.

sln

Andy Shevchenko

2015-11-04 23:06 UTC #5 lore raw

In reply to Nelson, Shannon

Quoted 17 lines · Shannon

Quoted 8 lines
From: Andy Shevchenko [mailto:andy.shevchenko at gmail.com]
Sent: Wednesday, November 04, 2015 11:59 AM
Quoted 3 lines · Sowmini Varadhan
This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC address in Open Firmware or IDPROM").
[...]
Quoted 6 lines
Quoted 6 lines
drivers/net/ethernet/intel/i40e/i40e_main.c +6 -0
+       }
+
+       memset(&element, 0, sizeof(element));
+       ether_addr_copy(element.mac_addr, macaddr);
+       element.flags = cpu_to_le16(I40E_AQC_MACVLAN_ADD_PERFECT_MATCH);
+       ret = i40e_aq_add_macvlan(&vsi->back->hw, vsi->seid, &element,

Quoted 6 lines

Quoted 4 lines
1, NULL);
Quoted 1 line
+ aq_err = vsi->back->hw.aq.asq_last_status;
Do you really need a separate variable (aq_err)?
These are two separate error values that we're tracking - one from the communication between the driver and the firmware (aq_err) and one from the driver activity. Sometimes there may be an AQ error that we want to report, but it might not actually be a driver error. Alternatively, there are times when the AQ error needs to get interpreted different ways depending on which task the driver is performing. Lastly, the AQ error gives us more detail on whatever the transaction error may have been which gives us more useful debug info.

Understandable, though in this certain function I don't see why we can't drop it. The usage of it like this:

var x;

x = y;
if (x) {
...
}

Which is just
if (y) {
...
}

Quoted 2 lines

sln

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Sowmini Varadhan

author

2015-12-03 15:48 UTC #6 lore raw

In reply to Nelson, Shannon

Hi,

The patch here: http://patchwork.ozlabs.org/patch/540218/
is marked "Awaiting Upstream", I think that means it has to first show up in some other repo first (which one?)?

On addiitonal testing, we found a bug in the patch: if
we did not find the macaddr from Open Firmwre or IDPROM (i.e., defaults were ok) then you dont want to be doing i40e_macaddr_init again, else you will get a failure like this (truncated dump_stack shown below)

[ 8127.050926] WARNING: CPU: 18 PID: 878 at kernel/irq/manage.c:1346 __free_irq+0x9f/0x230()
[ 8127.050927] Trying to free already-free IRQ 177
  :
[ 8127.051013]  [<ffffffffa043ddd0>] i40e_clear_interrupt_scheme+0xb0/0xc0 [i40e]
[ 8127.051018]  [<ffffffffa044b538>] i40e_probe.part.64+0x1018/0x1320 [i40e]
  :
[ 8127.051057]  [<ffffffffa044b862>] i40e_probe+0x22/0x30 [i40e]

I can think of a couple of ways to fix this- one (the ugly
way) is to ifdef the i40e_macaddr_init invocation for CONFIG_OF or CONFIG_SPARC only. Another way to solve this is to track some bit in struct i40e_hw that indicates that the macaddr is not the default, thus i40e_macaddr_init() should be called before register_netdev only if that bit is set. (Dont know if there are cache-line considerations that exist in i40e_hw that need to be taken into account for the second version)

In order to send a fix out for review, what should I clone? should I just apply the patch/540218 to net-next and send the update?

--Sowmini

David Miller

maintainer

2015-12-03 17:13 UTC #7 lore raw

From: Sowmini Varadhan <[email protected]>
Date: Thu, 3 Dec 2015 10:48:23 -0500

Quoted 3 lines

The patch here: http://patchwork.ozlabs.org/patch/540218/
is marked "Awaiting Upstream", I think that means it has to first show up in some other repo first (which one?)?

This indication means that I expect to get the change via another maintainer. In this case it means I expect to get this via one of the Intel ethernet trees that Jeff maintains.

Jeff Kirsher

2015-12-03 21:35 UTC #8 lore raw

In reply to Sowmini Varadhan

Quoted 33 lines · Sowmini Varadhan

The patch here: http://patchwork.ozlabs.org/patch/540218/
is marked "Awaiting Upstream", I think that means it has to first show up in some other repo first (which one?)?
On addiitonal testing, we found a bug in the patch: if
we did not find the macaddr from Open Firmwre or IDPROM (i.e., defaults
were ok) then you dont want to be doing i40e_macaddr_init again, else you will get a failure like this (truncated dump_stack shown below)
[ 8127.050926] WARNING: CPU: 18 PID: 878 at kernel/irq/manage.c:1346
__free_irq+0x9f/0x230()
[ 8127.050927] Trying to free already-free IRQ 177
  :
[ 8127.051013]  [<ffffffffa043ddd0>]
i40e_clear_interrupt_scheme+0xb0/0xc0 [i40e]
[ 8127.051018]  [<ffffffffa044b538>] i40e_probe.part.64+0x1018/0x1320
[i40e]
  :
[ 8127.051057]  [<ffffffffa044b862>] i40e_probe+0x22/0x30 [i40e]
I can think of a couple of ways to fix this- one (the ugly way) is to ifdef the i40e_macaddr_init invocation for CONFIG_OF or CONFIG_SPARC
only. Another way to solve this is to track some bit in struct i40e_hw that
indicates that the macaddr is not the default, thus i40e_macaddr_init() should
be called before register_netdev only if that bit is set. (Dont know if there are cache-line considerations that exist in i40e_hw that need to be taken into account for the second version)

Quoted 2 lines

In order to send a fix out for review, what should I clone? should I just apply the patch/540218 to net-next and send the update?

I will drop your current patch in my next-queue tree (dev-queue branch) and will await an updated patch.

Sowmini Varadhan

author

2015-12-03 21:43 UTC #9 lore raw

Quoted 3 lines · Jeff Kirsher

I will drop your current patch in my next-queue tree (dev-queue branch) and will await an updated patch.

sounds good. Let me work with Shannon and Andrew to make sure we cover all cases.

--Sowmini