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

[Andy Shevchenko]

On Thu, Nov 5, 2015 at 12:53 AM, Nelson, Shannon
 wrote:
>> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
>> Sent: Wednesday, November 04, 2015 11:59 AM
>>
>> On Wed, Nov 4, 2015 at 9:39 PM, Sowmini Varadhan
>>  wrote:
>> >
>> > This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
>> > address in Open Firmware or IDPROM").
>
> [...]
>
>> > +   }
>> > +
>> > +   memset(, 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(>back->hw, vsi->seid, ,
>> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Nelson, Shannon]

> From: Sowmini Varadhan [mailto:sowmini.varad...@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 
> Signed-off-by: Sowmini Varadhan 
> ---
> 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 
> +#include 
> +#include 
> +
> +#ifdef CONFIG_SPARC
> +#include 
> +#include 
> +#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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Nelson, Shannon]

> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Wednesday, November 04, 2015 11:59 AM
> 
> On Wed, Nov 4, 2015 at 9:39 PM, Sowmini Varadhan
>  wrote:
> >
> > This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
> > address in Open Firmware or IDPROM").

[...]

> > +   }
> > +
> > +   memset(, 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(>back->hw, vsi->seid, ,
> 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

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

[Andy Shevchenko]

On Wed, Nov 4, 2015 at 10:06 PM, Sowmini Varadhan
 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Sowmini Varadhan]

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(>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(>back->hw, vsi->seid, , 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(>back->pdev->dev,
> > +"add filter failed err %s aq_err %s\n",
> > +i40e_stat_str(>back->hw, ret),
> > +i40e_aq_str(>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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Andy Shevchenko]

On Wed, Nov 4, 2015 at 9:39 PM, Sowmini Varadhan
 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 

> Reviewed-by: Martin K. Petersen 
> Signed-off-by: Sowmini Varadhan 
> ---
> 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 
> +#include 
> +#include 
> +
> +#ifdef CONFIG_SPARC
> +#include 
> +#include 
> +#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(>back->hw,
> +   I40E_AQC_WRITE_TYPE_LAA_WOL,
> +   macaddr, NULL);
> +   if (ret) {
> +   dev_info(>back->pdev->dev,
> +"Addr change for VSI failed: %d\n", ret);

dev_err() or dev_warn() I would say.

> +   return -EADDRNOTAVAIL;
> +   }
> +
> +   memset(, 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(>back->hw, vsi->seid, , 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(>back->pdev->dev,
> +"add filter failed err %s aq_err %s\n",
> +i40e_stat_str(>back->hw, ret),
> +i40e_aq_str(>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) {
> + 

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

[Andy Shevchenko]

On Wed, Nov 4, 2015 at 10:06 PM, Sowmini Varadhan
 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Nelson, Shannon]

> From: Sowmini Varadhan [mailto:sowmini.varad...@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 
> Signed-off-by: Sowmini Varadhan 
> ---
> 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 
> +#include 
> +#include 
> +
> +#ifdef CONFIG_SPARC
> +#include 
> +#include 
> +#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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Nelson, Shannon]

> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Wednesday, November 04, 2015 11:59 AM
> 
> On Wed, Nov 4, 2015 at 9:39 PM, Sowmini Varadhan
>  wrote:
> >
> > This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
> > address in Open Firmware or IDPROM").

[...]

> > +   }
> > +
> > +   memset(, 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(>back->hw, vsi->seid, ,
> 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

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

[Andy Shevchenko]

On Thu, Nov 5, 2015 at 12:53 AM, Nelson, Shannon
 wrote:
>> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
>> Sent: Wednesday, November 04, 2015 11:59 AM
>>
>> On Wed, Nov 4, 2015 at 9:39 PM, Sowmini Varadhan
>>  wrote:
>> >
>> > This is the i40e equivalent of commit c762dff24c06 ("ixgbe: Look up MAC
>> > address in Open Firmware or IDPROM").
>
> [...]
>
>> > +   }
>> > +
>> > +   memset(, 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(>back->hw, vsi->seid, ,
>> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Andy Shevchenko]

On Wed, Nov 4, 2015 at 9:39 PM, Sowmini Varadhan
 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 

> Reviewed-by: Martin K. Petersen 
> Signed-off-by: Sowmini Varadhan 
> ---
> 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 
> +#include 
> +#include 
> +
> +#ifdef CONFIG_SPARC
> +#include 
> +#include 
> +#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(>back->hw,
> +   I40E_AQC_WRITE_TYPE_LAA_WOL,
> +   macaddr, NULL);
> +   if (ret) {
> +   dev_info(>back->pdev->dev,
> +"Addr change for VSI failed: %d\n", ret);

dev_err() or dev_warn() I would say.

> +   return -EADDRNOTAVAIL;
> +   }
> +
> +   memset(, 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(>back->hw, vsi->seid, , 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(>back->pdev->dev,
> +"add filter failed err %s aq_err %s\n",
> +i40e_stat_str(>back->hw, ret),
> +i40e_aq_str(>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 = 

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

[Sowmini Varadhan]

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(>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(>back->hw, vsi->seid, , 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(>back->pdev->dev,
> > +"add filter failed err %s aq_err %s\n",
> > +i40e_stat_str(>back->hw, ret),
> > +i40e_aq_str(>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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/