[dpdk-dev] [PATCH v3 3/7] drivers/net/bnxt new driver for Broadcom bnxt
2016-04-21 11:00, Bruce Richardson: > On Wed, Apr 20, 2016 at 02:32:18PM -0700, Stephen Hurd wrote: > > > It's not for testing, more for code review and to help understand the code > > > [though > > > as you say, we do need to ensure that each commit doesn't actually break > > > the > > > build]. > > > Right now, the driver code goes in as a single commit - which makes it a > > > hard > > > enough task to review and see what is in there. One suggestion that > > > hopefully > > > wouldn't be too much work might be to split the code up into: basic device > > > init code, RX and TX functions, and then any additional features based on > > > top > > > of that [ideally one patch per added feature]. > > > > > > > The current driver doesn't have much beyond basic TX/RX, but we can give it > > a shot. "Too much work" is relative of course, but splitting it into > > self-contained easily understood chunks will absolutely be a lot of work. > > > > Adding Ajit who will also be working on this. Since he's coming up to > > speed on the driver code, this could be a good way for him to fully > > familiarize himself with it. > > > Sure, thanks. Any splitting you can do at all would be appreciated and help > with > reviews. I'd add that it helps newcomers to understand the driver by reading the git history. Thanks
[dpdk-dev] [PATCH v3 3/7] drivers/net/bnxt new driver for Broadcom bnxt
On Wed, Apr 20, 2016 at 02:32:18PM -0700, Stephen Hurd wrote: > > It's not for testing, more for code review and to help understand the code > > [though > > as you say, we do need to ensure that each commit doesn't actually break > > the > > build]. > > Right now, the driver code goes in as a single commit - which makes it a > > hard > > enough task to review and see what is in there. One suggestion that > > hopefully > > wouldn't be too much work might be to split the code up into: basic device > > init code, RX and TX functions, and then any additional features based on > > top > > of that [ideally one patch per added feature]. > > > > The current driver doesn't have much beyond basic TX/RX, but we can give it > a shot. "Too much work" is relative of course, but splitting it into > self-contained easily understood chunks will absolutely be a lot of work. > > Adding Ajit who will also be working on this. Since he's coming up to > speed on the driver code, this could be a good way for him to fully > familiarize himself with it. > Sure, thanks. Any splitting you can do at all would be appreciated and help with reviews. /Bruce
[dpdk-dev] [PATCH v3 3/7] drivers/net/bnxt new driver for Broadcom bnxt
> It's not for testing, more for code review and to help understand the code > [though > as you say, we do need to ensure that each commit doesn't actually break > the > build]. > Right now, the driver code goes in as a single commit - which makes it a > hard > enough task to review and see what is in there. One suggestion that > hopefully > wouldn't be too much work might be to split the code up into: basic device > init code, RX and TX functions, and then any additional features based on > top > of that [ideally one patch per added feature]. > The current driver doesn't have much beyond basic TX/RX, but we can give it a shot. "Too much work" is relative of course, but splitting it into self-contained easily understood chunks will absolutely be a lot of work. Adding Ajit who will also be working on this. Since he's coming up to speed on the driver code, this could be a good way for him to fully familiarize himself with it. -- Stephen Hurd
[dpdk-dev] [PATCH v3 3/7] drivers/net/bnxt new driver for Broadcom bnxt
On Tue, Apr 19, 2016 at 01:51:32PM -0700, Stephen Hurd wrote: > On Tue, Apr 19, 2016 at 7:19 AM, Bruce Richardson < > bruce.richardson at intel.com> wrote: > > > On Fri, Mar 04, 2016 at 01:05:24PM -0800, Stephen Hurd wrote: > > > New driver for Broadcom bnxt (NexXtreme C-series) devices. > > > > This seems a single huge commit. Can this be split up into separate commits > > with self-contained changes in each one, e.g. a feature per commit, > > starting > > with basic init, then RX and TX etc. etc.? > > > > I suppose it could, I'm not sure what the value of that approach would be > though... it would just be a long process of copy/paste/commit/repeat. > Internally, it was developed as a port/rewrite of another driver, so we > don't actually have history without a large commit. > > Assuming each individual commit needs to be buildable, this will end up > burning a lot of time, and I doubt each separate commit could be reasonably > tested. > > It's not for testing, more for code review and to help understand the code [though as you say, we do need to ensure that each commit doesn't actually break the build]. Right now, the driver code goes in as a single commit - which makes it a hard enough task to review and see what is in there. One suggestion that hopefully wouldn't be too much work might be to split the code up into: basic device init code, RX and TX functions, and then any additional features based on top of that [ideally one patch per added feature]. Regards, /Bruce
[dpdk-dev] [PATCH v3 3/7] drivers/net/bnxt new driver for Broadcom bnxt
On Fri, Mar 04, 2016 at 01:05:24PM -0800, Stephen Hurd wrote: > New driver for Broadcom bnxt (NexXtreme C-series) devices. > > Standards-compliant 10/25/50G support with 30MPPS full-duplex throughput > http://www.broadcom.com/press/release.php?id=s923886 > > Signed-off-by: Stephen Hurd > --- > v3: > * Fix incorrect format specifier compilation error on i686 > (PRIx64 instead of lx for uint64_t) on line 1337 > > drivers/net/bnxt/Makefile | 79 ++ > drivers/net/bnxt/bnxt.h | 217 > drivers/net/bnxt/bnxt_cpr.c | 138 +++ > drivers/net/bnxt/bnxt_cpr.h | 117 ++ > drivers/net/bnxt/bnxt_ethdev.c| 1381 ++ > drivers/net/bnxt/bnxt_filter.c| 175 +++ > drivers/net/bnxt/bnxt_filter.h| 74 ++ > drivers/net/bnxt/bnxt_hwrm.c | 1554 > drivers/net/bnxt/bnxt_hwrm.h | 105 ++ > drivers/net/bnxt/bnxt_irq.c | 154 +++ > drivers/net/bnxt/bnxt_irq.h | 51 + > drivers/net/bnxt/bnxt_ring.c | 306 + > drivers/net/bnxt/bnxt_ring.h | 104 ++ > drivers/net/bnxt/bnxt_rxq.c | 383 ++ > drivers/net/bnxt/bnxt_rxq.h | 75 ++ > drivers/net/bnxt/bnxt_rxr.c | 369 ++ > drivers/net/bnxt/bnxt_rxr.h | 73 ++ > drivers/net/bnxt/bnxt_stats.c | 190 +++ > drivers/net/bnxt/bnxt_stats.h | 44 + > drivers/net/bnxt/bnxt_txq.c | 164 +++ > drivers/net/bnxt/bnxt_txq.h | 76 ++ > drivers/net/bnxt/bnxt_txr.c | 326 + > drivers/net/bnxt/bnxt_txr.h | 71 ++ > drivers/net/bnxt/bnxt_vnic.c | 285 + > drivers/net/bnxt/bnxt_vnic.h | 80 ++ > drivers/net/bnxt/hsi_struct_def_dpdk.h| 1832 > + > drivers/net/bnxt/rte_pmd_bnxt_version.map |4 + > 27 files changed, 8427 insertions(+) > create mode 100644 drivers/net/bnxt/Makefile > create mode 100644 drivers/net/bnxt/bnxt.h > create mode 100644 drivers/net/bnxt/bnxt_cpr.c > create mode 100644 drivers/net/bnxt/bnxt_cpr.h > create mode 100644 drivers/net/bnxt/bnxt_ethdev.c > create mode 100644 drivers/net/bnxt/bnxt_filter.c > create mode 100644 drivers/net/bnxt/bnxt_filter.h > create mode 100644 drivers/net/bnxt/bnxt_hwrm.c > create mode 100644 drivers/net/bnxt/bnxt_hwrm.h > create mode 100644 drivers/net/bnxt/bnxt_irq.c > create mode 100644 drivers/net/bnxt/bnxt_irq.h > create mode 100644 drivers/net/bnxt/bnxt_ring.c > create mode 100644 drivers/net/bnxt/bnxt_ring.h > create mode 100644 drivers/net/bnxt/bnxt_rxq.c > create mode 100644 drivers/net/bnxt/bnxt_rxq.h > create mode 100644 drivers/net/bnxt/bnxt_rxr.c > create mode 100644 drivers/net/bnxt/bnxt_rxr.h > create mode 100644 drivers/net/bnxt/bnxt_stats.c > create mode 100644 drivers/net/bnxt/bnxt_stats.h > create mode 100644 drivers/net/bnxt/bnxt_txq.c > create mode 100644 drivers/net/bnxt/bnxt_txq.h > create mode 100644 drivers/net/bnxt/bnxt_txr.c > create mode 100644 drivers/net/bnxt/bnxt_txr.h > create mode 100644 drivers/net/bnxt/bnxt_vnic.c > create mode 100644 drivers/net/bnxt/bnxt_vnic.h > create mode 100644 drivers/net/bnxt/hsi_struct_def_dpdk.h > create mode 100644 drivers/net/bnxt/rte_pmd_bnxt_version.map This seems a single huge commit. Can this be split up into separate commits with self-contained changes in each one, e.g. a feature per commit, starting with basic init, then RX and TX etc. etc.? /Bruce
[dpdk-dev] [PATCH v3 3/7] drivers/net/bnxt new driver for Broadcom bnxt
On Tue, Apr 19, 2016 at 7:19 AM, Bruce Richardson < bruce.richardson at intel.com> wrote: > On Fri, Mar 04, 2016 at 01:05:24PM -0800, Stephen Hurd wrote: > > New driver for Broadcom bnxt (NexXtreme C-series) devices. > > This seems a single huge commit. Can this be split up into separate commits > with self-contained changes in each one, e.g. a feature per commit, > starting > with basic init, then RX and TX etc. etc.? > I suppose it could, I'm not sure what the value of that approach would be though... it would just be a long process of copy/paste/commit/repeat. Internally, it was developed as a port/rewrite of another driver, so we don't actually have history without a large commit. Assuming each individual commit needs to be buildable, this will end up burning a lot of time, and I doubt each separate commit could be reasonably tested. -- Stephen Hurd
[dpdk-dev] [PATCH v3 3/7] drivers/net/bnxt new driver for Broadcom bnxt
On Fri, Mar 4, 2016 at 3:02 PM, Stephen Hemminger < stephen at networkplumber.org> wrote: > > > New driver for Broadcom bnxt (NexXtreme C-series) devices.> > > Looks good, I just have a couple of functionality comments. > > 1. Driver does not appear to support Link State interrupt. Not a big >deal, but would be good to have. > Correct, it currently doesn't, but this is on The List of things to do. > 2. Driver does not support hotplug > I haven't looked into enabling this at all yet. > 3. Since driver does not support scattered receive, it should check >and error out on enable_scatter (ditto for other features in rxmode). >This will save pain in future when some application asks device to do >something it can not do. > Good catch, will do. 4. Does driver support SECONDARY process model, it appears secondary >device will reset hardware. > I don't actually know what this is, I'll look into it. 5. Driver does not supper per-receive queue data interrupts. >This is necessary for power-saving NAPI like code. > This is also on The List. As a somewhat-related process question, should I be submitting this as a growing patch series until the merge window opens up again, or should I submit further changes as new patches? I'm not really sure on this point, and have been holding off adding the header comment commits until I figured out the right answer.
[dpdk-dev] [PATCH v3 3/7] drivers/net/bnxt new driver for Broadcom bnxt
On Fri, 4 Mar 2016 13:05:24 -0800 Stephen Hurd wrote: > New driver for Broadcom bnxt (NexXtreme C-series) devices. > > Standards-compliant 10/25/50G support with 30MPPS full-duplex throughput > http://www.broadcom.com/press/release.php?id=s923886 > > Signed-off-by: Stephen Hurd > --- > v3: > * Fix incorrect format specifier compilation error on i686 > (PRIx64 instead of lx for uint64_t) on line 1337 > > drivers/net/bnxt/Makefile | 79 ++ > drivers/net/bnxt/bnxt.h | 217 > drivers/net/bnxt/bnxt_cpr.c | 138 +++ > drivers/net/bnxt/bnxt_cpr.h | 117 ++ > drivers/net/bnxt/bnxt_ethdev.c| 1381 ++ > drivers/net/bnxt/bnxt_filter.c| 175 +++ > drivers/net/bnxt/bnxt_filter.h| 74 ++ > drivers/net/bnxt/bnxt_hwrm.c | 1554 > drivers/net/bnxt/bnxt_hwrm.h | 105 ++ > drivers/net/bnxt/bnxt_irq.c | 154 +++ > drivers/net/bnxt/bnxt_irq.h | 51 + > drivers/net/bnxt/bnxt_ring.c | 306 + > drivers/net/bnxt/bnxt_ring.h | 104 ++ > drivers/net/bnxt/bnxt_rxq.c | 383 ++ > drivers/net/bnxt/bnxt_rxq.h | 75 ++ > drivers/net/bnxt/bnxt_rxr.c | 369 ++ > drivers/net/bnxt/bnxt_rxr.h | 73 ++ > drivers/net/bnxt/bnxt_stats.c | 190 +++ > drivers/net/bnxt/bnxt_stats.h | 44 + > drivers/net/bnxt/bnxt_txq.c | 164 +++ > drivers/net/bnxt/bnxt_txq.h | 76 ++ > drivers/net/bnxt/bnxt_txr.c | 326 + > drivers/net/bnxt/bnxt_txr.h | 71 ++ > drivers/net/bnxt/bnxt_vnic.c | 285 + > drivers/net/bnxt/bnxt_vnic.h | 80 ++ > drivers/net/bnxt/hsi_struct_def_dpdk.h| 1832 > + > drivers/net/bnxt/rte_pmd_bnxt_version.map |4 + > 27 files changed, 8427 insertions(+) > create mode 100644 drivers/net/bnxt/Makefile > create mode 100644 drivers/net/bnxt/bnxt.h > create mode 100644 drivers/net/bnxt/bnxt_cpr.c > create mode 100644 drivers/net/bnxt/bnxt_cpr.h > create mode 100644 drivers/net/bnxt/bnxt_ethdev.c > create mode 100644 drivers/net/bnxt/bnxt_filter.c > create mode 100644 drivers/net/bnxt/bnxt_filter.h > create mode 100644 drivers/net/bnxt/bnxt_hwrm.c > create mode 100644 drivers/net/bnxt/bnxt_hwrm.h > create mode 100644 drivers/net/bnxt/bnxt_irq.c > create mode 100644 drivers/net/bnxt/bnxt_irq.h > create mode 100644 drivers/net/bnxt/bnxt_ring.c > create mode 100644 drivers/net/bnxt/bnxt_ring.h > create mode 100644 drivers/net/bnxt/bnxt_rxq.c > create mode 100644 drivers/net/bnxt/bnxt_rxq.h > create mode 100644 drivers/net/bnxt/bnxt_rxr.c > create mode 100644 drivers/net/bnxt/bnxt_rxr.h > create mode 100644 drivers/net/bnxt/bnxt_stats.c > create mode 100644 drivers/net/bnxt/bnxt_stats.h > create mode 100644 drivers/net/bnxt/bnxt_txq.c > create mode 100644 drivers/net/bnxt/bnxt_txq.h > create mode 100644 drivers/net/bnxt/bnxt_txr.c > create mode 100644 drivers/net/bnxt/bnxt_txr.h > create mode 100644 drivers/net/bnxt/bnxt_vnic.c > create mode 100644 drivers/net/bnxt/bnxt_vnic.h > create mode 100644 drivers/net/bnxt/hsi_struct_def_dpdk.h > create mode 100644 drivers/net/bnxt/rte_pmd_bnxt_version.map Looks good, I just have a couple of functionality comments. 1. Driver does not appear to support Link State interrupt. Not a big deal, but would be good to have. 2. Driver does not support hotplug 3. Since driver does not support scattered receive, it should check and error out on enable_scatter (ditto for other features in rxmode). This will save pain in future when some application asks device to do something it can not do. 4. Does driver support SECONDARY process model, it appears secondary device will reset hardware. 5. Driver does not supper per-receive queue data interrupts. This is necessary for power-saving NAPI like code.
[dpdk-dev] [PATCH v3 3/7] drivers/net/bnxt new driver for Broadcom bnxt
New driver for Broadcom bnxt (NexXtreme C-series) devices. Standards-compliant 10/25/50G support with 30MPPS full-duplex throughput http://www.broadcom.com/press/release.php?id=s923886 Signed-off-by: Stephen Hurd --- v3: * Fix incorrect format specifier compilation error on i686 (PRIx64 instead of lx for uint64_t) on line 1337 drivers/net/bnxt/Makefile | 79 ++ drivers/net/bnxt/bnxt.h | 217 drivers/net/bnxt/bnxt_cpr.c | 138 +++ drivers/net/bnxt/bnxt_cpr.h | 117 ++ drivers/net/bnxt/bnxt_ethdev.c| 1381 ++ drivers/net/bnxt/bnxt_filter.c| 175 +++ drivers/net/bnxt/bnxt_filter.h| 74 ++ drivers/net/bnxt/bnxt_hwrm.c | 1554 drivers/net/bnxt/bnxt_hwrm.h | 105 ++ drivers/net/bnxt/bnxt_irq.c | 154 +++ drivers/net/bnxt/bnxt_irq.h | 51 + drivers/net/bnxt/bnxt_ring.c | 306 + drivers/net/bnxt/bnxt_ring.h | 104 ++ drivers/net/bnxt/bnxt_rxq.c | 383 ++ drivers/net/bnxt/bnxt_rxq.h | 75 ++ drivers/net/bnxt/bnxt_rxr.c | 369 ++ drivers/net/bnxt/bnxt_rxr.h | 73 ++ drivers/net/bnxt/bnxt_stats.c | 190 +++ drivers/net/bnxt/bnxt_stats.h | 44 + drivers/net/bnxt/bnxt_txq.c | 164 +++ drivers/net/bnxt/bnxt_txq.h | 76 ++ drivers/net/bnxt/bnxt_txr.c | 326 + drivers/net/bnxt/bnxt_txr.h | 71 ++ drivers/net/bnxt/bnxt_vnic.c | 285 + drivers/net/bnxt/bnxt_vnic.h | 80 ++ drivers/net/bnxt/hsi_struct_def_dpdk.h| 1832 + drivers/net/bnxt/rte_pmd_bnxt_version.map |4 + 27 files changed, 8427 insertions(+) create mode 100644 drivers/net/bnxt/Makefile create mode 100644 drivers/net/bnxt/bnxt.h create mode 100644 drivers/net/bnxt/bnxt_cpr.c create mode 100644 drivers/net/bnxt/bnxt_cpr.h create mode 100644 drivers/net/bnxt/bnxt_ethdev.c create mode 100644 drivers/net/bnxt/bnxt_filter.c create mode 100644 drivers/net/bnxt/bnxt_filter.h create mode 100644 drivers/net/bnxt/bnxt_hwrm.c create mode 100644 drivers/net/bnxt/bnxt_hwrm.h create mode 100644 drivers/net/bnxt/bnxt_irq.c create mode 100644 drivers/net/bnxt/bnxt_irq.h create mode 100644 drivers/net/bnxt/bnxt_ring.c create mode 100644 drivers/net/bnxt/bnxt_ring.h create mode 100644 drivers/net/bnxt/bnxt_rxq.c create mode 100644 drivers/net/bnxt/bnxt_rxq.h create mode 100644 drivers/net/bnxt/bnxt_rxr.c create mode 100644 drivers/net/bnxt/bnxt_rxr.h create mode 100644 drivers/net/bnxt/bnxt_stats.c create mode 100644 drivers/net/bnxt/bnxt_stats.h create mode 100644 drivers/net/bnxt/bnxt_txq.c create mode 100644 drivers/net/bnxt/bnxt_txq.h create mode 100644 drivers/net/bnxt/bnxt_txr.c create mode 100644 drivers/net/bnxt/bnxt_txr.h create mode 100644 drivers/net/bnxt/bnxt_vnic.c create mode 100644 drivers/net/bnxt/bnxt_vnic.h create mode 100644 drivers/net/bnxt/hsi_struct_def_dpdk.h create mode 100644 drivers/net/bnxt/rte_pmd_bnxt_version.map diff --git a/drivers/net/bnxt/Makefile b/drivers/net/bnxt/Makefile new file mode 100644 index 000..74de642 --- /dev/null +++ b/drivers/net/bnxt/Makefile @@ -0,0 +1,79 @@ +# BSD LICENSE +# +# Copyright(c) 2010-2014 Intel Corporation. All rights reserved. +# Copyright(c) 2014 6WIND S.A. +# Copyright(c) 2015 Broadcom Corporation. All rights reserved. +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions +# are met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in +# the documentation and/or other materials provided with the +# distribution. +# * Neither the name of Intel Corporation nor the names of its +# contributors may be used to endorse or promote products derived +# from this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +# THEORY OF LIABILITY,
