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

On Wed, Nov 4, 2015 at 9:39 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.
>
> 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 <andy.shevchenko@gmail.com>

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

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


> + **/

No need two stars.

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

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

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

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

Same about kernel doc.

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

On (11/04/15 21:59), Andy Shevchenko wrote:
> 
> 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.
>  */
> 
> 
> > + **/
> 
> 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?
> > +                                       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.

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

> > +       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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Nov 4, 2015 at 10:06 PM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (11/04/15 21:59), Andy Shevchenko wrote:
>>
> See earlier response.

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

> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]

> Sent: Wednesday, November 04, 2015 11:59 AM

> 

> On Wed, Nov 4, 2015 at 9:39 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").


[...]

> > +       }

> > +

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


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

> From: Sowmini Varadhan [mailto:sowmini.varadhan@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 <martin.petersen@oracle.com>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
> 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(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index b825f97..a3883cf 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -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.

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


[...]

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Nov 5, 2015 at 12:53 AM, Nelson, Shannon
<shannon.nelson@intel.com> wrote:
>> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
>> Sent: Wednesday, November 04, 2015 11:59 AM
>>
>> On Wed, Nov 4, 2015 at 9:39 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").
>
> [...]
>
>> > +       }
>> > +
>> > +       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)?
>
> 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) {
...
}


>
> sln

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Thu, 3 Dec 2015 10:48:23 -0500

> 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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 2015-12-03 at 10:48 -0500, Sowmini Varadhan wrote:
> 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?

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

On (12/03/15 13:35), Jeff Kirsher wrote:
> 
> 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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html