Re: [net-next PATCH 1/2] igbvf: add new driver to support 82576 virtual functions
Andrew Morton wrote:
On Tue, 17 Mar 2009 17:12:48 -0700 Alexander Duyck
alexander.h.du...@intel.com wrote:
Thanks for all the comments. I tried to incorporate most of them into
the igbvf driver and also ended up porting some over to our other
drivers, specifically igb since the igbvf driver copies much of the code.
I have added my comments inline below.
Thanks,
Alex
Andrew Morton wrote:
On Tue, 10 Mar 2009 19:09:28 -0700 Jeff Kirsher jeffrey.t.kirs...@intel.com
wrote:
From: Alexander Duyck alexander.h.du...@intel.com
This adds an igbvf driver to handle virtual functions provided
by the igb driver.
The drive-by reader is now wondering what a virtual function is.
^^ this comment was missed.
I was indirectly asking for an overview (preferably in the changelog) of
what the whole patch actually does.
Sorry, while I missed the comment in my response I had gotten to
addressing it in the next version. I updated it to more thoroughly
describe what the VF driver is doing. I also included instructions on
how to enable the VFs from the PF so that they can be tested.
+static int igbvf_set_ringparam(struct net_device *netdev,
+ struct ethtool_ringparam *ring)
+{
+ struct igbvf_adapter *adapter = netdev_priv(netdev);
+ struct igbvf_ring *tx_ring, *tx_old;
+ struct igbvf_ring *rx_ring, *rx_old;
+ int err;
+
+ if ((ring-rx_mini_pending) || (ring-rx_jumbo_pending))
+ return -EINVAL;
+
+ while (test_and_set_bit(__IGBVF_RESETTING, adapter-state))
+ msleep(1);
No timeout needed here? Interrupts might not be working, for example..
This bit isn't set in interrupt context. This is always used out of
interrupt context and is just to prevent multiple setting changes at the
same time.
Oh. Can't use plain old mutex_lock()?
We have one or two spots that actually check to see if the bit is set
and just report a warning instead of actually waiting on the bit to clear.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next PATCH 1/2] igbvf: add new driver to support 82576 virtual functions
On Wed, 18 Mar 2009 08:22:46 -0700 Alexander Duyck
alexander.h.du...@intel.com wrote:
+static int igbvf_set_ringparam(struct net_device *netdev,
+ struct ethtool_ringparam *ring)
+{
+ struct igbvf_adapter *adapter = netdev_priv(netdev);
+ struct igbvf_ring *tx_ring, *tx_old;
+ struct igbvf_ring *rx_ring, *rx_old;
+ int err;
+
+ if ((ring-rx_mini_pending) || (ring-rx_jumbo_pending))
+ return -EINVAL;
+
+ while (test_and_set_bit(__IGBVF_RESETTING, adapter-state))
+ msleep(1);
No timeout needed here? Interrupts might not be working, for example..
This bit isn't set in interrupt context. This is always used out of
interrupt context and is just to prevent multiple setting changes at the
same time.
Oh. Can't use plain old mutex_lock()?
We have one or two spots that actually check to see if the bit is set
and just report a warning instead of actually waiting on the bit to clear.
mutex_is_locked()?
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [net-next PATCH 1/2] igbvf: add new driver to support 82576 virtual functions
Thanks for all the comments. I tried to incorporate most of them into
the igbvf driver and also ended up porting some over to our other
drivers, specifically igb since the igbvf driver copies much of the code.
I have added my comments inline below.
Thanks,
Alex
Andrew Morton wrote:
On Tue, 10 Mar 2009 19:09:28 -0700 Jeff Kirsher jeffrey.t.kirs...@intel.com
wrote:
From: Alexander Duyck alexander.h.du...@intel.com
This adds an igbvf driver to handle virtual functions provided
by the igb driver.
The drive-by reader is now wondering what a virtual function is.
...
+#define IGBVF_STAT(m, b) sizeof(((struct igbvf_adapter *)0)-m), \
+ offsetof(struct igbvf_adapter, m), \
+ offsetof(struct igbvf_adapter, b)
+static const struct igbvf_stats igbvf_gstrings_stats[] = {
+ { rx_packets, IGBVF_STAT(stats.gprc, stats.base_gprc) },
+ { tx_packets, IGBVF_STAT(stats.gptc, stats.base_gptc) },
+ { rx_bytes, IGBVF_STAT(stats.gorc, stats.base_gorc) },
+ { tx_bytes, IGBVF_STAT(stats.gotc, stats.base_gotc) },
+ { multicast, IGBVF_STAT(stats.mprc, stats.base_mprc) },
+ { lbrx_bytes, IGBVF_STAT(stats.gorlbc, stats.base_gorlbc) },
+ { lbrx_packets, IGBVF_STAT(stats.gprlbc, stats.base_gprlbc) },
+ { tx_restart_queue, IGBVF_STAT(restart_queue, zero_base) },
+ { rx_long_byte_count, IGBVF_STAT(stats.gorc, stats.base_gorc) },
+ { rx_csum_offload_good, IGBVF_STAT(hw_csum_good, zero_base) },
+ { rx_csum_offload_errors, IGBVF_STAT(hw_csum_err, zero_base) },
+ { rx_header_split, IGBVF_STAT(rx_hdr_split, zero_base) },
+ { alloc_rx_buff_failed, IGBVF_STAT(alloc_rx_buff_failed, zero_base) },
+};
stares at it for a while
It would be clearer if `m' and `b' were (much) more meaningful identifiers.
I agree, the values have been changed to current and base.
+#define IGBVF_GLOBAL_STATS_LEN \
+ (sizeof(igbvf_gstrings_stats) / sizeof(struct igbvf_stats))
This is ARRAY_SIZE().
+#define IGBVF_STATS_LEN (IGBVF_GLOBAL_STATS_LEN)
Why does this need to exist?
It doesn't, it has been dropped and references to it replaced with
IGBVF_GLOBAL_STATS_LEN.
...
+static int igbvf_set_ringparam(struct net_device *netdev,
+ struct ethtool_ringparam *ring)
+{
+ struct igbvf_adapter *adapter = netdev_priv(netdev);
+ struct igbvf_ring *tx_ring, *tx_old;
+ struct igbvf_ring *rx_ring, *rx_old;
+ int err;
+
+ if ((ring-rx_mini_pending) || (ring-rx_jumbo_pending))
+ return -EINVAL;
+
+ while (test_and_set_bit(__IGBVF_RESETTING, adapter-state))
+ msleep(1);
No timeout needed here? Interrupts might not be working, for example..
This bit isn't set in interrupt context. This is always used out of
interrupt context and is just to prevent multiple setting changes at the
same time.
+ if (netif_running(adapter-netdev))
+ igbvf_down(adapter);
+
+ tx_old = adapter-tx_ring;
+ rx_old = adapter-rx_ring;
+
+ err = -ENOMEM;
+ tx_ring = kzalloc(sizeof(struct igbvf_ring), GFP_KERNEL);
+ if (!tx_ring)
+ goto err_alloc_tx;
+ /*
+ * use a memcpy to save any previously configured
+ * items like napi structs from having to be
+ * reinitialized
+ */
+ memcpy(tx_ring, tx_old, sizeof(struct igbvf_ring));
+
+ rx_ring = kzalloc(sizeof(struct igbvf_ring), GFP_KERNEL);
+ if (!rx_ring)
+ goto err_alloc_rx;
+ memcpy(rx_ring, rx_old, sizeof(struct igbvf_ring));
+
+ adapter-tx_ring = tx_ring;
+ adapter-rx_ring = rx_ring;
+
+ rx_ring-count = max(ring-rx_pending, (u32)IGBVF_MIN_RXD);
+ rx_ring-count = min(rx_ring-count, (u32)(IGBVF_MAX_RXD));
+ rx_ring-count = ALIGN(rx_ring-count, REQ_RX_DESCRIPTOR_MULTIPLE);
+
+ tx_ring-count = max(ring-tx_pending, (u32)IGBVF_MIN_TXD);
+ tx_ring-count = min(tx_ring-count, (u32)(IGBVF_MAX_TXD));
+ tx_ring-count = ALIGN(tx_ring-count, REQ_TX_DESCRIPTOR_MULTIPLE);
+
+ if (netif_running(adapter-netdev)) {
+ /* Try to get new resources before deleting old */
+ err = igbvf_setup_rx_resources(adapter);
+ if (err)
+ goto err_setup_rx;
+ err = igbvf_setup_tx_resources(adapter);
+ if (err)
+ goto err_setup_tx;
+
+ /*
+ * restore the old in order to free it,
+ * then add in the new
+ */
+ adapter-rx_ring = rx_old;
+ adapter-tx_ring = tx_old;
+ igbvf_free_rx_resources(adapter);
+ igbvf_free_tx_resources(adapter);
+ kfree(tx_old);
+ kfree(rx_old);
That's odd-looking. Why take a copy of rx_old and tx_old when we're
about to free them?
This whole section has been redone. The approach here didn't work well
and so I redid it to match how we do this in igb.
+ adapter-rx_ring = rx_ring;
+
Re: [net-next PATCH 1/2] igbvf: add new driver to support 82576 virtual functions
On Tue, 10 Mar 2009 19:09:28 -0700 Jeff Kirsher jeffrey.t.kirs...@intel.com
wrote:
From: Alexander Duyck alexander.h.du...@intel.com
This adds an igbvf driver to handle virtual functions provided
by the igb driver.
The drive-by reader is now wondering what a virtual function is.
...
+#define IGBVF_STAT(m, b) sizeof(((struct igbvf_adapter *)0)-m), \
+ offsetof(struct igbvf_adapter, m), \
+ offsetof(struct igbvf_adapter, b)
+static const struct igbvf_stats igbvf_gstrings_stats[] = {
+ { rx_packets, IGBVF_STAT(stats.gprc, stats.base_gprc) },
+ { tx_packets, IGBVF_STAT(stats.gptc, stats.base_gptc) },
+ { rx_bytes, IGBVF_STAT(stats.gorc, stats.base_gorc) },
+ { tx_bytes, IGBVF_STAT(stats.gotc, stats.base_gotc) },
+ { multicast, IGBVF_STAT(stats.mprc, stats.base_mprc) },
+ { lbrx_bytes, IGBVF_STAT(stats.gorlbc, stats.base_gorlbc) },
+ { lbrx_packets, IGBVF_STAT(stats.gprlbc, stats.base_gprlbc) },
+ { tx_restart_queue, IGBVF_STAT(restart_queue, zero_base) },
+ { rx_long_byte_count, IGBVF_STAT(stats.gorc, stats.base_gorc) },
+ { rx_csum_offload_good, IGBVF_STAT(hw_csum_good, zero_base) },
+ { rx_csum_offload_errors, IGBVF_STAT(hw_csum_err, zero_base) },
+ { rx_header_split, IGBVF_STAT(rx_hdr_split, zero_base) },
+ { alloc_rx_buff_failed, IGBVF_STAT(alloc_rx_buff_failed, zero_base) },
+};
stares at it for a while
It would be clearer if `m' and `b' were (much) more meaningful identifiers.
+#define IGBVF_GLOBAL_STATS_LEN \
+ (sizeof(igbvf_gstrings_stats) / sizeof(struct igbvf_stats))
This is ARRAY_SIZE().
+#define IGBVF_STATS_LEN (IGBVF_GLOBAL_STATS_LEN)
Why does this need to exist?
...
+static int igbvf_set_ringparam(struct net_device *netdev,
+ struct ethtool_ringparam *ring)
+{
+ struct igbvf_adapter *adapter = netdev_priv(netdev);
+ struct igbvf_ring *tx_ring, *tx_old;
+ struct igbvf_ring *rx_ring, *rx_old;
+ int err;
+
+ if ((ring-rx_mini_pending) || (ring-rx_jumbo_pending))
+ return -EINVAL;
+
+ while (test_and_set_bit(__IGBVF_RESETTING, adapter-state))
+ msleep(1);
No timeout needed here? Interrupts might not be working, for example..
+ if (netif_running(adapter-netdev))
+ igbvf_down(adapter);
+
+ tx_old = adapter-tx_ring;
+ rx_old = adapter-rx_ring;
+
+ err = -ENOMEM;
+ tx_ring = kzalloc(sizeof(struct igbvf_ring), GFP_KERNEL);
+ if (!tx_ring)
+ goto err_alloc_tx;
+ /*
+ * use a memcpy to save any previously configured
+ * items like napi structs from having to be
+ * reinitialized
+ */
+ memcpy(tx_ring, tx_old, sizeof(struct igbvf_ring));
+
+ rx_ring = kzalloc(sizeof(struct igbvf_ring), GFP_KERNEL);
+ if (!rx_ring)
+ goto err_alloc_rx;
+ memcpy(rx_ring, rx_old, sizeof(struct igbvf_ring));
+
+ adapter-tx_ring = tx_ring;
+ adapter-rx_ring = rx_ring;
+
+ rx_ring-count = max(ring-rx_pending, (u32)IGBVF_MIN_RXD);
+ rx_ring-count = min(rx_ring-count, (u32)(IGBVF_MAX_RXD));
+ rx_ring-count = ALIGN(rx_ring-count, REQ_RX_DESCRIPTOR_MULTIPLE);
+
+ tx_ring-count = max(ring-tx_pending, (u32)IGBVF_MIN_TXD);
+ tx_ring-count = min(tx_ring-count, (u32)(IGBVF_MAX_TXD));
+ tx_ring-count = ALIGN(tx_ring-count, REQ_TX_DESCRIPTOR_MULTIPLE);
+
+ if (netif_running(adapter-netdev)) {
+ /* Try to get new resources before deleting old */
+ err = igbvf_setup_rx_resources(adapter);
+ if (err)
+ goto err_setup_rx;
+ err = igbvf_setup_tx_resources(adapter);
+ if (err)
+ goto err_setup_tx;
+
+ /*
+ * restore the old in order to free it,
+ * then add in the new
+ */
+ adapter-rx_ring = rx_old;
+ adapter-tx_ring = tx_old;
+ igbvf_free_rx_resources(adapter);
+ igbvf_free_tx_resources(adapter);
+ kfree(tx_old);
+ kfree(rx_old);
That's odd-looking. Why take a copy of rx_old and tx_old when we're
about to free them?
+ adapter-rx_ring = rx_ring;
+ adapter-tx_ring = tx_ring;
+ err = igbvf_up(adapter);
+ if (err)
+ goto err_setup;
+ }
+
+ clear_bit(__IGBVF_RESETTING, adapter-state);
+ return 0;
+err_setup_tx:
+ igbvf_free_rx_resources(adapter);
+err_setup_rx:
+ adapter-rx_ring = rx_old;
+ adapter-tx_ring = tx_old;
+ kfree(rx_ring);
+err_alloc_rx:
+ kfree(tx_ring);
+err_alloc_tx:
+ igbvf_up(adapter);
+err_setup:
+ clear_bit(__IGBVF_RESETTING, adapter-state);
+ return err;
+}
+
...
+static void igbvf_diag_test(struct net_device *netdev,
+struct ethtool_test
