RE: [Intel-wired-lan] Issue with driver i40e stat strings count mismatch

[Wyborny, Carolyn]

> -Original Message-
> From: Intel-wired-lan [mailto:intel-wired-lan-boun...@osuosl.org] On
> Behalf Of Jesper Dangaard Brouer
> Sent: Tuesday, July 31, 2018 7:29 AM
> To: Stefan Assmann ; Keller, Jacob E
> 
> Cc: netdev@vger.kernel.org; intel-wired-lan  l...@lists.osuosl.org>; bro...@redhat.com; Topel, Bjorn
> 
> Subject: Re: [Intel-wired-lan] Issue with driver i40e stat strings count
> mismatch
> 
> 
> On Tue, 31 Jul 2018 09:05:40 +0200 Stefan Assmann
>  wrote:
> 
> > From: Stefan Assmann 
> > To: Jesper Dangaard Brouer ,  Jeff Kirsher
> 
> > Cc: Björn Töpel ,
> "alexander.h.du...@intel.com" ,  intel-
> wired-lan ,  "netdev@vger.kernel.org"
> 
> > Subject: Re: Issue with driver i40e stat strings count mismatch
> > Date: Tue, 31 Jul 2018 09:05:40 +0200
> > User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
> >  Thunderbird/52.9.1
> > Message-ID: <64be8e6a-c285-2864-fd91-356eba645...@kpanic.de>
> >
> > On 10.07.2018 13:17, Jesper Dangaard Brouer wrote:
> > > Hi Intel-fokes,
> > >
> > > Your i40e driver have issues with it's ethtool stats.  A warning
> > > triggers at drivers/net/ethernet/intel/i40e/i40e_ethtool.c line 1907
> > > (see splash below) in func i40e_get_stat_strings().
> >
> > Hi Jesper,
> >
> > I ran into the same issue. Here's my proposed fix.
> 
> Thanks for following up Stefan :-)
> 
> I'm hoping some Intel people will look at evaluating this fix? ...
> 
Thanks, we have a patch in process for this fix and other related ones from 
Jake Keller.  We'll expedite it.

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 

 

RE: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY

[Wyborny, Carolyn]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-
> ow...@vger.kernel.org] On Behalf Of Florian Fainelli
> Sent: Thursday, July 27, 2017 10:58 AM
> To: Andrew Lunn ; Brown, Aaron F
> 
> Cc: John W. Linville ; netdev@vger.kernel.org; intel-
> wired-...@lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY
> 
> On 07/27/2017 08:37 AM, Andrew Lunn wrote:
> > On Thu, Jul 27, 2017 at 12:40:01AM +, Brown, Aaron F wrote:
> >>> From: Intel-wired-lan [mailto:intel-wired-lan-boun...@osuosl.org] On
> Behalf
> >>> Of John W. Linville
> >>> Sent: Friday, July 21, 2017 11:12 AM
> >>> To: netdev@vger.kernel.org
> >>> Cc: intel-wired-...@lists.osuosl.org; John W. Linville
> >>> 
> >>> Subject: [Intel-wired-lan] [PATCH] igb: support BCM54616 PHY
> >>>
> >>> The management port on an Edgecore AS7712-32 switch uses an igb MAC,
> >>> but
> >>> it uses a BCM54616 PHY. Without a patch like this, loading the igb
> >>> module produces dmesg output like this:
> >>>
> >>> [3.439125] igb: Copyright (c) 2007-2014 Intel Corporation.
> >>> [3.439866] igb: probe of :00:14.0 failed with error -2
> >>>
> >>> Signed-off-by: John W. Linville 
> >>> Cc: Jeff Kirsher 
> >>> ---
> >>>  drivers/net/ethernet/intel/igb/e1000_82575.c   | 6 ++
> >>>  drivers/net/ethernet/intel/igb/e1000_defines.h | 1 +
> >>>  drivers/net/ethernet/intel/igb/e1000_hw.h  | 1 +
> >>>  3 files changed, 8 insertions(+)
> >>
> >> I do not have the specific hardware (Edgecore switch) but as far as
> regression tests go this works fine.
> >> Tested-by: Aaron Brown 
> >
> > Sorry, missed the initial post, so replying to a reply.
> >
> > Linux has supported the BCM54616 PHY since April 2015. If the Intel
> > drivers used the Linux PHY drivers, you would not of had this problem.
> >
> > It would be good if somebody spent the time to migrate these MAC
> > drivers to use the common Linux PHY infrastructure.
> 
> I suspect there is a design pattern within the Intel drivers to share as
> much low-level code as possible between OSes and only have some
> Linux-ism where necessary (e.g: net_device, ethtool etc.).
> 
> PHY code is a pain in general, especially if you are serious about
> testing interoperability (which is where you can spend tons of $$$ with
> little reward but just say: yes it works), so it may make sense to share
> it across different OSes.
> 
> I too, wish there was more sharing, but considering that this works for
> the Intel driver, there is little incentive in doing this I suppose...
> --
> Florian

Yes Florian you are correct generally, but future implementations have not been 
ruled out.  With this driver, we had our custom phy init code tested and 
released for several products before phylib existed.  I began research on a 
phylib implementation for it at one point, but internal product decisions ended 
up lowering the priority of that work.

Thanks,

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 

RE: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Wyborny, Carolyn]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-
> ow...@vger.kernel.org] On Behalf Of Casey Leedom
> Sent: Thursday, July 06, 2017 2:54 PM
> To: Jakub Kicinski 
> Cc: Dustin Byford ; Andrew Lunn
> ; Roopa Prabhu ;
> da...@davemloft.net; linvi...@tuxdriver.com; netdev@vger.kernel.org;
> vidya.chowd...@gmail.com; ol...@cumulusnetworks.com; Manoj Malviya
> ; Santosh Rastapur ;
> yuval.mi...@qlogic.com; od...@mellanox.com; ari...@mellanox.com;
> g...@mellanox.com; Kirsher, Jeffrey T 
> Subject: Re: [PATCH net-next 1/3] net: ethtool: add support for forward error
> correction modes
> 
> | From: Jakub Kicinski 
> | Sent: Thursday, July 6, 2017 12:02 PM
> |
> | IMHO if something gets replugged all the settings should be reset.
> | I feel that it's not entirely unlike replugging a USB adapter.  Perhaps
> | we should introduce some (devlink) notifications for SFP module events
> | so userspace can apply whatever static user config it has?
> 
> Absolutely a valid approach.  As are all of the ones I outlined.
> 
[..]
> As I noted in my previous letter: this is something new that we've never
> faced before with any prior networking technology.  All previous networking
> technologies had a static set of Physical Port Capabilities fixed from the
> moment a Host Diver/Firmware first see a Port.  We're now facing a situation
> where these can change dynamically from moment to moment based on what
> Transceiver Module is inserted.

Agree with your points generally, but other networking hw vendors have dealt 
with this since SFP variant and other modules arrived on the scene.  

RE: [PATCH net-next 1/3] net: ethtool: add support for forward error correction modes

[Wyborny, Carolyn]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-
> ow...@vger.kernel.org] On Behalf Of Jakub Kicinski
> Sent: Thursday, July 06, 2017 12:02 PM
> To: Casey Leedom 
> Cc: Dustin Byford ; Andrew Lunn
> ; Roopa Prabhu ;
> da...@davemloft.net; linvi...@tuxdriver.com; netdev@vger.kernel.org;
> vidya.chowd...@gmail.com; ol...@cumulusnetworks.com; Manoj Malviya
> ; Santosh Rastapur ;
> yuval.mi...@qlogic.com; od...@mellanox.com; ari...@mellanox.com;
> g...@mellanox.com; Kirsher, Jeffrey T 
> Subject: Re: [PATCH net-next 1/3] net: ethtool: add support for forward error
> correction modes
> 
[..]
> > In our discussions of the above, we decided that we should err in the
> > direction of the absolutely simplest abstraction model, even when
> > that might result in a failure to establish a link.  Our feeling was
> > that doing anything else would result in endless user confusion.
> > Basically, if it takes longer than a simple paragraph to explain,
> > you're probably doing the wrong thing.  So, we decided that if a user
> > sets up a particular FEC setting, we keep that regardless of conflict
> > with different Transceiver Modules.  (But flag such issues in the
> > System Log in order to try to give the user a chance to understand
> > what the new cable they plugged in wasn't working.)
> 
> IMHO if something gets replugged all the settings should be reset.
> I feel that it's not entirely unlike replugging a USB adapter.  Perhaps
> we should introduce some (devlink) notifications for SFP module events
> so userspace can apply whatever static user config it has?

This is an interesting dichotomy and we've been trying to resolve it as well as 
the module variations and permutations grow.  I agree with Casey that this 
bleeds into link speeds as well.  

I can see both perspectives:  try to apply to user settings even if they do 
something dumb and notify if things go bad; and, swapping modules should reset. 
 Notifications of some kind would help the devices manage this.  We tend to go 
with timers today.

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 

RE: [Intel-wired-lan] [PATCH 1/2] i40e/i40evf: rename vf_offload_flags to vf_cap_flags in struct virtchnl_vf_resource

[Wyborny, Carolyn]

> -Original Message-
> From: Greg Rose [mailto:gvrose8...@gmail.com]
> Sent: Thursday, July 06, 2017 7:25 AM
> To: Wyborny, Carolyn <carolyn.wybo...@intel.com>
> Cc: Stefan Assmann <sassm...@kpanic.de>; intel-wired-...@lists.osuosl.org;
> netdev@vger.kernel.org; da...@davemloft.net
> Subject: Re: [Intel-wired-lan] [PATCH 1/2] i40e/i40evf: rename
> vf_offload_flags to vf_cap_flags in struct virtchnl_vf_resource
> 
> On Wed, Jul 5, 2017 at 10:15 PM, Wyborny, Carolyn
> <carolyn.wybo...@intel.com> wrote:
> >
> > > -Original Message-
> > > From: Intel-wired-lan [mailto:intel-wired-lan-boun...@osuosl.org] On
> Behalf
> > > Of Stefan Assmann
> > > Sent: Thursday, June 29, 2017 6:12 AM
> > > To: intel-wired-...@lists.osuosl.org
> > > Cc: netdev@vger.kernel.org; da...@davemloft.net;
> sassm...@kpanic.de
> > > Subject: [Intel-wired-lan] [PATCH 1/2] i40e/i40evf: rename
> vf_offload_flags to
> > > vf_cap_flags in struct virtchnl_vf_resource
> > >
> > > The current name of vf_offload_flags indicates that the bitmap is
> > > limited to offload related features. Make this more generic by renaming
> > > it to vf_cap_flags, which allows for other capabilities besides
> > > offloading to be added.
> > >
> > > Signed-off-by: Stefan Assmann <sassm...@kpanic.de>
> > > ---
> >
> > Hello Stefan,
> >
> > Thanks for the patch series, but we believe the vf should be ignorant of its
> trusted status.
> 
> Hi Carolyn,
> 
> Might I ask why?
> 
> Thanks,
> 
> - Greg

Hello Greg,

The trusted status of the vf is something that pf manages, mostly for security 
reasons.  The mailbox model of communication between them relies on the pf to 
have authority over the vf.  In most things, the vf asks permission from the pf 
for changes in its configuration and this keeps the pf as the gatekeeper 
between trusted and regular vf's.  However, the issue here is that changing the 
MAC address from within the vf does not go through the mailbox, so it cannot 
tell if the vf is trusted or not.   We are working on a redesign of this 
feature to fix that instead of having the vf sort of know its trusted.  If, for 
some reason, we are unable to redesign it that way we would reconsider this 
method instead.

Thanks,

Carolyn

RE: [Intel-wired-lan] [PATCH 1/2] i40e/i40evf: rename vf_offload_flags to vf_cap_flags in struct virtchnl_vf_resource

[Wyborny, Carolyn]

> -Original Message-
> From: Intel-wired-lan [mailto:intel-wired-lan-boun...@osuosl.org] On Behalf
> Of Stefan Assmann
> Sent: Thursday, June 29, 2017 6:12 AM
> To: intel-wired-...@lists.osuosl.org
> Cc: netdev@vger.kernel.org; da...@davemloft.net; sassm...@kpanic.de
> Subject: [Intel-wired-lan] [PATCH 1/2] i40e/i40evf: rename vf_offload_flags to
> vf_cap_flags in struct virtchnl_vf_resource
> 
> The current name of vf_offload_flags indicates that the bitmap is
> limited to offload related features. Make this more generic by renaming
> it to vf_cap_flags, which allows for other capabilities besides
> offloading to be added.
> 
> Signed-off-by: Stefan Assmann 
> ---

Hello Stefan,

Thanks for the patch series, but we believe the vf should be ignorant of its 
trusted status.  That being said, you've definitely pointed out a bug in our 
implementation so that if a vf ends up being configured as trusted, its unable 
to change its mac address itself when this is an allowed action for this 
configuration.  The pf can change the mac address for the vf, but the vf itself 
cannot, which is not correct.  We are working on an alternate solution that 
keeps the vf from knowing its trusted status, but its not a simple fix.

Thanks,

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 

  

RE: [net-next 05/11] i40e: fix for queue timing delays

[Wyborny, Carolyn]

> -Original Message-
> From: David Miller [mailto:da...@davemloft.net]
> Sent: Saturday, March 25, 2017 8:01 PM
> To: Kirsher, Jeffrey T <jeffrey.t.kirs...@intel.com>
> Cc: Wyborny, Carolyn <carolyn.wybo...@intel.com>;
> netdev@vger.kernel.org; nhor...@redhat.com; sassm...@redhat.com;
> jogre...@redhat.com
> Subject: Re: [net-next 05/11] i40e: fix for queue timing delays
> 
> From: Jeff Kirsher <jeffrey.t.kirs...@intel.com>
> Date: Sat, 25 Mar 2017 01:12:59 -0700
> 
> > From: Carolyn Wyborny <carolyn.wybo...@intel.com>
> >
> > This patch adds a delay to Rx queue disables to accommodate HW needs.
> >
[..]
> > +   /* HW needs up to 50ms to finish RX queue disable*/
> > +   mdelay(50);
> 
> You just polled a register which indicates that the request is finished.
> Needing futher time for the operation to complete doesn't make any
> sense.
> 
> If this is really needed, you need to explain in the commit message, in
> detail, what is really the problem here.
> 
> THanks.

It's a hw limitation, will submit an updated patch with more detail.

Thanks,

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 

RE: [Intel-wired-lan] [PATCH] net: intel: i40evf: use new api ethtool_{get|set}_link_ksettings

[Wyborny, Carolyn]

> -Original Message-
> From: Intel-wired-lan [mailto:intel-wired-lan-boun...@lists.osuosl.org] On
> Behalf Of Philippe Reynes
> Sent: Saturday, February 04, 2017 2:49 PM
> To: Kirsher, Jeffrey T ; da...@davemloft.net
> Cc: netdev@vger.kernel.org; intel-wired-...@lists.osuosl.org; linux-
> ker...@vger.kernel.org; Philippe Reynes 
> Subject: [Intel-wired-lan] [PATCH] net: intel: i40evf: use new api
> ethtool_{get|set}_link_ksettings
> 
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
> 
Thanks for the patch Phillippe,

We have an internal patch in process for this functionality.  It should be out 
very soon.  I thought it was upstream already but it must not be.  I'll find it 
and get it expedited.

Thanks again,

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation