DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] Questions about reporting auto-negotiation capability
@ 2021-03-29  4:02 Huisong Li
  2021-03-29  7:03 ` Thomas Monjalon
  2021-03-29  7:28 ` [dpdk-dev] [PATCH] drivers/net: remove useless autoneg capability Thomas Monjalon
  0 siblings, 2 replies; 9+ messages in thread
From: Huisong Li @ 2021-03-29  4:02 UTC (permalink / raw)
  To: dev, ferruh.yigit, thomas

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.




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] Questions about reporting auto-negotiation capability
  2021-03-29  4:02 [dpdk-dev] Questions about reporting auto-negotiation capability Huisong Li
@ 2021-03-29  7:03 ` Thomas Monjalon
  2021-03-29  7:19   ` Thomas Monjalon
  2021-03-29  7:28 ` [dpdk-dev] [PATCH] drivers/net: remove useless autoneg capability Thomas Monjalon
  1 sibling, 1 reply; 9+ messages in thread
From: Thomas Monjalon @ 2021-03-29  7:03 UTC (permalink / raw)
  To: Huisong Li; +Cc: dev, ferruh.yigit

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.



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] Questions about reporting auto-negotiation capability
  2021-03-29  7:03 ` Thomas Monjalon
@ 2021-03-29  7:19   ` Thomas Monjalon
  2021-03-29 11:44     ` Huisong Li
  2021-03-29 14:18     ` Ajit Khaparde
  0 siblings, 2 replies; 9+ messages in thread
From: Thomas Monjalon @ 2021-03-29  7:19 UTC (permalink / raw)
  To: Huisong Li; +Cc: dev, ferruh.yigit, andrew.rybchenko, ajit.khaparde, jerinj

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?



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [dpdk-dev] [PATCH] drivers/net: remove useless autoneg capability
  2021-03-29  4:02 [dpdk-dev] Questions about reporting auto-negotiation capability Huisong Li
  2021-03-29  7:03 ` Thomas Monjalon
@ 2021-03-29  7:28 ` Thomas Monjalon
  2021-03-29 14:12   ` Ajit Khaparde
  1 sibling, 1 reply; 9+ messages in thread
From: Thomas Monjalon @ 2021-03-29  7:28 UTC (permalink / raw)
  To: dev
  Cc: lihuisong, ferruh.yigit, andrew.rybchenko, ajit.khaparde, jerinj,
	Somnath Kotur, Matan Azrad, Shahaf Shuler, Viacheslav Ovsiienko

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] Questions about reporting auto-negotiation capability
  2021-03-29  7:19   ` Thomas Monjalon
@ 2021-03-29 11:44     ` Huisong Li
  2021-03-29 12:05       ` Thomas Monjalon
  2021-03-29 14:18     ` Ajit Khaparde
  1 sibling, 1 reply; 9+ messages in thread
From: Huisong Li @ 2021-03-29 11:44 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: dev, ferruh.yigit, andrew.rybchenko, ajit.khaparde, jerinj


在 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.


>
>
> .

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] Questions about reporting auto-negotiation capability
  2021-03-29 11:44     ` Huisong Li
@ 2021-03-29 12:05       ` Thomas Monjalon
  0 siblings, 0 replies; 9+ messages in thread
From: Thomas Monjalon @ 2021-03-29 12:05 UTC (permalink / raw)
  To: Huisong Li; +Cc: dev, ferruh.yigit, andrew.rybchenko, ajit.khaparde, jerinj

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.




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] drivers/net: remove useless autoneg capability
  2021-03-29  7:28 ` [dpdk-dev] [PATCH] drivers/net: remove useless autoneg capability Thomas Monjalon
@ 2021-03-29 14:12   ` Ajit Khaparde
  2021-04-01 13:26     ` Ferruh Yigit
  0 siblings, 1 reply; 9+ messages in thread
From: Ajit Khaparde @ 2021-03-29 14:12 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: dpdk-dev, lihuisong, Ferruh Yigit, Andrew Rybchenko,
	Jerin Jacob Kollanukkaran, Somnath Kotur, Matan Azrad,
	Shahaf Shuler, Viacheslav Ovsiienko

[-- 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
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] Questions about reporting auto-negotiation capability
  2021-03-29  7:19   ` Thomas Monjalon
  2021-03-29 11:44     ` Huisong Li
@ 2021-03-29 14:18     ` Ajit Khaparde
  1 sibling, 0 replies; 9+ messages in thread
From: Ajit Khaparde @ 2021-03-29 14:18 UTC (permalink / raw)
  To: Thomas Monjalon
  Cc: Huisong Li, dpdk-dev, Ferruh Yigit, Andrew Rybchenko,
	Jerin Jacob Kollanukkaran

[-- 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.

>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dpdk-dev] [PATCH] drivers/net: remove useless autoneg capability
  2021-03-29 14:12   ` Ajit Khaparde
@ 2021-04-01 13:26     ` Ferruh Yigit
  0 siblings, 0 replies; 9+ messages in thread
From: Ferruh Yigit @ 2021-04-01 13:26 UTC (permalink / raw)
  To: Ajit Khaparde, Thomas Monjalon
  Cc: dpdk-dev, lihuisong, Andrew Rybchenko, Jerin Jacob Kollanukkaran,
	Somnath Kotur, Matan Azrad, Shahaf Shuler, Viacheslav Ovsiienko

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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-04-01 13:26 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-29  4:02 [dpdk-dev] Questions about reporting auto-negotiation capability Huisong Li
2021-03-29  7:03 ` Thomas Monjalon
2021-03-29  7:19   ` Thomas Monjalon
2021-03-29 11:44     ` Huisong Li
2021-03-29 12:05       ` Thomas Monjalon
2021-03-29 14:18     ` Ajit Khaparde
2021-03-29  7:28 ` [dpdk-dev] [PATCH] drivers/net: remove useless autoneg capability Thomas Monjalon
2021-03-29 14:12   ` Ajit Khaparde
2021-04-01 13:26     ` Ferruh Yigit

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git