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

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

[Tengfei Zhang]

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

Signed-off-by: Tengfei Zhang <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

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

[Wang, Haiyue]

> -----Original Message-----
> From: Tengfei Zhang <zypscode@outlook.com>
> Sent: Saturday, June 19, 2021 01:27
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; Tengfei Zhang <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 <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

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

[张 杨]

发件人: Wang, Haiyue <haiyue.wang@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 <zypscode@outlook.com>
> Sent: Saturday, June 19, 2021 01:27
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; Tengfei Zhang <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 <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.

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

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

[Wang, Haiyue]

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

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

[Tengfei Zhang]

On 2021/6/22 上午10:16, Wang, Haiyue wrote:
> 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
>
>
I think "get" operation should not crash  in  any  process.

and on different  nics ,  this operation  should  be consistent.


Yes , "e1000_setup_init_funcs" not only set function pointers, this 
patch is not good enough,  just happens to  work.

I look forward to   a better solution