NAK: [pull request][Trusty] (upstream) bnx2x: Utilize FW 7.10.51

[Brad Figg]

On Wed, Mar 02, 2016 at 09:03:26AM -0500, Dan Streetman wrote:
> On Fri, Feb 26, 2016 at 3:47 PM, Brad Figg <brad.figg at canonical.com> wrote:
> > On Fri, Feb 26, 2016 at 02:26:20PM -0500, Dan Streetman wrote:
> >> BugLink: http://bugs.launchpad.net/bugs/1454286
> >>
> >> Please pull git://git.launchpad.net/~ddstreet/+git/linux lp1454286
> >>
> >> First, the problem; under some conditions, the chip hangs in firmware.
> >> We have had reports of this happening.  From the 7.10.51 fw changelog:
> >> "Chip may stall in very rare cases under heavy traffic with FW GRO
> >> enabled".  So to fix the problem, the driver must be updated to use
> >> the new firmware level.  The commit in lts-vivid to update to the new
> >> fw is e42780b, and:
> >>
> >> $ git log --no-merges --oneline ubuntu/trusty/master..e42780b
> >> drivers/net/ethernet/broadcom/bnx2x | wc -l
> >> 73
> >>
> >> however, some of those don't need to be included, specifically:
> >>
> >> $ ( git log --no-merges --oneline ubuntu/trusty/master..e42780b
> >> drivers/net/ethernet/broadcom/bnx2x ; git log --no-merges --oneline
> >> ubuntu/trusty/master..lp1454286 drivers/net/ethernet/broadcom/bnx2x )
> >> | sort -k 2 | uniq -f 1 -u
> >> 4e857c5 arch: Mass conversion of smp_mb__*()
> >> 0c0e634 bnx2x: Adapter not recovery from EEH error injection
> >> fe26566d bnx2x: fix crash during TSO tunneling
> >> 9aaae04 bnx2x: Fix kernel crash and data miscompare after EEH recovery
> >> dad91ee bnx2x: Fix link for KR with swapped polarity lane
> >> 07b0f00 bnx2x: fix possible panic under memory stress
> >> fe62d00 ethtool: Replace ethtool_ops::{get,set}_rxfh_indir() with
> >> {get,set}_rxfh()
> >> 99932d4 netdevice: add queue selection fallback handler for ndo_select_queue
> >> 7ad24ea net: get rid of SET_ETHTOOL_OPS
> >> ed61668 net-next:v4: Add support to configure SR-IOV VF minimum and
> >> maximum Tx rate through ip tool.
> >> 9baa3c3 PCI: Remove DEFINE_PCI_DEVICE_TABLE macro use
> >> c265947 Update setapp/getapp prototypes in dcbnl_rtnl_ops to return
> >> int instead of u8
> >>
> >> of those, all the bnx2x: ones are already included in trusty/master.
> >> The others are all cleanup/renaming commits, or commits that change
> >> structures or functions, but those changes aren't used by bnx2x and
> >> aren't required by any of the commits in the pull.
> >>
> >> So what does the pull change?  It *only* changes the bnx2x driver
> >> (except changing the bnx2x owner in MAINTAINERS, and changing a bnx2
> >> mod param mode from 0 to 444).
> >>
> >> $ git diff --stat ubuntu/trusty/master..lp1454286 MAINTAINERS
> >>                                    |    2 +-
> >>  drivers/net/ethernet/broadcom/bnx2.c                    |    2 +-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x.h             |   64 +-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c         |  386 +++---
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h         |  180 +--
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c         |   10 +-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.h         |    2 +-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_ethtool.c     |  142 ++-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_fw_defs.h     |  223 ++--
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_fw_file_hdr.h |    4 +-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_hsi.h         |  203 +++-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_init.h        |    4 +-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_init_ops.h    |    4 +-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.c        |   77 +-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_link.h        |   10 +-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c        |  676 ++++++++---
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_reg.h         |    1 +
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.c          |  498 +++-----
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_sp.h          |   73 +-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c       | 2263
> >> +++++++++++++----------------------
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.h       |  428 ++-----
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c       |    2 +-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.h       |    2 +-
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c        |  746 ++++++------
> >>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.h        |   49 +-
> >>  25 files changed, 2820 insertions(+), 3231 deletions(-)
> >>
> >>
> >> Why not just cherry-pick the single commit that updates the driver to
> >> use the new fw?
> >>
> >> I tried this, and on boot the driver fell into a loop failing to talk
> >> correctly to the hardware/fw.  So at least 1 additional commit is
> >> required for it to properly work with the new firmware.
> >>
> >>
> >> Why do we need to pull *all* the commits?  Why not cherry-pick the fw
> >> update commit, and then work backwards pulling only the commit(s)
> >> required for the driver to start working with the updated fw?
> >>
> >> In short, testing.
> >> I can spend the time to find the specific commits required to get the
> >> driver to boot with the new fw, and I can perform basic tests, but I
> >> don't have the hardware or the time to fully test the driver's
> >> functionality in all configurations.  However the chip vendor
> >> (broadcom/qlogic) *has* done testing with the driver when the
> >> fw-update commit was added upstream, but that testing was done with
> >> all the previous commits.  So leaving out commits is likely to be less
> >> tested, and more problematic, than just including all commits leading
> >> up to fw update commit.
> >>
> >> If this bug was in the kernel, I'd definitely agree that only the
> >> specific commit(s) that fix the bug should be backported.  However,
> >> since the bug is in the firmware, and we must update the driver to use
> >> the new firmware to fix the bug, I feel the driver should be fully
> >> updated to the level that was vendor-tested with the new fw level.
> >>
> >> Ok, let the fun begin...
> >>
> >> --
> >> kernel-team mailing list
> >> kernel-team at lists.ubuntu.com
> >> https://lists.ubuntu.com/mailman/listinfo/kernel-team
> >
> > There is no indication of positive testing in the bug. Without that I will
> > not accept this pull request.
> 
> I've tested it for basic functionality - boots, can connect to the
> interface, ran iperf and several flent tests between it and a second
> system - but as far as testing to fix the specific bug, I can't
> reproduce it and so can't test for that, and the original
> reporter/customer of the problem is now refusing to update their
> kernel at all, even just to test and verify the driver update fixes
> their problem.  So...unless they change their mind, or someone else
> with this problem tests, this pull request can die.
> 
> 
> >
> > --
> > Brad Figg brad.figg at canonical.com http://www.canonical.com

No testing.

-- 
Brad Figg brad.figg at canonical.com http://www.canonical.com