> -----Original Message----- > From: ethan zhao [mailto:[email protected]] > Sent: Monday, January 19, 2015 6:06 PM > To: Dev, Vasu > Cc: Kirsher, Jeffrey T; Ethan Zhao; Ronciak, John; Brandeburg, Jesse; Allan, > Bruce W; Wyborny, Carolyn; Skidmore, Donald C; Rose, Gregory V; Vick, > Matthew; Williams, Mitch A; Parikh, Neerav; Linux NICS; e1000- > [email protected]; [email protected]; linux- > [email protected]; [email protected] > Subject: Re: [PATCH] i40e: don't enable and init FCOE by default when do PF > reset > > > On 2015/1/20 5:10, Dev, Vasu wrote: > >> -----Original Message----- > >> From: ethan zhao [mailto:[email protected]] > >> Sent: Friday, January 16, 2015 7:01 PM > >> To: Kirsher, Jeffrey T > >> Cc: Dev, Vasu; Ethan Zhao; Ronciak, John; Brandeburg, Jesse; Allan, > >> Bruce W; Wyborny, Carolyn; Skidmore, Donald C; Rose, Gregory V; Vick, > >> Matthew; Williams, Mitch A; Parikh, Neerav; Linux NICS; e1000- > >> [email protected]; [email protected]; linux- > >> [email protected]; [email protected] > >> Subject: Re: [PATCH] i40e: don't enable and init FCOE by default when > >> do PF reset > >> > >> Vasu, > >> > >> What' your idea about the v2, any suggestion ? Jeff is looking > >> forward to see it. > >> > > Jeff was asking for v2 in response to your last comment as "disable FCOE as > default configuration as a temporary step" but I think that is the fix and > user > should n't enable FCoE until they have FCoE enabled X710 FCoE with either > fabric or VN2VN mode FCoE setup. > As a Linux distro, we don't know users have FCoE capable X710 or not, so > we couldn't disable the FCoE configuration > by default in the released kernel except FCoE is officially not supported > yet > with X710, but if yes, fix those bugs I > mentioned is the only choice. >
Yes must be fixed but I don't see your VLAN issue in my XL710 setup having engineering FW 4.33 with FCoE enabled, I could create VLANs and bring them up or down. As for the patch under discussion, it won't help any way whether fcoe enabled or not as I explained before. So I guess my setup differs in XL710 FW version, so upgrading FW should fix the issue and then you could keep FCoE configuration enabled in your distro w/o any concern to XL710 FCoE capable or not in XL710. We can take any FW related further discussion off list. Thanks, Vasu > > Thanks, > Ethan > > > > > > Thanks, > > Vasu > > > >> Thanks, > >> Ethan > >> > >> > >> On 2015/1/16 22:47, Jeff Kirsher wrote: > >>> On Fri, 2015-01-16 at 09:48 +0800, ethan zhao wrote: > >>>> Vasu, > >>>> > >>>> OK, disable FCOE as default configuration as a temporary step > >>>> to make it work. > >>> Sounds like I should expect a v2 coming, correct? > >>> > >>>> Thanks, > >>>> Ethan > >>>> > >>>> On 2015/1/16 7:45, Dev, Vasu wrote: > >>>>>> -----Original Message----- > >>>>>> From: ethan zhao [mailto:[email protected]] > >>>>>> Sent: Tuesday, January 13, 2015 6:41 PM > >>>>>> To: Dev, Vasu > >>>>>> Cc: Ethan Zhao; Ronciak, John; Kirsher, Jeffrey T; Brandeburg, > >>>>>> Jesse; Allan, Bruce W; Wyborny, Carolyn; Skidmore, Donald C; > >>>>>> Rose, Gregory V; Vick, Matthew; Williams, Mitch A; Parikh, > >>>>>> Neerav; Linux NICS; e1000- [email protected]; > >>>>>> [email protected]; > >>>>>> linux- [email protected]; [email protected] > >>>>>> Subject: Re: [PATCH] i40e: don't enable and init FCOE by default > >>>>>> when do PF reset > >>>>>> > >>>>>> Vasu, > >>>>>> > >>>>>> On 2015/1/14 3:38, Dev, Vasu wrote: > >>>>>>>> -----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 ? > >>>>>> The func_caps.fcoe is assigned by following call path, under > >>>>>> our test environment, > >>>>>> > >>>>>> i40e_probe() > >>>>>> ->i40e_get_capabilities() > >>>>>> ->i40e_aq_discover_capabilities() > >>>>>> ->i40e_parse_discover_capabilities() > >>>>>> > >>>>>> Or > >>>>>> > >>>>>> i40e_reset_and_rebuild() > >>>>>> ->i40e_get_capabilities() > >>>>>> ->i40e_aq_discover_capabilities() > >>>>>> ->i40e_parse_discover_capabilities() > >>>>>> > >>>>>> Under our test environment, the "pf->hw.func_caps.fcoe" is > >>>>>> true. so if > >>>>>> i40e_reset_and_rebuild() is called for VLAN setup, ethtool > >>>>>> diagnostic > >> test. > >>>>>> And then i40e_init_pf_fcoe() is to be called, > >>>>>> > >>>>>> While if (!pf->hw.func_caps.fcoe) wouldn't return, > >>>>>> > >>>>> I said it would return with "pf->hw.func_caps.fcoe == false" in my > >>>>> last > >> response, more details below. > >>>>>> So pf->flags is set to I40E_FLAG_FCOE_ENABLED. > >>>>>> > >>>>>> With my patch, i40e_init_pf_fcoe() is only called after > >>>>>> I40E_FLAG_FCOE_ENABLED is set before reset. > >>>>>> > >>>>>> Enable FCOE in i40e_probe() or not is another issue. > >>>>>> > >>>>> Nope since both cases we should do i40e_init_pf_fcoe() or don't > >>>>> based > >> on fcoe cap true or false. > >>>>> I don't have much to add as I described before with the your patch > >>>>> that > >> "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". > >>>>> May be I'm missing something, I guess next either go with > >> CONFIG_I40E_FCOE disable as I suggested before and now it in upstream > >> kernel or we can have further off list discussion to fix the issue > >> you are trying to fix with the patch. > >>>>> Thanks, > >>>>> Vasu > >>>>> > >>>>>> Thanks, > >>>>>> Ethan > >>>>>> > >>>>>> > >>>>>>>>> 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 <[email protected]> > >>>>>>>>> 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: <[email protected]> > >>>>>>>>> Signed-off-by: Vasu Dev <[email protected]> > >>>>>>>>> Tested-by: Jim Young <[email protected]> > >>>>>>>>> > >>>>>>>>> --- > >>>>>>>>> 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
