diff mbox series

[v4,2/3] net/enetc: set random MAC in case no MAC for SI

Message ID 20191021105027.14792-3-g.singh@nxp.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series enetc PMD specific changes |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Gagandeep Singh Oct. 21, 2019, 10:50 a.m. UTC 
  for SGMII interfaces, there can be 0 value
written on MAC registers.
This patch set the random MAC address for those
interfaces.

Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
---
 drivers/net/enetc/Makefile       |  2 +-
 drivers/net/enetc/enetc_ethdev.c | 35 +++++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 2 deletions(-)

Comments

Ferruh Yigit Oct. 21, 2019, 4:04 p.m. UTC | #1 
On 10/21/2019 11:50 AM, Gagandeep Singh wrote:
> for SGMII interfaces, there can be 0 value
> written on MAC registers.
> This patch set the random MAC address for those
> interfaces.
> 
> Signed-off-by: Gagandeep Singh <g.singh@nxp.com>
> ---
>  drivers/net/enetc/Makefile       |  2 +-
>  drivers/net/enetc/enetc_ethdev.c | 35 +++++++++++++++++++++++++++++++-
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/enetc/Makefile b/drivers/net/enetc/Makefile
> index 9895501db..312b37245 100644
> --- a/drivers/net/enetc/Makefile
> +++ b/drivers/net/enetc/Makefile
> @@ -17,7 +17,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_ENETC_PMD) += enetc_ethdev.c
>  SRCS-$(CONFIG_RTE_LIBRTE_ENETC_PMD) += enetc_rxtx.c
>  
>  LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool
> -LDLIBS += -lrte_ethdev
> +LDLIBS += -lrte_ethdev -lrte_net
>  LDLIBS += -lrte_bus_pci
>  
>  include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/drivers/net/enetc/enetc_ethdev.c b/drivers/net/enetc/enetc_ethdev.c
> index 4e978348c..475ec77c3 100644
> --- a/drivers/net/enetc/enetc_ethdev.c
> +++ b/drivers/net/enetc/enetc_ethdev.c
> @@ -4,6 +4,7 @@
>  
>  #include <stdbool.h>
>  #include <rte_ethdev_pci.h>
> +#include <rte_random.h>
>  
>  #include "enetc_logs.h"
>  #include "enetc.h"
> @@ -123,11 +124,22 @@ enetc_link_update(struct rte_eth_dev *dev, int wait_to_complete __rte_unused)
>  	return rte_eth_linkstatus_set(dev, &link);
>  }
>  
> +static void
> +print_ethaddr(const char *name, const struct rte_ether_addr *eth_addr)
> +{
> +	char buf[RTE_ETHER_ADDR_FMT_SIZE];
> +
> +	rte_ether_format_addr(buf, RTE_ETHER_ADDR_FMT_SIZE, eth_addr);
> +	ENETC_PMD_INFO("%s%s\n", name, buf);
> +}
> +
>  static int
>  enetc_hardware_init(struct enetc_eth_hw *hw)
>  {
>  	struct enetc_hw *enetc_hw = &hw->hw;
>  	uint32_t *mac = (uint32_t *)hw->mac.addr;
> +	uint32_t high_mac = 0;
> +	uint16_t low_mac = 0;
>  
>  	PMD_INIT_FUNC_TRACE();
>  	/* Calculating and storing the base HW addresses */
> @@ -138,8 +150,29 @@ enetc_hardware_init(struct enetc_eth_hw *hw)
>  	enetc_wr(enetc_hw, ENETC_SIMR, ENETC_SIMR_EN);
>  
>  	*mac = (uint32_t)enetc_port_rd(enetc_hw, ENETC_PSIPMAR0(0));
> +	high_mac = (uint32_t)*mac;
>  	mac++;
>  	*mac = (uint16_t)enetc_port_rd(enetc_hw, ENETC_PSIPMAR1(0));
> +	low_mac = (uint16_t)*mac;
> +
> +	if ((high_mac | low_mac) == 0) {
> +		char *first_byte;
> +
> +		ENETC_PMD_INFO("MAC is not available for this SI, "
> +			       "set random MAC\n");
> +		mac = (uint32_t *)hw->mac.addr;
> +		*mac = (uint32_t)rte_rand();
> +		first_byte = (char *)mac;
> +		*first_byte &= 0xfe;	/* clear multicast bit */
> +		*first_byte |= 0x02;	/* set local assignment bit (IEEE802) */
> +
> +		enetc_port_wr(enetc_hw, ENETC_PSIPMAR0(0), *mac);
> +		mac++;
> +		*mac = (uint16_t)rte_rand();
> +		enetc_port_wr(enetc_hw, ENETC_PSIPMAR1(0), *mac);
> +		print_ethaddr("New address: ",
> +			      (const struct rte_ether_addr *)hw->mac.addr);
> +	}
>  
>  	return 0;
>  }
> @@ -906,5 +939,5 @@ RTE_INIT(enetc_pmd_init_log)
>  {
>  	enetc_logtype_pmd = rte_log_register("pmd.net.enetc");
>  	if (enetc_logtype_pmd >= 0)
> -		rte_log_set_level(enetc_logtype_pmd, RTE_LOG_NOTICE);
> +		rte_log_set_level(enetc_logtype_pmd, RTE_LOG_INFO);


This part look unrelated with the what commit log describes. And why you are
making the driver more verbose by default?
Gagandeep Singh Oct. 22, 2019, 5:31 a.m. UTC | #2 
<...>
> >
> >  include $(RTE_SDK)/mk/rte.lib.mk
> > diff --git a/drivers/net/enetc/enetc_ethdev.c
> b/drivers/net/enetc/enetc_ethdev.c
> > index 4e978348c..475ec77c3 100644
> > --- a/drivers/net/enetc/enetc_ethdev.c
> > +++ b/drivers/net/enetc/enetc_ethdev.c
> > @@ -4,6 +4,7 @@
> >
> >  #include <stdbool.h>
> >  #include <rte_ethdev_pci.h>
> > +#include <rte_random.h>
> >
> >  #include "enetc_logs.h"
> >  #include "enetc.h"
> > @@ -123,11 +124,22 @@ enetc_link_update(struct rte_eth_dev *dev, int
> wait_to_complete __rte_unused)
> >  	return rte_eth_linkstatus_set(dev, &link);
> >  }
> >
> > +static void
> > +print_ethaddr(const char *name, const struct rte_ether_addr *eth_addr)
> > +{
> > +	char buf[RTE_ETHER_ADDR_FMT_SIZE];
> > +
> > +	rte_ether_format_addr(buf, RTE_ETHER_ADDR_FMT_SIZE, eth_addr);
> > +	ENETC_PMD_INFO("%s%s\n", name, buf);
> > +}
> > +
> >  static int
> >  enetc_hardware_init(struct enetc_eth_hw *hw)
> >  {
> >  	struct enetc_hw *enetc_hw = &hw->hw;
> >  	uint32_t *mac = (uint32_t *)hw->mac.addr;
> > +	uint32_t high_mac = 0;
> > +	uint16_t low_mac = 0;
> >
> >  	PMD_INIT_FUNC_TRACE();
> >  	/* Calculating and storing the base HW addresses */
> > @@ -138,8 +150,29 @@ enetc_hardware_init(struct enetc_eth_hw *hw)
> >  	enetc_wr(enetc_hw, ENETC_SIMR, ENETC_SIMR_EN);
> >
> >  	*mac = (uint32_t)enetc_port_rd(enetc_hw, ENETC_PSIPMAR0(0));
> > +	high_mac = (uint32_t)*mac;
> >  	mac++;
> >  	*mac = (uint16_t)enetc_port_rd(enetc_hw, ENETC_PSIPMAR1(0));
> > +	low_mac = (uint16_t)*mac;
> > +
> > +	if ((high_mac | low_mac) == 0) {
> > +		char *first_byte;
> > +
> > +		ENETC_PMD_INFO("MAC is not available for this SI, "
> > +			       "set random MAC\n");
> > +		mac = (uint32_t *)hw->mac.addr;
> > +		*mac = (uint32_t)rte_rand();
> > +		first_byte = (char *)mac;
> > +		*first_byte &= 0xfe;	/* clear multicast bit */
> > +		*first_byte |= 0x02;	/* set local assignment bit (IEEE802) */
> > +
> > +		enetc_port_wr(enetc_hw, ENETC_PSIPMAR0(0), *mac);
> > +		mac++;
> > +		*mac = (uint16_t)rte_rand();
> > +		enetc_port_wr(enetc_hw, ENETC_PSIPMAR1(0), *mac);
> > +		print_ethaddr("New address: ",
> > +			      (const struct rte_ether_addr *)hw->mac.addr);
> > +	}
> >
> >  	return 0;
> >  }
> > @@ -906,5 +939,5 @@ RTE_INIT(enetc_pmd_init_log)
> >  {
> >  	enetc_logtype_pmd = rte_log_register("pmd.net.enetc");
> >  	if (enetc_logtype_pmd >= 0)
> > -		rte_log_set_level(enetc_logtype_pmd, RTE_LOG_NOTICE);
> > +		rte_log_set_level(enetc_logtype_pmd, RTE_LOG_INFO);
> 
> 
> This part look unrelated with the what commit log describes. And why you are
> making the driver more verbose by default?
>
I changed this to print MAC address by default. Currently, enetc driver is not supporting
NOTICE type log and the next supported log level is INFO and we are using INFO type log
only while setting the random MAC addresses.
If INFO type log is not ok to print this information, please suggest me. Should I add NOTICE
Type log? Or  just add a printf to display the MAC address.
Ferruh Yigit Oct. 22, 2019, 8:59 a.m. UTC | #3 
On 10/22/2019 6:31 AM, Gagandeep Singh wrote:
> 
> <...>
>>>
>>>  include $(RTE_SDK)/mk/rte.lib.mk
>>> diff --git a/drivers/net/enetc/enetc_ethdev.c
>> b/drivers/net/enetc/enetc_ethdev.c
>>> index 4e978348c..475ec77c3 100644
>>> --- a/drivers/net/enetc/enetc_ethdev.c
>>> +++ b/drivers/net/enetc/enetc_ethdev.c
>>> @@ -4,6 +4,7 @@
>>>
>>>  #include <stdbool.h>
>>>  #include <rte_ethdev_pci.h>
>>> +#include <rte_random.h>
>>>
>>>  #include "enetc_logs.h"
>>>  #include "enetc.h"
>>> @@ -123,11 +124,22 @@ enetc_link_update(struct rte_eth_dev *dev, int
>> wait_to_complete __rte_unused)
>>>  	return rte_eth_linkstatus_set(dev, &link);
>>>  }
>>>
>>> +static void
>>> +print_ethaddr(const char *name, const struct rte_ether_addr *eth_addr)
>>> +{
>>> +	char buf[RTE_ETHER_ADDR_FMT_SIZE];
>>> +
>>> +	rte_ether_format_addr(buf, RTE_ETHER_ADDR_FMT_SIZE, eth_addr);
>>> +	ENETC_PMD_INFO("%s%s\n", name, buf);
>>> +}
>>> +
>>>  static int
>>>  enetc_hardware_init(struct enetc_eth_hw *hw)
>>>  {
>>>  	struct enetc_hw *enetc_hw = &hw->hw;
>>>  	uint32_t *mac = (uint32_t *)hw->mac.addr;
>>> +	uint32_t high_mac = 0;
>>> +	uint16_t low_mac = 0;
>>>
>>>  	PMD_INIT_FUNC_TRACE();
>>>  	/* Calculating and storing the base HW addresses */
>>> @@ -138,8 +150,29 @@ enetc_hardware_init(struct enetc_eth_hw *hw)
>>>  	enetc_wr(enetc_hw, ENETC_SIMR, ENETC_SIMR_EN);
>>>
>>>  	*mac = (uint32_t)enetc_port_rd(enetc_hw, ENETC_PSIPMAR0(0));
>>> +	high_mac = (uint32_t)*mac;
>>>  	mac++;
>>>  	*mac = (uint16_t)enetc_port_rd(enetc_hw, ENETC_PSIPMAR1(0));
>>> +	low_mac = (uint16_t)*mac;
>>> +
>>> +	if ((high_mac | low_mac) == 0) {
>>> +		char *first_byte;
>>> +
>>> +		ENETC_PMD_INFO("MAC is not available for this SI, "
>>> +			       "set random MAC\n");
>>> +		mac = (uint32_t *)hw->mac.addr;
>>> +		*mac = (uint32_t)rte_rand();
>>> +		first_byte = (char *)mac;
>>> +		*first_byte &= 0xfe;	/* clear multicast bit */
>>> +		*first_byte |= 0x02;	/* set local assignment bit (IEEE802) */
>>> +
>>> +		enetc_port_wr(enetc_hw, ENETC_PSIPMAR0(0), *mac);
>>> +		mac++;
>>> +		*mac = (uint16_t)rte_rand();
>>> +		enetc_port_wr(enetc_hw, ENETC_PSIPMAR1(0), *mac);
>>> +		print_ethaddr("New address: ",
>>> +			      (const struct rte_ether_addr *)hw->mac.addr);
>>> +	}
>>>
>>>  	return 0;
>>>  }
>>> @@ -906,5 +939,5 @@ RTE_INIT(enetc_pmd_init_log)
>>>  {
>>>  	enetc_logtype_pmd = rte_log_register("pmd.net.enetc");
>>>  	if (enetc_logtype_pmd >= 0)
>>> -		rte_log_set_level(enetc_logtype_pmd, RTE_LOG_NOTICE);
>>> +		rte_log_set_level(enetc_logtype_pmd, RTE_LOG_INFO);
>>
>>
>> This part look unrelated with the what commit log describes. And why you are
>> making the driver more verbose by default?
>>
> I changed this to print MAC address by default. Currently, enetc driver is not supporting
> NOTICE type log and the next supported log level is INFO and we are using INFO type log
> only while setting the random MAC addresses.
> If INFO type log is not ok to print this information, please suggest me. Should I add NOTICE
> Type log? Or  just add a printf to display the MAC address.
> 

There is no strict rule, but mostly* drivers set the default log level to
NOTICE, the intention is to make logging less noisy by default.

Is the log you have mentioned should be printed always for all applications by
default, if so I suggest updating its level to NOTICE, (adding NOTICE level is
easy or you can use 'ENETC_PMD_LOG' directly for it.)

But if that log is not required for all, I suggest keeping it INFO and the
default level NOTICE.
Gagandeep Singh Oct. 22, 2019, 11:32 a.m. UTC | #4 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Tuesday, October 22, 2019 2:30 PM
> To: Gagandeep Singh <G.Singh@nxp.com>; dev@dpdk.org
> Subject: Re: [PATCH v4 2/3] net/enetc: set random MAC in case no MAC for SI
> 
> On 10/22/2019 6:31 AM, Gagandeep Singh wrote:
> >
> > <...>
> >>>
> >>>  include $(RTE_SDK)/mk/rte.lib.mk
> >>> diff --git a/drivers/net/enetc/enetc_ethdev.c
> >> b/drivers/net/enetc/enetc_ethdev.c
> >>> index 4e978348c..475ec77c3 100644
> >>> --- a/drivers/net/enetc/enetc_ethdev.c
> >>> +++ b/drivers/net/enetc/enetc_ethdev.c
> >>> @@ -4,6 +4,7 @@
> >>>
> >>>  #include <stdbool.h>
> >>>  #include <rte_ethdev_pci.h>
> >>> +#include <rte_random.h>
> >>>
> >>>  #include "enetc_logs.h"
> >>>  #include "enetc.h"
> >>> @@ -123,11 +124,22 @@ enetc_link_update(struct rte_eth_dev *dev, int
> >> wait_to_complete __rte_unused)
> >>>  	return rte_eth_linkstatus_set(dev, &link);
> >>>  }
> >>>
> >>> +static void
> >>> +print_ethaddr(const char *name, const struct rte_ether_addr *eth_addr)
> >>> +{
> >>> +	char buf[RTE_ETHER_ADDR_FMT_SIZE];
> >>> +
> >>> +	rte_ether_format_addr(buf, RTE_ETHER_ADDR_FMT_SIZE, eth_addr);
> >>> +	ENETC_PMD_INFO("%s%s\n", name, buf);
> >>> +}
> >>> +
> >>>  static int
> >>>  enetc_hardware_init(struct enetc_eth_hw *hw)
> >>>  {
> >>>  	struct enetc_hw *enetc_hw = &hw->hw;
> >>>  	uint32_t *mac = (uint32_t *)hw->mac.addr;
> >>> +	uint32_t high_mac = 0;
> >>> +	uint16_t low_mac = 0;
> >>>
> >>>  	PMD_INIT_FUNC_TRACE();
> >>>  	/* Calculating and storing the base HW addresses */
> >>> @@ -138,8 +150,29 @@ enetc_hardware_init(struct enetc_eth_hw *hw)
> >>>  	enetc_wr(enetc_hw, ENETC_SIMR, ENETC_SIMR_EN);
> >>>
> >>>  	*mac = (uint32_t)enetc_port_rd(enetc_hw, ENETC_PSIPMAR0(0));
> >>> +	high_mac = (uint32_t)*mac;
> >>>  	mac++;
> >>>  	*mac = (uint16_t)enetc_port_rd(enetc_hw, ENETC_PSIPMAR1(0));
> >>> +	low_mac = (uint16_t)*mac;
> >>> +
> >>> +	if ((high_mac | low_mac) == 0) {
> >>> +		char *first_byte;
> >>> +
> >>> +		ENETC_PMD_INFO("MAC is not available for this SI, "
> >>> +			       "set random MAC\n");
> >>> +		mac = (uint32_t *)hw->mac.addr;
> >>> +		*mac = (uint32_t)rte_rand();
> >>> +		first_byte = (char *)mac;
> >>> +		*first_byte &= 0xfe;	/* clear multicast bit */
> >>> +		*first_byte |= 0x02;	/* set local assignment bit (IEEE802) */
> >>> +
> >>> +		enetc_port_wr(enetc_hw, ENETC_PSIPMAR0(0), *mac);
> >>> +		mac++;
> >>> +		*mac = (uint16_t)rte_rand();
> >>> +		enetc_port_wr(enetc_hw, ENETC_PSIPMAR1(0), *mac);
> >>> +		print_ethaddr("New address: ",
> >>> +			      (const struct rte_ether_addr *)hw->mac.addr);
> >>> +	}
> >>>
> >>>  	return 0;
> >>>  }
> >>> @@ -906,5 +939,5 @@ RTE_INIT(enetc_pmd_init_log)
> >>>  {
> >>>  	enetc_logtype_pmd = rte_log_register("pmd.net.enetc");
> >>>  	if (enetc_logtype_pmd >= 0)
> >>> -		rte_log_set_level(enetc_logtype_pmd, RTE_LOG_NOTICE);
> >>> +		rte_log_set_level(enetc_logtype_pmd, RTE_LOG_INFO);
> >>
> >>
> >> This part look unrelated with the what commit log describes. And why you
> are
> >> making the driver more verbose by default?
> >>
> > I changed this to print MAC address by default. Currently, enetc driver is not
> supporting
> > NOTICE type log and the next supported log level is INFO and we are using
> INFO type log
> > only while setting the random MAC addresses.
> > If INFO type log is not ok to print this information, please suggest me. Should I
> add NOTICE
> > Type log? Or  just add a printf to display the MAC address.
> >
> 
> There is no strict rule, but mostly* drivers set the default log level to
> NOTICE, the intention is to make logging less noisy by default.
> 
> Is the log you have mentioned should be printed always for all applications by
> default, if so I suggest updating its level to NOTICE, (adding NOTICE level is
> easy or you can use 'ENETC_PMD_LOG' directly for it.)
> 
> But if that log is not required for all, I suggest keeping it INFO and the
> default level NOTICE.

Ok, will add the NOTICE level.
diff mbox series

Patch

diff --git a/drivers/net/enetc/Makefile b/drivers/net/enetc/Makefile
index 9895501db..312b37245 100644
--- a/drivers/net/enetc/Makefile
+++ b/drivers/net/enetc/Makefile
@@ -17,7 +17,7 @@  SRCS-$(CONFIG_RTE_LIBRTE_ENETC_PMD) += enetc_ethdev.c
 SRCS-$(CONFIG_RTE_LIBRTE_ENETC_PMD) += enetc_rxtx.c
 
 LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool
-LDLIBS += -lrte_ethdev
+LDLIBS += -lrte_ethdev -lrte_net
 LDLIBS += -lrte_bus_pci
 
 include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/drivers/net/enetc/enetc_ethdev.c b/drivers/net/enetc/enetc_ethdev.c
index 4e978348c..475ec77c3 100644
--- a/drivers/net/enetc/enetc_ethdev.c
+++ b/drivers/net/enetc/enetc_ethdev.c
@@ -4,6 +4,7 @@ 
 
 #include <stdbool.h>
 #include <rte_ethdev_pci.h>
+#include <rte_random.h>
 
 #include "enetc_logs.h"
 #include "enetc.h"
@@ -123,11 +124,22 @@  enetc_link_update(struct rte_eth_dev *dev, int wait_to_complete __rte_unused)
 	return rte_eth_linkstatus_set(dev, &link);
 }
 
+static void
+print_ethaddr(const char *name, const struct rte_ether_addr *eth_addr)
+{
+	char buf[RTE_ETHER_ADDR_FMT_SIZE];
+
+	rte_ether_format_addr(buf, RTE_ETHER_ADDR_FMT_SIZE, eth_addr);
+	ENETC_PMD_INFO("%s%s\n", name, buf);
+}
+
 static int
 enetc_hardware_init(struct enetc_eth_hw *hw)
 {
 	struct enetc_hw *enetc_hw = &hw->hw;
 	uint32_t *mac = (uint32_t *)hw->mac.addr;
+	uint32_t high_mac = 0;
+	uint16_t low_mac = 0;
 
 	PMD_INIT_FUNC_TRACE();
 	/* Calculating and storing the base HW addresses */
@@ -138,8 +150,29 @@  enetc_hardware_init(struct enetc_eth_hw *hw)
 	enetc_wr(enetc_hw, ENETC_SIMR, ENETC_SIMR_EN);
 
 	*mac = (uint32_t)enetc_port_rd(enetc_hw, ENETC_PSIPMAR0(0));
+	high_mac = (uint32_t)*mac;
 	mac++;
 	*mac = (uint16_t)enetc_port_rd(enetc_hw, ENETC_PSIPMAR1(0));
+	low_mac = (uint16_t)*mac;
+
+	if ((high_mac | low_mac) == 0) {
+		char *first_byte;
+
+		ENETC_PMD_INFO("MAC is not available for this SI, "
+			       "set random MAC\n");
+		mac = (uint32_t *)hw->mac.addr;
+		*mac = (uint32_t)rte_rand();
+		first_byte = (char *)mac;
+		*first_byte &= 0xfe;	/* clear multicast bit */
+		*first_byte |= 0x02;	/* set local assignment bit (IEEE802) */
+
+		enetc_port_wr(enetc_hw, ENETC_PSIPMAR0(0), *mac);
+		mac++;
+		*mac = (uint16_t)rte_rand();
+		enetc_port_wr(enetc_hw, ENETC_PSIPMAR1(0), *mac);
+		print_ethaddr("New address: ",
+			      (const struct rte_ether_addr *)hw->mac.addr);
+	}
 
 	return 0;
 }
@@ -906,5 +939,5 @@  RTE_INIT(enetc_pmd_init_log)
 {
 	enetc_logtype_pmd = rte_log_register("pmd.net.enetc");
 	if (enetc_logtype_pmd >= 0)
-		rte_log_set_level(enetc_logtype_pmd, RTE_LOG_NOTICE);
+		rte_log_set_level(enetc_logtype_pmd, RTE_LOG_INFO);
 }