Re: [PATCH v9 4/9] net/zxdh: add msg chan and msg hwlock init

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 11/1/2024 6:21 AM, Junlong Wang wrote:
> Add msg channel and hwlock init implementation.
> 
> Signed-off-by: Junlong Wang <wang.junlong1@zte.com.cn>
> 

<...>

> @@ -83,9 +84,23 @@ static int zxdh_eth_dev_init(struct rte_eth_dev *eth_dev)
>      if (ret < 0)
>          goto err_zxdh_init;
>  
> +    ret = zxdh_msg_chan_init();
> +    if (ret != 0) {
> +        PMD_INIT_LOG(ERR, "Failed to init bar msg chan");
> +        goto err_zxdh_init;
> +    }
> +    hw->msg_chan_init = 1;
> +
> +    ret = zxdh_msg_chan_hwlock_init(eth_dev);
> +    if (ret != 0) {
> +        PMD_INIT_LOG(ERR, "zxdh_msg_chan_hwlock_init failed ret %d", ret);
> +        goto err_zxdh_init;
> +    }
> +
>      return ret;
>  
>  err_zxdh_init:
> +    zxdh_bar_msg_chan_exit();
>

Should 'zxdh_bar_msg_chan_exit()' called during zxdh_eth_dev_uninit()?

<...>

> +
> +struct zxdh_dev_stat {
> +    bool is_mpf_scanned;
> +    bool is_res_init;
> +    int16_t dev_cnt; /* probe cnt */
> +};
> +struct zxdh_dev_stat g_dev_stat = {0};
>

Is there a reason to not make this global variable 'static'?
Please remember, when a DPDK application compiled, this will be all
application and other driver and libraries, if there is really a good
reason, please keep all global variables in the scope of driver.

And no need to initialize global variable to 0, that is done by default.

> +
> +struct zxdh_seqid_item {
> +    void *reps_addr;
> +    uint16_t id;
> +    uint16_t buffer_len;
> +    uint16_t flag;
> +};
> +
> +struct zxdh_seqid_ring {
> +    uint16_t cur_id;
> +    rte_spinlock_t lock;
> +    struct zxdh_seqid_item reps_info_tbl[ZXDH_BAR_SEQID_NUM_MAX];
> +};
> +struct zxdh_seqid_ring g_seqid_ring = {0};
> +

ditto

<...>

> +/**
> + * Fun: PF init hard_spinlock addr
> + */
> +static int bar_chan_pf_init_spinlock(uint16_t pcie_id, uint64_t bar_base_addr)
> +{
> +    int lock_id = pcie_id_to_hard_lock(pcie_id, ZXDH_MSG_CHAN_END_RISC);
> +
> +    zxdh_spinlock_unlock(lock_id, bar_base_addr + ZXDH_BAR0_SPINLOCK_OFFSET,
> +            bar_base_addr + ZXDH_HW_LABEL_OFFSET);
> +    lock_id = pcie_id_to_hard_lock(pcie_id, ZXDH_MSG_CHAN_END_VF);
> +    zxdh_spinlock_unlock(lock_id, bar_base_addr + ZXDH_BAR0_SPINLOCK_OFFSET,
> +            bar_base_addr + ZXDH_HW_LABEL_OFFSET);
> +    return 0;
> +}
> +
> +int zxdh_msg_chan_hwlock_init(struct rte_eth_dev *dev)
> +{
> +    struct zxdh_hw *hw = dev->data->dev_private;
> +
> +    if (!hw->is_pf)
> +        return 0;
> +    return bar_chan_pf_init_spinlock(hw->pcie_id, (uint64_t)(hw-
>>bar_addr[ZXDH_BAR0_INDEX]));
> +}
> +
> +static rte_spinlock_t chan_lock;
>

please move global variables to the top of the file, otherwise it is
very easy to miss them.