RE: [PATCH] i40e: don't enable and init FCOE by default when do PF reset

["Dev, Vasu" <vasu.dev@intel.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 6425 bytes --]

> -----Original Message-----
> >> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> >> > b/drivers/net/ethernet/intel/i40e/i40e_main.c
> >> > index a5f2660..a2572cc 100644
> >> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> >> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> >> > @@ -6180,9 +6180,12 @@ static void i40e_reset_and_rebuild(struct
> >> > i40e_pf *pf, bool reinit)
> >> >     }
> >> >  #endif /* CONFIG_I40E_DCB */
> >> >  #ifdef I40E_FCOE
> >> > -   ret = i40e_init_pf_fcoe(pf);
> >> > -   if (ret)
> >> > -           dev_info(&pf->pdev->dev, "init_pf_fcoe failed: %d\n", ret);
> >> > +   if (pf->flags & I40E_FLAG_FCOE_ENABLED) {
> >> > +           ret = i40e_init_pf_fcoe(pf);
> >
> > Calling i40e_init_pf_fcoe() here conflicts with its
> I40E_FLAG_FCOE_ENABLED pre-condition since I40E_FLAG_FCOE_ENABLED is
> set by very same i40e_init_pf_fcoe(), in turn i40e_init_pf_fcoe() will never get
> called.
> 
> I don't think so,  here ,i40e_reset_and_rebuild()  is not the only and the first
> place that  i40e_init_pf_fcoe() is called, see i40e_probe(), that is the first
> chance.
> 
> i40e_probe()
> -->i40e_sw_init()
>      -->i40e_init_pf_fcoe()
> 
> And the I40E_FLAG_FCOE_ENABLED is possible be set by
> i40e_fcoe_enable() or i40e_fcoe_disable() interface before the reset action is
> to be done.
> 

It is set by i40e_init_pf_fcoe() and you are right that the modified call flow by your patch won't impact setting of I40E_FLAG_FCOE_ENABLED anyway which could have prevented calling i40e_init_pf_fcoe() as I described above, so this is not an issue with the patch.

> BTW, the reason I post this patch is that we hit a bug, after setup vlan, the PF
> is enabled to FCOE.
> 

Then that BUG would still remain un-fixed and calling i40e_init_pf_fcoe() under I40E_FLAG_FCOE_ENABLED  flag really won't affect call flow to fix anything. I mean I40E_FLAG_FCOE_ENABLED  condition will be true with "pf->hw.func_caps.fcoe == true" and otherwise calling i40e_init_pf_fcoe() simply returns back early on after checking "pf->hw.func_caps.fcoe == false", so how that bug is fixed here by added I40E_FLAG_FCOE_ENABLED  condition ? What is the bug ?

> >
> > Jeff Kirsher should be getting out a patch queued by me which adds
> I40E_FCoE Kbuild option, in that FCoE is disabled by default and  user could
> enable FCoE only if needed, that patch would do same of skipping
> i40e_init_pf_fcoe() whether FCoE capability in device enabled or not in
> default config.
> >
> 
> The following patch will not fix the above issue -- configuration of PF will be
> changed via reset.
> How about the FCOE is configured and disabled by  i40e_fcoe_disable() ,
> then reset happens ?
> 

May be but if the BUG is due to FCoE being enabled then having it disabled in config will avoid the bug for non FCoE config option and once bug is understood then that has to be fixed for FCoE enabled config also as I asked above.

Thanks Ethan for detailed response.
Vasu

> > From patchwork Wed Oct  2 23:26:08 2013
> > Content-Type: text/plain; charset="utf-8"
> > MIME-Version: 1.0
> > Content-Transfer-Encoding: 7bit
> > Subject: [net] i40e: adds FCoE configure option
> > Date: Thu, 03 Oct 2013 07:26:08 -0000
> > From: Vasu Dev <vasu.dev@intel.com>
> > X-Patchwork-Id: 11797
> >
> > Adds FCoE config option I40E_FCOE, so that FCoE can be enabled as
> > needed but otherwise have it disabled by default.
> >
> > This also eliminate multiple FCoE config checks, instead now just one
> > config check for CONFIG_I40E_FCOE.
> >
> > The I40E FCoE was added with 3.17 kernel and therefore this patch
> > shall be applied to stable 3.17 kernel also.
> >
> > CC: <stable@vger.kernel.org>
> > Signed-off-by: Vasu Dev <vasu.dev@intel.com>
> > Tested-by: Jim Young <jamesx.m.young@intel.com>
> >
> > ---
> > drivers/net/ethernet/intel/Kconfig           |   11 +++++++++++
> >  drivers/net/ethernet/intel/i40e/Makefile     |    2 +-
> >  drivers/net/ethernet/intel/i40e/i40e_osdep.h |    4 ++--
> >  3 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/Kconfig
> > b/drivers/net/ethernet/intel/Kconfig
> > index 5b8300a..4d61ef5 100644
> > --- a/drivers/net/ethernet/intel/Kconfig
> > +++ b/drivers/net/ethernet/intel/Kconfig
> > @@ -281,6 +281,17 @@ config I40E_DCB
> >
> >           If unsure, say N.
> >
> > +config I40E_FCOE
> > +       bool "Fibre Channel over Ethernet (FCoE)"
> > +       default n
> > +       depends on I40E && DCB && FCOE
> > +       ---help---
> > +         Say Y here if you want to use Fibre Channel over Ethernet (FCoE)
> > +         in the driver. This will create new netdev for exclusive FCoE
> > +         use with XL710 FCoE offloads enabled.
> > +
> > +         If unsure, say N.
> > +
> >  config I40EVF
> >         tristate "Intel(R) XL710 X710 Virtual Function Ethernet support"
> >         depends on PCI_MSI
> > diff --git a/drivers/net/ethernet/intel/i40e/Makefile
> > b/drivers/net/ethernet/intel/i40e/Makefile
> > index 4b94ddb..c405819 100644
> > --- a/drivers/net/ethernet/intel/i40e/Makefile
> > +++ b/drivers/net/ethernet/intel/i40e/Makefile
> > @@ -44,4 +44,4 @@ i40e-objs := i40e_main.o \
> >         i40e_virtchnl_pf.o
> >
> >  i40e-$(CONFIG_I40E_DCB) += i40e_dcb.o i40e_dcb_nl.o
> > -i40e-$(CONFIG_FCOE:m=y) += i40e_fcoe.o
> > +i40e-$(CONFIG_I40E_FCOE) += i40e_fcoe.o
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_osdep.h
> > b/drivers/net/ethernet/intel/i40e/i40e_osdep.h
> > index 045b5c4..ad802dd 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_osdep.h
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_osdep.h
> > @@ -78,7 +78,7 @@ do {                                                            \
> >  } while (0)
> >
> >  typedef enum i40e_status_code i40e_status; -#if defined(CONFIG_FCOE)
> > || defined(CONFIG_FCOE_MODULE)
> > +#ifdef CONFIG_I40E_FCOE
> >  #define I40E_FCOE
> > -#endif /* CONFIG_FCOE or CONFIG_FCOE_MODULE */
> > +#endif
> >  #endif /* _I40E_OSDEP_H_ */
> >
> >> > +           if (ret)
> >> > +                   dev_info(&pf->pdev->dev,
> >> > +                            "init_pf_fcoe failed: %d\n", ret);
> >> > +   }
> >> >
> >> >  #endif
> >> >     /* do basic switch setup */
> >> > --
> >> > 1.8.3.1
> >
> 
> Thanks,
> Ethan
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥