Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1262550 > unrolled thread

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

Started bySowmini Varadhan <sowmini.varadhan@oracle.com>
First post2015-11-04 20:50 +0100
Last post2015-11-05 00:10 +0100
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM Sowmini Varadhan <sowmini.varadhan@oracle.com> - 2015-11-04 20:50 +0100
    Re: [PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-04 21:00 +0100
      Re: [PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM Sowmini Varadhan <sowmini.varadhan@oracle.com> - 2015-11-04 21:10 +0100
        Re: [PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-04 22:40 +0100
      RE: [PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM "Nelson, Shannon" <shannon.nelson@intel.com> - 2015-11-05 00:00 +0100
        Re: [PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-05 00:10 +0100
    RE: [PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM "Nelson, Shannon" <shannon.nelson@intel.com> - 2015-11-05 00:10 +0100

#1262550 — [PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM

FromSowmini Varadhan <sowmini.varadhan@oracle.com>
Date2015-11-04 20:50 +0100
Subject[PATCH v5] i40e: Look up MAC address in Open Firmware or IDPROM
Message-ID<qrcee-4Ne-15@gated-at.bofh.it>
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.
+ *
+ * @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;
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1262554

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2015-11-04 21:00 +0100
Message-ID<qrcnT-4Qx-5@gated-at.bofh.it>
In reply to#1262550
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
>



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262557

FromSowmini Varadhan <sowmini.varadhan@oracle.com>
Date2015-11-04 21:10 +0100
Message-ID<qrcxA-59d-5@gated-at.bofh.it>
In reply to#1262554
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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262609

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2015-11-04 22:40 +0100
Message-ID<qrdWG-5VT-13@gated-at.bofh.it>
In reply to#1262557
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.


-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262694

From"Nelson, Shannon" <shannon.nelson@intel.com>
Date2015-11-05 00:00 +0100
Message-ID<qrfc5-6Fb-5@gated-at.bofh.it>
In reply to#1262554
PiBGcm9tOiBBbmR5IFNoZXZjaGVua28gW21haWx0bzphbmR5LnNoZXZjaGVua29AZ21haWwuY29t
XQ0KPiBTZW50OiBXZWRuZXNkYXksIE5vdmVtYmVyIDA0LCAyMDE1IDExOjU5IEFNDQo+IA0KPiBP
biBXZWQsIE5vdiA0LCAyMDE1IGF0IDk6MzkgUE0sIFNvd21pbmkgVmFyYWRoYW4NCj4gPHNvd21p
bmkudmFyYWRoYW5Ab3JhY2xlLmNvbT4gd3JvdGU6DQo+ID4NCj4gPiBUaGlzIGlzIHRoZSBpNDBl
IGVxdWl2YWxlbnQgb2YgY29tbWl0IGM3NjJkZmYyNGMwNiAoIml4Z2JlOiBMb29rIHVwIE1BQw0K
PiA+IGFkZHJlc3MgaW4gT3BlbiBGaXJtd2FyZSBvciBJRFBST00iKS4NCg0KWy4uLl0NCg0KPiA+
ICsgICAgICAgfQ0KPiA+ICsNCj4gPiArICAgICAgIG1lbXNldCgmZWxlbWVudCwgMCwgc2l6ZW9m
KGVsZW1lbnQpKTsNCj4gPiArICAgICAgIGV0aGVyX2FkZHJfY29weShlbGVtZW50Lm1hY19hZGRy
LCBtYWNhZGRyKTsNCj4gPiArICAgICAgIGVsZW1lbnQuZmxhZ3MgPSBjcHVfdG9fbGUxNihJNDBF
X0FRQ19NQUNWTEFOX0FERF9QRVJGRUNUX01BVENIKTsNCj4gPiArICAgICAgIHJldCA9IGk0MGVf
YXFfYWRkX21hY3ZsYW4oJnZzaS0+YmFjay0+aHcsIHZzaS0+c2VpZCwgJmVsZW1lbnQsDQo+IDEs
IE5VTEwpOw0KPiA+ICsgICAgICAgYXFfZXJyID0gdnNpLT5iYWNrLT5ody5hcS5hc3FfbGFzdF9z
dGF0dXM7DQo+IA0KPiBEbyB5b3UgcmVhbGx5IG5lZWQgYSBzZXBhcmF0ZSB2YXJpYWJsZSAoYXFf
ZXJyKT8NCg0KVGhlc2UgYXJlIHR3byBzZXBhcmF0ZSBlcnJvciB2YWx1ZXMgdGhhdCB3ZSdyZSB0
cmFja2luZyAtIG9uZSBmcm9tIHRoZSBjb21tdW5pY2F0aW9uIGJldHdlZW4gdGhlIGRyaXZlciBh
bmQgdGhlIGZpcm13YXJlIChhcV9lcnIpIGFuZCBvbmUgZnJvbSB0aGUgZHJpdmVyIGFjdGl2aXR5
LiAgU29tZXRpbWVzIHRoZXJlIG1heSBiZSBhbiBBUSBlcnJvciB0aGF0IHdlIHdhbnQgdG8gcmVw
b3J0LCBidXQgaXQgbWlnaHQgbm90IGFjdHVhbGx5IGJlIGEgZHJpdmVyIGVycm9yLiAgQWx0ZXJu
YXRpdmVseSwgdGhlcmUgYXJlIHRpbWVzIHdoZW4gdGhlIEFRIGVycm9yIG5lZWRzIHRvIGdldCBp
bnRlcnByZXRlZCBkaWZmZXJlbnQgd2F5cyBkZXBlbmRpbmcgb24gd2hpY2ggdGFzayB0aGUgZHJp
dmVyIGlzIHBlcmZvcm1pbmcuICBMYXN0bHksIHRoZSBBUSBlcnJvciBnaXZlcyB1cyBtb3JlIGRl
dGFpbCBvbiB3aGF0ZXZlciB0aGUgdHJhbnNhY3Rpb24gZXJyb3IgbWF5IGhhdmUgYmVlbiB3aGlj
aCBnaXZlcyB1cyBtb3JlIHVzZWZ1bCBkZWJ1ZyBpbmZvLg0KDQpzbG4NCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262714

FromAndy Shevchenko <andy.shevchenko@gmail.com>
Date2015-11-05 00:10 +0100
Message-ID<qrflM-6Yk-13@gated-at.bofh.it>
In reply to#1262694
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



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262710

From"Nelson, Shannon" <shannon.nelson@intel.com>
Date2015-11-05 00:10 +0100
Message-ID<qrflL-6Yk-1@gated-at.bofh.it>
In reply to#1262550
> 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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web