Re: [dpdk-dev] [PATCH] net/e1000: fix nic ops function was no initialized in secondary process

[张 杨]

发件人: Wang, Haiyue <haiyue.w...@intel.com>
发送时间: 2021年6月21日星期一 15:31
收件人: Tengfei Zhang
抄送: dev@dpdk.org; Zhang, Qi Z; Lin, Xueqin
主题: RE: [PATCH] net/e1000: fix nic ops function was no initialized in secondary 
process

> -----Original Message-----
> From: Tengfei Zhang <zypsc...@outlook.com>
> Sent: Saturday, June 19, 2021 01:27
> To: Wang, Haiyue <haiyue.w...@intel.com>
> Cc: dev@dpdk.org; Tengfei Zhang <zypsc...@outlook.com>
> Subject: [PATCH] net/e1000: fix nic ops function was no initialized in 
> secondary process
>
> 'e1000_setup_init_funcs' was not called in secondary process,
> it initialize mac,phy,nvm ops.
> when secondary process get link status,it will coredump.

Thanks, Tengfei.

Since primary / secondary is so complicated, AFAIK, the control path is in
primary, the secondary is mainly for rx/tx ops officially, like new Intel
ice PMD:

        if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
                ice_set_rx_function(dev);
                ice_set_tx_function(dev);
                return 0;
        }

So you can keep your patch as private for special secondary usage. ;-)

>
> Signed-off-by: Tengfei Zhang <zypsc...@outlook.com>
> ---
>  drivers/net/e1000/em_ethdev.c  | 1 +
>  drivers/net/e1000/igb_ethdev.c | 2 ++
>  2 files changed, 3 insertions(+)
>
> diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
> index a0ca371b02..cd5faa4228 100644
> --- a/drivers/net/e1000/em_ethdev.c
> +++ b/drivers/net/e1000/em_ethdev.c
> @@ -258,6 +258,7 @@ eth_em_dev_init(struct rte_eth_dev *eth_dev)
>         * has already done this work. Only check we don't need a different
>         * RX function */
>        if (rte_eal_process_type() != RTE_PROC_PRIMARY){
> +             e1000_setup_init_funcs(hw, TRUE);
>                if (eth_dev->data->scattered_rx)
>                        eth_dev->rx_pkt_burst =
>                                (eth_rx_burst_t)&eth_em_recv_scattered_pkts;
> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
> index 10ee0f3341..7d9d60497d 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -737,6 +737,7 @@ eth_igb_dev_init(struct rte_eth_dev *eth_dev)
>         * has already done this work. Only check we don't need a different
>         * RX function */
>        if (rte_eal_process_type() != RTE_PROC_PRIMARY){
> +             e1000_setup_init_funcs(hw, TRUE);
>                if (eth_dev->data->scattered_rx)
>                        eth_dev->rx_pkt_burst = &eth_igb_recv_scattered_pkts;
>                return 0;
> @@ -931,6 +932,7 @@ eth_igbvf_dev_init(struct rte_eth_dev *eth_dev)
>         * has already done this work. Only check we don't need a different
>         * RX function */
>        if (rte_eal_process_type() != RTE_PROC_PRIMARY){
> +             e1000_setup_init_funcs(hw, TRUE);
>                if (eth_dev->data->scattered_rx)
>                        eth_dev->rx_pkt_burst = &eth_igb_recv_scattered_pkts;
>                return 0;
> --
> 2.26.2





this issue does not appear in  ice, i40e, vmxnet3 PMD drivers. Only in e1000 , 
ixgbe drivers.
Ice  pmd driver gets link status by read reg directly.

I agree with what you said "primary, the secondary is mainly for rx/tx ops 
officially".
My opinion is the "set actions" shouldn't called in secondary process, but "get 
actions" was very common operation, they shouldn't be banned.

Thanks for your reply