[dpdk-dev] [PATCH v13 6/8] ethdev: redesign link speed config

[Xing, Beilei]

> -Original Message-
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Thursday, March 31, 2016 8:29 PM
> To: Xing, Beilei 
> Cc: dev at dpdk.org; Marc ; Xu, Qian Q
> ; Ananyev, Konstantin  intel.com>;
> Lu, Wenzhuo ; Richardson, Bruce
> ; Glynn, Michael J  intel.com>
> Subject: Re: [dpdk-dev] [PATCH v13 6/8] ethdev: redesign link speed config
> 
> 2016-03-31 00:57, Xing, Beilei:
> > I?ve verified v13 + the modification, and it works.
> 
> Please, what have you tested? Which drivers? Which configurations?
> 
> It is important to test several uses of rte_eth_conf.link_speeds:
> - ETH_LINK_SPEED_AUTONEG (all speeds)
> - ETH_LINK_SPEED_FIXED (only one speed)
> - a subset of speeds to advertise and negotiate

I tested basic tx/rx on ixgbe and autoneg only on ixgbe.
I'll comment in another mail later, sorry for inconvenience.

[dpdk-dev] [PATCH v13 6/8] ethdev: redesign link speed config

[Thomas Monjalon]

2016-03-31 00:57, Xing, Beilei:
> I?ve verified v13 + the modification, and it works.

Please, what have you tested? Which drivers? Which configurations?

It is important to test several uses of rte_eth_conf.link_speeds:
- ETH_LINK_SPEED_AUTONEG (all speeds)
- ETH_LINK_SPEED_FIXED (only one speed)
- a subset of speeds to advertise and negotiate

[dpdk-dev] [PATCH v13 6/8] ethdev: redesign link speed config

[Xing, Beilei]

Marc,

I?ve verified v13 + the modification, and it works.

Best Regards
Beilei Xing

From: marc.sune at gmail.com [mailto:marc.s...@gmail.com] On Behalf Of Marc
Sent: Wednesday, March 30, 2016 4:00 PM
To: Xing, Beilei 
Cc: Thomas Monjalon ; Xu, Qian Q ; dev at dpdk.org; Ananyev, Konstantin ; Lu, Wenzhuo ; Richardson, Bruce 
; Glynn, Michael J 
Subject: Re: [PATCH v13 6/8] ethdev: redesign link speed config



On 29 March 2016 at 08:18, Xing, Beilei mailto:beilei.xing at intel.com>> wrote:


> -Original Message-
> From: Marc Sune [mailto:marcdevel at gmail.com]
> Sent: Saturday, March 26, 2016 9:27 AM
> To: Thomas Monjalon mailto:thomas.monjalon at 
> 6wind.com>>; Xu, Qian Q
> mailto:qian.q.xu at intel.com>>; Xing, Beilei 
> mailto:beilei.xing at intel.com>>; dev at 
> dpdk.org;
> Ananyev, Konstantin  intel.com>; Lu, Wenzhuo
> mailto:wenzhuo.lu at intel.com>>; Richardson, Bruce 
> mailto:bruce.richardson at intel.com>>;
> Glynn, Michael J mailto:michael.j.glynn at 
> intel.com>>
> Cc: Marc Sune mailto:marcdevel at gmail.com>>
> Subject: [PATCH v13 6/8] ethdev: redesign link speed config
>

> a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index a98e8eb..6cc2da0 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2193,32 +2195,21 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
>   if (err)
>   goto error;
>
> + speed = 0x0;
> + if (*link_speeds & ETH_LINK_SPEED_10G)
> + speed |= IXGBE_LINK_SPEED_10GB_FULL;
> + if (*link_speeds & ETH_LINK_SPEED_1G)
> + speed |= IXGBE_LINK_SPEED_1GB_FULL;
> + if (*link_speeds & ETH_LINK_SPEED_100M)
> + speed |= IXGBE_LINK_SPEED_100_FULL;
> +
>   err = ixgbe_setup_link(hw, speed, link_up);
>   if (err)
>   goto error;

Hi Marc,
According to ixgbe HW, link speed shouldn't be 0 when setting up,
Otherwise device will start fail. So we need to set speed if link_speed
is ETH_LINK_SPEED_AUTONEG. Following code is for reference.

speed = 0x0;
if ((*link_speeds & 0x1) == ETH_LINK_SPEED_AUTONEG)
speed = (hw->mac.type != ixgbe_mac_82598EB) ?
IXGBE_LINK_SPEED_82599_AUTONEG :
IXGBE_LINK_SPEED_82598_AUTONEG;
else {
if (*link_speeds & ETH_LINK_SPEED_10G)
speed |= IXGBE_LINK_SPEED_10GB_FULL;
if (*link_speeds & ETH_LINK_SPEED_1G)
speed |= IXGBE_LINK_SPEED_1GB_FULL;
if (*link_speeds & ETH_LINK_SPEED_100M)
speed |= IXGBE_LINK_SPEED_100_FULL;
}

Beilei,

OK, thanks.

Can you/someone please try v13 + this modification, so that we make sure this 
is the final version for ixgbe?

Regards
Marc


Beilei Xing
Thanks

[dpdk-dev] [PATCH v13 6/8] ethdev: redesign link speed config

[Marc]

On 29 March 2016 at 08:18, Xing, Beilei  wrote:

>
>
> > -Original Message-
> > From: Marc Sune [mailto:marcdevel at gmail.com]
> > Sent: Saturday, March 26, 2016 9:27 AM
> > To: Thomas Monjalon ; Xu, Qian Q
> > ; Xing, Beilei ;
> dev at dpdk.org;
> > Ananyev, Konstantin ; Lu, Wenzhuo
> > ; Richardson, Bruce  > intel.com>;
> > Glynn, Michael J 
> > Cc: Marc Sune 
> > Subject: [PATCH v13 6/8] ethdev: redesign link speed config
> >
>
> > a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> > index a98e8eb..6cc2da0 100644
> > --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> > @@ -2193,32 +2195,21 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
> >   if (err)
> >   goto error;
> >
> > + speed = 0x0;
> > + if (*link_speeds & ETH_LINK_SPEED_10G)
> > + speed |= IXGBE_LINK_SPEED_10GB_FULL;
> > + if (*link_speeds & ETH_LINK_SPEED_1G)
> > + speed |= IXGBE_LINK_SPEED_1GB_FULL;
> > + if (*link_speeds & ETH_LINK_SPEED_100M)
> > + speed |= IXGBE_LINK_SPEED_100_FULL;
> > +
> >   err = ixgbe_setup_link(hw, speed, link_up);
> >   if (err)
> >   goto error;
>
> Hi Marc,
> According to ixgbe HW, link speed shouldn't be 0 when setting up,
> Otherwise device will start fail. So we need to set speed if link_speed
> is ETH_LINK_SPEED_AUTONEG. Following code is for reference.
>
> speed = 0x0;
> if ((*link_speeds & 0x1) == ETH_LINK_SPEED_AUTONEG)
> speed = (hw->mac.type != ixgbe_mac_82598EB) ?
> IXGBE_LINK_SPEED_82599_AUTONEG :
> IXGBE_LINK_SPEED_82598_AUTONEG;
> else {
> if (*link_speeds & ETH_LINK_SPEED_10G)
> speed |= IXGBE_LINK_SPEED_10GB_FULL;
> if (*link_speeds & ETH_LINK_SPEED_1G)
> speed |= IXGBE_LINK_SPEED_1GB_FULL;
> if (*link_speeds & ETH_LINK_SPEED_100M)
> speed |= IXGBE_LINK_SPEED_100_FULL;
> }
>

Beilei,

OK, thanks.

Can you/someone please try v13 + this modification, so that we make sure
this is the final version for ixgbe?

Regards
Marc


>
> Beilei Xing
> Thanks
>

[dpdk-dev] [PATCH v13 6/8] ethdev: redesign link speed config

[Xing, Beilei]

> -Original Message-
> From: Marc Sune [mailto:marcdevel at gmail.com]
> Sent: Saturday, March 26, 2016 9:27 AM
> To: Thomas Monjalon ; Xu, Qian Q
> ; Xing, Beilei ; dev at 
> dpdk.org;
> Ananyev, Konstantin ; Lu, Wenzhuo
> ; Richardson, Bruce ;
> Glynn, Michael J 
> Cc: Marc Sune 
> Subject: [PATCH v13 6/8] ethdev: redesign link speed config
> 

> a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
> index a98e8eb..6cc2da0 100644
> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
> @@ -2193,32 +2195,21 @@ ixgbe_dev_start(struct rte_eth_dev *dev)
>   if (err)
>   goto error;
> 
> + speed = 0x0;
> + if (*link_speeds & ETH_LINK_SPEED_10G)
> + speed |= IXGBE_LINK_SPEED_10GB_FULL;
> + if (*link_speeds & ETH_LINK_SPEED_1G)
> + speed |= IXGBE_LINK_SPEED_1GB_FULL;
> + if (*link_speeds & ETH_LINK_SPEED_100M)
> + speed |= IXGBE_LINK_SPEED_100_FULL;
> +
>   err = ixgbe_setup_link(hw, speed, link_up);
>   if (err)
>   goto error;

Hi Marc,
According to ixgbe HW, link speed shouldn't be 0 when setting up, 
Otherwise device will start fail. So we need to set speed if link_speed 
is ETH_LINK_SPEED_AUTONEG. Following code is for reference.

speed = 0x0;
if ((*link_speeds & 0x1) == ETH_LINK_SPEED_AUTONEG)
speed = (hw->mac.type != ixgbe_mac_82598EB) ?
IXGBE_LINK_SPEED_82599_AUTONEG :
IXGBE_LINK_SPEED_82598_AUTONEG;
else {
if (*link_speeds & ETH_LINK_SPEED_10G)
speed |= IXGBE_LINK_SPEED_10GB_FULL;
if (*link_speeds & ETH_LINK_SPEED_1G)
speed |= IXGBE_LINK_SPEED_1GB_FULL;
if (*link_speeds & ETH_LINK_SPEED_100M)
speed |= IXGBE_LINK_SPEED_100_FULL;
}

Beilei Xing
Thanks

[dpdk-dev] [PATCH v13 6/8] ethdev: redesign link speed config

[Marc Sune]

This patch redesigns the API to set the link speed/s configuration
of an ethernet port. Specifically:

- it allows to define a set of advertised speeds for
  auto-negociation.
- it allows to disable link auto-negociation (single fixed speed).
- default: auto-negociate all supported speeds.

A flag autoneg in struct rte_eth_link indicates if link speed was a
result of auto-negociation or was fixed by configuration.

Signed-off-by: Marc Sune 
Tested-by: Nelio Laranjeiro 
Signed-off-by: Thomas Monjalon 
---
 app/test-pmd/cmdline.c|  26 
 doc/guides/rel_notes/deprecation.rst  |   3 -
 doc/guides/rel_notes/release_16_04.rst|   9 +++
 drivers/net/af_packet/rte_eth_af_packet.c |   1 +
 drivers/net/bnx2x/bnx2x_ethdev.c  |   4 +-
 drivers/net/bonding/rte_eth_bond_8023ad.c |   2 +-
 drivers/net/e1000/em_ethdev.c | 103 +++---
 drivers/net/e1000/igb_ethdev.c|  95 ++-
 drivers/net/i40e/i40e_ethdev.c|  48 +++---
 drivers/net/i40e/i40e_ethdev_vf.c |   7 +-
 drivers/net/ixgbe/ixgbe_ethdev.c  |  51 ++-
 drivers/net/mlx4/mlx4.c   |   2 +
 drivers/net/mlx5/mlx5_ethdev.c|   2 +
 drivers/net/mpipe/mpipe_tilegx.c  |   2 +
 drivers/net/null/rte_eth_null.c   |   1 +
 drivers/net/pcap/rte_eth_pcap.c   |   1 +
 drivers/net/ring/rte_eth_ring.c   |   1 +
 drivers/net/szedata2/rte_eth_szedata2.c   |   2 +
 drivers/net/vmxnet3/vmxnet3_ethdev.c  |   1 +
 drivers/net/xenvirt/rte_eth_xenvirt.c |   1 +
 examples/ip_pipeline/config_parse.c   |   3 +-
 lib/librte_ether/rte_ethdev.h |  29 +
 22 files changed, 207 insertions(+), 187 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 815b53b..741cac3 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -989,7 +989,7 @@ struct cmd_config_speed_all {
 };

 static int
-parse_and_check_speed_duplex(char *speedstr, char *duplexstr, uint16_t *speed)
+parse_and_check_speed_duplex(char *speedstr, char *duplexstr, uint32_t *speed)
 {

int duplex;
@@ -1006,20 +1006,22 @@ parse_and_check_speed_duplex(char *speedstr, char 
*duplexstr, uint16_t *speed)
}

if (!strcmp(speedstr, "10")) {
-   *speed = ETH_SPEED_NUM_10M;
+   *speed = (duplex == ETH_LINK_HALF_DUPLEX) ?
+   ETH_LINK_SPEED_10M_HD : ETH_LINK_SPEED_10M;
} else if (!strcmp(speedstr, "100")) {
-   *speed = ETH_SPEED_NUM_100M;
+   *speed = (duplex == ETH_LINK_HALF_DUPLEX) ?
+   ETH_LINK_SPEED_100M_HD : ETH_LINK_SPEED_100M;
} else {
if (duplex != ETH_LINK_FULL_DUPLEX) {
printf("Invalid speed/duplex parameters\n");
return -1;
}
if (!strcmp(speedstr, "1000")) {
-   *speed = ETH_SPEED_NUM_1G;
+   *speed = ETH_LINK_SPEED_1G;
} else if (!strcmp(speedstr, "1")) {
-   *speed = ETH_SPEED_NUM_10G;
+   *speed = ETH_LINK_SPEED_10G;
} else if (!strcmp(speedstr, "4")) {
-   *speed = ETH_SPEED_NUM_40G;
+   *speed = ETH_LINK_SPEED_40G;
} else if (!strcmp(speedstr, "auto")) {
*speed = ETH_LINK_SPEED_AUTONEG;
} else {
@@ -1037,8 +1039,7 @@ cmd_config_speed_all_parsed(void *parsed_result,
__attribute__((unused)) void *data)
 {
struct cmd_config_speed_all *res = parsed_result;
-   uint16_t link_speed = ETH_LINK_SPEED_AUTONEG;
-   uint16_t link_duplex = 0;
+   uint32_t link_speed;
portid_t pid;

if (!all_ports_stopped()) {
@@ -1051,8 +1052,7 @@ cmd_config_speed_all_parsed(void *parsed_result,
return;

FOREACH_PORT(pid, ports) {
-   ports[pid].dev_conf.link_speed = link_speed;
-   ports[pid].dev_conf.link_duplex = link_duplex;
+   ports[pid].dev_conf.link_speeds = link_speed;
}

cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
@@ -1110,8 +1110,7 @@ cmd_config_speed_specific_parsed(void *parsed_result,
__attribute__((unused)) void *data)
 {
struct cmd_config_speed_specific *res = parsed_result;
-   uint16_t link_speed = ETH_LINK_SPEED_AUTONEG;
-   uint16_t link_duplex = 0;
+   uint32_t link_speed;

if (!all_ports_stopped()) {
printf("Please stop all ports first\n");
@@ -1125,8 +1124,7 @@ cmd_config_speed_specific_parsed(void *parsed_result,
_speed) < 0)
return;

-   ports[res->id].dev_conf.link_speed = link_speed;
-   ports[res->id].dev_conf.link_duplex = link_duplex;
+