[dpdk-dev] [PATCH v1] net/i40e: avoid invalid operations after reset

[dpdk-dev] [PATCH v1] net/i40e: avoid invalid operations after reset

[Zhirun Yan]

if reset but not reinit adminq, some operations in i40evf_dev_close()
like i40evf_dev_promiscuous_disable() and
i40evf_dev_allmulticast_disable() will result in failures.

Fixes: cae18d2b0fb4 ("net/i40e: add workaround promiscuous disable")

Signed-off-by: Zhirun Yan <zhirun.yan@intel.com>
Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
---
 drivers/net/i40e/i40e_ethdev_vf.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev_vf.c b/drivers/net/i40e/i40e_ethdev_vf.c
index ae55b9b18..a1f4a729f 100644
--- a/drivers/net/i40e/i40e_ethdev_vf.c
+++ b/drivers/net/i40e/i40e_ethdev_vf.c
@@ -2245,6 +2245,7 @@ static void
 i40evf_dev_close(struct rte_eth_dev *dev)
 {
 	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct i40e_vf *vf = I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
 
 	i40evf_dev_stop(dev);
 	i40e_dev_free_queues(dev);
@@ -2253,10 +2254,12 @@ i40evf_dev_close(struct rte_eth_dev *dev)
 	 * it is a workaround solution when work with kernel driver
 	 * and it is not the normal way
 	 */
-	i40evf_dev_promiscuous_disable(dev);
-	i40evf_dev_allmulticast_disable(dev);
+	if (!vf->vf_reset) {
+		i40evf_dev_promiscuous_disable(dev);
+		i40evf_dev_allmulticast_disable(dev);
+		i40evf_reset_vf(hw);
+	}
 
-	i40evf_reset_vf(hw);
 	i40e_shutdown_adminq(hw);
 	i40evf_disable_irq0(hw);
 	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
-- 
2.17.1

Re: [dpdk-dev] [PATCH v1] net/i40e: avoid invalid operations after reset

[Zhang, Qi Z]

> -----Original Message-----
> From: Yan, Zhirun
> Sent: Tuesday, December 11, 2018 12:05 AM
> To: dev@dpdk.org; Wang, Haiyue <haiyue.wang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: Yan, Zhirun <zhirun.yan@intel.com>
> Subject: [PATCH v1] net/i40e: avoid invalid operations after reset
> 
> if reset but not reinit adminq, some operations in i40evf_dev_close() like
> i40evf_dev_promiscuous_disable() and
> i40evf_dev_allmulticast_disable() will result in failures.
> 
> Fixes: cae18d2b0fb4 ("net/i40e: add workaround promiscuous disable")
> 
> Signed-off-by: Zhirun Yan <zhirun.yan@intel.com>
> Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> ---
>  drivers/net/i40e/i40e_ethdev_vf.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> b/drivers/net/i40e/i40e_ethdev_vf.c
> index ae55b9b18..a1f4a729f 100644
> --- a/drivers/net/i40e/i40e_ethdev_vf.c
> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> @@ -2245,6 +2245,7 @@ static void
>  i40evf_dev_close(struct rte_eth_dev *dev)  {
>  	struct i40e_hw *hw =
> I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct i40e_vf *vf =
> I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> 
>  	i40evf_dev_stop(dev);
>  	i40e_dev_free_queues(dev);
> @@ -2253,10 +2254,12 @@ i40evf_dev_close(struct rte_eth_dev *dev)
>  	 * it is a workaround solution when work with kernel driver
>  	 * and it is not the normal way
>  	 */
> -	i40evf_dev_promiscuous_disable(dev);
> -	i40evf_dev_allmulticast_disable(dev);
> +	if (!vf->vf_reset) {

I didn't see when vf_reset will be cleared to false? Is it another bug?


> +		i40evf_dev_promiscuous_disable(dev);
> +		i40evf_dev_allmulticast_disable(dev);
> +		i40evf_reset_vf(hw);
> +	}

> -	i40evf_reset_vf(hw);
>  	i40e_shutdown_adminq(hw);
>  	i40evf_disable_irq0(hw);
>  	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
> --
> 2.17.1

Re: [dpdk-dev] [PATCH v1] net/i40e: avoid invalid operations after reset

[Yan, Zhirun]

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, December 10, 2018 9:13 PM
> To: Yan, Zhirun <zhirun.yan@intel.com>; dev@dpdk.org; Wang, Haiyue
> <haiyue.wang@intel.com>
> Subject: RE: [PATCH v1] net/i40e: avoid invalid operations after reset
> 
> 
> 
> > -----Original Message-----
> > From: Yan, Zhirun
> > Sent: Tuesday, December 11, 2018 12:05 AM
> > To: dev@dpdk.org; Wang, Haiyue <haiyue.wang@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>
> > Cc: Yan, Zhirun <zhirun.yan@intel.com>
> > Subject: [PATCH v1] net/i40e: avoid invalid operations after reset
> >
> > if reset but not reinit adminq, some operations in i40evf_dev_close()
> > like
> > i40evf_dev_promiscuous_disable() and
> > i40evf_dev_allmulticast_disable() will result in failures.
> >
> > Fixes: cae18d2b0fb4 ("net/i40e: add workaround promiscuous disable")
> >
> > Signed-off-by: Zhirun Yan <zhirun.yan@intel.com>
> > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> > ---
> >  drivers/net/i40e/i40e_ethdev_vf.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > b/drivers/net/i40e/i40e_ethdev_vf.c
> > index ae55b9b18..a1f4a729f 100644
> > --- a/drivers/net/i40e/i40e_ethdev_vf.c
> > +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> > @@ -2245,6 +2245,7 @@ static void
> >  i40evf_dev_close(struct rte_eth_dev *dev)  {
> >  	struct i40e_hw *hw =
> > I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > +	struct i40e_vf *vf =
> > I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> >
> >  	i40evf_dev_stop(dev);
> >  	i40e_dev_free_queues(dev);
> > @@ -2253,10 +2254,12 @@ i40evf_dev_close(struct rte_eth_dev *dev)
> >  	 * it is a workaround solution when work with kernel driver
> >  	 * and it is not the normal way
> >  	 */
> > -	i40evf_dev_promiscuous_disable(dev);
> > -	i40evf_dev_allmulticast_disable(dev);
> > +	if (!vf->vf_reset) {
> 
> I didn't see when vf_reset will be cleared to false? Is it another bug?
> 
After reset VF, vf->vf_reset will be cleared to false after calling i40evf_request_queues().
This patch is used to fix request_queues.

> 
> > +		i40evf_dev_promiscuous_disable(dev);
> > +		i40evf_dev_allmulticast_disable(dev);
> > +		i40evf_reset_vf(hw);
> > +	}
> 
> > -	i40evf_reset_vf(hw);
> >  	i40e_shutdown_adminq(hw);
> >  	i40evf_disable_irq0(hw);
> >  	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
> > --
> > 2.17.1

Re: [dpdk-dev] [PATCH v1] net/i40e: avoid invalid operations after reset

[Zhang, Qi Z]

> -----Original Message-----
> From: Yan, Zhirun
> Sent: Tuesday, December 11, 2018 1:32 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org; Wang, Haiyue
> <haiyue.wang@intel.com>
> Subject: RE: [PATCH v1] net/i40e: avoid invalid operations after reset
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Monday, December 10, 2018 9:13 PM
> > To: Yan, Zhirun <zhirun.yan@intel.com>; dev@dpdk.org; Wang, Haiyue
> > <haiyue.wang@intel.com>
> > Subject: RE: [PATCH v1] net/i40e: avoid invalid operations after reset
> >
> >
> >
> > > -----Original Message-----
> > > From: Yan, Zhirun
> > > Sent: Tuesday, December 11, 2018 12:05 AM
> > > To: dev@dpdk.org; Wang, Haiyue <haiyue.wang@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>
> > > Cc: Yan, Zhirun <zhirun.yan@intel.com>
> > > Subject: [PATCH v1] net/i40e: avoid invalid operations after reset
> > >
> > > if reset but not reinit adminq, some operations in
> > > i40evf_dev_close() like
> > > i40evf_dev_promiscuous_disable() and
> > > i40evf_dev_allmulticast_disable() will result in failures.
> > >
> > > Fixes: cae18d2b0fb4 ("net/i40e: add workaround promiscuous disable")
> > >
> > > Signed-off-by: Zhirun Yan <zhirun.yan@intel.com>
> > > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> > > ---
> > >  drivers/net/i40e/i40e_ethdev_vf.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> > > b/drivers/net/i40e/i40e_ethdev_vf.c
> > > index ae55b9b18..a1f4a729f 100644
> > > --- a/drivers/net/i40e/i40e_ethdev_vf.c
> > > +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> > > @@ -2245,6 +2245,7 @@ static void
> > >  i40evf_dev_close(struct rte_eth_dev *dev)  {
> > >  	struct i40e_hw *hw =
> > > I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > > +	struct i40e_vf *vf =
> > > I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
> > >
> > >  	i40evf_dev_stop(dev);
> > >  	i40e_dev_free_queues(dev);
> > > @@ -2253,10 +2254,12 @@ i40evf_dev_close(struct rte_eth_dev *dev)
> > >  	 * it is a workaround solution when work with kernel driver
> > >  	 * and it is not the normal way
> > >  	 */
> > > -	i40evf_dev_promiscuous_disable(dev);
> > > -	i40evf_dev_allmulticast_disable(dev);
> > > +	if (!vf->vf_reset) {
> >
> > I didn't see when vf_reset will be cleared to false? Is it another bug?
> >
> After reset VF, vf->vf_reset will be cleared to false after calling
> i40evf_request_queues().
> This patch is used to fix request_queues.

If the patch is going to fix another new patch, then it should be part of that :)

But my suggestion is we could keep this patch as independent, 
because strictly even without the request queue patch, the issue is still valid, since there will be other cases that pf will reset VF.

But the problem here is your fix rely on that vf_reset flag should always be updated correctly, but actually it does not, it is not cleared always, that means once vf reset happened, 
in following dev_close, you will always skip those adminq command.

>From my view, there should be a fix to clear the vf_reset and vf->pend_msg, and i40evf_check_vf_reset_done looks like the right place to do this. 
ideally your "request queues" patch should not touch these flag

> 
> >
> > > +		i40evf_dev_promiscuous_disable(dev);
> > > +		i40evf_dev_allmulticast_disable(dev);
> > > +		i40evf_reset_vf(hw);
> > > +	}
> >
> > > -	i40evf_reset_vf(hw);
> > >  	i40e_shutdown_adminq(hw);
> > >  	i40evf_disable_irq0(hw);
> > >  	rte_eal_alarm_cancel(i40evf_dev_alarm_handler, dev);
> > > --
> > > 2.17.1