From: 张 杨 <zypsc...@outlook.com> Sent: Monday, June 21, 2021 16:35 To: Wang, Haiyue <haiyue.w...@intel.com> Cc: dev@dpdk.org; Zhang, Qi Z <qi.z.zh...@intel.com>; Lin, Xueqin <xueqin....@intel.com> Subject: Re: [PATCH] net/e1000: fix nic ops function was no initialized in secondary process
发件人: Wang, Haiyue <mailto:haiyue.w...@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:zypsc...@outlook.com> > Sent: Saturday, June 19, 2021 01:27 > To: Wang, Haiyue <mailto:haiyue.w...@intel.com> > Cc: mailto:dev@dpdk.org; Tengfei Zhang <mailto: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 <mailto: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)ð_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 = ð_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 = ð_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
