[PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[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 <martin.petersen@oracle.com>
Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
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(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index 2d74c6e..3edfe19 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -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)
-- 
1.7.1

Re: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Andy Shevchenko]

On Fri, Oct 30, 2015 at 5:03 PM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
>
> 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 <martin.petersen@oracle.com>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>

Few nitpicks below, otherwise:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> ---
> 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(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
> index 2d74c6e..3edfe19 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
> @@ -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...

> +#ifdef CONFIG_SPARC
> +#include <asm/idprom.h>
> +#include <asm/prom.h>
> +#endif

...and here.

> +#include "i40e.h"

Why do you need this one exactly?

>  #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
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Sowmini Varadhan]

On (10/30/15 20:06), Andy Shevchenko wrote:
> 
> > +#include "i40e.h"
> 
> Why do you need this one exactly?

I needed it to find pf->pdev below.

> > +       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

Re: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Andy Shevchenko]

On Fri, Oct 30, 2015 at 8:12 PM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (10/30/15 20:06), Andy Shevchenko wrote:
>>
>> > +#include "i40e.h"
>>
>> Why do you need this one exactly?
>
> I needed it to find pf->pdev below.
>
>> > +       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

> 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.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -----Original Message-----
> From: Sowmini Varadhan [mailto:sowmini.varadhan@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
> 
> 
> 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


> 
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
> 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(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c
> b/drivers/net/ethernet/intel/i40e/i40e_common.c
> index 2d74c6e..3edfe19 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_common.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
> @@ -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)
> --
> 1.7.1

Re: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Sowmini Varadhan]

On (10/30/15 18:28), Nelson, Shannon wrote:
> 
> 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.

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.

> 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?

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

RE: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -----Original Message-----
> From: Sowmini Varadhan [mailto:sowmini.varadhan@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
> IDPROM
> 
> On (10/30/15 18:28), Nelson, Shannon wrote:
> >
> > 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.
> 
> 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.

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.

> 
> > 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?
> 
> 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

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

Re: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Sowmini Varadhan]

On (10/30/15 18:57), Nelson, Shannon wrote:
> > >
> > > 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.
       :
> 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..]

> 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?

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

RE: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -----Original Message-----
> From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
> Sent: Friday, October 30, 2015 12:25 PM
> 
> On (10/30/15 18:57), Nelson, Shannon wrote:
> > > >
> > > > 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.
>        :
> > 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.

The more common idiom in our driver would be

	err = i40e_get_platform_mac_addr(..);
	if (err) {
		...
	}

> 
> [Discussion about ndo_set_mac_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?
> 
> 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?

> 
> 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

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

Re: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Sowmini Varadhan]

On (10/30/15 22:03), Nelson, Shannon wrote:
> The more common idiom in our driver would be
> 
> 	err = i40e_get_platform_mac_addr(..);
> 	if (err) {

Ok.

> Have you tested this beyond a compile?
> Do you have a DT model to try this against?

yes.

> 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.

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

Re: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Sowmini Varadhan]

On (10/30/15 19:13), Sowmini Varadhan wrote:
> > 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.
>
> 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

RE: [PATCH v3 net] i40e: Look up MAC address in Open Firmware or IDPROM

[Nelson, Shannon]

> -----Original Message-----
> From: Sowmini Varadhan [mailto:sowmini.varadhan@oracle.com]
> Sent: Sunday, November 01, 2015 8:24 AM
> 

[...]

> 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

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