diff mbox series

net/e1000: fix nic ops function was no initialized in secondary process

Message ID ME3P282MB16687E51A5B5DDCD990B867FD10D9@ME3P282MB1668.AUSP282.PROD.OUTLOOK.COM (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series net/e1000: fix nic ops function was no initialized in secondary process |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot fail github build: failed
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-mellanox-Functional fail Functional Testing issues
ci/iol-intel-Performance success Performance Testing PASS

Commit Message

Tengfei Zhang June 18, 2021, 5:26 p.m. UTC 
  '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(+)

Comments

Wang, Haiyue June 21, 2021, 7:31 a.m. UTC | #1 
> -----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
Tengfei Zhang June 21, 2021, 8:34 a.m. UTC | #2 
发件人: 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
Wang, Haiyue June 22, 2021, 2:16 a.m. UTC | #3 
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
Tengfei Zhang June 22, 2021, 1:56 p.m. UTC | #4 
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
diff mbox series

Patch

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;