i40e: Look up MAC address in Open Firmware or IDPROM
Subsystems (3)
PATCH v3 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.
---
v2: andy shevchenko comments
v3: more andy shevchenko comments
drivers/net/ethernet/intel/i40e/i40e_common.c | 30 +++++++++++++++++++++++++ 1 files changed, 30 insertions(+), 0 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_common.c +30 -0
@@ -24,6 +24,14 @@* ******************************************************************************/ +#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 +#include "i40e.h" #include "i40e_type.h" #include "i40e_adminq.h" #include "i40e_prototype.h"
@@ -1008,6 +1016,25 @@ i40e_status i40e_aq_mac_address_write(struct i40e_hw *hw,return status; } +static int i40e_get_platform_mac_addr (struct i40e_hw *hw,u8 *mac_addr) +{ +struct i40e_pf *pf = hw->back; +struct device_node *dp =pci_device_to_OF_node (pf->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 ; +#endif /* CONFIG_SPARC */ +return 1 ; +} +/** * i40e_get_mac_addr - get MAC address * @hw: pointer to the HW structure
@@ -1021,6 +1048,9 @@ i40e_status i40e_get_mac_addr(struct i40e_hw *hw, u8 *mac_addr)i40e_status status;u16 flags =0 ; +if (!i40e_get_platform_mac_addr (hw, mac_addr)) +return I40E_SUCCESS ; + status =i40e_aq_mac_address_read (hw, &flags, &addrs,NULL );if (flags &I40E_AQC_LAN_ADDR_VALID )
Signature · 1 line
1.7.1
Quoted 10 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.
Reviewed-by Martin K. Petersen <[email protected]>Signed-off-by Sowmini Varadhan <[email protected]>
Few nitpicks below, otherwise:
Quoted 11 lines
---
v2: andy shevchenko comments
v3: more andy shevchenko commentsdrivers/net/ethernet/intel/i40e/i40e_common.c | 30 +++++++++++++++++++++++++ 1 files changed, 30 insertions(+), 0 deletions(-)drivers/net/ethernet/intel/i40e/i40e_common.c +0
-0
drivers/net/ethernet/intel/i40e/i40e_common.c line 29
Quoted 6 lines
drivers/net/ethernet/intel/i40e/i40e_common.c +3
-0@@ -24,6 +24,14 @@* ******************************************************************************/ +#include <linux/etherdevice.h> +#include <linux/of_net.h> +#include <linux/pci.h>
Would be nice to have empty line here...
drivers/net/ethernet/intel/i40e/i40e_common.c line 33
Quoted 4 lines
drivers/net/ethernet/intel/i40e/i40e_common.c +4
-0+#ifdef CONFIG_SPARC +#include <asm/idprom.h> +#include <asm/prom.h> +#endif
...and here.
drivers/net/ethernet/intel/i40e/i40e_common.c line 34
Quoted 1 line
drivers/net/ethernet/intel/i40e/i40e_common.c +1
-0+#include "i40e.h"
Why do you need this one exactly?
Quoted 42 lines
drivers/net/ethernet/intel/i40e/i40e_common.c +22
-1#include "i40e_type.h" #include "i40e_adminq.h" #include "i40e_prototype.h" @@ -1008,6 +1016,25 @@ i40e_status i40e_aq_mac_address_write(struct i40e_hw *hw,return status; } +static int i40e_get_platform_mac_addr (struct i40e_hw *hw,u8 *mac_addr) +{ +struct i40e_pf *pf = hw->back; +struct device_node *dp =pci_device_to_OF_node (pf->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 ; +#endif /* CONFIG_SPARC */ +return 1 ; +} +/** * i40e_get_mac_addr - get MAC address * @hw: pointer to the HW structure @@ -1021,6 +1048,9 @@ i40e_status i40e_get_mac_addr(struct i40e_hw *hw, u8 *mac_addr)i40e_status status;u16 flags =0 ; +if (!i40e_get_platform_mac_addr (hw, mac_addr)) +return I40E_SUCCESS ; + status =i40e_aq_mac_address_read (hw, &flags, &addrs,NULL );if (flags &I40E_AQC_LAN_ADDR_VALID ) --1.7.1
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 4 lines · Andy Shevchenko
Quoted 2 lines
+#include "
i40e.h"Why do you need this one exactly?
I needed it to find pf->pdev below.
Quoted 1 line
Quoted 1 line
+ struct
device_node*dp =pci_device_to_OF_node(pf->pdev);
Without it, you will get:
:
CC [M] drivers/net/ethernet/intel/i40e/i40e_common.o
drivers/net/ethernet/intel/i40e/i40e_common.c: In function ?i40e_get_platform_mac_addr?:
drivers/net/ethernet/intel/i40e/i40e_common.c:1021: error: dereferencing pointer to incomplete type
Unless you feel passionately about the \n nits, I'm going to pass on those. Thanks for reviewing.
--Sowmini
Quoted 8 lines · Sowmini Varadhan
Quoted 4 lines · Andy Shevchenko
Quoted 2 lines
+#include "
i40e.h"Why do you need this one exactly?
I needed it to find
pf->pdevbelow.
Quoted 7 lines
Quoted 1 line
Quoted 1 line
+ struct
device_node*dp =pci_device_to_OF_node(pf->pdev);Without it, you will get:
:
CC [M]drivers/net/ethernet/intel/i40e/i40e_common.o
drivers/net/ethernet/intel/i40e/i40e_common.c: In function ?i40e_get_platform_mac_addr?:
drivers/net/ethernet/intel/i40e/i40e_common.c:1021: error: dereferencing pointer to incomplete type
OK
Quoted 2 lines
Unless you feel passionately about the \n nits, I'm going to pass on those. Thanks for reviewing.
I'm okay, whatever maintainer now accepts.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 7 lines
-----Original Message-----
From: Sowmini Varadhan [mailto:sowmini.varadhan at oracle.com]
Sent: Friday, October 30, 2015 8:04 AM
To:
Subject: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM
Quoted 6 lines
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.
Going along with this being the equivalent of the ixgbe patch, I'd prefer the new code to be in i40e_main.c, rather than in i40e_common.c. In the design of our drivers, the common file is essentially a device specific layer, and the OS and platform related stuff should stay in i40e_main.c. That would also take care of one of the include file nits that Andy mentioned.
At the risk of flying my Device Tree ignorance for all to see, I have a couple questions on how this is used.
Since the mac address is specific to the individual device, how does it get into the device tree? I thought the device tree was compiled ahead of time for the platform it is used on. Is this a generic DT pattern just in case some platform actually has the mac-address? Or does the device mac-address get saved into the DT on the first time through this path so it can be found next time?
If the Device Tree has a different mac address than the HW, when does the kernel tell the HW to use a different mac address? Does the DT management eventually call the ndo_set_mac_address call so the HW knows to use a different mac address?
Thanks,
sln
Quoted 70 lines
Reviewed-by Martin K. Petersen <[email protected]>Signed-off-by Sowmini Varadhan <[email protected]>---
v2: andy shevchenko comments
v3: more andy shevchenko commentsdrivers/net/ethernet/intel/i40e/i40e_common.c | 30 +++++++++++++++++++++++++ 1 files changed, 30 insertions(+), 0 deletions(-)drivers/net/ethernet/intel/i40e/i40e_common.c +8
-0@@ -24,6 +24,14 @@* ******************************************************************************/ +#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 +#include "i40e.h" #include "i40e_type.h" #include "i40e_adminq.h" #include "
i40e_prototype.h"drivers/net/ethernet/intel/i40e/i40e_common.c +19
-0@@ -1008,6 +1016,25 @@ i40e_status i40e_aq_mac_address_write(struct i40e_hw *hw, return status; } +static int i40e_get_platform_mac_addr (struct i40e_hw *hw,u8 *mac_addr) +{ +struct i40e_pf *pf = hw->back; +struct device_node *dp =pci_device_to_OF_node (pf->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 ; +#endif /* CONFIG_SPARC */ +return 1 ; +} +/** * i40e_get_mac_addr - get MAC address * @hw: pointer to the HW structure
drivers/net/ethernet/intel/i40e/i40e_common.c +3
-0@@ -1021,6 +1048,9 @@ i40e_status i40e_get_mac_addr(struct i40e_hw *hw, u8 *mac_addr)i40e_status status;u16 flags =0 ; +if (!i40e_get_platform_mac_addr (hw, mac_addr)) +return I40E_SUCCESS ; + status =i40e_aq_mac_address_read (hw, &flags, &addrs,NULL );if (flags &
I40E_AQC_LAN_ADDR_VALID)
--
1.7.1
Quoted 7 lines · Shannon
Going along with this being the equivalent of the ixgbe patch, I'd prefer the new code to be in
i40e_main.c, rather than ini40e_common.c. In the design of our drivers, the common file is essentially a device specific layer, and the OS and platform related stuff should stay ini40e_main.c. That would also take care of one of the include file nits that Andy mentioned.
ok, and that was my initial instinct as well, except that I noticed that the existing i40e code tries interesting variations from the typical intel driver model by calling i40e_get_mac_addr() which lives in (for reasons not obvious to me) i40e_common.c
So I assumed there must be some other deeper wisdom at work here.
Quoted 10 lines
At the risk of flying my Device Tree ignorance for all to see, I have a couple questions on how this is used.
Since the mac address is specific to the individual device, how does it get into the device tree? I thought the device tree was compiled ahead of time for the platform it is used on. Is this a generic DT pattern just in case some platform actually has the mac-address? Or does the device mac-address get saved into the DT on the first time through this path so it can be found next time?
Quoted 4 lines
If the Device Tree has a different mac address than the HW, when does the kernel tell the HW to use a different mac address? Does the DT management eventually call the
ndo_set_mac_addresscall so the HW knows to use a different mac address?
yes, and here I was hoping for some feedback from the intel folks as well. Commit c762dff24c06 sets hw->mac.perm_addr. I dont know if there is some similar i40e state that needs to be set.
Please share insights.
--Sowmini
Quoted 16 lines
-----Original Message-----
From: Sowmini Varadhan [mailto:sowmini.varadhan at oracle.com]
Sent: Friday, October 30, 2015 11:37 AM
To:
Subject: Re: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROMQuoted 7 lines · Shannon
Going along with this being the equivalent of the ixgbe patch, I'd prefer the new code to be in
i40e_main.c, rather than ini40e_common.c. In the design of our drivers, the common file is essentially a device specific layer, and the OS and platform related stuff should stay ini40e_main.c. That would also take care of one of the include file nits that Andy mentioned.
Quoted 6 lines
ok, and that was my initial instinct as well, except that I noticed that the existing i40e code tries interesting variations from the typical intel driver model by calling
i40e_get_mac_addr()which lives in (for reasons not obvious to me)i40e_common.cSo I assumed there must be some other deeper wisdom at work here.
I'm not sure about any deeper wisdom, but the HW/FW model that this device uses is rather different from our previous devices, so our SW abstractions are a little different from ixgbe and igb.
Quoted 16 lines
Quoted 11 lines
At the risk of flying my Device Tree ignorance for all to see, I have a couple questions on how this is used.
Since the mac address is specific to the individual device, how does it get into the device tree? I thought the device tree was compiled ahead of time for the platform it is used on. Is this a generic DT pattern just in case some platform actually has the mac-address? Or does the device mac-address get saved into the DT on the first time through this path so it can be found next time?
Quoted 4 lines
If the Device Tree has a different mac address than the HW, when does the kernel tell the HW to use a different mac address? Does the DT management eventually call the
ndo_set_mac_addresscall so the HW knows to use a different mac address?
Quoted 7 lines
yes, and here I was hoping for some feedback from the intel folks as well. Commit
c762dff24c06setshw->mac.perm_addr. I dont know if there is some similar i40e state that needs to be set.Please share insights.
--Sowmini
Forgive me if you're already up with this, I just want to be sure we're thinking along the same lines.
The HW has a mac address burned into its own eeprom which it will use by default. The driver merely queries the HW to find out what that mac address is and report it to the OS so it can be registered correctly in the netdev.
If a different mac address is desired, perhaps from the user issuing an "ip link" or ifconfig command, the OS will call the netdev's ndo_set_mac_address to tell the driver. In our case, the driver then tells the HW device to filter for the new mac address rather than the eeprom'd address. This remains until a reset when the HW goes back to its eeprom'd address.
In the cases in which I'm aware, the platform manufacturer normally will burn a different mac-address into the device's eeprom at manufacturing time if they want something other than what came from Intel. I suppose a Device Tree oriented platform could have a different address in the DT, but somehow that needs to get communicated to the device driver so that it can tell the HW.
My question to you remains: does this ndo_set_mac_address happen from the stack above the driver or not?
Thanks,
sln
Quoted 7 lines · Shannon
Quoted 7 lines
Quoted 7 lines
Going along with this being the equivalent of the ixgbe patch, I'd prefer the new code to be in
i40e_main.c, rather than ini40e_common.c. In the design of our drivers, the common file is essentially a device specific layer, and the OS and platform related stuff should stay ini40e_main.c. That would also take care of one of the include file nits that Andy mentioned.
:
Quoted 3 lines
I'm not sure about any deeper wisdom, but the HW/FW model that this device uses is rather different from our previous devices, so our SW abstractions are a little different from ixgbe and igb.
Ok, in that case I can make i40e_main.c to do something like
static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent) {
:
if (i40e_get_platform_mac_addr(..) != I40E_SUCCESS)
i40e_get_mac_addr(..);
:
}
and i agree that doing this from i40e_main will simplify a lot of the other stuff around getting the pdev etc.
[Discussion about ndo_set_mac_address..]
Quoted 9 lines
In the cases in which I'm aware, the platform manufacturer normally will burn a different mac-address into the device's eeprom at manufacturing time if they want something other than what came from Intel. I suppose a Device Tree oriented platform could have a different address in the DT, but somehow that needs to get communicated to the device driver so that it can tell the HW.
My question to you remains: does this
ndo_set_mac_addresshappen from the stack above the driver or not?
I'm probably even less clued in to the DT arch than you in this case, so input from the intel experts would be useful here.
But both in this case, and for the ixgbe template on which I tried to model this, the OF/idprom probing happens from the ->probe when the driver comes up, and ndo_set_mac_address is not involved.
I dont know if it is easier (or even feasible to do this from ->probe) to just call the i40e_set_mac to make sure all the state-bits in the driver are correctly set.
--Sowmini
Quoted 29 lines
-----Original Message-----
From: Sowmini Varadhan [mailto:sowmini.varadhan at oracle.com]
Sent: Friday, October 30, 2015 12:25 PMQuoted 7 lines · Shannon
Quoted 7 lines
Quoted 7 lines
Going along with this being the equivalent of the ixgbe patch, I'd prefer the new code to be in
i40e_main.c, rather than ini40e_common.c. In the design of our drivers, the common file is essentially a device specific layer, and the OS and platform related stuff should stay ini40e_main.c. That would also take care of one of the include file nits that Andy mentioned.:Quoted 3 lines
I'm not sure about any deeper wisdom, but the HW/FW model that this device uses is rather different from our previous devices, so our SW abstractions are a little different from ixgbe and igb.
Ok, in that case I can make
i40e_main.cto do something likestatic int
i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent){ : if (i40e_get_platform_mac_addr(..) != I40E_SUCCESS) i40e_get_mac_addr(..); : }
Quoted 2 lines
and i agree that doing this from
i40e_mainwill simplify a lot of the other stuff around getting the pdev etc.
The more common idiom in our driver would be
err = i40e_get_platform_mac_addr(..);
if (err) {
...
}Quoted 10 lines
[Discussion about
ndo_set_mac_address..]Quoted 7 lines
In the cases in which I'm aware, the platform manufacturer normally will burn a different mac-address into the device's eeprom at manufacturing time if they want something other than what came from Intel. I suppose a Device Tree oriented platform could have a different address in the DT, but somehow that needs to get communicated to the device driver so that it can tell the HW.
Quoted 5 lines
Quoted 2 lines
My question to you remains: does this
ndo_set_mac_addresshappen from the stack above the driver or not?I'm probably even less clued in to the DT arch than you in this case, so input from the intel experts would be useful here.
This worries me - if you're not clued into the DT architecture, why are you making these changes? Have you tested this beyond a compile? Do you have a DT model to try this against?
Quoted 10 lines
But both in this case, and for the ixgbe template on which I tried to model this, the OF/idprom probing happens from the ->probe when the driver comes up, and
ndo_set_mac_addressis not involved.I dont know if it is easier (or even feasible to do this from ->probe) to just call the
i40e_set_macto make sure all the state-bits in the driver are correctly set.--Sowmini
In looking at a couple other drivers, I see the difference being that they typically are writing the primary mac filter on probe (and any other reset), whereas the i40e "knows" that the default mac address is already set up as the filter and doesn't bother with a redundant write. If you want to add this Open Filter code, you'll need to arrange for this write to happen. You can't call i40e_set_mac() to do it, but you can see the i40e_aq_mac_address_write() code there that is involved in updating the mac address as an example. You probably will want to look at section 4.2.1.5.3 of the XL710 data sheet in order to know how to use i40e_aq_mac_address_write() for your situation.
sln
Quoted 4 lines · Shannon
The more common idiom in our driver would be
err = i40e_get_platform_mac_addr(..); if (err) {
Ok.
Quoted 2 lines
Have you tested this beyond a compile?
Do you have a DT model to try this against?
yes.
Quoted 10 lines
In looking at a couple other drivers, I see the difference being that they typically are writing the primary mac filter on probe (and any other reset), whereas the i40e "knows" that the default mac address is already set up as the filter and doesn't bother with a redundant write. If you want to add this Open Filter code, you'll need to arrange for this write to happen. You can't call
i40e_set_mac()to do it, but you can see thei40e_aq_mac_address_write()code there that is involved in updating the mac address as an example. You probably will want to look at section 4.2.1.5.3 of the XL710 data sheet in order to know how to usei40e_aq_mac_address_write()for your situation.
ok. I'll look into it (and also why this did not show up in my testing).
fwiw, the ixgbe patch is quite clearly missing in i40e, and hopefully we wont be opportunistically fixing this one driver at a time in the future.
--Sowmini
Quoted 11 lines · Sowmini Varadhan
Quoted 10 lines
In looking at a couple other drivers, I see the difference being that they typically are writing the primary mac filter on probe (and any other reset), whereas the i40e "knows" that the default mac address is already set up as the filter and doesn't bother with a redundant write. If you want to add this Open Filter code, you'll need to arrange for this write to happen. You can't call
i40e_set_mac()to do it, but you can see thei40e_aq_mac_address_write()code there that is involved in updating the mac address as an example. You probably will want to look at section 4.2.1.5.3 of the XL710 data sheet in order to know how to usei40e_aq_mac_address_write()for your situation.
Quoted 1 line
ok. I'll look into it (and also why this did not show up in my testing).
So I figured out why it all "seemed to work" - my test env had another obscure init process that was marking the link promiscuous. I guess that was having the side-effect of somehow setting the filters above.
But looks like there's more to getting this right than just calling i40e_aq_mac_address_write() - I think it also needs a i40e_aq_add_macvlan().
I was able to get this to work by calling a the core part of i40e_set_mac just before register_netdev. In my patch (RFC patch in a separate thread - please review) I now have this sequence in i40e_probe
err = i40e_get_platform_mac_addr(pdev, hw->mac.addr); if (err) i40e_get_mac_addr(hw, hw->mac.addr); : i40e_setup_pf_switch(..);
And the resulting i40e_vsi_setup() from i40e_setup_pf_switch() will end up doing the right thing by invoking the guts of i40e_set_mac(), which is basically the sequence:
i40e_aq_mac_address_write() i40e_aq_add_macvlan()
I dont know if it is necessary/possible/important to set up the filters sooner in the sequence- the add_macvlan needs an "seid", and I could not tell when (in the ":" code above) the right seid can be found.
Please review the RFC patch I'll be sending shortly.
--Sowmini
Quoted 4 lines
-----Original Message-----
From: Sowmini Varadhan [mailto:sowmini.varadhan at oracle.com]
Sent: Sunday, November 01, 2015 8:24 AM
[...]
Quoted 25 lines
So I figured out why it all "seemed to work" - my test env had another obscure init process that was marking the link promiscuous. I guess that was having the side-effect of somehow setting the filters above.
But looks like there's more to getting this right than just calling
i40e_aq_mac_address_write()- I think it also needs ai40e_aq_add_macvlan().I was able to get this to work by calling a the core part of
i40e_set_macjust beforeregister_netdev. In my patch (RFC patch in a separate thread - please review) I now have this sequence ini40e_probeerr = i40e_get_platform_mac_addr(pdev, hw->mac.addr); if (err) i40e_get_mac_addr(hw, hw->mac.addr); : i40e_setup_pf_switch(..);And the resulting
i40e_vsi_setup()fromi40e_setup_pf_switch()will end up doing the right thing by invoking the guts ofi40e_set_mac(), which is basically the sequence:i40e_aq_mac_address_write() i40e_aq_add_macvlan()
Quoted 8 lines
I dont know if it is necessary/possible/important to set up the filters sooner in the sequence- the
add_macvlanneeds an "seid", and I could not tell when (in the ":" code above) the right seid can be found.Please review the RFC patch I'll be sending shortly.
--Sowmini
Yeah, because of the underlying HW we need to manage, and the fact that we can't ask it for the filters it knows, it becomes a bit convoluted. To manage and replay the filter lists, we use the i40e_{add,del}_filter() routines on the individual VSI filter lists. I'm thinking you're on the right track, but we may not want to bypass the VSI's filter list.
My brain's not in gear this weekend so I'll review the patch later. In the meantime, be sure to test what happens over a reset, such as what happens when the MTU is changed. This will make sure that the replay of mac and vlan filters happens correctly. You'll want to test this with and without vlans.
sln
Quoted 4 lines · Shannon
.. In the meantime, be sure to test what happens over a reset, such as what happens when the MTU is changed. This will make sure that the replay of mac and vlan filters happens correctly. You'll want to test this with and without vlans.
I assume you mean .1q (aka linux macvlan) as opposed to access/trunk vlans? I will test that tomorrow but I did a quick sanity check on mtu, as well as turning tso on/off which also restarts the driver (I believe), and it was "fine", i.e., able to ping offlink hosts.
--Sowmini
Quoted 7 lines
-----Original Message-----
From: Sowmini Varadhan [mailto:[email protected]]
Sent: Sunday, November 01, 2015 4:07 PMOn (11/01/15 21:03), Nelson, Shannon wrote:
Quoted 8 lines
Quoted 1 line
.. In the meantime, be sure to test what happens over a reset, such as
what
Quoted 3 lines
happens when the MTU is changed. This will make sure that the replay of mac and vlan filters happens correctly. You'll want to test this with and without vlans.
I assume you mean .1q (aka linux macvlan) as opposed to access/trunk vlans?
Yes, this is what I had in mind.
Quoted 5 lines
I will test that tomorrow but I did a quick sanity check on mtu, as well as turning tso on/off which also restarts the driver (I believe), and it was "fine", i.e., able to ping offlink hosts.
--Sowmini
Great, thanks.
sln
Quoted 3 lines · Shannon
Quoted 1 line
I assume you mean .1q
Yes, this is what I had in mind.
I dont think we're quite there yet, even without vlans.
If I turn on/off tcpdump, there's something about the way that the link is bounced that leaves the device down while tcpdump is running. Then after I exit tcpdump, it bounces things a few more times again, packets flow for a brief interval, and then there's silence.
Seems like there's is a workq that results in i40e_service_task->i40e_sync_vsi_filters that periodically resets things.
Doing 'ip link set eth0 promisc on' keeps things nice and steady.
How is this all supposed to work if I change the macaddr from /sbin/ip using i40e_set_mac() and then jiggle the promisc (either just the flag, or with tcpdump)? (I cant tell because I dont have an x86 machine with i40e handy)
To frame the question differently, where all should I be invoking the new i40e_macaddr_init() function from?
--Sowmini
Quoted 7 lines · Sowmini Varadhan
Quoted 3 lines · Shannon
Quoted 1 line
I assume you mean .1q
Yes, this is what I had in mind.
I dont think we're quite there yet, even without vlans.
Ok finally got all the .1q stuff verified (took a bit longer than it should, because of some lab/administrative hassles in getting the vlans adjusted at intermediate switches). But the fix as I'd sent has been tested, and it is good for .1q.
The issues I was seeing with promisc were orthogonal to my fix, or even to my platform - I understand those are being addressed separately.
I'm probably too late for the merge window, and I dont know if this fix qualifies as "critical" for net, so I'll re-target v5 for net-next, to be conservative.
--Sowmini