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

["Wang, Haiyue" <haiyue.wang@intel.com>]

From: 张 杨 <zypscode@outlook.com> 
Sent: Monday, June 21, 2021 16:35
To: Wang, Haiyue <haiyue.wang@intel.com>
Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Lin, Xueqin <xueqin.lin@intel.com>
Subject: Re: [PATCH] net/e1000: fix nic ops function was no initialized in secondary process

发件人: Wang, Haiyue <mailto:haiyue.wang@intel.com>
发送时间: 2021年6月21日星期一 15:31
收件人: Tengfei Zhang
抄送: mailto: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 <mailto:zypscode@outlook.com>
> Sent: Saturday, June 19, 2021 01:27
> To: Wang, Haiyue <mailto:haiyue.wang@intel.com>
> Cc: mailto:dev@dpdk.org; Tengfei Zhang <mailto:zypscode@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 <mailto:zypscode@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.

For making primary & secondary working well, these drivers try to avoid save
the global data and ops function in shared data at the design of beginning. 


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

It's not banned, just because e1000's design introduces the global data ops in the
shared data, which is not good for sharing and accessing, since the address of the
global data ops is changed in secondary process.

As you can see in " e1000_setup_init_funcs", they not only set function pointers,
but also call them, not sure this will break other things or not. ;-)

> Thanks for your reply