Re: [PATCH v1 1/3] net/rnp: add check firmware respond info

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 18 Jun 2025 20:11:11 +0800
Wenbo Cao <caowenbo@mucse.com> wrote:

> diff --git a/drivers/net/rnp/rnp_ethdev.c b/drivers/net/rnp/rnp_ethdev.c
> index de1c077f61..7b996913c8 100644
> --- a/drivers/net/rnp/rnp_ethdev.c
> +++ b/drivers/net/rnp/rnp_ethdev.c
> @@ -751,17 +751,17 @@ rnp_get_speed_caps(struct rte_eth_dev *dev)
>  {
>  	struct rnp_eth_port *port = RNP_DEV_TO_PORT(dev);
>  	uint32_t speed_cap = 0;
> -	uint32_t i = 0, speed;
>  	uint32_t support_link;
> -	uint32_t link_types;
> +	uint32_t speed = 0;
> +	int bit_pos = 0;
>  
>  	support_link = port->attr.phy_meta.supported_link;
> -	link_types = rte_popcount64(support_link);
> -	if (!link_types)
> +	if (support_link == 0)
>  		return 0;
> -	for (i = 0; i < link_types; i++) {
> -		speed = ffs(support_link) - 1;
> -		switch (RTE_BIT32(speed)) {
> +	while (support_link) {
> +		bit_pos = ffs(support_link) - 1;
> +		speed = RTE_BIT32(bit_pos) & RNP_SPEED_VALID_MASK;
> +		switch (speed) {
>  		case RNP_SPEED_CAP_10M_FULL:
>  			speed_cap |= RTE_ETH_LINK_SPEED_10M;
>  			break;
> @@ -789,7 +789,7 @@ rnp_get_speed_caps(struct rte_eth_dev *dev)
>  		default:
>  			speed_cap |= 0;
>  		}
> -		support_link &= ~RTE_BIT32(speed);
> +		support_link &= ~(1ULL << bit_pos);
>  	}
>  	if (!port->attr.phy_meta.link_autoneg)
>  		speed_cap |= RTE_ETH_LINK_SPEED_FIXED;


Prefer not to mix raw masks and use of rte_bitops.
Better to stick to rte_bitops which is portable to Windows as well.
Can be simplified like:

static uint32_t
rnp_get_speed_caps(struct rte_eth_dev *dev)
{
	struct rnp_eth_port *port = RNP_DEV_TO_PORT(dev);
	uint32_t speed_cap = 0;
	uint32_t support_link = port->attr.phy_meta.supported_link & RNP_SPEED_VALID_MASK;

	while (support_link != 0) {
		unsigned int bit_pos = rte_ffs32(support_link);
		uint32_t speed = RTE_BIT32(bit_pos);

		switch (speed) {
		case RNP_SPEED_CAP_10M_FULL:
			speed_cap |= RTE_ETH_LINK_SPEED_10M;
			break;
		case RNP_SPEED_CAP_100M_FULL:
			speed_cap |= RTE_ETH_LINK_SPEED_100M;
			break;
		case RNP_SPEED_CAP_1GB_FULL:
			speed_cap |= RTE_ETH_LINK_SPEED_1G;
			break;
		case RNP_SPEED_CAP_10GB_FULL:
			speed_cap |= RTE_ETH_LINK_SPEED_10G;
			break;
		case RNP_SPEED_CAP_40GB_FULL:
			speed_cap |= RTE_ETH_LINK_SPEED_40G;
			break;
		case RNP_SPEED_CAP_25GB_FULL:
			speed_cap |= RTE_ETH_LINK_SPEED_25G;
			break;
		case RNP_SPEED_CAP_10M_HALF:
			speed_cap |= RTE_ETH_LINK_SPEED_10M_HD;
			break;
		case RNP_SPEED_CAP_100M_HALF:
			speed_cap |= RTE_ETH_LINK_SPEED_100M_HD;
			break;
		}
		support_link &= ~speed;
	}
	if (!port->attr.phy_meta.link_autoneg)
		speed_cap |= RTE_ETH_LINK_SPEED_FIXED;

	return speed_cap;
}