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

[Stephen Hemminger]

On Wed, 18 Jun 2025 20:11:11 +0800
Wenbo Cao <caowe...@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;
}