* Re: [dpdk-dev] [PATCH v1] net/i40e: avoid invalid operations after reset
2018-12-10 16:04 [dpdk-dev] [PATCH v1] net/i40e: avoid invalid operations after reset Zhirun Yan
@ 2018-12-10 13:12 ` Zhang, Qi Z
2018-12-11 5:31 ` Yan, Zhirun
0 siblings, 1 reply; 4+ messages in thread
From: Zhang, Qi Z @ 2018-12-10 13:12 UTC (permalink / raw)
To: Yan, Zhirun, dev, Wang, Haiyue
> -----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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [dpdk-dev] [PATCH v1] net/i40e: avoid invalid operations after reset
@ 2018-12-10 16:04 Zhirun Yan
2018-12-10 13:12 ` Zhang, Qi Z
0 siblings, 1 reply; 4+ messages in thread
From: Zhirun Yan @ 2018-12-10 16:04 UTC (permalink / raw)
To: dev, haiyue.wang, qi.z.zhang; +Cc: 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-dev] [PATCH v1] net/i40e: avoid invalid operations after reset
2018-12-10 13:12 ` Zhang, Qi Z
@ 2018-12-11 5:31 ` Yan, Zhirun
2018-12-11 6:58 ` Zhang, Qi Z
0 siblings, 1 reply; 4+ messages in thread
From: Yan, Zhirun @ 2018-12-11 5:31 UTC (permalink / raw)
To: Zhang, Qi Z, dev, Wang, Haiyue
> -----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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-dev] [PATCH v1] net/i40e: avoid invalid operations after reset
2018-12-11 5:31 ` Yan, Zhirun
@ 2018-12-11 6:58 ` Zhang, Qi Z
0 siblings, 0 replies; 4+ messages in thread
From: Zhang, Qi Z @ 2018-12-11 6:58 UTC (permalink / raw)
To: Yan, Zhirun, dev, Wang, Haiyue
> -----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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-12-11 6:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-10 16:04 [dpdk-dev] [PATCH v1] net/i40e: avoid invalid operations after reset Zhirun Yan
2018-12-10 13:12 ` Zhang, Qi Z
2018-12-11 5:31 ` Yan, Zhirun
2018-12-11 6:58 ` Zhang, Qi Z
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).