[dpdk-dev] Questions about reporting auto-negotiation capability

[dpdk-dev] Questions about reporting auto-negotiation capability

[Huisong Li]

Hi, all

         'speed_capa' in struct rte_eth_dev_info is defined as follows:

uint32_t speed_capa;  /**< Supported speeds bitmap (ETH_LINK_SPEED_). */


       Most PMD drivers use this field to report the speeds capability 
supported by the device to the upper-layer app.

But it seems that few NICs report their auto-negotiation capability 
through this field. If NIC also uses it to report

their auto-negotiation capability through this field, and should set it 
to ETH_LINK_SPEED_AUTONEG(0) based on

the definition of ETH_LINK_SPEED_xxx. In this case, it conflicts the 
report of the speeds capability .


I don't know how to correctly report the auto-negotiation capability of 
the device. Thanks for your reply.

Re: [dpdk-dev] Questions about reporting auto-negotiation capability

[Thomas Monjalon]

29/03/2021 06:02, Huisong Li:
> Hi, all
> 
>          'speed_capa' in struct rte_eth_dev_info is defined as follows:
> 
> uint32_t speed_capa;  /**< Supported speeds bitmap (ETH_LINK_SPEED_). */
> 
> 
>        Most PMD drivers use this field to report the speeds capability 
> supported by the device to the upper-layer app.
> 
> But it seems that few NICs report their auto-negotiation capability 
> through this field. If NIC also uses it to report
> 
> their auto-negotiation capability through this field, and should set it 
> to ETH_LINK_SPEED_AUTONEG(0) based on
> 
> the definition of ETH_LINK_SPEED_xxx. In this case, it conflicts the 
> report of the speeds capability .
> 
> 
> I don't know how to correctly report the auto-negotiation capability of 
> the device. Thanks for your reply.

ETH_LINK_SPEED_AUTONEG is not for capabilities.
Anyway, if it is set, it changes nothing (0) in the bitmap.
I see mlx5 is wrongly using it.

speed_capa is only for enumerating speeds.

Re: [dpdk-dev] Questions about reporting auto-negotiation capability

[Thomas Monjalon]

29/03/2021 09:03, Thomas Monjalon:
> 29/03/2021 06:02, Huisong Li:
> >          'speed_capa' in struct rte_eth_dev_info is defined as follows:
> > 
> > uint32_t speed_capa;  /**< Supported speeds bitmap (ETH_LINK_SPEED_). */
> > 
> > 
> >        Most PMD drivers use this field to report the speeds capability 
> > supported by the device to the upper-layer app.
> > 
> > But it seems that few NICs report their auto-negotiation capability 
> > through this field. If NIC also uses it to report
> > their auto-negotiation capability through this field, and should set it 
> > to ETH_LINK_SPEED_AUTONEG(0) based on
> > the definition of ETH_LINK_SPEED_xxx. In this case, it conflicts the 
> > report of the speeds capability .
> > 
> > I don't know how to correctly report the auto-negotiation capability of 
> > the device. Thanks for your reply.
> 
> ETH_LINK_SPEED_AUTONEG is not for capabilities.
> Anyway, if it is set, it changes nothing (0) in the bitmap.
> I see mlx5 is wrongly using it.
> 
> speed_capa is only for enumerating speeds.

I see some drivers are advertising ETH_LINK_SPEED_FIXED in speed_capa
if the device cannot support autonegotiation.
Should we add a note in doxygen?

[dpdk-dev] [PATCH] drivers/net: remove useless autoneg capability

[Thomas Monjalon]

The flag ETH_LINK_SPEED_AUTONEG is 0,
so it cannot be used in a capability bitmap.

Having 0 in speed capability means all speeds are accepted.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 drivers/net/bnxt/bnxt_ethdev.c          | 2 --
 drivers/net/mlx5/linux/mlx5_ethdev_os.c | 4 ----
 2 files changed, 6 deletions(-)

diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index f1dd40591f..a2debdbb64 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -889,8 +889,6 @@ uint32_t bnxt_get_speed_capabilities(struct bnxt *bp)
 	if (bp->link_info->auto_mode ==
 	    HWRM_PORT_PHY_QCFG_OUTPUT_AUTO_MODE_NONE)
 		speed_capa |= ETH_LINK_SPEED_FIXED;
-	else
-		speed_capa |= ETH_LINK_SPEED_AUTONEG;
 
 	return speed_capa;
 }
diff --git a/drivers/net/mlx5/linux/mlx5_ethdev_os.c b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
index 0e8de9439e..58d56913ae 100644
--- a/drivers/net/mlx5/linux/mlx5_ethdev_os.c
+++ b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
@@ -409,8 +409,6 @@ mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev,
 	else
 		dev_link.link_speed = link_speed;
 	priv->link_speed_capa = 0;
-	if (edata.supported & SUPPORTED_Autoneg)
-		priv->link_speed_capa |= ETH_LINK_SPEED_AUTONEG;
 	if (edata.supported & (SUPPORTED_1000baseT_Full |
 			       SUPPORTED_1000baseKX_Full))
 		priv->link_speed_capa |= ETH_LINK_SPEED_1G;
@@ -517,8 +515,6 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev,
 	sc = ecmd->link_mode_masks[0] |
 		((uint64_t)ecmd->link_mode_masks[1] << 32);
 	priv->link_speed_capa = 0;
-	if (sc & MLX5_BITSHIFT(ETHTOOL_LINK_MODE_Autoneg_BIT))
-		priv->link_speed_capa |= ETH_LINK_SPEED_AUTONEG;
 	if (sc & (MLX5_BITSHIFT(ETHTOOL_LINK_MODE_1000baseT_Full_BIT) |
 		  MLX5_BITSHIFT(ETHTOOL_LINK_MODE_1000baseKX_Full_BIT)))
 		priv->link_speed_capa |= ETH_LINK_SPEED_1G;
-- 
2.30.1

Re: [dpdk-dev] Questions about reporting auto-negotiation capability

[Huisong Li]

在 2021/3/29 15:19, Thomas Monjalon 写道:
> 29/03/2021 09:03, Thomas Monjalon:
>> 29/03/2021 06:02, Huisong Li:
>>>           'speed_capa' in struct rte_eth_dev_info is defined as follows:
>>>
>>> uint32_t speed_capa;  /**< Supported speeds bitmap (ETH_LINK_SPEED_). */
>>>
>>>
>>>         Most PMD drivers use this field to report the speeds capability
>>> supported by the device to the upper-layer app.
>>>
>>> But it seems that few NICs report their auto-negotiation capability
>>> through this field. If NIC also uses it to report
>>> their auto-negotiation capability through this field, and should set it
>>> to ETH_LINK_SPEED_AUTONEG(0) based on
>>> the definition of ETH_LINK_SPEED_xxx. In this case, it conflicts the
>>> report of the speeds capability .
>>>
>>> I don't know how to correctly report the auto-negotiation capability of
>>> the device. Thanks for your reply.
>> ETH_LINK_SPEED_AUTONEG is not for capabilities.
>> Anyway, if it is set, it changes nothing (0) in the bitmap.
>> I see mlx5 is wrongly using it.
>>
>> speed_capa is only for enumerating speeds.
> I see some drivers are advertising ETH_LINK_SPEED_FIXED in speed_capa
> if the device cannot support autonegotiation.
> Should we add a note in doxygen?

Can we use bit 0 to indicate whether the device supports auto-negotiation?

Like,

1/ If device doesn't support auto-negotiation, set bit(0) of the 
'speed_capa' to 1, namely, "speed_capa |= ETH_LINK_SPEED_FIXED".

2/ Other bits of the 'speed_capa' report all the speed capabilities 
supported by the device regardless of the value of bit(0) .

The above behavior is similar to cxgbe, bnxt, and mlx5. In this way, 
users can know whether the device supports auto-negotiation

based on bit(0) and detect the supported speed capabilities based on 
other bits.

After all, this 'speed_capa' and the 'link_speeds' in "rte_eth_conf" 
struct have different purposes.


>
>
> .

Re: [dpdk-dev] Questions about reporting auto-negotiation capability

[Thomas Monjalon]

29/03/2021 13:44, Huisong Li:
> 
> 在 2021/3/29 15:19, Thomas Monjalon 写道:
> > 29/03/2021 09:03, Thomas Monjalon:
> >> 29/03/2021 06:02, Huisong Li:
> >>>           'speed_capa' in struct rte_eth_dev_info is defined as follows:
> >>>
> >>> uint32_t speed_capa;  /**< Supported speeds bitmap (ETH_LINK_SPEED_). */
> >>>
> >>>
> >>>         Most PMD drivers use this field to report the speeds capability
> >>> supported by the device to the upper-layer app.
> >>>
> >>> But it seems that few NICs report their auto-negotiation capability
> >>> through this field. If NIC also uses it to report
> >>> their auto-negotiation capability through this field, and should set it
> >>> to ETH_LINK_SPEED_AUTONEG(0) based on
> >>> the definition of ETH_LINK_SPEED_xxx. In this case, it conflicts the
> >>> report of the speeds capability .
> >>>
> >>> I don't know how to correctly report the auto-negotiation capability of
> >>> the device. Thanks for your reply.
> >> ETH_LINK_SPEED_AUTONEG is not for capabilities.
> >> Anyway, if it is set, it changes nothing (0) in the bitmap.
> >> I see mlx5 is wrongly using it.
> >>
> >> speed_capa is only for enumerating speeds.
> > I see some drivers are advertising ETH_LINK_SPEED_FIXED in speed_capa
> > if the device cannot support autonegotiation.
> > Should we add a note in doxygen?
> 
> Can we use bit 0 to indicate whether the device supports auto-negotiation?
> 
> Like,
> 
> 1/ If device doesn't support auto-negotiation, set bit(0) of the 
> 'speed_capa' to 1, namely, "speed_capa |= ETH_LINK_SPEED_FIXED".

Yes I think this is what FIXED means: cannot negotiate.

> 2/ Other bits of the 'speed_capa' report all the speed capabilities 
> supported by the device regardless of the value of bit(0) .
> 
> The above behavior is similar to cxgbe, bnxt, and mlx5. In this way, 
> users can know whether the device supports auto-negotiation
> 
> based on bit(0) and detect the supported speed capabilities based on 
> other bits.
> 
> After all, this 'speed_capa' and the 'link_speeds' in "rte_eth_conf" 
> struct have different purposes.

Re: [dpdk-dev] [PATCH] drivers/net: remove useless autoneg capability

[Ajit Khaparde]

[-- Attachment #1: Type: text/plain, Size: 2316 bytes --]

On Mon, Mar 29, 2021 at 12:29 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> The flag ETH_LINK_SPEED_AUTONEG is 0,
> so it cannot be used in a capability bitmap.
>
> Having 0 in speed capability means all speeds are accepted.
>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  drivers/net/bnxt/bnxt_ethdev.c          | 2 --
For bnxt
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

>  drivers/net/mlx5/linux/mlx5_ethdev_os.c | 4 ----
>  2 files changed, 6 deletions(-)
>
> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> index f1dd40591f..a2debdbb64 100644
> --- a/drivers/net/bnxt/bnxt_ethdev.c
> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> @@ -889,8 +889,6 @@ uint32_t bnxt_get_speed_capabilities(struct bnxt *bp)
>         if (bp->link_info->auto_mode ==
>             HWRM_PORT_PHY_QCFG_OUTPUT_AUTO_MODE_NONE)
>                 speed_capa |= ETH_LINK_SPEED_FIXED;
> -       else
> -               speed_capa |= ETH_LINK_SPEED_AUTONEG;
>
>         return speed_capa;
>  }
> diff --git a/drivers/net/mlx5/linux/mlx5_ethdev_os.c b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
> index 0e8de9439e..58d56913ae 100644
> --- a/drivers/net/mlx5/linux/mlx5_ethdev_os.c
> +++ b/drivers/net/mlx5/linux/mlx5_ethdev_os.c
> @@ -409,8 +409,6 @@ mlx5_link_update_unlocked_gset(struct rte_eth_dev *dev,
>         else
>                 dev_link.link_speed = link_speed;
>         priv->link_speed_capa = 0;
> -       if (edata.supported & SUPPORTED_Autoneg)
> -               priv->link_speed_capa |= ETH_LINK_SPEED_AUTONEG;
>         if (edata.supported & (SUPPORTED_1000baseT_Full |
>                                SUPPORTED_1000baseKX_Full))
>                 priv->link_speed_capa |= ETH_LINK_SPEED_1G;
> @@ -517,8 +515,6 @@ mlx5_link_update_unlocked_gs(struct rte_eth_dev *dev,
>         sc = ecmd->link_mode_masks[0] |
>                 ((uint64_t)ecmd->link_mode_masks[1] << 32);
>         priv->link_speed_capa = 0;
> -       if (sc & MLX5_BITSHIFT(ETHTOOL_LINK_MODE_Autoneg_BIT))
> -               priv->link_speed_capa |= ETH_LINK_SPEED_AUTONEG;
>         if (sc & (MLX5_BITSHIFT(ETHTOOL_LINK_MODE_1000baseT_Full_BIT) |
>                   MLX5_BITSHIFT(ETHTOOL_LINK_MODE_1000baseKX_Full_BIT)))
>                 priv->link_speed_capa |= ETH_LINK_SPEED_1G;
> --
> 2.30.1
>

Re: [dpdk-dev] Questions about reporting auto-negotiation capability

[Ajit Khaparde]

[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]

On Mon, Mar 29, 2021 at 12:19 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 29/03/2021 09:03, Thomas Monjalon:
> > 29/03/2021 06:02, Huisong Li:
> > >          'speed_capa' in struct rte_eth_dev_info is defined as follows:
> > >
> > > uint32_t speed_capa;  /**< Supported speeds bitmap (ETH_LINK_SPEED_). */
> > >
> > >
> > >        Most PMD drivers use this field to report the speeds capability
> > > supported by the device to the upper-layer app.
> > >
> > > But it seems that few NICs report their auto-negotiation capability
> > > through this field. If NIC also uses it to report
> > > their auto-negotiation capability through this field, and should set it
> > > to ETH_LINK_SPEED_AUTONEG(0) based on
> > > the definition of ETH_LINK_SPEED_xxx. In this case, it conflicts the
> > > report of the speeds capability .
> > >
> > > I don't know how to correctly report the auto-negotiation capability of
> > > the device. Thanks for your reply.
> >
> > ETH_LINK_SPEED_AUTONEG is not for capabilities.
> > Anyway, if it is set, it changes nothing (0) in the bitmap.
> > I see mlx5 is wrongly using it.
> >
> > speed_capa is only for enumerating speeds.
>
> I see some drivers are advertising ETH_LINK_SPEED_FIXED in speed_capa
> if the device cannot support autonegotiation.
> Should we add a note in doxygen?
I think we should do that.

>
>

Re: [dpdk-dev] [PATCH] drivers/net: remove useless autoneg capability

[Ferruh Yigit]

On 3/29/2021 3:12 PM, Ajit Khaparde wrote:
> On Mon, Mar 29, 2021 at 12:29 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>> The flag ETH_LINK_SPEED_AUTONEG is 0,
>> so it cannot be used in a capability bitmap.
>>
>> Having 0 in speed capability means all speeds are accepted.
>>
>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>> ---
>>   drivers/net/bnxt/bnxt_ethdev.c          | 2 --
> For bnxt
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> 

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/main, thanks.