[dpdk-dev] [PATCH v2 1/2] Added ETH_SPEED_CAP bitmap in rte_eth_dev_info

[dpdk-dev] [PATCH v2 1/2] Added ETH_SPEED_CAP bitmap in rte_eth_dev_info

[Morten Brørup]

Regarding the PHY speed ABI:

 

1. The Ethernet PHY ABI for speed, duplex, etc. should be common throughout the entire DPDK. It might be confusing if some structures/functions use a bitmask to indicate PHY speed/duplex/personality/etc. and other structures/functions use a combination of an unsigned integer, duplex flag, personality enumeration etc. (By personality enumeration, I am referring to PHYs with multiple electrical interfaces. E.g. a dual personality PHY might have both an RJ45 copper interface and an SFP module interface, whereof only one can be active at any time.)

 

2. The auto-negotiation standard allows the PHY to announce (to its link partner) any subset of its capabilities to its link partner. E.g. a standard 10/100/100 Ethernet PHY (which can handle both 10 and 100 Mbit/s in both half and full duplex and 1 Gbit/s full duplex) can be configured to announce 10 Mbit/s half duplex and 100 Mbit/s full duplex capabilities to its link partner. (Of course, more useful combinations are normally announced, but the purpose of the example is to show that any combination is possible.)

 

The ABI for auto-negotiation should include options to select the list of capabilities to announce to the link partner. The Linux PHY ABI only allows forcing a selected speed and duplex (thereby disabling auto-negotiation) or enabling auto-negotiation (thereby announcing all possible speeds and duplex combinations the PHY is capable of). Don't make the same mistake in DPDK.

 

PS: While working for Vitesse Semiconductors (an Ethernet chip company) a long time ago, I actually wrote the API for their line of Ethernet PHYs. So I have hands on experience in this area.

 

 

Med venlig hilsen / kind regards

 

Morten Brørup

CTO

 

 

 

SmartShare Systems A/S

Tonsbakken 16-18

DK-2740 Skovlunde

Denmark

 

Office      +45 70 20 00 93

Direct      +45 89 93 50 22

Mobile      +45 25 40 82 12

 

mb@smartsharesystems.com <mailto:mb@smartsharesystems.com> 

www.smartsharesystems.com <http://www.smartsharesystems.com/> 

 

Re: [dpdk-dev] [PATCH v2 1/2] Added ETH_SPEED_CAP bitmap in rte_eth_dev_info

[Marc Sune]

On 18/06/15 16:43, Morten Brørup wrote:
> Regarding the PHY speed ABI:
>
>   
>
> 1. The Ethernet PHY ABI for speed, duplex, etc. should be common throughout the entire DPDK. It might be confusing if some structures/functions use a bitmask to indicate PHY speed/duplex/personality/etc. and other structures/functions use a combination of an unsigned integer, duplex flag, personality enumeration etc. (By personality enumeration, I am referring to PHYs with multiple electrical interfaces. E.g. a dual personality PHY might have both an RJ45 copper interface and an SFP module interface, whereof only one can be active at any time.)

Thomas was sending a similar comment and I agreed to do a unified speed 
bitmap for both capabilities and link negotiation/link info (v3, waiting 
for 2.2 merge window):

http://dpdk.org/ml/archives/dev/2015-June/019207.html

>
>   
>
> 2. The auto-negotiation standard allows the PHY to announce (to its link partner) any subset of its capabilities to its link partner. E.g. a standard 10/100/100 Ethernet PHY (which can handle both 10 and 100 Mbit/s in both half and full duplex and 1 Gbit/s full duplex) can be configured to announce 10 Mbit/s half duplex and 100 Mbit/s full duplex capabilities to its link partner. (Of course, more useful combinations are normally announced, but the purpose of the example is to show that any combination is possible.)
>
>   
>
> The ABI for auto-negotiation should include options to select the list of capabilities to announce to the link partner. The Linux PHY ABI only allows forcing a selected speed and duplex (thereby disabling auto-negotiation) or enabling auto-negotiation (thereby announcing all possible speeds and duplex combinations the PHY is capable of). Don't make the same mistake in DPDK.

I see what you mean, and you are probably right. In any case this is for 
a separate patch, if we think it is a necessary feature to implement.

Nevertheless, this makes me rethink about the proposal from Thomas about 
unifying _100_HD/_100_FD to 100M, because you will need this 
granularity, eventually. @Thomas: opinions?

Thanks
Marc

p.s. Please configure your email client to reply using "In-Reply-To:" to 
allow clients and ML archives to use threading.

>   
>
> PS: While working for Vitesse Semiconductors (an Ethernet chip company) a long time ago, I actually wrote the API for their line of Ethernet PHYs. So I have hands on experience in this area.
>
>   
>
>   
>
> Med venlig hilsen / kind regards
>
>   
>
> Morten Brørup
>
> CTO
>
>   
>
>   
>
>   
>
> SmartShare Systems A/S
>
> Tonsbakken 16-18
>
> DK-2740 Skovlunde
>
> Denmark
>
>   
>
> Office      +45 70 20 00 93
>
> Direct      +45 89 93 50 22
>
> Mobile      +45 25 40 82 12
>
>   
>
> mb@smartsharesystems.com <mailto:mb@smartsharesystems.com>
>
> www.smartsharesystems.com <http://www.smartsharesystems.com/>
>
>   
>

Re: [dpdk-dev] [PATCH v2 1/2] Added ETH_SPEED_CAP bitmap in rte_eth_dev_info

[Thomas Monjalon]

2015-06-18 17:06, Marc Sune:
> On 18/06/15 16:43, Morten Brørup wrote:
> > Regarding the PHY speed ABI:
> >
> > 1. The Ethernet PHY ABI for speed, duplex, etc. should be common throughout the entire DPDK. It might be confusing if some structures/functions use a bitmask to indicate PHY speed/duplex/personality/etc. and other structures/functions use a combination of an unsigned integer, duplex flag, personality enumeration etc. (By personality enumeration, I am referring to PHYs with multiple electrical interfaces. E.g. a dual personality PHY might have both an RJ45 copper interface and an SFP module interface, whereof only one can be active at any time.)
> 
> Thomas was sending a similar comment and I agreed to do a unified speed 
> bitmap for both capabilities and link negotiation/link info (v3, waiting 
> for 2.2 merge window):
> 
> http://dpdk.org/ml/archives/dev/2015-June/019207.html

It would be better to try merging it in 2.1 while keeping an ABI backward
compatibility.

> > 2. The auto-negotiation standard allows the PHY to announce (to its link
> > partner) any subset of its capabilities to its link partner. E.g. a
> > standard 10/100/100 Ethernet PHY (which can handle both 10 and 100 Mbit/s
> > in both half and full duplex and 1 Gbit/s full duplex) can be configured
> > to announce 10 Mbit/s half duplex and 100 Mbit/s full duplex capabilities
> > to its link partner. (Of course, more useful combinations are normally
> > announced, but the purpose of the example is to show that any combination
> > is possible.)
> >
> > The ABI for auto-negotiation should include options to select the list of
> > capabilities to announce to the link partner. The Linux PHY ABI only
> > allows forcing a selected speed and duplex (thereby disabling
> > auto-negotiation) or enabling auto-negotiation (thereby announcing all
> > possible speeds and duplex combinations the PHY is capable of). Don't make
> > the same mistake in DPDK.
> 
> I see what you mean, and you are probably right. In any case this is for 
> a separate patch, if we think it is a necessary feature to implement.
> 
> Nevertheless, this makes me rethink about the proposal from Thomas about 
> unifying _100_HD/_100_FD to 100M, because you will need this 
> granularity, eventually. @Thomas: opinions?

I think Morten's advice is good.
Are we going to have half duplex links for PHY faster than 100M?
If not, we can manage them with a hackish define 100M_HD and a 100M
which implicitly means full duplex.

[dpdk-dev] [RFC PATCH 0/2] ethdev: add port speed capability bitmap

[Marc Sune]

The current rte_eth_dev_info abstraction does not provide any mechanism to
know the supported speed(s) of an ethdev.

For some drivers (e.g. ixgbe), an educated guess can be done based on the
driver's name (driver_name in rte_eth_dev_info), see:

http://dpdk.org/ml/archives/dev/2013-August/000412.html

However, i) doing string comparisons is annoying, and can silently
break existing applications if PMDs change their names ii) it does not
provide all the supported capabilities of the ethdev iii) for some drivers it
is impossible determine correctly the (max) speed by the application
(e.g. in i40, distinguish between XL710 and X710).

This small patch adds speed_capa bitmap in rte_eth_dev_info, which is filled
by the PMDs according to the physical device capabilities.

WARNING: the patch is only tested with e1000s, and should be reviewed for
accuracy.


Marc Sune (2):
  Added ETH_SPEED_CAP bitmap in rte_eth_dev_info
  Filling speed capability bitmaps in the PMDs

 lib/librte_ether/rte_ethdev.h       | 24 ++++++++++++++++++++++++
 lib/librte_pmd_e1000/em_ethdev.c    |  6 ++++++
 lib/librte_pmd_e1000/igb_ethdev.c   |  6 ++++++
 lib/librte_pmd_fm10k/fm10k_ethdev.c |  3 +++
 lib/librte_pmd_i40e/i40e_ethdev.c   |  9 +++++++++
 lib/librte_pmd_ixgbe/ixgbe_ethdev.c | 10 ++++++++++
 lib/librte_pmd_mlx4/mlx4.c          |  6 ++++++
 7 files changed, 64 insertions(+)

-- 
2.1.4

[dpdk-dev] [PATCH v2 0/2] ethdev: add port speed capability bitmap

[Marc Sune]

The current rte_eth_dev_info abstraction does not provide any mechanism to
get the supported speed(s) of an ethdev.

For some drivers (e.g. ixgbe), an educated guess can be done based on the
driver's name (driver_name in rte_eth_dev_info), see:

http://dpdk.org/ml/archives/dev/2013-August/000412.html

However, i) doing string comparisons is annoying, and can silently
break existing applications if PMDs change their names ii) it does not
provide all the supported capabilities of the ethdev iii) for some drivers it
is impossible determine correctly the (max) speed by the application
(e.g. in i40, distinguish between XL710 and X710).

This small patch adds speed_capa bitmap in rte_eth_dev_info, which is filled
by the PMDs according to the physical device capabilities.

v2: rebase, converted speed_capa into 32 bits bitmap, fixed alignment
(checkpatch).

Marc Sune (2):
  Added ETH_SPEED_CAP bitmap in rte_eth_dev_info
  Filling speed capability bitmaps in the PMDs

 drivers/net/e1000/em_ethdev.c    |  6 ++++++
 drivers/net/e1000/igb_ethdev.c   |  6 ++++++
 drivers/net/fm10k/fm10k_ethdev.c |  3 +++
 drivers/net/i40e/i40e_ethdev.c   |  9 +++++++++
 drivers/net/ixgbe/ixgbe_ethdev.c | 10 ++++++++++
 drivers/net/mlx4/mlx4.c          |  6 ++++++
 lib/librte_ether/rte_ethdev.h    | 24 ++++++++++++++++++++++++
 7 files changed, 64 insertions(+)

-- 
2.1.4

[dpdk-dev] [PATCH v2 1/2] Added ETH_SPEED_CAP bitmap in rte_eth_dev_info

[Marc Sune]

Added constants and bitmap to struct rte_eth_dev_info to be used by PMDs.

Signed-off-by: Marc Sune <marc.sune@bisdn.de>
---
 lib/librte_ether/rte_ethdev.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 16dbe00..f57676b 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -900,6 +900,29 @@ struct rte_eth_conf {
 #define DEV_TX_OFFLOAD_UDP_TSO     0x00000040
 #define DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM 0x00000080 /**< Used for tunneling packet. */
 
+/**
+ * Device supported speeds
+ */
+#define ETH_SPEED_CAP_NOT_PHY	(0)  /*< No phy media > */
+#define ETH_SPEED_CAP_10M_HD	(1 << 0)  /*< 10 Mbps half-duplex> */
+#define ETH_SPEED_CAP_10M_FD	(1 << 1)  /*< 10 Mbps full-duplex> */
+#define ETH_SPEED_CAP_100M_HD	(1 << 2)  /*< 100 Mbps half-duplex> */
+#define ETH_SPEED_CAP_100M_FD	(1 << 3)  /*< 100 Mbps full-duplex> */
+#define ETH_SPEED_CAP_1G	(1 << 4)  /*< 1 Gbps > */
+#define ETH_SPEED_CAP_2_5G	(1 << 5)  /*< 2.5 Gbps > */
+#define ETH_SPEED_CAP_5G	(1 << 6)  /*< 5 Gbps > */
+#define ETH_SPEED_CAP_10G	(1 << 7)  /*< 10 Mbps > */
+#define ETH_SPEED_CAP_20G	(1 << 8)  /*< 20 Gbps > */
+#define ETH_SPEED_CAP_25G	(1 << 9)  /*< 25 Gbps > */
+#define ETH_SPEED_CAP_40G	(1 << 10)  /*< 40 Gbps > */
+#define ETH_SPEED_CAP_50G	(1 << 11)  /*< 50 Gbps > */
+#define ETH_SPEED_CAP_56G	(1 << 12)  /*< 56 Gbps > */
+#define ETH_SPEED_CAP_100G	(1 << 13)  /*< 100 Gbps > */
+
+
+/**
+ * Ethernet device information
+ */
 struct rte_eth_dev_info {
 	struct rte_pci_device *pci_dev; /**< Device PCI information. */
 	const char *driver_name; /**< Device Driver name. */
@@ -925,6 +948,7 @@ struct rte_eth_dev_info {
 	uint16_t vmdq_queue_base; /**< First queue ID for VMDQ pools. */
 	uint16_t vmdq_queue_num;  /**< Queue number for VMDQ pools. */
 	uint16_t vmdq_pool_base;  /**< First ID of VMDQ pools. */
+	uint32_t speed_capa;  /**< Supported speeds bitmap (ETH_SPEED_CAP_). */
 };
 
 /** Maximum name length for extended statistics counters */
-- 
2.1.4

Re: [dpdk-dev] [PATCH v2 1/2] Added ETH_SPEED_CAP bitmap in rte_eth_dev_info

[Thomas Monjalon]

Hi Marc,

2015-05-26 21:50, Marc Sune:
> Added constants and bitmap to struct rte_eth_dev_info to be used by PMDs.
> 
> Signed-off-by: Marc Sune <marc.sune@bisdn.de>
[...]
> +/**
> + * Device supported speeds
> + */
> +#define ETH_SPEED_CAP_NOT_PHY	(0)  /*< No phy media > */

Why not starting with lower values? Some new drivers may be interested
by lower speed.

> +#define ETH_SPEED_CAP_10M_HD	(1 << 0)  /*< 10 Mbps half-duplex> */
> +#define ETH_SPEED_CAP_10M_FD	(1 << 1)  /*< 10 Mbps full-duplex> */
> +#define ETH_SPEED_CAP_100M_HD	(1 << 2)  /*< 100 Mbps half-duplex> */
> +#define ETH_SPEED_CAP_100M_FD	(1 << 3)  /*< 100 Mbps full-duplex> */
> +#define ETH_SPEED_CAP_1G	(1 << 4)  /*< 1 Gbps > */
> +#define ETH_SPEED_CAP_2_5G	(1 << 5)  /*< 2.5 Gbps > */
> +#define ETH_SPEED_CAP_5G	(1 << 6)  /*< 5 Gbps > */
> +#define ETH_SPEED_CAP_10G	(1 << 7)  /*< 10 Mbps > */
> +#define ETH_SPEED_CAP_20G	(1 << 8)  /*< 20 Gbps > */
> +#define ETH_SPEED_CAP_25G	(1 << 9)  /*< 25 Gbps > */
> +#define ETH_SPEED_CAP_40G	(1 << 10)  /*< 40 Gbps > */
> +#define ETH_SPEED_CAP_50G	(1 << 11)  /*< 50 Gbps > */
> +#define ETH_SPEED_CAP_56G	(1 << 12)  /*< 56 Gbps > */
> +#define ETH_SPEED_CAP_100G	(1 << 13)  /*< 100 Gbps > */

We should note that rte_eth_link is using ETH_LINK_SPEED_* constants
which are not some bitmaps so we have to create these new constants.
Furthermore, rte_eth_link.link_speed is an uint16_t so it is limited
to 40G. Should we use some constant bitmaps here also?
What about removing _CAP suffix from your constants?

[...]
> +	uint32_t speed_capa;  /**< Supported speeds bitmap (ETH_SPEED_CAP_). */

If the constants are ETH_SPEED_CAP, why not wording this variable speed_cap?

Re: [dpdk-dev] [PATCH v2 1/2] Added ETH_SPEED_CAP bitmap in rte_eth_dev_info

[Marc Sune]

On 27/05/15 06:02, Thomas Monjalon wrote:
> Hi Marc,
>
> 2015-05-26 21:50, Marc Sune:
>> Added constants and bitmap to struct rte_eth_dev_info to be used by PMDs.
>>
>> Signed-off-by: Marc Sune <marc.sune@bisdn.de>
> [...]
>> +/**
>> + * Device supported speeds
>> + */
>> +#define ETH_SPEED_CAP_NOT_PHY	(0)  /*< No phy media > */
> Why not starting with lower values? Some new drivers may be interested
> by lower speed.

Ok, but which values? 1Mbps FD/HD? Even lower than that?

If you have some NIC(s) in mind with lower values, please point me to 
that and I will collect&add the missing speeds.

>
>> +#define ETH_SPEED_CAP_10M_HD	(1 << 0)  /*< 10 Mbps half-duplex> */
>> +#define ETH_SPEED_CAP_10M_FD	(1 << 1)  /*< 10 Mbps full-duplex> */
>> +#define ETH_SPEED_CAP_100M_HD	(1 << 2)  /*< 100 Mbps half-duplex> */
>> +#define ETH_SPEED_CAP_100M_FD	(1 << 3)  /*< 100 Mbps full-duplex> */
>> +#define ETH_SPEED_CAP_1G	(1 << 4)  /*< 1 Gbps > */
>> +#define ETH_SPEED_CAP_2_5G	(1 << 5)  /*< 2.5 Gbps > */
>> +#define ETH_SPEED_CAP_5G	(1 << 6)  /*< 5 Gbps > */
>> +#define ETH_SPEED_CAP_10G	(1 << 7)  /*< 10 Mbps > */
>> +#define ETH_SPEED_CAP_20G	(1 << 8)  /*< 20 Gbps > */
>> +#define ETH_SPEED_CAP_25G	(1 << 9)  /*< 25 Gbps > */
>> +#define ETH_SPEED_CAP_40G	(1 << 10)  /*< 40 Gbps > */
>> +#define ETH_SPEED_CAP_50G	(1 << 11)  /*< 50 Gbps > */
>> +#define ETH_SPEED_CAP_56G	(1 << 12)  /*< 56 Gbps > */
>> +#define ETH_SPEED_CAP_100G	(1 << 13)  /*< 100 Gbps > */
> We should note that rte_eth_link is using ETH_LINK_SPEED_* constants
> which are not some bitmaps so we have to create these new constants.

Yes, I can add that to the patch description (1/2).

> Furthermore, rte_eth_link.link_speed is an uint16_t so it is limited
> to 40G. Should we use some constant bitmaps here also?

I also thought about converting link_speed into a bitmap to unify the 
constants before starting the patch (there is redundancy), but I wanted 
to be minimally invasive; changing link to a bitmap can break existing apps.

I can also merge them if we think is a better idea.
> What about removing _CAP suffix from your constants?

I added the suffix to make clearer the distinction with link speeds. I 
can remove it if we merge both or if we consider it is not necessary.

>
> [...]
>> +	uint32_t speed_capa;  /**< Supported speeds bitmap (ETH_SPEED_CAP_). */
> If the constants are ETH_SPEED_CAP, why not wording this variable speed_cap?

I followed the convention of the existing rx/tx offload capability bitmaps:

marc@dev:~/git/bisdn/msune/xdpd/libs/dpdk/lib$ grep _capa\; * -R
librte_ether/rte_ethdev.h:    uint32_t rx_offload_capa; /**< Device RX 
offload capabilities. */
librte_ether/rte_ethdev.h:    uint32_t tx_offload_capa; /**< Device TX 
offload capabilities. */

I am fine with speed_cap or speed_caps, but I think we should have some 
consistency on how we name bitmaps.

If we would want to make the bitmaps more explicit, we could define some 
helper typedefs in EAL:

typedef uint16_t bitmap16_t;
typedef uint32_t bitmap32_t;
typedef uint64_t bitmap64_t;

and replace the bitmaps of the structs, again specially the ones used by 
the users.

marc

>

Re: [dpdk-dev] [PATCH v2 1/2] Added ETH_SPEED_CAP bitmap in rte_eth_dev_info

[Thomas Monjalon]

2015-05-27 11:15, Marc Sune:
> On 27/05/15 06:02, Thomas Monjalon wrote:
> > Why not starting with lower values? Some new drivers may be interested
> > by lower speed.
> 
> Ok, but which values? 1Mbps FD/HD? Even lower than that?
> 
> If you have some NIC(s) in mind with lower values, please point me to 
> that and I will collect&add the missing speeds.

No sorry, I missed how low your first values were.

> >> +#define ETH_SPEED_CAP_10M_HD	(1 << 0)  /*< 10 Mbps half-duplex> */
> >> +#define ETH_SPEED_CAP_10M_FD	(1 << 1)  /*< 10 Mbps full-duplex> */
> >> +#define ETH_SPEED_CAP_100M_HD	(1 << 2)  /*< 100 Mbps half-duplex> */
> >> +#define ETH_SPEED_CAP_100M_FD	(1 << 3)  /*< 100 Mbps full-duplex> */
> >> +#define ETH_SPEED_CAP_1G	(1 << 4)  /*< 1 Gbps > */
> >> +#define ETH_SPEED_CAP_2_5G	(1 << 5)  /*< 2.5 Gbps > */
> >> +#define ETH_SPEED_CAP_5G	(1 << 6)  /*< 5 Gbps > */
> >> +#define ETH_SPEED_CAP_10G	(1 << 7)  /*< 10 Mbps > */
> >> +#define ETH_SPEED_CAP_20G	(1 << 8)  /*< 20 Gbps > */
> >> +#define ETH_SPEED_CAP_25G	(1 << 9)  /*< 25 Gbps > */
> >> +#define ETH_SPEED_CAP_40G	(1 << 10)  /*< 40 Gbps > */
> >> +#define ETH_SPEED_CAP_50G	(1 << 11)  /*< 50 Gbps > */
> >> +#define ETH_SPEED_CAP_56G	(1 << 12)  /*< 56 Gbps > */
> >> +#define ETH_SPEED_CAP_100G	(1 << 13)  /*< 100 Gbps > */
> > We should note that rte_eth_link is using ETH_LINK_SPEED_* constants
> > which are not some bitmaps so we have to create these new constants.
> 
> Yes, I can add that to the patch description (1/2).
> 
> > Furthermore, rte_eth_link.link_speed is an uint16_t so it is limited
> > to 40G. Should we use some constant bitmaps here also?
> 
> I also thought about converting link_speed into a bitmap to unify the 
> constants before starting the patch (there is redundancy), but I wanted 
> to be minimally invasive; changing link to a bitmap can break existing apps.
> 
> I can also merge them if we think is a better idea.

Maybe. Someone against this idea?

> > What about removing _CAP suffix from your constants?
> 
> I added the suffix to make clearer the distinction with link speeds. I 
> can remove it if we merge both or if we consider it is not necessary.
> 
> >
> > [...]
> >> +	uint32_t speed_capa;  /**< Supported speeds bitmap (ETH_SPEED_CAP_). */
> > If the constants are ETH_SPEED_CAP, why not wording this variable speed_cap?
> 
> I followed the convention of the existing rx/tx offload capability bitmaps:
> 
> marc@dev:~/git/bisdn/msune/xdpd/libs/dpdk/lib$ grep _capa\; * -R
> librte_ether/rte_ethdev.h:    uint32_t rx_offload_capa; /**< Device RX 
> offload capabilities. */
> librte_ether/rte_ethdev.h:    uint32_t tx_offload_capa; /**< Device TX 
> offload capabilities. */
> 
> I am fine with speed_cap or speed_caps, but I think we should have some 
> consistency on how we name bitmaps.

You're right.

> If we would want to make the bitmaps more explicit, we could define some 
> helper typedefs in EAL:
> 
> typedef uint16_t bitmap16_t;
> typedef uint32_t bitmap32_t;
> typedef uint64_t bitmap64_t;
> 
> and replace the bitmaps of the structs, again specially the ones used by 
> the users.

No, if we want to show this variable is a bitmap, the variable name
may be changed, not the type. It would bring clarity when reading code
using this variable but I think it's not really needed.

Re: [dpdk-dev] [PATCH v2 1/2] Added ETH_SPEED_CAP bitmap in rte_eth_dev_info

[Marc Sune]

On 29/05/15 20:23, Thomas Monjalon wrote:
> 2015-05-27 11:15, Marc Sune:
>> On 27/05/15 06:02, Thomas Monjalon wrote:
>>> Why not starting with lower values? Some new drivers may be interested
>>> by lower speed.
>> Ok, but which values? 1Mbps FD/HD? Even lower than that?
>>
>> If you have some NIC(s) in mind with lower values, please point me to
>> that and I will collect&add the missing speeds.
> No sorry, I missed how low your first values were.
>
>>>> +#define ETH_SPEED_CAP_10M_HD	(1 << 0)  /*< 10 Mbps half-duplex> */
>>>> +#define ETH_SPEED_CAP_10M_FD	(1 << 1)  /*< 10 Mbps full-duplex> */
>>>> +#define ETH_SPEED_CAP_100M_HD	(1 << 2)  /*< 100 Mbps half-duplex> */
>>>> +#define ETH_SPEED_CAP_100M_FD	(1 << 3)  /*< 100 Mbps full-duplex> */
>>>> +#define ETH_SPEED_CAP_1G	(1 << 4)  /*< 1 Gbps > */
>>>> +#define ETH_SPEED_CAP_2_5G	(1 << 5)  /*< 2.5 Gbps > */
>>>> +#define ETH_SPEED_CAP_5G	(1 << 6)  /*< 5 Gbps > */
>>>> +#define ETH_SPEED_CAP_10G	(1 << 7)  /*< 10 Mbps > */
>>>> +#define ETH_SPEED_CAP_20G	(1 << 8)  /*< 20 Gbps > */
>>>> +#define ETH_SPEED_CAP_25G	(1 << 9)  /*< 25 Gbps > */
>>>> +#define ETH_SPEED_CAP_40G	(1 << 10)  /*< 40 Gbps > */
>>>> +#define ETH_SPEED_CAP_50G	(1 << 11)  /*< 50 Gbps > */
>>>> +#define ETH_SPEED_CAP_56G	(1 << 12)  /*< 56 Gbps > */
>>>> +#define ETH_SPEED_CAP_100G	(1 << 13)  /*< 100 Gbps > */
>>> We should note that rte_eth_link is using ETH_LINK_SPEED_* constants
>>> which are not some bitmaps so we have to create these new constants.
>> Yes, I can add that to the patch description (1/2).
>>
>>> Furthermore, rte_eth_link.link_speed is an uint16_t so it is limited
>>> to 40G. Should we use some constant bitmaps here also?
>> I also thought about converting link_speed into a bitmap to unify the
>> constants before starting the patch (there is redundancy), but I wanted
>> to be minimally invasive; changing link to a bitmap can break existing apps.
>>
>> I can also merge them if we think is a better idea.
> Maybe. Someone against this idea?

Me. I tried implementing this unified speed constantss, but the problem 
is that for the capabilities full-duplex/half-duplex speeds are unrolled 
(e.g. 100M_HD/100_FD). There is no generic 100M to set a specific speed, 
so if you want a fiex speed and duplex auto-negociation witht the 
current set of constants, it would look weird; e.g. 
link_speed=ETH_SPEED_100M_HD and then set 
link_duplex=ETH_LINK_AUTONEG_DUPLEX):

  232 struct rte_eth_link {
  233         uint16_t link_speed;      /**< ETH_LINK_SPEED_[10, 100, 
1000, 10000] */
  234         uint16_t link_duplex;     /**< ETH_LINK_[HALF_DUPLEX, 
FULL_DUPLEX] */
  235         uint8_t  link_status : 1; /**< 1 -> link up, 0 -> link 
down */
  236 }__attribute__((aligned(8)));     /**< aligned for atomic64 
read/write */

There is another minor point, which is when setting the speed in 
rte_eth_conf:

  840 struct rte_eth_conf {
  841         uint16_t link_speed;
  842         /**< ETH_LINK_SPEED_10[0|00|000], or 0 for autonegotation */

0 is used for speed auto-negociation, but 0 is also used in the 
capabilities bitmap to indicate no PHY_MEDIA (virtual interface). I 
would have to define something like:

906 #define ETH_SPEED_NOT_PHY   (0)  /*< No phy media > */
907 #define ETH_SPEED_AUTONEG   (0)  /*< Autonegociate speed > */

And use (only) NOT_PHY for a capabilities and _AUTONEG for rte_eth_conf.

The options I see:

a) add to the the list of the current speeds generic 10M/100M/1G speeds 
without HD/FD, and just use these speeds in rte_eth_conf.
b) leave them separated.

I would vote for b), since the a) is not completely clean. 
Opinions&other alternatives welcome.

Marc

>
>>> What about removing _CAP suffix from your constants?
>> I added the suffix to make clearer the distinction with link speeds. I
>> can remove it if we merge both or if we consider it is not necessary.
>>
>>> [...]
>>>> +	uint32_t speed_capa;  /**< Supported speeds bitmap (ETH_SPEED_CAP_). */
>>> If the constants are ETH_SPEED_CAP, why not wording this variable speed_cap?
>> I followed the convention of the existing rx/tx offload capability bitmaps:
>>
>> marc@dev:~/git/bisdn/msune/xdpd/libs/dpdk/lib$ grep _capa\; * -R
>> librte_ether/rte_ethdev.h:    uint32_t rx_offload_capa; /**< Device RX
>> offload capabilities. */
>> librte_ether/rte_ethdev.h:    uint32_t tx_offload_capa; /**< Device TX
>> offload capabilities. */
>>
>> I am fine with speed_cap or speed_caps, but I think we should have some
>> consistency on how we name bitmaps.
> You're right.
>
>> If we would want to make the bitmaps more explicit, we could define some
>> helper typedefs in EAL:
>>
>> typedef uint16_t bitmap16_t;
>> typedef uint32_t bitmap32_t;
>> typedef uint64_t bitmap64_t;
>>
>> and replace the bitmaps of the structs, again specially the ones used by
>> the users.
> No, if we want to show this variable is a bitmap, the variable name
> may be changed, not the type. It would bring clarity when reading code
> using this variable but I think it's not really needed.
>

Re: [dpdk-dev] [PATCH v2 1/2] Added ETH_SPEED_CAP bitmap in rte_eth_dev_info

[Thomas Monjalon]

2015-06-08 10:50, Marc Sune:
> On 29/05/15 20:23, Thomas Monjalon wrote:
> > 2015-05-27 11:15, Marc Sune:
> >> On 27/05/15 06:02, Thomas Monjalon wrote:
> >>>> +#define ETH_SPEED_CAP_10M_HD	(1 << 0)  /*< 10 Mbps half-duplex> */
> >>>> +#define ETH_SPEED_CAP_10M_FD	(1 << 1)  /*< 10 Mbps full-duplex> */
> >>>> +#define ETH_SPEED_CAP_100M_HD	(1 << 2)  /*< 100 Mbps half-duplex> */
> >>>> +#define ETH_SPEED_CAP_100M_FD	(1 << 3)  /*< 100 Mbps full-duplex> */
> >>>> +#define ETH_SPEED_CAP_1G	(1 << 4)  /*< 1 Gbps > */
> >>>> +#define ETH_SPEED_CAP_2_5G	(1 << 5)  /*< 2.5 Gbps > */
> >>>> +#define ETH_SPEED_CAP_5G	(1 << 6)  /*< 5 Gbps > */
> >>>> +#define ETH_SPEED_CAP_10G	(1 << 7)  /*< 10 Mbps > */
> >>>> +#define ETH_SPEED_CAP_20G	(1 << 8)  /*< 20 Gbps > */
> >>>> +#define ETH_SPEED_CAP_25G	(1 << 9)  /*< 25 Gbps > */
> >>>> +#define ETH_SPEED_CAP_40G	(1 << 10)  /*< 40 Gbps > */
> >>>> +#define ETH_SPEED_CAP_50G	(1 << 11)  /*< 50 Gbps > */
> >>>> +#define ETH_SPEED_CAP_56G	(1 << 12)  /*< 56 Gbps > */
> >>>> +#define ETH_SPEED_CAP_100G	(1 << 13)  /*< 100 Gbps > */
> >>> We should note that rte_eth_link is using ETH_LINK_SPEED_* constants
> >>> which are not some bitmaps so we have to create these new constants.
> >> Yes, I can add that to the patch description (1/2).
> >>
> >>> Furthermore, rte_eth_link.link_speed is an uint16_t so it is limited
> >>> to 40G. Should we use some constant bitmaps here also?
> >> I also thought about converting link_speed into a bitmap to unify the
> >> constants before starting the patch (there is redundancy), but I wanted
> >> to be minimally invasive; changing link to a bitmap can break existing apps.
> >>
> >> I can also merge them if we think is a better idea.
> > Maybe. Someone against this idea?
> 
> Me. I tried implementing this unified speed constantss, but the problem 
> is that for the capabilities full-duplex/half-duplex speeds are unrolled 
> (e.g. 100M_HD/100_FD). There is no generic 100M to set a specific speed, 

Or we can define ETH_SPEED_CAP_100M and ETH_SPEED_CAP_100M_FD.
Is it possible to have a NIC doing 100M_FD but not 100M_HD?

> so if you want a fiex speed and duplex auto-negociation witht the 
> current set of constants, it would look weird; e.g. 
> link_speed=ETH_SPEED_100M_HD and then set 
> link_duplex=ETH_LINK_AUTONEG_DUPLEX):
> 
>   232 struct rte_eth_link {
>   233         uint16_t link_speed;      /**< ETH_LINK_SPEED_[10, 100, 
> 1000, 10000] */
>   234         uint16_t link_duplex;     /**< ETH_LINK_[HALF_DUPLEX, 
> FULL_DUPLEX] */
>   235         uint8_t  link_status : 1; /**< 1 -> link up, 0 -> link 
> down */
>   236 }__attribute__((aligned(8)));     /**< aligned for atomic64 
> read/write */
> 
> There is another minor point, which is when setting the speed in 
> rte_eth_conf:
> 
>   840 struct rte_eth_conf {
>   841         uint16_t link_speed;
>   842         /**< ETH_LINK_SPEED_10[0|00|000], or 0 for autonegotation */
> 
> 0 is used for speed auto-negociation, but 0 is also used in the 
> capabilities bitmap to indicate no PHY_MEDIA (virtual interface). I 
> would have to define something like:
> 
> 906 #define ETH_SPEED_NOT_PHY   (0)  /*< No phy media > */
> 907 #define ETH_SPEED_AUTONEG   (0)  /*< Autonegociate speed > */

Or something like SPEED_UNDEFINED

> And use (only) NOT_PHY for a capabilities and _AUTONEG for rte_eth_conf.
> 
> The options I see:
> 
> a) add to the the list of the current speeds generic 10M/100M/1G speeds 
> without HD/FD, and just use these speeds in rte_eth_conf.
> b) leave them separated.
> 
> I would vote for b), since the a) is not completely clean. 
> Opinions&other alternatives welcome.
> 
> Marc

Re: [dpdk-dev] [PATCH v2 1/2] Added ETH_SPEED_CAP bitmap in rte_eth_dev_info

[Marc Sune]

On 11/06/15 11:08, Thomas Monjalon wrote:
> 2015-06-08 10:50, Marc Sune:
>> On 29/05/15 20:23, Thomas Monjalon wrote:
>>> 2015-05-27 11:15, Marc Sune:
>>>> On 27/05/15 06:02, Thomas Monjalon wrote:
>>>>>> +#define ETH_SPEED_CAP_10M_HD	(1 << 0)  /*< 10 Mbps half-duplex> */
>>>>>> +#define ETH_SPEED_CAP_10M_FD	(1 << 1)  /*< 10 Mbps full-duplex> */
>>>>>> +#define ETH_SPEED_CAP_100M_HD	(1 << 2)  /*< 100 Mbps half-duplex> */
>>>>>> +#define ETH_SPEED_CAP_100M_FD	(1 << 3)  /*< 100 Mbps full-duplex> */
>>>>>> +#define ETH_SPEED_CAP_1G	(1 << 4)  /*< 1 Gbps > */
>>>>>> +#define ETH_SPEED_CAP_2_5G	(1 << 5)  /*< 2.5 Gbps > */
>>>>>> +#define ETH_SPEED_CAP_5G	(1 << 6)  /*< 5 Gbps > */
>>>>>> +#define ETH_SPEED_CAP_10G	(1 << 7)  /*< 10 Mbps > */
>>>>>> +#define ETH_SPEED_CAP_20G	(1 << 8)  /*< 20 Gbps > */
>>>>>> +#define ETH_SPEED_CAP_25G	(1 << 9)  /*< 25 Gbps > */
>>>>>> +#define ETH_SPEED_CAP_40G	(1 << 10)  /*< 40 Gbps > */
>>>>>> +#define ETH_SPEED_CAP_50G	(1 << 11)  /*< 50 Gbps > */
>>>>>> +#define ETH_SPEED_CAP_56G	(1 << 12)  /*< 56 Gbps > */
>>>>>> +#define ETH_SPEED_CAP_100G	(1 << 13)  /*< 100 Gbps > */
>>>>> We should note that rte_eth_link is using ETH_LINK_SPEED_* constants
>>>>> which are not some bitmaps so we have to create these new constants.
>>>> Yes, I can add that to the patch description (1/2).
>>>>
>>>>> Furthermore, rte_eth_link.link_speed is an uint16_t so it is limited
>>>>> to 40G. Should we use some constant bitmaps here also?
>>>> I also thought about converting link_speed into a bitmap to unify the
>>>> constants before starting the patch (there is redundancy), but I wanted
>>>> to be minimally invasive; changing link to a bitmap can break existing apps.
>>>>
>>>> I can also merge them if we think is a better idea.
>>> Maybe. Someone against this idea?
>> Me. I tried implementing this unified speed constantss, but the problem
>> is that for the capabilities full-duplex/half-duplex speeds are unrolled
>> (e.g. 100M_HD/100_FD). There is no generic 100M to set a specific speed,
> Or we can define ETH_SPEED_CAP_100M and ETH_SPEED_CAP_100M_FD.
> Is it possible to have a NIC doing 100M_FD but not 100M_HD?

Did not check in detail, but I guess this is mostly legacy NICs, not 
supported by DPDK anyway, and that is safe to assume 10M/100M meaning 
supports both HD/FD.

>
>> so if you want a fiex speed and duplex auto-negociation witht the
>> current set of constants, it would look weird; e.g.
>> link_speed=ETH_SPEED_100M_HD and then set
>> link_duplex=ETH_LINK_AUTONEG_DUPLEX):
>>
>>    232 struct rte_eth_link {
>>    233         uint16_t link_speed;      /**< ETH_LINK_SPEED_[10, 100,
>> 1000, 10000] */
>>    234         uint16_t link_duplex;     /**< ETH_LINK_[HALF_DUPLEX,
>> FULL_DUPLEX] */
>>    235         uint8_t  link_status : 1; /**< 1 -> link up, 0 -> link
>> down */
>>    236 }__attribute__((aligned(8)));     /**< aligned for atomic64
>> read/write */
>>
>> There is another minor point, which is when setting the speed in
>> rte_eth_conf:
>>
>>    840 struct rte_eth_conf {
>>    841         uint16_t link_speed;
>>    842         /**< ETH_LINK_SPEED_10[0|00|000], or 0 for autonegotation */
>>
>> 0 is used for speed auto-negociation, but 0 is also used in the
>> capabilities bitmap to indicate no PHY_MEDIA (virtual interface). I
>> would have to define something like:
>>
>> 906 #define ETH_SPEED_NOT_PHY   (0)  /*< No phy media > */
>> 907 #define ETH_SPEED_AUTONEG   (0)  /*< Autonegociate speed > */
> Or something like SPEED_UNDEFINED

Ok. I will prepare the patch and circulate a v3.

After briefly chatting offline with Thomas, it seems I was not clearly 
stating in my original v1 that this patch is targeting DPDK v2.2, due to 
ABI(and API) issues.

It is, and so I will hold v3 until 2.2 window starts, to make it more clear.

thanks
Marc
>
>> And use (only) NOT_PHY for a capabilities and _AUTONEG for rte_eth_conf.
>>
>> The options I see:
>>
>> a) add to the the list of the current speeds generic 10M/100M/1G speeds
>> without HD/FD, and just use these speeds in rte_eth_conf.
>> b) leave them separated.
>>
>> I would vote for b), since the a) is not completely clean.
>> Opinions&other alternatives welcome.
>>
>> Marc