* [net-next 07/15] i40evf: lower message level
From: Jeff Kirsher @ 2017-09-30 0:44 UTC (permalink / raw)
To: davem; +Cc: Mitch Williams, netdev, nhorman, sassmann, jogreene, Jeff Kirsher
In-Reply-To: <20170930004507.20072-1-jeffrey.t.kirsher@intel.com>
From: Mitch Williams <mitch.a.williams@intel.com>
We see this message regularly on VF reset or unload (which invokes a
reset). It's essentially meaningless unless it's happening constantly.
To prevent consternation, lower the log level to debug so it's not seen
under normal circumstance.
Signed-off-by: Mitch Williams <mitch.a.williams@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/i40evf/i40evf_virtchnl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_virtchnl.c b/drivers/net/ethernet/intel/i40evf/i40evf_virtchnl.c
index 85876f4fb1fb..2bb0fe00361f 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_virtchnl.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_virtchnl.c
@@ -52,7 +52,7 @@ static int i40evf_send_pf_msg(struct i40evf_adapter *adapter,
err = i40e_aq_send_msg_to_pf(hw, op, 0, msg, len, NULL);
if (err)
- dev_err(&adapter->pdev->dev, "Unable to send opcode %d to PF, err %s, aq_err %s\n",
+ dev_dbg(&adapter->pdev->dev, "Unable to send opcode %d to PF, err %s, aq_err %s\n",
op, i40evf_stat_str(hw, err),
i40evf_aq_str(hw, hw->aq.asq_last_status));
return err;
--
2.14.1
^ permalink raw reply related
* [net-next 08/15] i40e: use separate state bit for miscellaneous IRQ setup
From: Jeff Kirsher @ 2017-09-30 0:45 UTC (permalink / raw)
To: davem; +Cc: Jacob Keller, netdev, nhorman, sassmann, jogreene, Jeff Kirsher
In-Reply-To: <20170930004507.20072-1-jeffrey.t.kirsher@intel.com>
From: Jacob Keller <jacob.e.keller@intel.com>
We currently (mis)use the __I40E_RECOVERY_PENDING bit to determine when
we should actually request a new IRQ in i40e_setup_misc_vector().
This led to a design mistake where we open-coded the re-setup of the
miscellaneous vector in i40e_resume() instead of using the function
provided. If we did not open-code this and instead tried to use the
i40e_setup_misc_vector() function, it would lead to never reallocating
the IRQ.
This would lead to a second i40e_suspend() call failing to free the
vector due to a NULL pointer dereference.
A future patch is going to re-work how the i40e_suspend() and
i40e_resume() flows work to clear all IRQ vectors, which would require
us to use i40e_setup_misc_vector() directly. Since during this time the
__I40E_RECOVERY_PENDING bit is set, we'll never re-allocate the vector.
Rather than leaving the open-coded setup in i40e_resume() lets just fix
the problem properly in i40e_setup_misc_vector().
Introduce a new state bit which indicates when the IRQ has been
assigned, which will be set when i40e_setup_misc_vector is first called.
This ultimately resolves the issue of re-requesting the vector, without
overloading the __I40E_RECOVERY_PENDING state. This ensures that the
suspend/resume cycle can use the setup function instead of open-coding
the re-request during resume.
Additionally, since the only callers of i40e_stop_misc_vector also want
to free it, move this code directly into the function to avoid
duplication. Due to the new functionality, rename it to
i40e_free_misc_vector().
This lets us drop the extra calls to free and re-enable the vector
during i40e_suspend() and i40e_resume(). We don't need to call
i40e_setup_misc_Vector() in i40e_resume() because it gets called by the
i40e_rebuild() call.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e.h | 1 +
drivers/net/ethernet/intel/i40e/i40e_main.c | 39 +++++++++++------------------
2 files changed, 15 insertions(+), 25 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
index d0c1bf5441d8..b7a539cdca00 100644
--- a/drivers/net/ethernet/intel/i40e/i40e.h
+++ b/drivers/net/ethernet/intel/i40e/i40e.h
@@ -136,6 +136,7 @@ enum i40e_state_t {
__I40E_MDD_EVENT_PENDING,
__I40E_VFLR_EVENT_PENDING,
__I40E_RESET_RECOVERY_PENDING,
+ __I40E_MISC_IRQ_REQUESTED,
__I40E_RESET_INTR_RECEIVED,
__I40E_REINIT_REQUESTED,
__I40E_PF_RESET_REQUESTED,
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 638f5bad0bd7..3ea4f8b942c3 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -3604,14 +3604,20 @@ static int i40e_vsi_enable_irq(struct i40e_vsi *vsi)
}
/**
- * i40e_stop_misc_vector - Stop the vector that handles non-queue events
+ * i40e_free_misc_vector - Free the vector that handles non-queue events
* @pf: board private structure
**/
-static void i40e_stop_misc_vector(struct i40e_pf *pf)
+static void i40e_free_misc_vector(struct i40e_pf *pf)
{
/* Disable ICR 0 */
wr32(&pf->hw, I40E_PFINT_ICR0_ENA, 0);
i40e_flush(&pf->hw);
+
+ if (pf->flags & I40E_FLAG_MSIX_ENABLED && pf->msix_entries) {
+ synchronize_irq(pf->msix_entries[0].vector);
+ free_irq(pf->msix_entries[0].vector, pf);
+ clear_bit(__I40E_MISC_IRQ_REQUESTED, pf->state);
+ }
}
/**
@@ -4466,11 +4472,7 @@ static void i40e_clear_interrupt_scheme(struct i40e_pf *pf)
{
int i;
- i40e_stop_misc_vector(pf);
- if (pf->flags & I40E_FLAG_MSIX_ENABLED && pf->msix_entries) {
- synchronize_irq(pf->msix_entries[0].vector);
- free_irq(pf->msix_entries[0].vector, pf);
- }
+ i40e_free_misc_vector(pf);
i40e_put_lump(pf->irq_pile, pf->iwarp_base_vector,
I40E_IWARP_IRQ_PILE_ID);
@@ -8365,13 +8367,12 @@ static int i40e_setup_misc_vector(struct i40e_pf *pf)
struct i40e_hw *hw = &pf->hw;
int err = 0;
- /* Only request the irq if this is the first time through, and
- * not when we're rebuilding after a Reset
- */
- if (!test_bit(__I40E_RESET_RECOVERY_PENDING, pf->state)) {
+ /* Only request the IRQ once, the first time through. */
+ if (!test_and_set_bit(__I40E_MISC_IRQ_REQUESTED, pf->state)) {
err = request_irq(pf->msix_entries[0].vector,
i40e_intr, 0, pf->int_name, pf);
if (err) {
+ clear_bit(__I40E_MISC_IRQ_REQUESTED, pf->state);
dev_info(&pf->pdev->dev,
"request_irq for %s failed: %d\n",
pf->int_name, err);
@@ -12069,11 +12070,8 @@ static int i40e_suspend(struct pci_dev *pdev, pm_message_t state)
wr32(hw, I40E_PFPM_APM, (pf->wol_en ? I40E_PFPM_APM_APME_MASK : 0));
wr32(hw, I40E_PFPM_WUFC, (pf->wol_en ? I40E_PFPM_WUFC_MAG_MASK : 0));
- i40e_stop_misc_vector(pf);
- if (pf->msix_entries) {
- synchronize_irq(pf->msix_entries[0].vector);
- free_irq(pf->msix_entries[0].vector, pf);
- }
+ i40e_free_misc_vector(pf);
+
retval = pci_save_state(pdev);
if (retval)
return retval;
@@ -12113,15 +12111,6 @@ static int i40e_resume(struct pci_dev *pdev)
/* handling the reset will rebuild the device state */
if (test_and_clear_bit(__I40E_SUSPENDED, pf->state)) {
clear_bit(__I40E_DOWN, pf->state);
- if (pf->msix_entries) {
- err = request_irq(pf->msix_entries[0].vector,
- i40e_intr, 0, pf->int_name, pf);
- if (err) {
- dev_err(&pf->pdev->dev,
- "request_irq for %s failed: %d\n",
- pf->int_name, err);
- }
- }
i40e_reset_and_rebuild(pf, false, false);
}
--
2.14.1
^ permalink raw reply related
* [net-next 13/15] i40evf: fix ring to vector mapping
From: Jeff Kirsher @ 2017-09-30 0:45 UTC (permalink / raw)
To: davem; +Cc: Alan Brady, netdev, nhorman, sassmann, jogreene, Jeff Kirsher
In-Reply-To: <20170930004507.20072-1-jeffrey.t.kirsher@intel.com>
From: Alan Brady <alan.brady@intel.com>
The current implementation for mapping queues to vectors is broken
because it attempts to map each Tx and Rx ring to its own vector,
however we use combined queues so we should actually be mapping the
Tx/Rx rings together on one vector.
Also in the current implementation, in the case where we have more
queues than vectors, we attempt to group the queues together into
'chunks' and map each 'chunk' of queues to a vector. Chunking them
together would be more ideal if, and only if, we only had RSS because of
the way the hashing algorithm works but in the case of a future patch
that enables VF ADq, round robin assignment is better and still works
with RSS.
This patch resolves both those issues and simplifies the code needed to
accomplish this. Instead of treating the case where we have more queues
than vectors as special, if we notice our vector index is greater than
vectors, reset the vector index to zero and continue mapping. This
should ensure that in both cases, whether we have enough vectors for
each queue or not, the queues get appropriately mapped.
Signed-off-by: Alan Brady <alan.brady@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/i40evf/i40evf_main.c | 48 ++++++-------------------
1 file changed, 10 insertions(+), 38 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40evf/i40evf_main.c b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
index 80ade6510279..69ef6c1d5364 100644
--- a/drivers/net/ethernet/intel/i40evf/i40evf_main.c
+++ b/drivers/net/ethernet/intel/i40evf/i40evf_main.c
@@ -432,52 +432,24 @@ i40evf_map_vector_to_txq(struct i40evf_adapter *adapter, int v_idx, int t_idx)
**/
static int i40evf_map_rings_to_vectors(struct i40evf_adapter *adapter)
{
+ int rings_remaining = adapter->num_active_queues;
+ int ridx = 0, vidx = 0;
int q_vectors;
- int v_start = 0;
- int rxr_idx = 0, txr_idx = 0;
- int rxr_remaining = adapter->num_active_queues;
- int txr_remaining = adapter->num_active_queues;
- int i, j;
- int rqpv, tqpv;
int err = 0;
q_vectors = adapter->num_msix_vectors - NONQ_VECS;
- /* The ideal configuration...
- * We have enough vectors to map one per queue.
- */
- if (q_vectors >= (rxr_remaining * 2)) {
- for (; rxr_idx < rxr_remaining; v_start++, rxr_idx++)
- i40evf_map_vector_to_rxq(adapter, v_start, rxr_idx);
+ for (; ridx < rings_remaining; ridx++) {
+ i40evf_map_vector_to_rxq(adapter, vidx, ridx);
+ i40evf_map_vector_to_txq(adapter, vidx, ridx);
- for (; txr_idx < txr_remaining; v_start++, txr_idx++)
- i40evf_map_vector_to_txq(adapter, v_start, txr_idx);
- goto out;
- }
-
- /* If we don't have enough vectors for a 1-to-1
- * mapping, we'll have to group them so there are
- * multiple queues per vector.
- * Re-adjusting *qpv takes care of the remainder.
- */
- for (i = v_start; i < q_vectors; i++) {
- rqpv = DIV_ROUND_UP(rxr_remaining, q_vectors - i);
- for (j = 0; j < rqpv; j++) {
- i40evf_map_vector_to_rxq(adapter, i, rxr_idx);
- rxr_idx++;
- rxr_remaining--;
- }
- }
- for (i = v_start; i < q_vectors; i++) {
- tqpv = DIV_ROUND_UP(txr_remaining, q_vectors - i);
- for (j = 0; j < tqpv; j++) {
- i40evf_map_vector_to_txq(adapter, i, txr_idx);
- txr_idx++;
- txr_remaining--;
- }
+ /* In the case where we have more queues than vectors, continue
+ * round-robin on vectors until all queues are mapped.
+ */
+ if (++vidx >= q_vectors)
+ vidx = 0;
}
-out:
adapter->aq_required |= I40EVF_FLAG_AQ_MAP_VECTORS;
return err;
--
2.14.1
^ permalink raw reply related
* [net-next 10/15] i40e: don't clear suspended state until we finish resuming
From: Jeff Kirsher @ 2017-09-30 0:45 UTC (permalink / raw)
To: davem; +Cc: Jacob Keller, netdev, nhorman, sassmann, jogreene, Jeff Kirsher
In-Reply-To: <20170930004507.20072-1-jeffrey.t.kirsher@intel.com>
From: Jacob Keller <jacob.e.keller@intel.com>
When handling suspend and resume callbacks we want to make sure that (a)
we don't suspend again if we're already suspended and (b) we don't
resume again if we're already resuming. Lets make sure we test_and_set
the __I40E_SUSPENDED bit in i40e_suspend which ensures that a suspend
call when already suspended will exit early. Additionally, if
__I40E_SUSPENDED is not set when we begin resuming, exit early as well.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index c82360437024..494cafde6b26 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -12059,7 +12059,10 @@ static int i40e_suspend(struct device *dev)
struct i40e_pf *pf = pci_get_drvdata(pdev);
struct i40e_hw *hw = &pf->hw;
- set_bit(__I40E_SUSPENDED, pf->state);
+ /* If we're already suspended, then there is nothing to do */
+ if (test_and_set_bit(__I40E_SUSPENDED, pf->state))
+ return 0;
+
set_bit(__I40E_DOWN, pf->state);
if (pf->wol_en && (pf->hw_features & I40E_HW_WOL_MC_MAGIC_PKT_WAKE))
@@ -12084,11 +12087,15 @@ static int i40e_resume(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
struct i40e_pf *pf = pci_get_drvdata(pdev);
- /* handling the reset will rebuild the device state */
- if (test_and_clear_bit(__I40E_SUSPENDED, pf->state)) {
- clear_bit(__I40E_DOWN, pf->state);
- i40e_reset_and_rebuild(pf, false, false);
- }
+ /* If we're not suspended, then there is nothing to do */
+ if (!test_bit(__I40E_SUSPENDED, pf->state))
+ return 0;
+
+ clear_bit(__I40E_DOWN, pf->state);
+ i40e_reset_and_rebuild(pf, false, false);
+
+ /* Clear suspended state last after everything is recovered */
+ clear_bit(__I40E_SUSPENDED, pf->state);
return 0;
}
--
2.14.1
^ permalink raw reply related
* [net-next 09/15] i40e: use newer generic PM support instead of legacy PM callbacks
From: Jeff Kirsher @ 2017-09-30 0:45 UTC (permalink / raw)
To: davem; +Cc: Jacob Keller, netdev, nhorman, sassmann, jogreene, Jeff Kirsher
In-Reply-To: <20170930004507.20072-1-jeffrey.t.kirsher@intel.com>
From: Jacob Keller <jacob.e.keller@intel.com>
Stop using the old legacy PM support, since we now have stable support
for the newer generic PM callbacks.
This has several advantages. First, we no longer have to manage our
own pci_save_state() and power changes, as it's preferred to have the
PCI stack do this. Second, these routines get called for both hibernate
and suspend to ram, so we can have the driver properly handle all the
suspend/resume flows that it needs to.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 54 +++++++++--------------------
1 file changed, 17 insertions(+), 37 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 3ea4f8b942c3..c82360437024 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -12050,14 +12050,14 @@ static void i40e_shutdown(struct pci_dev *pdev)
#ifdef CONFIG_PM
/**
- * i40e_suspend - PCI callback for moving to D3
- * @pdev: PCI device information struct
+ * i40e_suspend - PM callback for moving to D3
+ * @dev: generic device information structure
**/
-static int i40e_suspend(struct pci_dev *pdev, pm_message_t state)
+static int i40e_suspend(struct device *dev)
{
+ struct pci_dev *pdev = to_pci_dev(dev);
struct i40e_pf *pf = pci_get_drvdata(pdev);
struct i40e_hw *hw = &pf->hw;
- int retval = 0;
set_bit(__I40E_SUSPENDED, pf->state);
set_bit(__I40E_DOWN, pf->state);
@@ -12072,41 +12072,17 @@ static int i40e_suspend(struct pci_dev *pdev, pm_message_t state)
i40e_free_misc_vector(pf);
- retval = pci_save_state(pdev);
- if (retval)
- return retval;
-
- pci_wake_from_d3(pdev, pf->wol_en);
- pci_set_power_state(pdev, PCI_D3hot);
-
- return retval;
+ return 0;
}
/**
- * i40e_resume - PCI callback for waking up from D3
- * @pdev: PCI device information struct
+ * i40e_resume - PM callback for waking up from D3
+ * @dev: generic device information structure
**/
-static int i40e_resume(struct pci_dev *pdev)
+static int i40e_resume(struct device *dev)
{
+ struct pci_dev *pdev = to_pci_dev(dev);
struct i40e_pf *pf = pci_get_drvdata(pdev);
- u32 err;
-
- pci_set_power_state(pdev, PCI_D0);
- pci_restore_state(pdev);
- /* pci_restore_state() clears dev->state_saves, so
- * call pci_save_state() again to restore it.
- */
- pci_save_state(pdev);
-
- err = pci_enable_device_mem(pdev);
- if (err) {
- dev_err(&pdev->dev, "Cannot enable PCI device from suspend\n");
- return err;
- }
- pci_set_master(pdev);
-
- /* no wakeup events while running */
- pci_wake_from_d3(pdev, false);
/* handling the reset will rebuild the device state */
if (test_and_clear_bit(__I40E_SUSPENDED, pf->state)) {
@@ -12117,22 +12093,26 @@ static int i40e_resume(struct pci_dev *pdev)
return 0;
}
-#endif
+#endif /* CONFIG_PM */
+
static const struct pci_error_handlers i40e_err_handler = {
.error_detected = i40e_pci_error_detected,
.slot_reset = i40e_pci_error_slot_reset,
.resume = i40e_pci_error_resume,
};
+static SIMPLE_DEV_PM_OPS(i40e_pm_ops, i40e_suspend, i40e_resume);
+
static struct pci_driver i40e_driver = {
.name = i40e_driver_name,
.id_table = i40e_pci_tbl,
.probe = i40e_probe,
.remove = i40e_remove,
#ifdef CONFIG_PM
- .suspend = i40e_suspend,
- .resume = i40e_resume,
-#endif
+ .driver = {
+ .pm = &i40e_pm_ops,
+ },
+#endif /* CONFIG_PM */
.shutdown = i40e_shutdown,
.err_handler = &i40e_err_handler,
.sriov_configure = i40e_pci_sriov_configure,
--
2.14.1
^ permalink raw reply related
* [net-next 01/15] i40e/i40evf: rename bytes_per_int to bytes_per_usec
From: Jeff Kirsher @ 2017-09-30 0:44 UTC (permalink / raw)
To: davem; +Cc: Jacob Keller, netdev, nhorman, sassmann, jogreene, Jeff Kirsher
In-Reply-To: <20170930004507.20072-1-jeffrey.t.kirsher@intel.com>
From: Jacob Keller <jacob.e.keller@intel.com>
This value is not calculating bytes_per_int, which would actually just
be bytes/ITR_COUNTDOWN_START, but rather it's calculating bytes/usecs.
Rename the variable for clarity so that future developers understand
what the value is actually calculating.
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 12 ++++++------
drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 12 ++++++------
2 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
index f426762bd83a..d9fdf69bbc6e 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
@@ -960,14 +960,14 @@ static bool i40e_set_new_dynamic_itr(struct i40e_ring_container *rc)
{
enum i40e_latency_range new_latency_range = rc->latency_range;
u32 new_itr = rc->itr;
- int bytes_per_int;
+ int bytes_per_usec;
unsigned int usecs, estimated_usecs;
if (rc->total_packets == 0 || !rc->itr)
return false;
usecs = (rc->itr << 1) * ITR_COUNTDOWN_START;
- bytes_per_int = rc->total_bytes / usecs;
+ bytes_per_usec = rc->total_bytes / usecs;
/* The calculations in this algorithm depend on interrupts actually
* firing at the ITR rate. This may not happen if the packet rate is
@@ -993,18 +993,18 @@ static bool i40e_set_new_dynamic_itr(struct i40e_ring_container *rc)
*/
switch (new_latency_range) {
case I40E_LOWEST_LATENCY:
- if (bytes_per_int > 10)
+ if (bytes_per_usec > 10)
new_latency_range = I40E_LOW_LATENCY;
break;
case I40E_LOW_LATENCY:
- if (bytes_per_int > 20)
+ if (bytes_per_usec > 20)
new_latency_range = I40E_BULK_LATENCY;
- else if (bytes_per_int <= 10)
+ else if (bytes_per_usec <= 10)
new_latency_range = I40E_LOWEST_LATENCY;
break;
case I40E_BULK_LATENCY:
default:
- if (bytes_per_int <= 20)
+ if (bytes_per_usec <= 20)
new_latency_range = I40E_LOW_LATENCY;
break;
}
diff --git a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
index c32c62462c84..37e1de886d48 100644
--- a/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
+++ b/drivers/net/ethernet/intel/i40evf/i40e_txrx.c
@@ -358,14 +358,14 @@ static bool i40e_set_new_dynamic_itr(struct i40e_ring_container *rc)
{
enum i40e_latency_range new_latency_range = rc->latency_range;
u32 new_itr = rc->itr;
- int bytes_per_int;
+ int bytes_per_usec;
unsigned int usecs, estimated_usecs;
if (rc->total_packets == 0 || !rc->itr)
return false;
usecs = (rc->itr << 1) * ITR_COUNTDOWN_START;
- bytes_per_int = rc->total_bytes / usecs;
+ bytes_per_usec = rc->total_bytes / usecs;
/* The calculations in this algorithm depend on interrupts actually
* firing at the ITR rate. This may not happen if the packet rate is
@@ -391,18 +391,18 @@ static bool i40e_set_new_dynamic_itr(struct i40e_ring_container *rc)
*/
switch (new_latency_range) {
case I40E_LOWEST_LATENCY:
- if (bytes_per_int > 10)
+ if (bytes_per_usec > 10)
new_latency_range = I40E_LOW_LATENCY;
break;
case I40E_LOW_LATENCY:
- if (bytes_per_int > 20)
+ if (bytes_per_usec > 20)
new_latency_range = I40E_BULK_LATENCY;
- else if (bytes_per_int <= 10)
+ else if (bytes_per_usec <= 10)
new_latency_range = I40E_LOWEST_LATENCY;
break;
case I40E_BULK_LATENCY:
default:
- if (bytes_per_int <= 20)
+ if (bytes_per_usec <= 20)
new_latency_range = I40E_LOW_LATENCY;
break;
}
--
2.14.1
^ permalink raw reply related
* [net-next 00/15][pull request] 40GbE Intel Wired LAN Driver Updates 2017-09-29
From: Jeff Kirsher @ 2017-09-30 0:44 UTC (permalink / raw)
To: davem; +Cc: Jeff Kirsher, netdev, nhorman, sassmann, jogreene
This series contains updates to i40e and i40evf only.
Jake provides several of the changes starting with the renaming of a
variable to clarify what the value is actually calculating. Found we
were misusing the __I40E_RECOVERY_PENDING bit to determine when we
should actually request a new IRQ in i40e_setup_misc_vector(), which
lead to a design mistake, so to resolve the issue, use a separate
state bit for miscellaneous IRQ setup and fix up the design while we
are at it. Cleaned up the old legacy PM support in the driver since
we support the newer generic PM callbacks. Fixed a failure to
hibernate issue, where on some platforms with a large number of CPUs,
we would allocate many IRQ vectors which we would try to migrate to
CPU0 when hibernating.
Sudheer cleans up a check for unqualified module inside i40e_up_complete()
because the link state information is in flux at time, so log messages
are getting logged with incorrect link state information. Also provided
additional log message cleanups and simplify member variable access in
the printing of the link messages.
Mariusz relaxes the firmware check since Fortville and Fort Park NICs
can and do have different firmware versions, so only warn for older
Fortville firmware. Fixed an errata with a flow director statistic that
was not wrapping as expected, simply reset after reading.
Mitch prevents consternation by lowering the log level to debug on a
message seen regularly on VF reset or unload, which is meaningless under
normal circumstances. Refactor the firmware version checking since
Fortville and Fort Park devices can have different firmware versions.
Alan fixes a ring to vector mapping, where the past implementation
attempted to map each Tx and Rx ring to its own vector, however we use
combined queues so we should be mapping the Tx/Rx rings together on one
vector. Adds the ability for the VF to request a different number of
queues allocated to it.
The following are changes since commit fa8fefaa678ea390b873195d19c09930da84a4bb:
net: ipv4: remove fib_info arg to fib_check_nh
and are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue 40GbE
Alan Brady (2):
i40evf: fix ring to vector mapping
i40e: Enable VF to negotiate number of allocated queues
Jacob Keller (6):
i40e/i40evf: rename bytes_per_int to bytes_per_usec
i40e: use separate state bit for miscellaneous IRQ setup
i40e: use newer generic PM support instead of legacy PM callbacks
i40e: don't clear suspended state until we finish resuming
i40e: prevent service task from running while we're suspended
i40e: shutdown all IRQs and disable MSI-X when suspended
Mariusz Stachura (2):
i40e: relax warning message in case of version mismatch
i40e: fix for flow director counters not wrapping as expected
Mitch Williams (2):
i40evf: lower message level
i40e: refactor FW version checking
Sudheer Mogilappagari (3):
i40e: Fix unqualified module message while bringing link up
i40e: Fix link down message when interface is brought up
i40e: simplify member variable accesses
drivers/net/ethernet/intel/i40e/i40e.h | 2 +
drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h | 10 +-
drivers/net/ethernet/intel/i40e/i40e_common.c | 6 +-
drivers/net/ethernet/intel/i40e/i40e_main.c | 249 +++++++++++++--------
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 12 +-
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 75 +++++++
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h | 1 +
.../net/ethernet/intel/i40evf/i40e_adminq_cmd.h | 10 +-
drivers/net/ethernet/intel/i40evf/i40e_txrx.c | 12 +-
drivers/net/ethernet/intel/i40evf/i40evf_main.c | 50 +----
.../net/ethernet/intel/i40evf/i40evf_virtchnl.c | 2 +-
include/linux/avf/virtchnl.h | 20 ++
12 files changed, 294 insertions(+), 155 deletions(-)
--
2.14.1
^ permalink raw reply
* [net-next 02/15] i40e: Fix unqualified module message while bringing link up
From: Jeff Kirsher @ 2017-09-30 0:44 UTC (permalink / raw)
To: davem
Cc: Sudheer Mogilappagari, netdev, nhorman, sassmann, jogreene,
Jeff Kirsher
In-Reply-To: <20170930004507.20072-1-jeffrey.t.kirsher@intel.com>
From: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
In current driver, when ifconfig ethx up is done, the link state
doesn't transition to UP inside i40e_open(). It changes after AQ
command response is handled in i40e_handle_link_event().
When pf->hw.phy.link_info.link_info is DOWN inside i40e_open(),
The state is transient and invalid. So log message gets printed
based on incorrect info (i.e link_info and an_info).
This commit removes check for unqualified module inside
i40e_up_complete(). The existing check in i40e_handle_link_event()
logs the error message based on correct link state information.
Signed-off-by: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 6498da8806cb..b235a27232a8 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -5470,15 +5470,6 @@ static int i40e_up_complete(struct i40e_vsi *vsi)
i40e_print_link_message(vsi, true);
netif_tx_start_all_queues(vsi->netdev);
netif_carrier_on(vsi->netdev);
- } else if (vsi->netdev) {
- i40e_print_link_message(vsi, false);
- /* need to check for qualified module here*/
- if ((pf->hw.phy.link_info.link_info &
- I40E_AQ_MEDIA_AVAILABLE) &&
- (!(pf->hw.phy.link_info.an_info &
- I40E_AQ_QUALIFIED_MODULE)))
- netdev_err(vsi->netdev,
- "the driver failed to link because an unqualified module was detected.");
}
/* replay FDIR SB filters */
--
2.14.1
^ permalink raw reply related
* [net-next 03/15] i40e: Fix link down message when interface is brought up
From: Jeff Kirsher @ 2017-09-30 0:44 UTC (permalink / raw)
To: davem
Cc: Sudheer Mogilappagari, netdev, nhorman, sassmann, jogreene,
Jeff Kirsher
In-Reply-To: <20170930004507.20072-1-jeffrey.t.kirsher@intel.com>
From: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
i40e_print_link_message() is intended to compare new
link state with current link state and print log message
only if the new state is different from current state.
However in current driver the new state does not get updated
when link is going down because of the if condition. When an
interface is brought down, vsi->state is set to I40E_VSI_DOWN
in i40e_vsi_close() and later i40e_print_link_message() does
not get invoked in i40e_link_event due to if condition. Hence
link down message doesn't appear when link is going down. The
down state is seen later during i40e_open() and old state
gets printed. The actual link state doesn't get updated in
i40e_close() or i40e_open() but when i40e_handle_link_event is
called inside i40e_clean_adminq_subtask.
This change allows i40e_print_link_message() to be called when
interface is going down and keeps the state information updated.
Signed-off-by: Sudheer Mogilappagari <sudheer.mogilappagari@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index b235a27232a8..2e8fe6186b38 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -6420,8 +6420,7 @@ static void i40e_link_event(struct i40e_pf *pf)
new_link == netif_carrier_ok(vsi->netdev)))
return;
- if (!test_bit(__I40E_VSI_DOWN, vsi->state))
- i40e_print_link_message(vsi, new_link);
+ i40e_print_link_message(vsi, new_link);
/* Notify the base of the switch tree connected to
* the link. Floating VEBs are not notified.
--
2.14.1
^ permalink raw reply related
* Re: [PATCH] net: hns3: fix null pointer dereference before null check
From: Yunsheng Lin @ 2017-09-30 0:43 UTC (permalink / raw)
To: Colin King, Yisen Zhuang, Salil Mehta, David S . Miller, netdev
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20170929195123.4189-1-colin.king@canonical.com>
Hi, Colin
On 2017/9/30 3:51, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> pointer ndev is being dereferenced with the call to netif_running
> before it is being null checked. Re-order the code to only dereference
> ndev after it has been null checked.
Thanks for fixing it.
>
> Detected by CoverityScan, CID#1457206 ("Dereference before null check")
>
> Fixes: 9df8f79a4d29 ("net: hns3: Add DCB support when interacting with network stack")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
> index 4a0890f98b70..c31506514e5d 100644
> --- a/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c
> @@ -2865,7 +2865,7 @@ static int hns3_client_setup_tc(struct hnae3_handle *handle, u8 tc)
> {
> struct hnae3_knic_private_info *kinfo = &handle->kinfo;
> struct net_device *ndev = kinfo->netdev;
> - bool if_running = netif_running(ndev);
> + bool if_running;
> int ret;
> u8 i;
>
> @@ -2875,6 +2875,8 @@ static int hns3_client_setup_tc(struct hnae3_handle *handle, u8 tc)
> if (!ndev)
> return -ENODEV;
>
> + if_running = netif_running(ndev);
> +
> ret = netdev_set_num_tc(ndev, tc);
> if (ret)
> return ret;
>
^ permalink raw reply
* [next-queue PATCH v2 3/5] net/sched: Introduce the user API for the CBS shaper
From: Vinicius Costa Gomes @ 2017-09-30 0:26 UTC (permalink / raw)
To: netdev, intel-wired-lan
Cc: Vinicius Costa Gomes, jhs, xiyou.wangcong, jiri, andre.guedes,
ivan.briano, jesus.sanchez-palencia, boon.leong.ong,
richardcochran, henrik, levipearson, rodney.cummings
In-Reply-To: <20170930002657.15291-1-vinicius.gomes@intel.com>
Export the API necessary for configuring the CBS shaper (implemented
in the next patch) via the tc tool.
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
include/uapi/linux/pkt_sched.h | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 099bf5528fed..27c849c053cf 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -871,4 +871,21 @@ struct tc_pie_xstats {
__u32 maxq; /* maximum queue size */
__u32 ecn_mark; /* packets marked with ecn*/
};
+
+/* CBS */
+struct tc_cbs_qopt {
+ __s32 hicredit;
+ __s32 locredit;
+ __s32 idleslope;
+ __s32 sendslope;
+};
+
+enum {
+ TCA_CBS_UNSPEC,
+ TCA_CBS_PARMS,
+ __TCA_CBS_MAX,
+};
+
+#define TCA_CBS_MAX (__TCA_CBS_MAX - 1)
+
#endif
--
2.14.2
^ permalink raw reply related
* [next-queue PATCH v2 5/5] igb: Add support for CBS offload
From: Vinicius Costa Gomes @ 2017-09-30 0:26 UTC (permalink / raw)
To: netdev, intel-wired-lan
Cc: Andre Guedes, jhs, xiyou.wangcong, jiri, ivan.briano,
jesus.sanchez-palencia, boon.leong.ong, richardcochran, henrik,
levipearson, rodney.cummings
In-Reply-To: <20170930002657.15291-1-vinicius.gomes@intel.com>
From: Andre Guedes <andre.guedes@intel.com>
This patch adds support for Credit-Based Shaper (CBS) qdisc offload
from Traffic Control system. This support enable us to leverage the
Forwarding and Queuing for Time-Sensitive Streams (FQTSS) features
from Intel i210 Ethernet Controller. FQTSS is the former 802.1Qav
standard which was merged into 802.1Q in 2014. It enables traffic
prioritization and bandwidth reservation via the Credit-Based Shaper
which is implemented in hardware by i210 controller.
The patch introduces the igb_setup_tc() function which implements the
support for CBS qdisc hardware offload in the IGB driver. CBS offload
is the only traffic control offload supported by the driver at the
moment.
FQTSS transmission mode from i210 controller is automatically enabled
by the IGB driver when the CBS is enabled for the first hardware
queue. Likewise, FQTSS mode is automatically disabled when CBS is
disabled for the last hardware queue. Changing FQTSS mode requires NIC
reset.
FQTSS feature is supported by i210 controller only.
Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
drivers/net/ethernet/intel/igb/e1000_defines.h | 23 ++
drivers/net/ethernet/intel/igb/e1000_regs.h | 8 +
drivers/net/ethernet/intel/igb/igb.h | 6 +
drivers/net/ethernet/intel/igb/igb_main.c | 347 +++++++++++++++++++++++++
4 files changed, 384 insertions(+)
diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
index 1de82f247312..83cabff1e0ab 100644
--- a/drivers/net/ethernet/intel/igb/e1000_defines.h
+++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
@@ -353,7 +353,18 @@
#define E1000_RXPBS_CFG_TS_EN 0x80000000
#define I210_RXPBSIZE_DEFAULT 0x000000A2 /* RXPBSIZE default */
+#define I210_RXPBSIZE_MASK 0x0000003F
+#define I210_RXPBSIZE_PB_32KB 0x00000020
#define I210_TXPBSIZE_DEFAULT 0x04000014 /* TXPBSIZE default */
+#define I210_TXPBSIZE_MASK 0xC0FFFFFF
+#define I210_TXPBSIZE_PB0_8KB (8 << 0)
+#define I210_TXPBSIZE_PB1_8KB (8 << 6)
+#define I210_TXPBSIZE_PB2_4KB (4 << 12)
+#define I210_TXPBSIZE_PB3_4KB (4 << 18)
+
+#define I210_DTXMXPKTSZ_DEFAULT 0x00000098
+
+#define I210_SR_QUEUES_NUM 2
/* SerDes Control */
#define E1000_SCTL_DISABLE_SERDES_LOOPBACK 0x0400
@@ -1051,4 +1062,16 @@
#define E1000_VLAPQF_P_VALID(_n) (0x1 << (3 + (_n) * 4))
#define E1000_VLAPQF_QUEUE_MASK 0x03
+/* TX Qav Control fields */
+#define E1000_TQAVCTRL_XMIT_MODE BIT(0)
+#define E1000_TQAVCTRL_DATAFETCHARB BIT(4)
+#define E1000_TQAVCTRL_DATATRANARB BIT(8)
+
+/* TX Qav Credit Control fields */
+#define E1000_TQAVCC_IDLESLOPE_MASK 0xFFFF
+#define E1000_TQAVCC_QUEUEMODE BIT(31)
+
+/* Transmit Descriptor Control fields */
+#define E1000_TXDCTL_PRIORITY BIT(27)
+
#endif
diff --git a/drivers/net/ethernet/intel/igb/e1000_regs.h b/drivers/net/ethernet/intel/igb/e1000_regs.h
index 58adbf234e07..8eee081d395f 100644
--- a/drivers/net/ethernet/intel/igb/e1000_regs.h
+++ b/drivers/net/ethernet/intel/igb/e1000_regs.h
@@ -421,6 +421,14 @@ do { \
#define E1000_I210_FLA 0x1201C
+#define E1000_I210_DTXMXPKTSZ 0x355C
+
+#define E1000_I210_TXDCTL(_n) (0x0E028 + ((_n) * 0x40))
+
+#define E1000_I210_TQAVCTRL 0x3570
+#define E1000_I210_TQAVCC(_n) (0x3004 + ((_n) * 0x40))
+#define E1000_I210_TQAVHC(_n) (0x300C + ((_n) * 0x40))
+
#define E1000_INVM_DATA_REG(_n) (0x12120 + 4*(_n))
#define E1000_INVM_SIZE 64 /* Number of INVM Data Registers */
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 06ffb2bc713e..92845692087a 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -281,6 +281,11 @@ struct igb_ring {
u16 count; /* number of desc. in the ring */
u8 queue_index; /* logical index of the ring*/
u8 reg_idx; /* physical index of the ring */
+ bool cbs_enable; /* indicates if CBS is enabled */
+ s32 idleslope; /* idleSlope in kbps */
+ s32 sendslope; /* sendSlope in kbps */
+ s32 hicredit; /* hiCredit in bytes */
+ s32 locredit; /* loCredit in bytes */
/* everything past this point are written often */
u16 next_to_clean;
@@ -621,6 +626,7 @@ struct igb_adapter {
#define IGB_FLAG_EEE BIT(14)
#define IGB_FLAG_VLAN_PROMISC BIT(15)
#define IGB_FLAG_RX_LEGACY BIT(16)
+#define IGB_FLAG_FQTSS BIT(17)
/* Media Auto Sense */
#define IGB_MAS_ENABLE_0 0X0001
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index fd4a46b03cc8..03b8d0f4acfd 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -34,6 +34,7 @@
#include <linux/slab.h>
#include <net/checksum.h>
#include <net/ip6_checksum.h>
+#include <net/pkt_sched.h>
#include <linux/net_tstamp.h>
#include <linux/mii.h>
#include <linux/ethtool.h>
@@ -62,6 +63,17 @@
#define BUILD 0
#define DRV_VERSION __stringify(MAJ) "." __stringify(MIN) "." \
__stringify(BUILD) "-k"
+
+enum queue_mode {
+ QUEUE_MODE_STRICT_PRIORITY,
+ QUEUE_MODE_STREAM_RESERVATION,
+};
+
+enum tx_queue_prio {
+ TX_QUEUE_PRIO_HIGH,
+ TX_QUEUE_PRIO_LOW,
+};
+
char igb_driver_name[] = "igb";
char igb_driver_version[] = DRV_VERSION;
static const char igb_driver_string[] =
@@ -1271,6 +1283,12 @@ static int igb_alloc_q_vector(struct igb_adapter *adapter,
ring->count = adapter->tx_ring_count;
ring->queue_index = txr_idx;
+ ring->cbs_enable = false;
+ ring->idleslope = 0;
+ ring->sendslope = 0;
+ ring->hicredit = 0;
+ ring->locredit = 0;
+
u64_stats_init(&ring->tx_syncp);
u64_stats_init(&ring->tx_syncp2);
@@ -1598,6 +1616,284 @@ static void igb_get_hw_control(struct igb_adapter *adapter)
ctrl_ext | E1000_CTRL_EXT_DRV_LOAD);
}
+static void enable_fqtss(struct igb_adapter *adapter, bool enable)
+{
+ struct net_device *netdev = adapter->netdev;
+ struct e1000_hw *hw = &adapter->hw;
+
+ WARN_ON(hw->mac.type != e1000_i210);
+
+ if (enable)
+ adapter->flags |= IGB_FLAG_FQTSS;
+ else
+ adapter->flags &= ~IGB_FLAG_FQTSS;
+
+ if (netif_running(netdev))
+ schedule_work(&adapter->reset_task);
+}
+
+static bool is_fqtss_enabled(struct igb_adapter *adapter)
+{
+ return (adapter->flags & IGB_FLAG_FQTSS) ? true : false;
+}
+
+static void set_tx_desc_fetch_prio(struct e1000_hw *hw, int queue,
+ enum tx_queue_prio prio)
+{
+ u32 val;
+
+ WARN_ON(hw->mac.type != e1000_i210);
+ WARN_ON(queue < 0 || queue > 4);
+
+ val = rd32(E1000_I210_TXDCTL(queue));
+
+ if (prio == TX_QUEUE_PRIO_HIGH)
+ val |= E1000_TXDCTL_PRIORITY;
+ else
+ val &= ~E1000_TXDCTL_PRIORITY;
+
+ wr32(E1000_I210_TXDCTL(queue), val);
+}
+
+static void set_queue_mode(struct e1000_hw *hw, int queue, enum queue_mode mode)
+{
+ u32 val;
+
+ WARN_ON(hw->mac.type != e1000_i210);
+ WARN_ON(queue < 0 || queue > 1);
+
+ val = rd32(E1000_I210_TQAVCC(queue));
+
+ if (mode == QUEUE_MODE_STREAM_RESERVATION)
+ val |= E1000_TQAVCC_QUEUEMODE;
+ else
+ val &= ~E1000_TQAVCC_QUEUEMODE;
+
+ wr32(E1000_I210_TQAVCC(queue), val);
+}
+
+/**
+ * igb_configure_cbs - Configure Credit-Based Shaper (CBS)
+ * @adapter: pointer to adapter struct
+ * @queue: queue number
+ * @enable: true = enable CBS, false = disable CBS
+ * @idleslope: idleSlope in kbps
+ * @sendslope: sendSlope in kbps
+ * @hicredit: hiCredit in bytes
+ * @locredit: loCredit in bytes
+ *
+ * Configure CBS for a given hardware queue. When disabling, idleslope,
+ * sendslope, hicredit, locredit arguments are ignored. Returns 0 if
+ * success. Negative otherwise.
+ **/
+static void igb_configure_cbs(struct igb_adapter *adapter, int queue,
+ bool enable, int idleslope, int sendslope,
+ int hicredit, int locredit)
+{
+ struct net_device *netdev = adapter->netdev;
+ struct e1000_hw *hw = &adapter->hw;
+ u32 tqavcc;
+ u16 value;
+
+ WARN_ON(hw->mac.type != e1000_i210);
+ WARN_ON(queue < 0 || queue > 1);
+
+ if (enable) {
+ set_tx_desc_fetch_prio(hw, queue, TX_QUEUE_PRIO_HIGH);
+ set_queue_mode(hw, queue, QUEUE_MODE_STREAM_RESERVATION);
+
+ /* According to i210 datasheet section 7.2.7.7, we should set
+ * the 'idleSlope' field from TQAVCC register following the
+ * equation:
+ *
+ * For 100 Mbps link speed:
+ *
+ * value = BW * 0x7735 * 0.2 (E1)
+ *
+ * For 1000Mbps link speed:
+ *
+ * value = BW * 0x7735 * 2 (E2)
+ *
+ * E1 and E2 can be merged into one equation as shown below.
+ * Note that 'link-speed' is in Mbps.
+ *
+ * value = BW * 0x7735 * 2 * link-speed
+ * -------------- (E3)
+ * 1000
+ *
+ * 'BW' is the percentage bandwidth out of full link speed
+ * which can be found with the following equation. Note that
+ * idleSlope here is the parameter from this function which
+ * is in kbps.
+ *
+ * BW = idleSlope
+ * ----------------- (E4)
+ * link-speed * 1000
+ *
+ * That said, we can come up with a generic equation to
+ * calculate the value we should set it TQAVCC register by
+ * replacing 'BW' in E3 by E4. The resulting equation is:
+ *
+ * value = idleSlope * 0x7735 * 2 * link-speed
+ * ----------------- -------------- (E5)
+ * link-speed * 1000 1000
+ *
+ * 'link-speed' is present in both sides of the fraction so
+ * it is canceled out. The final equation is the following:
+ *
+ * value = idleSlope * 61034
+ * ----------------- (E6)
+ * 1000000
+ */
+ value = DIV_ROUND_UP_ULL(idleslope * 61034ULL, 1000000);
+
+ tqavcc = rd32(E1000_I210_TQAVCC(queue));
+ tqavcc &= ~E1000_TQAVCC_IDLESLOPE_MASK;
+ tqavcc |= value;
+ wr32(E1000_I210_TQAVCC(queue), tqavcc);
+
+ wr32(E1000_I210_TQAVHC(queue), 0x80000000 + hicredit * 0x7735);
+ } else {
+ set_tx_desc_fetch_prio(hw, queue, TX_QUEUE_PRIO_LOW);
+ set_queue_mode(hw, queue, QUEUE_MODE_STRICT_PRIORITY);
+
+ /* Set idleSlope to zero. */
+ tqavcc = rd32(E1000_I210_TQAVCC(queue));
+ tqavcc &= ~E1000_TQAVCC_IDLESLOPE_MASK;
+ wr32(E1000_I210_TQAVCC(queue), tqavcc);
+
+ /* Set hiCredit to zero. */
+ wr32(E1000_I210_TQAVHC(queue), 0);
+ }
+
+ /* XXX: In i210 controller the sendSlope and loCredit parameters from
+ * CBS are not configurable by software so we don't do any 'controller
+ * configuration' in respect to these parameters.
+ */
+
+ netdev_dbg(netdev, "CBS %s: queue %d idleslope %d sendslope %d hiCredit %d locredit %d\n",
+ (enable) ? "enabled" : "disabled", queue,
+ idleslope, sendslope, hicredit, locredit);
+}
+
+static int igb_save_cbs_params(struct igb_adapter *adapter, int queue,
+ bool enable, int idleslope, int sendslope,
+ int hicredit, int locredit)
+{
+ struct igb_ring *ring;
+
+ if (queue < 0 || queue > adapter->num_tx_queues)
+ return -EINVAL;
+
+ ring = adapter->tx_ring[queue];
+
+ ring->cbs_enable = enable;
+ ring->idleslope = idleslope;
+ ring->sendslope = sendslope;
+ ring->hicredit = hicredit;
+ ring->locredit = locredit;
+
+ return 0;
+}
+
+static bool is_any_cbs_enabled(struct igb_adapter *adapter)
+{
+ struct igb_ring *ring;
+ int i;
+
+ for (i = 0; i < adapter->num_tx_queues; i++) {
+ ring = adapter->tx_ring[i];
+
+ if (ring->cbs_enable)
+ return true;
+ }
+
+ return false;
+}
+
+static void igb_setup_tx_mode(struct igb_adapter *adapter)
+{
+ struct net_device *netdev = adapter->netdev;
+ struct e1000_hw *hw = &adapter->hw;
+ u32 val;
+
+ /* Only i210 controller supports changing the transmission mode. */
+ if (hw->mac.type != e1000_i210)
+ return;
+
+ if (is_fqtss_enabled(adapter)) {
+ int i, max_queue;
+
+ /* Configure TQAVCTRL register: set transmit mode to 'Qav',
+ * set data fetch arbitration to 'round robin' and set data
+ * transfer arbitration to 'credit shaper algorithm.
+ */
+ val = rd32(E1000_I210_TQAVCTRL);
+ val |= E1000_TQAVCTRL_XMIT_MODE | E1000_TQAVCTRL_DATATRANARB;
+ val &= ~E1000_TQAVCTRL_DATAFETCHARB;
+ wr32(E1000_I210_TQAVCTRL, val);
+
+ /* Configure Tx and Rx packet buffers sizes as described in
+ * i210 datasheet section 7.2.7.7.
+ */
+ val = rd32(E1000_TXPBS);
+ val &= ~I210_TXPBSIZE_MASK;
+ val |= I210_TXPBSIZE_PB0_8KB | I210_TXPBSIZE_PB1_8KB |
+ I210_TXPBSIZE_PB2_4KB | I210_TXPBSIZE_PB3_4KB;
+ wr32(E1000_TXPBS, val);
+
+ val = rd32(E1000_RXPBS);
+ val &= ~I210_RXPBSIZE_MASK;
+ val |= I210_RXPBSIZE_PB_32KB;
+ wr32(E1000_RXPBS, val);
+
+ /* Section 8.12.9 states that MAX_TPKT_SIZE from DTXMXPKTSZ
+ * register should not exceed the buffer size programmed in
+ * TXPBS. The smallest buffer size programmed in TXPBS is 4kB
+ * so according to the datasheet we should set MAX_TPKT_SIZE to
+ * 4kB / 64.
+ *
+ * However, when we do so, no frame from queue 2 and 3 are
+ * transmitted. It seems the MAX_TPKT_SIZE should not be great
+ * or _equal_ to the buffer size programmed in TXPBS. For this
+ * reason, we set set MAX_ TPKT_SIZE to (4kB - 1) / 64.
+ */
+ val = (4096 - 1) / 64;
+ wr32(E1000_I210_DTXMXPKTSZ, val);
+
+ /* Since FQTSS mode is enabled, apply any CBS configuration
+ * previously set. If no previous CBS configuration has been
+ * done, then the initial configuration is applied, which means
+ * CBS is disabled.
+ */
+ max_queue = (adapter->num_tx_queues < I210_SR_QUEUES_NUM) ?
+ adapter->num_tx_queues : I210_SR_QUEUES_NUM;
+
+ for (i = 0; i < max_queue; i++) {
+ struct igb_ring *ring = adapter->tx_ring[i];
+
+ igb_configure_cbs(adapter, i, ring->cbs_enable,
+ ring->idleslope, ring->sendslope,
+ ring->hicredit, ring->locredit);
+ }
+ } else {
+ wr32(E1000_RXPBS, I210_RXPBSIZE_DEFAULT);
+ wr32(E1000_TXPBS, I210_TXPBSIZE_DEFAULT);
+ wr32(E1000_I210_DTXMXPKTSZ, I210_DTXMXPKTSZ_DEFAULT);
+
+ val = rd32(E1000_I210_TQAVCTRL);
+ /* According to Section 8.12.21, the other flags we've set when
+ * enabling FQTSS are not relevant when disabling FQTSS so we
+ * don't set they here.
+ */
+ val &= ~E1000_TQAVCTRL_XMIT_MODE;
+ wr32(E1000_I210_TQAVCTRL, val);
+ }
+
+ netdev_dbg(netdev, "FQTSS %s\n", (is_fqtss_enabled(adapter)) ?
+ "enabled" : "disabled");
+}
+
/**
* igb_configure - configure the hardware for RX and TX
* @adapter: private board structure
@@ -1609,6 +1905,7 @@ static void igb_configure(struct igb_adapter *adapter)
igb_get_hw_control(adapter);
igb_set_rx_mode(netdev);
+ igb_setup_tx_mode(adapter);
igb_restore_vlan(adapter);
@@ -2150,6 +2447,55 @@ igb_features_check(struct sk_buff *skb, struct net_device *dev,
return features;
}
+static int igb_offload_cbs(struct igb_adapter *adapter,
+ struct tc_cbs_qopt_offload *qopt)
+{
+ struct e1000_hw *hw = &adapter->hw;
+ int err;
+
+ /* CBS offloading is only supported by i210 controller. */
+ if (hw->mac.type != e1000_i210)
+ return -EOPNOTSUPP;
+
+ /* CBS offloading is only supported by queue 0 and queue 1. */
+ if (qopt->queue < 0 || qopt->queue > 1)
+ return -EINVAL;
+
+ err = igb_save_cbs_params(adapter, qopt->queue, qopt->enable,
+ qopt->idleslope, qopt->sendslope,
+ qopt->hicredit, qopt->locredit);
+ if (err)
+ return err;
+
+ if (is_fqtss_enabled(adapter)) {
+ igb_configure_cbs(adapter, qopt->queue, qopt->enable,
+ qopt->idleslope, qopt->sendslope,
+ qopt->hicredit, qopt->locredit);
+
+ if (!is_any_cbs_enabled(adapter))
+ enable_fqtss(adapter, false);
+
+ } else {
+ enable_fqtss(adapter, true);
+ }
+
+ return 0;
+}
+
+static int igb_setup_tc(struct net_device *dev, enum tc_setup_type type,
+ void *type_data)
+{
+ struct igb_adapter *adapter = netdev_priv(dev);
+
+ switch (type) {
+ case TC_SETUP_CBS:
+ return igb_offload_cbs(adapter, type_data);
+
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
static const struct net_device_ops igb_netdev_ops = {
.ndo_open = igb_open,
.ndo_stop = igb_close,
@@ -2175,6 +2521,7 @@ static const struct net_device_ops igb_netdev_ops = {
.ndo_set_features = igb_set_features,
.ndo_fdb_add = igb_ndo_fdb_add,
.ndo_features_check = igb_features_check,
+ .ndo_setup_tc = igb_setup_tc,
};
/**
--
2.14.2
^ permalink raw reply related
* [next-queue PATCH v2 4/5] net/sched: Introduce Credit Based Shaper (CBS) qdisc
From: Vinicius Costa Gomes @ 2017-09-30 0:26 UTC (permalink / raw)
To: netdev, intel-wired-lan
Cc: Vinicius Costa Gomes, jhs, xiyou.wangcong, jiri, andre.guedes,
ivan.briano, jesus.sanchez-palencia, boon.leong.ong,
richardcochran, henrik, levipearson, rodney.cummings
In-Reply-To: <20170930002657.15291-1-vinicius.gomes@intel.com>
This queueing discipline implements the shaper algorithm defined by
the 802.1Q-2014 Section 8.6.8.2 and detailed in Annex L.
It's primary usage is to apply some bandwidth reservation to user
defined traffic classes, which are mapped to different queues via the
mqprio qdisc.
Initially, it only supports offloading the traffic shaping work to
supporting controllers.
Later, when a software implementation is added, the current dependency
on being installed "under" mqprio can be lifted.
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
---
include/linux/netdevice.h | 1 +
include/net/pkt_sched.h | 9 ++
net/sched/Kconfig | 11 +++
net/sched/Makefile | 1 +
net/sched/sch_cbs.c | 225 ++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 247 insertions(+)
create mode 100644 net/sched/sch_cbs.c
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f535779d9dc1..5d6fb06fd80f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -775,6 +775,7 @@ enum tc_setup_type {
TC_SETUP_CLSFLOWER,
TC_SETUP_CLSMATCHALL,
TC_SETUP_CLSBPF,
+ TC_SETUP_CBS,
};
/* These structures hold the attributes of xdp state that are being passed
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index 259bc191ba59..7c597b050b36 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -146,4 +146,13 @@ static inline bool is_classid_clsact_egress(u32 classid)
TC_H_MIN(classid) == TC_H_MIN(TC_H_MIN_EGRESS);
}
+struct tc_cbs_qopt_offload {
+ u8 enable;
+ s32 queue;
+ s32 hicredit;
+ s32 locredit;
+ s32 idleslope;
+ s32 sendslope;
+};
+
#endif
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index e70ed26485a2..c03d86a7775e 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -172,6 +172,17 @@ config NET_SCH_TBF
To compile this code as a module, choose M here: the
module will be called sch_tbf.
+config NET_SCH_CBS
+ tristate "Credit Based Shaper (CBS)"
+ ---help---
+ Say Y here if you want to use the Credit Based Shaper (CBS) packet
+ scheduling algorithm.
+
+ See the top of <file:net/sched/sch_cbs.c> for more details.
+
+ To compile this code as a module, choose M here: the
+ module will be called sch_cbs.
+
config NET_SCH_GRED
tristate "Generic Random Early Detection (GRED)"
---help---
diff --git a/net/sched/Makefile b/net/sched/Makefile
index 7b915d226de7..80c8f92d162d 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_NET_SCH_FQ_CODEL) += sch_fq_codel.o
obj-$(CONFIG_NET_SCH_FQ) += sch_fq.o
obj-$(CONFIG_NET_SCH_HHF) += sch_hhf.o
obj-$(CONFIG_NET_SCH_PIE) += sch_pie.o
+obj-$(CONFIG_NET_SCH_CBS) += sch_cbs.o
obj-$(CONFIG_NET_CLS_U32) += cls_u32.o
obj-$(CONFIG_NET_CLS_ROUTE4) += cls_route.o
diff --git a/net/sched/sch_cbs.c b/net/sched/sch_cbs.c
new file mode 100644
index 000000000000..3e0fb0b92160
--- /dev/null
+++ b/net/sched/sch_cbs.c
@@ -0,0 +1,225 @@
+/*
+ * net/sched/sch_cbs.c Credit Based Shaper
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * Authors: Vinicius Costa Gomes <vinicius.gomes@intel.com>
+ *
+ */
+
+/* Credit Based Shaper (CBS)
+ =========================
+
+ This is a simple rate-limiting shaper aimed at TSN applications on
+ systems with known traffic workloads.
+
+ Its algorithm is defined by the IEEE 802.1Q-2014 Specification,
+ Section 8.6.8.2, and explained in more detail in the Annex L of the
+ same specification.
+
+ There are four tunables to be considered:
+
+ 'idleslope': Idleslope is the rate of credits that is
+ accumulated (in kilobits per second) when there is at least
+ one packet waiting for transmission. Packets are transmitted
+ when the current value of credits is equal or greater than
+ zero. When there is no packet to be transmitted the amount of
+ credits is set to zero. This is the main tunable of the CBS
+ algorithm.
+
+ 'sendslope':
+ Sendslope is the rate of credits that is depleted (it should be a
+ negative number of kilobits per second) when a transmission is
+ ocurring. It can be calculated as follows, (IEEE 802.1Q-2014 Section
+ 8.6.8.2 item g):
+
+ sendslope = idleslope - port_transmit_rate
+
+ 'hicredit': Hicredit defines the maximum amount of credits (in
+ bytes) that can be accumulated. Hicredit depends on the
+ characteristics of interfering traffic,
+ 'max_interference_size' is the maximum size of any burst of
+ traffic that can delay the transmission of a frame that is
+ available for transmission for this traffic class, (IEEE
+ 802.1Q-2014 Annex L, Equation L-3):
+
+ hicredit = max_interference_size * (idleslope / port_transmit_rate)
+
+ 'locredit': Locredit is the minimum amount of credits that can
+ be reached. It is a function of the traffic flowing through
+ this qdisc (IEEE 802.1Q-2014 Annex L, Equation L-2):
+
+ locredit = max_frame_size * (sendslope / port_transmit_rate)
+*/
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/errno.h>
+#include <linux/skbuff.h>
+#include <net/netlink.h>
+#include <net/sch_generic.h>
+#include <net/pkt_sched.h>
+
+struct cbs_sched_data {
+ s32 queue;
+ s32 locredit;
+ s32 hicredit;
+ s32 sendslope;
+ s32 idleslope;
+};
+
+static int cbs_enqueue(struct sk_buff *skb, struct Qdisc *sch,
+ struct sk_buff **to_free)
+{
+ return qdisc_enqueue_tail(skb, sch);
+}
+
+static const struct nla_policy cbs_policy[TCA_CBS_MAX + 1] = {
+ [TCA_CBS_PARMS] = { .len = sizeof(struct tc_cbs_qopt) },
+};
+
+static int cbs_change(struct Qdisc *sch, struct nlattr *opt)
+{
+ struct cbs_sched_data *q = qdisc_priv(sch);
+ struct tc_cbs_qopt_offload cbs = { };
+ struct nlattr *tb[TCA_CBS_MAX + 1];
+ const struct net_device_ops *ops;
+ struct tc_cbs_qopt *qopt;
+ struct net_device *dev;
+ int err;
+
+ err = nla_parse_nested(tb, TCA_CBS_MAX, opt, cbs_policy, NULL);
+ if (err < 0)
+ return err;
+
+ err = -EINVAL;
+ if (!tb[TCA_CBS_PARMS])
+ goto done;
+
+ qopt = nla_data(tb[TCA_CBS_PARMS]);
+
+ dev = qdisc_dev(sch);
+ ops = dev->netdev_ops;
+
+ cbs.queue = q->queue;
+ cbs.enable = 1;
+ cbs.hicredit = qopt->hicredit;
+ cbs.locredit = qopt->locredit;
+ cbs.idleslope = qopt->idleslope;
+ cbs.sendslope = qopt->sendslope;
+
+ err = -EOPNOTSUPP;
+ if (!ops->ndo_setup_tc)
+ goto done;
+
+ err = ops->ndo_setup_tc(dev, TC_SETUP_CBS, &cbs);
+ if (err < 0)
+ goto done;
+
+ q->hicredit = cbs.hicredit;
+ q->locredit = cbs.locredit;
+ q->idleslope = cbs.idleslope;
+ q->sendslope = cbs.sendslope;
+
+done:
+ return err;
+}
+
+static int cbs_init(struct Qdisc *sch, struct nlattr *opt)
+{
+ struct cbs_sched_data *q = qdisc_priv(sch);
+ struct net_device *dev = qdisc_dev(sch);
+
+ if (!opt)
+ return -EINVAL;
+
+ q->queue = sch->dev_queue - netdev_get_tx_queue(dev, 0);
+
+ return cbs_change(sch, opt);
+}
+
+static void cbs_destroy(struct Qdisc *sch)
+{
+ struct cbs_sched_data *q = qdisc_priv(sch);
+ struct tc_cbs_qopt_offload cbs = { };
+ const struct net_device_ops *ops;
+ struct net_device *dev;
+ int err;
+
+ q->hicredit = 0;
+ q->locredit = 0;
+ q->idleslope = 0;
+ q->sendslope = 0;
+
+ dev = qdisc_dev(sch);
+ ops = dev->netdev_ops;
+
+ if (!ops->ndo_setup_tc)
+ return;
+
+ cbs.queue = q->queue;
+ cbs.enable = 0;
+
+ err = ops->ndo_setup_tc(dev, TC_SETUP_CBS, &cbs);
+ if (err < 0)
+ pr_warn("Couldn't reset queue %d to default values\n",
+ cbs.queue);
+}
+
+static int cbs_dump(struct Qdisc *sch, struct sk_buff *skb)
+{
+ struct cbs_sched_data *q = qdisc_priv(sch);
+ struct nlattr *nest;
+ struct tc_cbs_qopt opt;
+
+ nest = nla_nest_start(skb, TCA_OPTIONS);
+ if (!nest)
+ goto nla_put_failure;
+
+ opt.hicredit = q->hicredit;
+ opt.locredit = q->locredit;
+ opt.sendslope = q->sendslope;
+ opt.idleslope = q->idleslope;
+
+ if (nla_put(skb, TCA_CBS_PARMS, sizeof(opt), &opt))
+ goto nla_put_failure;
+
+ return nla_nest_end(skb, nest);
+
+nla_put_failure:
+ nla_nest_cancel(skb, nest);
+ return -1;
+}
+
+static struct Qdisc_ops cbs_qdisc_ops __read_mostly = {
+ .next = NULL,
+ .id = "cbs",
+ .priv_size = sizeof(struct cbs_sched_data),
+ .enqueue = cbs_enqueue,
+ .dequeue = qdisc_dequeue_head,
+ .peek = qdisc_peek_dequeued,
+ .init = cbs_init,
+ .reset = qdisc_reset_queue,
+ .destroy = cbs_destroy,
+ .change = cbs_change,
+ .dump = cbs_dump,
+ .owner = THIS_MODULE,
+};
+
+static int __init cbs_module_init(void)
+{
+ return register_qdisc(&cbs_qdisc_ops);
+}
+
+static void __exit cbs_module_exit(void)
+{
+ unregister_qdisc(&cbs_qdisc_ops);
+}
+module_init(cbs_module_init)
+module_exit(cbs_module_exit)
+MODULE_LICENSE("GPL");
--
2.14.2
^ permalink raw reply related
* [next-queue PATCH v2 2/5] net/sched: Fix accessing invalid dev_queue
From: Vinicius Costa Gomes @ 2017-09-30 0:26 UTC (permalink / raw)
To: netdev, intel-wired-lan
Cc: Jesus Sanchez-Palencia, jhs, xiyou.wangcong, jiri, andre.guedes,
ivan.briano, boon.leong.ong, richardcochran, henrik, levipearson,
rodney.cummings
In-Reply-To: <20170930002657.15291-1-vinicius.gomes@intel.com>
From: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
In qdisc_alloc() the dev_queue pointer was used without any checks being
performed. If qdisc_create() gets a null dev_queue pointer, it just
passes it along to qdisc_alloc(), leading to a crash. That happens if a
root qdisc implements select_queue() and returns a null dev_queue
pointer for an "invalid handle", for example.
One way to reproduce that is:
1) Setup mqprio
$ tc qdisc replace dev enp3s0 parent root mqprio num_tc 3 \
map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 queues 1@0 1@1 2@2 hw 0
2) Replace the first inner qdisc
$ tc qdisc replace dev enp3s0 parent 8001:1 pfifo_fast
This will lead to the following crash:
[15274.874506] BUG: unable to handle kernel NULL pointer dereference at 0000000000000058
[15274.875044] IP: qdisc_alloc+0xd/0xf0
[15274.875253] PGD 1a37d067 P4D 1a37d067 PUD 1a0cb067 PMD 0
[15274.875566] Oops: 0000 [#1] SMP
[15274.875813] Modules linked in:
[15274.875993] CPU: 0 PID: 2006 Comm: tc Not tainted 4.14.0-rc1+ #42
[15274.876415] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-20170228_101828-anatol 04/01/2014
[15274.877019] task: ffff88001eb91d00 task.stack: ffffc90000734000
[15274.877359] RIP: 0010:qdisc_alloc+0xd/0xf0
[15274.877606] RSP: 0018:ffffc90000737a88 EFLAGS: 00010286
[15274.877904] RAX: ffffffff81ef0da0 RBX: 0000000000000000 RCX: 0000000000000000
[15274.878331] RDX: ffff88001a046830 RSI: ffffffff81ef0da0 RDI: 0000000000000000
[15274.878757] RBP: 0000000001000001 R08: 000000000000006c R09: ffffc90000737b40
[15274.879165] R10: fffffffffffffc20 R11: 0000000000000000 R12: ffff88001e308000
[15274.879571] R13: 0000000000000088 R14: ffffc90000737b40 R15: 0000000000000000
[15274.879978] FS: 00007f790a90a700(0000) GS:ffff88001fc00000(0000) knlGS:0000000000000000
[15274.880457] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[15274.880784] CR2: 0000000000000058 CR3: 000000001a003004 CR4: 00000000003606f0
[15274.881203] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[15274.881620] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[15274.882026] Call Trace:
[15274.882179] qdisc_create+0xb0/0x380
[15274.882388] ? security_capable+0x2e/0x50
[15274.882616] ? nla_parse+0x32/0x100
[15274.882818] tc_modify_qdisc+0x47f/0x560
[15274.883046] rtnetlink_rcv_msg+0x1db/0x200
[15274.883284] ? __skb_try_recv_datagram+0xd6/0x150
[15274.883555] ? rtnl_calcit.isra.21+0xe0/0xe0
[15274.883814] netlink_rcv_skb+0x92/0xf0
[15274.884031] netlink_unicast+0x12c/0x1d0
[15274.884258] netlink_sendmsg+0x2ee/0x330
[15274.884486] sock_sendmsg+0x28/0x40
[15274.884689] ___sys_sendmsg+0x25d/0x280
[15274.884913] ? __alloc_pages_nodemask+0x120/0x180
[15274.885185] ? page_add_new_anon_rmap+0x90/0xb0
[15274.885448] ? __handle_mm_fault+0x74e/0xc40
[15274.885695] ? __sys_sendmsg+0x3e/0x70
[15274.885912] __sys_sendmsg+0x3e/0x70
[15274.886123] entry_SYSCALL_64_fastpath+0x13/0x94
[15274.886387] RIP: 0033:0x7f7909b1a1b7
[15274.886594] RSP: 002b:00007ffe14315028 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[15274.887042] RAX: ffffffffffffffda RBX: 0000000000661860 RCX: 00007f7909b1a1b7
[15274.887450] RDX: 0000000000000000 RSI: 00007ffe143150a0 RDI: 0000000000000003
[15274.887854] RBP: 00007ffe143150a0 R08: 0000000000000001 R09: 0000000000000000
[15274.888256] R10: 00000000000005f1 R11: 0000000000000246 R12: 00007ffe143150e0
[15274.888660] R13: 000000000066e620 R14: 00007ffe1431d180 R15: 0000000000000000
[15274.889070] Code: a8 01 74 09 48 89 df 5b e9 71 ff ff ff 5b c3 f3 c3 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 00 41 55 41 54 55 53 48 89 fb 44 8b 6e 20 <8b> 57 58 48 89 f5 4c 8b 27 be c0 80 40 01 41 8d bd 40 01 00 00
[15274.890154] RIP: qdisc_alloc+0xd/0xf0 RSP: ffffc90000737a88
[15274.890479] CR2: 0000000000000058
[15274.890677] ---[ end trace 6d0e946c11e46095 ]---
Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
---
net/sched/sch_generic.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index a0a198768aad..de2408f1ccd3 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -603,8 +603,14 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,
struct Qdisc *sch;
unsigned int size = QDISC_ALIGN(sizeof(*sch)) + ops->priv_size;
int err = -ENOBUFS;
- struct net_device *dev = dev_queue->dev;
+ struct net_device *dev;
+
+ if (!dev_queue) {
+ err = -EINVAL;
+ goto errout;
+ }
+ dev = dev_queue->dev;
p = kzalloc_node(size, GFP_KERNEL,
netdev_queue_numa_node_read(dev_queue));
--
2.14.2
^ permalink raw reply related
* [next-queue PATCH v2 1/5] mqprio: Implement select_queue class_ops
From: Vinicius Costa Gomes @ 2017-09-30 0:26 UTC (permalink / raw)
To: netdev, intel-wired-lan
Cc: Jesus Sanchez-Palencia, jhs, xiyou.wangcong, jiri, andre.guedes,
ivan.briano, boon.leong.ong, richardcochran, henrik, levipearson,
rodney.cummings
In-Reply-To: <20170930002657.15291-1-vinicius.gomes@intel.com>
From: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
When replacing a child qdisc from mqprio, tc_modify_qdisc() must fetch
the netdev_queue pointer that the current child qdisc is associated
with before creating the new qdisc.
Currently, when using mqprio as root qdisc, the kernel will end up
getting the queue #0 pointer from the mqprio (root qdisc), which leaves
any new child qdisc with a possibly wrong netdev_queue pointer.
Implementing the Qdisc_class_ops select_queue() on mqprio fixes this
issue and avoid an inconsistent state when child qdiscs are replaced.
Signed-off-by: Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
---
net/sched/sch_mqprio.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index 6bcdfe6e7b63..9d762ca4a76c 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c
@@ -396,6 +396,12 @@ static void mqprio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
}
}
+static struct netdev_queue *mqprio_select_queue(struct Qdisc *sch,
+ struct tcmsg *tcm)
+{
+ return mqprio_queue_get(sch, TC_H_MIN(tcm->tcm_parent));
+}
+
static const struct Qdisc_class_ops mqprio_class_ops = {
.graft = mqprio_graft,
.leaf = mqprio_leaf,
@@ -403,6 +409,7 @@ static const struct Qdisc_class_ops mqprio_class_ops = {
.walk = mqprio_walk,
.dump = mqprio_dump_class,
.dump_stats = mqprio_dump_class_stats,
+ .select_queue = mqprio_select_queue,
};
static struct Qdisc_ops mqprio_qdisc_ops __read_mostly = {
--
2.14.2
^ permalink raw reply related
* [next-queue PATCH v2 0/5] TSN: Add qdisc based config interface for CBS
From: Vinicius Costa Gomes @ 2017-09-30 0:26 UTC (permalink / raw)
To: netdev, intel-wired-lan
Cc: Vinicius Costa Gomes, jhs, xiyou.wangcong, jiri, andre.guedes,
ivan.briano, jesus.sanchez-palencia, boon.leong.ong,
richardcochran, henrik, levipearson, rodney.cummings
Hi,
Changes since v1:
- Solved the mqprio dependency;
- Fixed a mqprio bug, that caused the inner qdisc to have a wrong
dev_queue associated with it;
Changes from the RFC:
- Fixed comments from Henrik Austad;
- Simplified the Qdisc, using the generic implementation of callbacks
where possible;
- Small refactor on the driver (igb) code;
This patchset is a proposal of how the Traffic Control subsystem can
be used to offload the configuration of the Credit Based Shaper
(defined in the IEEE 802.1Q-2014 Section 8.6.8.2) into supported
network devices.
As part of this work, we've assessed previous public discussions
related to TSN enabling: patches from Henrik Austad (Cisco), the
presentation from Eric Mann at Linux Plumbers 2012, patches from
Gangfeng Huang (National Instruments) and the current state of the
OpenAVNU project (https://github.com/AVnu/OpenAvnu/).
Overview
========
Time-sensitive Networking (TSN) is a set of standards that aim to
address resources availability for providing bandwidth reservation and
bounded latency on Ethernet based LANs. The proposal described here
aims to cover mainly what is needed to enable the following standards:
802.1Qat and 802.1Qav.
The initial target of this work is the Intel i210 NIC, but other
controllers' datasheet were also taken into account, like the Renesas
RZ/A1H RZ/A1M group and the Synopsis DesignWare Ethernet QoS
controller.
Proposal
========
Feature-wise, what is covered here is the configuration interfaces for
HW implementations of the Credit-Based shaper (CBS, 802.1Qav). CBS is
a per-queue shaper. Given that this feature is related to traffic
shaping, and that the traffic control subsystem already provides a
queueing discipline that offloads config into the device driver (i.e.
mqprio), designing a new qdisc for the specific purpose of offloading
the config for the CBS shaper seemed like a good fit.
For steering traffic into the correct queues, we use the socket option
SO_PRIORITY and then a mechanism to map priority to traffic classes /
Tx queues. The qdisc mqprio is currently used in our tests.
As for the CBS config interface, this patchset is proposing a new
qdisc called 'cbs'. Its 'tc' cmd line is:
$ tc qdisc add dev IFACE parent ID cbs locredit N hicredit M sendslope S \
idleslope I
Note that the parameters for this qdisc are the ones defined by the
802.1Q-2014 spec, so no hardware specific functionality is exposed here.
Testing this RFC
================
Attached to this cover letter are:
- calculate_cbs_params.py: A Python script to calculate the
parameters to the CBS queueing discipline;
- tsn-talker.c: A sample C implementation of the talker side of a stream;
- tsn-listener.c: A sample C implementation of the listener side of a
stream;
For testing the patches of this series, you may want to use the
attached samples to this cover letter and use the 'mqprio' qdisc to
setup the priorities to Tx queues mapping, together with the 'cbs'
qdisc to configure the HW shaper of the i210 controller:
1) Setup priorities to traffic classes to hardware queues mapping
$ tc qdisc replace dev ens4 handle 100: parent root mqprio num_tc 3 \
map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 queues 1@0 1@1 2@2 hw 0
For a more detailed explanation, see mqprio(8), in short, this command
will map traffic with priority 3 to the hardware queue 0, traffic with
priority 2 to hardware queue 1, and the rest will be mapped to
hardware queues 2 and 3.
2) Check scheme. You want to get the inner qdiscs ID from the bottom up
$ tc -g class show dev ens4
Ex.:
+---(100:3) mqprio
| +---(100:6) mqprio
| +---(100:7) mqprio
|
+---(100:2) mqprio
| +---(100:5) mqprio
|
+---(100:1) mqprio
+---(100:4) mqprio
* Here '100:4' is Tx Queue #0 and '100:5' is Tx Queue #1.
3) Calculate CBS parameters for classes A and B. i.e. BW for A is 20Mbps and
for B is 10Mbps:
$ calc_cbs_params.py -A 20000 -a 1500 -B 10000 -b 1500
4) Configure CBS for traffic class A (priority 3) as provided by the script:
$ tc qdisc replace dev ens4 parent 100:4 cbs locredit -1470 \
hicredit 30 sendslope -980000 idleslope 20000
5) Configure CBS for traffic class B (priority 2):
$ tc qdisc replace dev ens4 parent 100:5 cbs \
locredit -1485 hicredit 31 sendslope -990000 idleslope 10000
6) Run Listener:
$ ./tsn-listener -d 01:AA:AA:AA:AA:AA -i ens4 -s 1500
7) Run Talker for class A (prio 3 here), compiled from samples/tsn/talker.c
$ ./tsn-talker -d 01:AA:AA:AA:AA:AA -i ens4 -p 3 -s 1500
* The bandwidth displayed on the listener output at this stage should be very
close to the one configured for class A.
8) You can also run a Talker for class B (prio 2 here and using a
different address):
$ ./tsn-talker -d 01:BB:BB:BB:BB:BB -i ens4 -s 1500
Authors
=======
- Andre Guedes <andre.guedes@intel.com>
- Ivan Briano <ivan.briano@intel.com>
- Jesus Sanchez-Palencia <jesus.sanchez-palencia@intel.com>
- Vinicius Gomes <vinicius.gomes@intel.com>
Andre Guedes (1):
igb: Add support for CBS offload
Vinicius Costa Gomes (2):
net/sched: Introduce the user API for the CBS shaper
net/sched: Introduce Credit Based Shaper (CBS) qdisc
drivers/net/ethernet/intel/igb/e1000_defines.h | 23 ++
drivers/net/ethernet/intel/igb/e1000_regs.h | 8 +
drivers/net/ethernet/intel/igb/igb.h | 6 +
drivers/net/ethernet/intel/igb/igb_main.c | 347 +++++++++++++++++++++++++
include/linux/netdevice.h | 1 +
include/net/pkt_sched.h | 9 +
include/uapi/linux/pkt_sched.h | 17 ++
net/sched/Kconfig | 12 +
net/sched/Makefile | 1 +
net/sched/sch_cbs.c | 229 ++++++++++++++++
10 files changed, 653 insertions(+)
create mode 100644 net/sched/sch_cbs.c
Annex: Sample files
===================
calc_cbs_params.py
--8<---------------cut here---------------start------------->8---
#!/usr/bin/env python
#
# Copyright (c) 2017, Intel Corporation
#
# 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, WHETHER IN CONTRACT, STRICT LIABILITY,
# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
import argparse
import math
def print_cbs_params_for_class_a(args):
idleslope = args.idleslope_a
sendslope = idleslope - args.link_speed
# According to 802.1Q-2014 spec, Annex L, hiCredit and
# loCredit for SR class A are calculated following the
# equations L-10 and L-12, respectively.
hicredit = math.ceil(idleslope * args.frame_non_sr / args.link_speed)
locredit = math.ceil(sendslope * args.frame_a / args.link_speed)
print("tc qdisc add dev <IFNAME> parent <QDISC-ID> cbs idleslope %d sendslope %d hicredit %d locredit %d" % \
(idleslope, sendslope, hicredit, locredit))
def print_cbs_params_for_class_b(args):
idleslope = args.idleslope_b
sendslope = idleslope - args.link_speed
# Annex L doesn't present a straightforward equation to
# calculate hiCredit for Class B so we have to derive it
# based on generic equations presented in that Annex.
#
# L-3 is the primary equation to calculate hiCredit. Section
# L.2 states that the 'maxInterferenceSize' for SR class B
# is the maximum burst size for SR class A plus the
# maxInterferenceSize from SR class A (which is equal to the
# maximum frame from non-SR traffic).
#
# The maximum burst size for SR class A equation is shown in
# L-16. Merging L-16 into L-3 we get the resulting equation
# which calculates hiCredit B (refer to section L.3 in case
# you're not familiar with the legend):
#
# hiCredit B = Rb * ( Mo Ma )
# ---------- + ------
# Ro - Ra Ro
#
hicredit = math.ceil(idleslope * \
((args.frame_non_sr / (args.link_speed - args.idleslope_a)) + \
(args.frame_a / args.link_speed)))
# loCredit B is calculated following equation L-2.
locredit = math.ceil(sendslope * args.frame_b / args.link_speed)
print("tc qdisc add dev <IFNAME> parent <QDISC-ID> cbs idleslope %d sendslope %d hicredit %d locredit %d" % \
(idleslope, sendslope, hicredit, locredit))
def main():
parser = argparse.ArgumentParser()
parser.add_argument('-S', dest='link_speed', default=1000000.0, type=float,
help='Link speed in kbps')
parser.add_argument('-s', dest='frame_non_sr', default=1500.0, type=float,
help='Maximum frame size from non-SR traffic (MTU size'
'usually')
parser.add_argument('-A', dest='idleslope_a', default=0, type=float,
help='Idleslope for SR class A in kbps')
parser.add_argument('-a', dest='frame_a', default=0, type=float,
help='Maximum frame size for SR class A traffic')
parser.add_argument('-B', dest='idleslope_b', default=0, type=float,
help='Idleslope for SR class B in kbps')
parser.add_argument('-b', dest='frame_b', default=0, type=float,
help='Maximum frame size for SR class B traffic')
args = parser.parse_args()
if args.idleslope_a > 0:
print_cbs_params_for_class_a(args)
if args.idleslope_b > 0:
print_cbs_params_for_class_b(args)
if __name__ == "__main__":
main()
--8<---------------cut here---------------end--------------->8---
tsn-talker.c
--8<---------------cut here---------------start------------->8---
/*
* Copyright (c) 2017, Intel Corporation
*
* 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, WHETHER IN CONTRACT,
* STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
* OF THE POSSIBILITY OF SUCH DAMAGE.
*/
#include <alloca.h>
#include <argp.h>
#include <arpa/inet.h>
#include <inttypes.h>
#include <linux/if.h>
#include <linux/if_ether.h>
#include <linux/if_packet.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ioctl.h>
#include <unistd.h>
#define MAGIC 0xCC
static uint8_t ifname[IFNAMSIZ];
static uint8_t macaddr[ETH_ALEN];
static int priority = -1;
static size_t size = 1500;
static uint64_t seq;
static int delay = -1;
static struct argp_option options[] = {
{"dst-addr", 'd', "MACADDR", 0, "Stream Destination MAC address" },
{"delay", 'D', "NUM", 0, "Delay (in us) between packet transmission" },
{"ifname", 'i', "IFNAME", 0, "Network Interface" },
{"prio", 'p', "NUM", 0, "SO_PRIORITY to be set in socket" },
{"packet-size", 's', "NUM", 0, "Size of packets to be transmitted" },
{ 0 }
};
static error_t parser(int key, char *arg, struct argp_state *state)
{
int res;
switch (key) {
case 'd':
res = sscanf(arg, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
&macaddr[0], &macaddr[1], &macaddr[2],
&macaddr[3], &macaddr[4], &macaddr[5]);
if (res != 6) {
printf("Invalid address\n");
exit(EXIT_FAILURE);
}
break;
case 'D':
delay = atoi(arg);
break;
case 'i':
strncpy(ifname, arg, sizeof(ifname) - 1);
break;
case 'p':
priority = atoi(arg);
break;
case 's':
size = atoi(arg);
break;
}
return 0;
}
static struct argp argp = { options, parser };
int main(int argc, char *argv[])
{
int fd, res;
struct ifreq req;
uint8_t *data;
struct sockaddr_ll sk_addr = {
.sll_family = AF_PACKET,
.sll_protocol = htons(ETH_P_TSN),
.sll_halen = ETH_ALEN,
};
argp_parse(&argp, argc, argv, 0, NULL, NULL);
fd = socket(AF_PACKET, SOCK_DGRAM, htons(ETH_P_TSN));
if (fd < 0) {
perror("Couldn't open socket");
return 1;
}
strncpy(req.ifr_name, ifname, sizeof(req.ifr_name));
res = ioctl(fd, SIOCGIFINDEX, &req);
if (res < 0) {
perror("Couldn't get interface index");
goto err;
}
sk_addr.sll_ifindex = req.ifr_ifindex;
memcpy(&sk_addr.sll_addr, macaddr, ETH_ALEN);
if (priority != -1) {
res = setsockopt(fd, SOL_SOCKET, SO_PRIORITY, &priority,
sizeof(priority));
if (res < 0) {
perror("Couldn't set priority");
goto err;
}
}
data = alloca(size);
memset(data, MAGIC, size);
printf("Sending packets...\n");
while (1) {
uint64_t *seq_ptr = (uint64_t *) &data[0];
ssize_t n;
*seq_ptr = seq++;
n = sendto(fd, data, size, 0, (struct sockaddr *) &sk_addr,
sizeof(sk_addr));
if (n < 0)
perror("Failed to send data");
if (delay > 0)
usleep(delay);
}
close(fd);
return 0;
err:
close(fd);
return 1;
}
--8<---------------cut here---------------end--------------->8---
tsn-listener.c
--8<---------------cut here---------------start------------->8---
/*
* Copyright (c) 2017, Intel Corporation
*
* 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, WHETHER IN CONTRACT,
* STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
* OF THE POSSIBILITY OF SUCH DAMAGE.
*/
#include <alloca.h>
#include <argp.h>
#include <arpa/inet.h>
#include <inttypes.h>
#include <linux/if.h>
#include <linux/if_ether.h>
#include <linux/if_packet.h>
#include <poll.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/ioctl.h>
#include <sys/timerfd.h>
#include <unistd.h>
static uint8_t ifname[IFNAMSIZ];
static uint8_t macaddr[ETH_ALEN];
static uint64_t data_count;
static int size = 1500;
static time_t interval = 1;
static bool check_seq = false;
static uint64_t expected_seq;
static struct argp_option options[] = {
{"check-seq", 'c', NULL, 0, "Check sequence number within packet" },
{"dst-addr", 'd', "MACADDR", 0, "Stream Destination MAC address" },
{"ifname", 'i', "IFNAME", 0, "Network Interface" },
{"interval", 'I', "SEC", 0, "Interval between bandwidth reports" },
{"packet-size", 's', "NUM", 0, "Expected packet size" },
{ 0 }
};
static error_t parser(int key, char *arg, struct argp_state *state)
{
int res;
switch (key) {
case 'c':
check_seq = true;
break;
case 'd':
res = sscanf(arg, "%hhx:%hhx:%hhx:%hhx:%hhx:%hhx",
&macaddr[0], &macaddr[1], &macaddr[2],
&macaddr[3], &macaddr[4], &macaddr[5]);
if (res != 6) {
printf("Invalid address\n");
exit(EXIT_FAILURE);
}
break;
case 'i':
strncpy(ifname, arg, sizeof(ifname) - 1);
break;
case 'I':
interval = atoi(arg);
break;
case 's':
size = atoi(arg);
break;
}
return 0;
}
static struct argp argp = { options, parser };
static int setup_timer(void)
{
int fd, res;
struct itimerspec tspec = { 0 };
fd = timerfd_create(CLOCK_MONOTONIC, 0);
if (fd < 0) {
perror("Couldn't create timer");
return -1;
}
tspec.it_value.tv_sec = interval;
tspec.it_interval.tv_sec = interval;
res = timerfd_settime(fd, 0, &tspec, NULL);
if (res < 0) {
perror("Couldn't set timer");
close(fd);
return -1;
}
return fd;
}
static int setup_socket(void)
{
int fd, res;
struct sockaddr_ll sk_addr = {
.sll_family = AF_PACKET,
.sll_protocol = htons(ETH_P_TSN),
};
fd = socket(AF_PACKET, SOCK_DGRAM, htons(ETH_P_TSN));
if (fd < 0) {
perror("Couldn't open socket");
return -1;
}
/* If user provided a network interface, bind() to it. */
if (ifname[0] != '\0') {
struct ifreq req;
strncpy(req.ifr_name, ifname, sizeof(req.ifr_name));
res = ioctl(fd, SIOCGIFINDEX, &req);
if (res < 0) {
perror("Couldn't get interface index");
goto err;
}
sk_addr.sll_ifindex = req.ifr_ifindex;
res = bind(fd, (struct sockaddr *) &sk_addr, sizeof(sk_addr));
if (res < 0) {
perror("Couldn't bind() to interface");
goto err;
}
}
/* If user provided the stream destination address, set it as multicast
* address.
*/
if (macaddr[0] != '\0') {
struct packet_mreq mreq;
mreq.mr_ifindex = sk_addr.sll_ifindex;
mreq.mr_type = PACKET_MR_MULTICAST;
mreq.mr_alen = ETH_ALEN;
memcpy(&mreq.mr_address, macaddr, ETH_ALEN);
res = setsockopt(fd, SOL_PACKET, PACKET_ADD_MEMBERSHIP,
&mreq, sizeof(struct packet_mreq));
if (res < 0) {
perror("Couldn't set PACKET_ADD_MEMBERSHIP");
goto err;
}
}
return fd;
err:
close(fd);
return -1;
}
static void recv_packet(int fd)
{
uint8_t *data = alloca(size);
ssize_t n = recv(fd, data, size, 0);
if (n < 0) {
perror("Failed to receive data");
return;
}
if (n != size)
printf("Size mismatch: expected %d, got %d\n", size, n);
if (check_seq) {
uint64_t *seq = (uint64_t *) &data[0];
/* If 'expected_seq' is equal to zero, it means this is the
* first packet we received so we don't know what sequence
* number to expect.
*/
if (expected_seq == 0)
expected_seq = *seq;
if (*seq != expected_seq) {
printf("Sequence mismatch: expected %llu, got %llu\n",
expected_seq, *seq);
expected_seq = *seq;
}
expected_seq++;
}
data_count += n;
}
static void report_bw(int fd)
{
uint64_t expirations;
ssize_t n = read(fd, &expirations, sizeof(uint64_t));
if (n < 0) {
perror("Couldn't read timerfd");
return;
}
if (expirations != 1)
printf("Some went wrong with timerfd\n");
printf("Receiving data rate: %llu kbps\n", (data_count * 8) / (1000 * interval));
data_count = 0;
}
int main(int argc, char *argv[])
{
int sk_fd, timer_fd, res;
struct pollfd fds[2];
argp_parse(&argp, argc, argv, 0, NULL, NULL);
sk_fd = setup_socket();
if (sk_fd < 0)
return 1;
timer_fd = setup_timer();
if (timer_fd < 0) {
close(sk_fd);
return 1;
}
fds[0].fd = sk_fd;
fds[0].events = POLLIN;
fds[1].fd = timer_fd;
fds[1].events = POLLIN;
printf("Waiting for packets...\n");
while (1) {
res = poll(fds, 2, -1);
if (res < 0) {
perror("Error on poll()");
goto err;
}
if (fds[0].revents & POLLIN)
recv_packet(fds[0].fd);
if (fds[1].revents & POLLIN) {
report_bw(fds[1].fd);
}
}
close(timer_fd);
close(sk_fd);
return 0;
err:
close(timer_fd);
close(sk_fd);
return 1;
}
--8<---------------cut here---------------end--------------->8---
^ permalink raw reply
* [PATCH 2/2] userns: control capabilities of some user namespaces
From: Mahesh Bandewar @ 2017-09-29 23:10 UTC (permalink / raw)
To: LKML
Cc: Netdev, Kernel-hardening, Linux API, Kees Cook, Serge Hallyn,
Eric W . Biederman, Eric Dumazet, David Miller, Mahesh Bandewar,
Mahesh Bandewar
From: Mahesh Bandewar <maheshb@google.com>
With this new notion of "controlled" user-namespaces, the controlled
user-namespaces are marked at the time of their creation while the
capabilities of processes that belong to them are controlled using the
global mask.
Init-user-ns is always uncontrolled and a process that has SYS_ADMIN
that belongs to uncontrolled user-ns can create another (child) user-
namespace that is uncontrolled. Any other process (that either does
not have SYS_ADMIN or belongs to a controlled user-ns) can only
create a user-ns that is controlled.
global-capability-whitelist (controlled_userns_caps_whitelist) is used
at the capability check-time and keeps the semantics for the processes
that belong to uncontrolled user-ns as it is. Processes that belong to
controlled user-ns however are subjected to different checks-
(a) if the capability in question is controlled and process belongs
to controlled user-ns, then it's always denied.
(b) if the capability in question is NOT controlled then fall back
to the traditional check.
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
include/linux/capability.h | 1 +
include/linux/user_namespace.h | 20 ++++++++++++++++++++
kernel/capability.c | 5 +++++
kernel/user_namespace.c | 3 +++
security/commoncap.c | 8 ++++++++
5 files changed, 37 insertions(+)
diff --git a/include/linux/capability.h b/include/linux/capability.h
index 6c0b9677c03f..b8c6cac18658 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -250,6 +250,7 @@ extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns);
extern int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data *cpu_caps);
int proc_douserns_caps_whitelist(struct ctl_table *table, int write,
void __user *buff, size_t *lenp, loff_t *ppos);
+bool is_capability_controlled(int cap);
extern int cap_convert_nscap(struct dentry *dentry, void **ivalue, size_t size);
diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
index c18e01252346..e890fe81b47e 100644
--- a/include/linux/user_namespace.h
+++ b/include/linux/user_namespace.h
@@ -22,6 +22,7 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */
};
#define USERNS_SETGROUPS_ALLOWED 1UL
+#define USERNS_CONTROLLED 2UL
#define USERNS_INIT_FLAGS USERNS_SETGROUPS_ALLOWED
@@ -102,6 +103,16 @@ static inline void put_user_ns(struct user_namespace *ns)
__put_user_ns(ns);
}
+static inline bool is_user_ns_controlled(const struct user_namespace *ns)
+{
+ return ns->flags & USERNS_CONTROLLED;
+}
+
+static inline void mark_user_ns_controlled(struct user_namespace *ns)
+{
+ ns->flags |= USERNS_CONTROLLED;
+}
+
struct seq_operations;
extern const struct seq_operations proc_uid_seq_operations;
extern const struct seq_operations proc_gid_seq_operations;
@@ -160,6 +171,15 @@ static inline struct ns_common *ns_get_owner(struct ns_common *ns)
{
return ERR_PTR(-EPERM);
}
+
+static inline bool is_user_ns_controlled(const struct user_namespace *ns)
+{
+ return false;
+}
+
+static inline void mark_user_ns_controlled(struct user_namespace *ns)
+{
+}
#endif
#endif /* _LINUX_USER_H */
diff --git a/kernel/capability.c b/kernel/capability.c
index 62dbe3350c1b..40a38cc4ff43 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -510,6 +510,11 @@ bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns)
}
/* Controlled-userns capabilities routines */
+bool is_capability_controlled(int cap)
+{
+ return !cap_raised(controlled_userns_caps_whitelist, cap);
+}
+
#ifdef CONFIG_SYSCTL
int proc_douserns_caps_whitelist(struct ctl_table *table, int write,
void __user *buff, size_t *lenp, loff_t *ppos)
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index c490f1e4313b..f393ea5108f0 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -53,6 +53,9 @@ static void set_cred_user_ns(struct cred *cred, struct user_namespace *user_ns)
cred->cap_effective = CAP_FULL_SET;
cred->cap_ambient = CAP_EMPTY_SET;
cred->cap_bset = CAP_FULL_SET;
+ if (!ns_capable(user_ns->parent, CAP_SYS_ADMIN) ||
+ is_user_ns_controlled(user_ns->parent))
+ mark_user_ns_controlled(user_ns);
#ifdef CONFIG_KEYS
key_put(cred->request_key_auth);
cred->request_key_auth = NULL;
diff --git a/security/commoncap.c b/security/commoncap.c
index 6bf72b175b49..26f41602da10 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -73,6 +73,14 @@ int cap_capable(const struct cred *cred, struct user_namespace *targ_ns,
{
struct user_namespace *ns = targ_ns;
+ /* If the capability is controlled and user-ns that process
+ * belongs-to is 'controlled' then return EPERM and no need
+ * to check the user-ns hierarchy.
+ */
+ if (is_user_ns_controlled(cred->user_ns) &&
+ is_capability_controlled(cap))
+ return -EPERM;
+
/* See if cred has the capability in the target user namespace
* by examining the target user namespace and all of the target
* user namespace's parents.
--
2.14.2.822.g60be5d43e6-goog
^ permalink raw reply related
* [PATCH 1/2] capability: introduce sysctl for controlled user-ns capability whitelist
From: Mahesh Bandewar @ 2017-09-29 23:10 UTC (permalink / raw)
To: LKML
Cc: Netdev, Kernel-hardening, Linux API, Kees Cook, Serge Hallyn,
Eric W . Biederman, Eric Dumazet, David Miller, Mahesh Bandewar,
Mahesh Bandewar
From: Mahesh Bandewar <maheshb@google.com>
Add a sysctl variable kernel.controlled_userns_caps_whitelist. This
takes input as capability mask expressed as two comma separated hex
u32 words. The mask, however, is stored in kernel as kernel_cap_t type.
Any capabilities that are not part of this mask will be controlled and
will not be allowed to processes in controlled user-ns.
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
Documentation/sysctl/kernel.txt | 21 ++++++++++++++++++
include/linux/capability.h | 3 +++
kernel/capability.c | 47 +++++++++++++++++++++++++++++++++++++++++
kernel/sysctl.c | 5 +++++
4 files changed, 76 insertions(+)
diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 694968c7523c..a1d39dbae847 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -25,6 +25,7 @@ show up in /proc/sys/kernel:
- bootloader_version [ X86 only ]
- callhome [ S390 only ]
- cap_last_cap
+- controlled_userns_caps_whitelist
- core_pattern
- core_pipe_limit
- core_uses_pid
@@ -187,6 +188,26 @@ CAP_LAST_CAP from the kernel.
==============================================================
+controlled_userns_caps_whitelist
+
+Capability mask that is whitelisted for "controlled" user namespaces.
+Any capability that is missing from this mask will not be allowed to
+any process that is attached to a controlled-userns. e.g. if CAP_NET_RAW
+is not part of this mask, then processes running inside any controlled
+userns's will not be allowed to perform action that needs CAP_NET_RAW
+capability. However, processes that are attached to a parent user-ns
+hierarchy that is *not* controlled and has CAP_NET_RAW can continue
+performing those actions. User-namespaces are marked "controlled" at
+the time of their creation based on the capabilities of the creator.
+A process that does not have CAP_SYS_ADMIN will create user-namespaces
+that are controlled.
+
+The value is expressed as two comma separated hex words (u32). This
+sysctl is avaialble in init-ns and users with CAP_SYS_ADMIN in init-ns
+are allowed to make changes.
+
+==============================================================
+
core_pattern:
core_pattern is used to specify a core dumpfile pattern name.
diff --git a/include/linux/capability.h b/include/linux/capability.h
index b52e278e4744..6c0b9677c03f 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -13,6 +13,7 @@
#define _LINUX_CAPABILITY_H
#include <uapi/linux/capability.h>
+#include <linux/sysctl.h>
#define _KERNEL_CAPABILITY_VERSION _LINUX_CAPABILITY_VERSION_3
@@ -247,6 +248,8 @@ extern bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns);
/* audit system wants to get cap info from files as well */
extern int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data *cpu_caps);
+int proc_douserns_caps_whitelist(struct ctl_table *table, int write,
+ void __user *buff, size_t *lenp, loff_t *ppos);
extern int cap_convert_nscap(struct dentry *dentry, void **ivalue, size_t size);
diff --git a/kernel/capability.c b/kernel/capability.c
index f97fe77ceb88..62dbe3350c1b 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -28,6 +28,8 @@ EXPORT_SYMBOL(__cap_empty_set);
int file_caps_enabled = 1;
+kernel_cap_t controlled_userns_caps_whitelist = CAP_FULL_SET;
+
static int __init file_caps_disable(char *str)
{
file_caps_enabled = 0;
@@ -506,3 +508,48 @@ bool ptracer_capable(struct task_struct *tsk, struct user_namespace *ns)
rcu_read_unlock();
return (ret == 0);
}
+
+/* Controlled-userns capabilities routines */
+#ifdef CONFIG_SYSCTL
+int proc_douserns_caps_whitelist(struct ctl_table *table, int write,
+ void __user *buff, size_t *lenp, loff_t *ppos)
+{
+ DECLARE_BITMAP(caps_bitmap, CAP_LAST_CAP);
+ struct ctl_table caps_table;
+ char tbuf[NAME_MAX];
+ int ret;
+
+ ret = bitmap_from_u32array(caps_bitmap, CAP_LAST_CAP,
+ controlled_userns_caps_whitelist.cap,
+ _KERNEL_CAPABILITY_U32S);
+ if (ret != CAP_LAST_CAP)
+ return -1;
+
+ scnprintf(tbuf, NAME_MAX, "%*pb", CAP_LAST_CAP, caps_bitmap);
+
+ caps_table.data = tbuf;
+ caps_table.maxlen = NAME_MAX;
+ caps_table.mode = table->mode;
+ ret = proc_dostring(&caps_table, write, buff, lenp, ppos);
+ if (ret)
+ return ret;
+ if (write) {
+ kernel_cap_t tmp;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ ret = bitmap_parse_user(buff, *lenp, caps_bitmap, CAP_LAST_CAP);
+ if (ret)
+ return ret;
+
+ ret = bitmap_to_u32array(tmp.cap, _KERNEL_CAPABILITY_U32S,
+ caps_bitmap, CAP_LAST_CAP);
+ if (ret != CAP_LAST_CAP)
+ return -1;
+
+ controlled_userns_caps_whitelist = tmp;
+ }
+ return 0;
+}
+#endif /* CONFIG_SYSCTL */
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 6648fbbb8157..9903cf0de287 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1229,6 +1229,11 @@ static struct ctl_table kern_table[] = {
.extra2 = &one,
},
#endif
+ {
+ .procname = "controlled_userns_caps_whitelist",
+ .mode = 0644,
+ .proc_handler = proc_douserns_caps_whitelist,
+ },
{ }
};
--
2.14.2.822.g60be5d43e6-goog
^ permalink raw reply related
* [PATCH 0/2] capability controlled user-namespaces
From: Mahesh Bandewar @ 2017-09-29 23:09 UTC (permalink / raw)
To: LKML
Cc: Netdev, Kernel-hardening, Linux API, Kees Cook, Serge Hallyn,
Eric W . Biederman, Eric Dumazet, David Miller, Mahesh Bandewar,
Mahesh Bandewar
From: Mahesh Bandewar <maheshb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
[Same as the previous RFC series sent on 9/21]
TL;DR version
-------------
Creating a sandbox environment with namespaces is challenging
considering what these sandboxed processes can engage into. e.g.
CVE-2017-6074, CVE-2017-7184, CVE-2017-7308 etc. just to name few.
Current form of user-namespaces, however, if changed a bit can allow
us to create a sandbox environment without locking down user-
namespaces.
Detailed version
----------------
Problem
-------
User-namespaces in the current form have increased the attack surface as
any process can acquire capabilities which are not available to them (by
default) by performing combination of clone()/unshare()/setns() syscalls.
#define _GNU_SOURCE
#include <stdio.h>
#include <sched.h>
#include <netinet/in.h>
int main(int ac, char **av)
{
int sock = -1;
printf("Attempting to open RAW socket before unshare()...\n");
sock = socket(AF_INET6, SOCK_RAW, IPPROTO_RAW);
if (sock < 0) {
perror("socket() SOCK_RAW failed: ");
} else {
printf("Successfully opened RAW-Sock before unshare().\n");
close(sock);
sock = -1;
}
if (unshare(CLONE_NEWUSER | CLONE_NEWNET) < 0) {
perror("unshare() failed: ");
return 1;
}
printf("Attempting to open RAW socket after unshare()...\n");
sock = socket(AF_INET6, SOCK_RAW, IPPROTO_RAW);
if (sock < 0) {
perror("socket() SOCK_RAW failed: ");
} else {
printf("Successfully opened RAW-Sock after unshare().\n");
close(sock);
sock = -1;
}
return 0;
}
The above example shows how easy it is to acquire NET_RAW capabilities
and once acquired, these processes could take benefit of above mentioned
or similar issues discovered/undiscovered with malicious intent. Note
that this is just an example and the problem/solution is not limited
to NET_RAW capability *only*.
The easiest fix one can apply here is to lock-down user-namespaces which
many of the distros do (i.e. don't allow users to create user namespaces),
but unfortunately that prevents everyone from using them.
Approach
--------
Introduce a notion of 'controlled' user-namespaces. Every process on
the host is allowed to create user-namespaces (governed by the limit
imposed by per-ns sysctl) however, mark user-namespaces created by
sandboxed processes as 'controlled'. Use this 'mark' at the time of
capability check in conjunction with a global capability whitelist.
If the capability is not whitelisted, processes that belong to
controlled user-namespaces will not be allowed.
Once a user-ns is marked as 'controlled'; all its child user-
namespaces are marked as 'controlled' too.
A global whitelist is list of capabilities governed by the
sysctl which is available to (privileged) user in init-ns to modify
while it's applicable to all controlled user-namespaces on the host.
Marking user-namespaces controlled without modifying the whitelist is
equivalent of the current behavior. The default value of whitelist includes
all capabilities so that the compatibility is maintained. However it gives
admins fine-grained ability to control various capabilities system wide
without locking down user-namespaces.
Please see individual patches in this series.
Mahesh Bandewar (2):
capability: introduce sysctl for controlled user-ns capability
whitelist
userns: control capabilities of some user namespaces
Documentation/sysctl/kernel.txt | 21 +++++++++++++++++
include/linux/capability.h | 4 ++++
include/linux/user_namespace.h | 20 ++++++++++++++++
kernel/capability.c | 52 +++++++++++++++++++++++++++++++++++++++++
kernel/sysctl.c | 5 ++++
kernel/user_namespace.c | 3 +++
security/commoncap.c | 8 +++++++
7 files changed, 113 insertions(+)
--
2.14.2.822.g60be5d43e6-goog
^ permalink raw reply
* Re: [PATCH net-next] net: bridge: add per-port group_fwd_mask with less restrictions
From: Roopa Prabhu @ 2017-09-29 22:21 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: Stephen Hemminger, netdev@vger.kernel.org, bridge
In-Reply-To: <5dd9e4e8-466d-47dc-1257-09823ba783b9@cumulusnetworks.com>
On Fri, Sep 29, 2017 at 3:11 PM, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> On 30/09/17 00:51, Stephen Hemminger wrote:
>> On Sat, 30 Sep 2017 00:01:24 +0300
>> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>>
>>> On 29/09/17 18:14, Stephen Hemminger wrote:
>>>> On Wed, 27 Sep 2017 16:12:44 +0300
>>>> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>>>>
>>>>> We need to be able to transparently forward most link-local frames via
>>>>> tunnels (e.g. vxlan, qinq). Currently the bridge's group_fwd_mask has a
>>>>> mask which restricts the forwarding of STP and LACP, but we need to be able
>>>>> to forward these over tunnels and control that forwarding on a per-port
>>>>> basis thus add a new per-port group_fwd_mask option which only disallows
>>>>> mac pause frames to be forwarded (they're always dropped anyway).
>>>>> The patch does not change the current default situation - all of the others
>>>>> are still restricted unless configured for forwarding.
>>>>> We have successfully tested this patch with LACP and STP forwarding over
>>>>> VxLAN and qinq tunnels.
>>>>>
>>>>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>>>
>>>> LACP is fine, but STP must not be forwarded if STP in user or kernel
>>>> mode is enabled.
>>>>
>>>> Please update this patch or revert it.
>>>>
>>>
>>> The default has not changed, STP is still _not_ forwarded. It can be only if explicitly
>>> requested by the user and that means the port might be participating in STP but not
>>> the bridge's STP, that is explicitly forward all STP frames from that port.
>>> I don't think we have to change anything.
>>>
>>
>> You need to fail if user does something stupid. And DNR.
>>
>
> I get the motivation, but it does not have to be stupid. It may be 802.1q in q, not 802.1ad
> where STP is already forwarded by default, or it may be that the port is participating
> in a different STP. Wouldn't be enough to warn the user that STP forwarding was enabled
> for that port, instead of forcing it off and having it only for 802.1ad ?
> Later when we upstream 802.1Q in q patches we'll have to work around that anyway.
>
>> From 16c42db4852234c13288922c1bffbf7650e08434 Mon Sep 17 00:00:00 2001
>> From: Stephen Hemminger <sthemmin@microsoft.com>
>> Date: Fri, 29 Sep 2017 14:48:17 -0700
>> Subject: [PATCH] bridge: check for attempt to forward STP PDU's with STP
>> enabled
>>
>> Move validation into set function and do not allow
>> configuring forwarding of STP packets if STP is enabled.
>>
>> Fixes: 5af48b59f35c ("net: bridge: add per-port group_fwd_mask with less restrictions")
>
> What if the user requested explicitly to forward these frames from that port ?
>
exactly and this could be on a provider bridge which is just tunneling
stp frames from a customer.
^ permalink raw reply
* Re: [PATCH net-next] net: bridge: add per-port group_fwd_mask with less restrictions
From: Nikolay Aleksandrov @ 2017-09-29 22:11 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, roopa, bridge
In-Reply-To: <20170929145103.1e30f73c@xeon-e3>
On 30/09/17 00:51, Stephen Hemminger wrote:
> On Sat, 30 Sep 2017 00:01:24 +0300
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>
>> On 29/09/17 18:14, Stephen Hemminger wrote:
>>> On Wed, 27 Sep 2017 16:12:44 +0300
>>> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
>>>
>>>> We need to be able to transparently forward most link-local frames via
>>>> tunnels (e.g. vxlan, qinq). Currently the bridge's group_fwd_mask has a
>>>> mask which restricts the forwarding of STP and LACP, but we need to be able
>>>> to forward these over tunnels and control that forwarding on a per-port
>>>> basis thus add a new per-port group_fwd_mask option which only disallows
>>>> mac pause frames to be forwarded (they're always dropped anyway).
>>>> The patch does not change the current default situation - all of the others
>>>> are still restricted unless configured for forwarding.
>>>> We have successfully tested this patch with LACP and STP forwarding over
>>>> VxLAN and qinq tunnels.
>>>>
>>>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>>>
>>> LACP is fine, but STP must not be forwarded if STP in user or kernel
>>> mode is enabled.
>>>
>>> Please update this patch or revert it.
>>>
>>
>> The default has not changed, STP is still _not_ forwarded. It can be only if explicitly
>> requested by the user and that means the port might be participating in STP but not
>> the bridge's STP, that is explicitly forward all STP frames from that port.
>> I don't think we have to change anything.
>>
>
> You need to fail if user does something stupid. And DNR.
>
I get the motivation, but it does not have to be stupid. It may be 802.1q in q, not 802.1ad
where STP is already forwarded by default, or it may be that the port is participating
in a different STP. Wouldn't be enough to warn the user that STP forwarding was enabled
for that port, instead of forcing it off and having it only for 802.1ad ?
Later when we upstream 802.1Q in q patches we'll have to work around that anyway.
> From 16c42db4852234c13288922c1bffbf7650e08434 Mon Sep 17 00:00:00 2001
> From: Stephen Hemminger <sthemmin@microsoft.com>
> Date: Fri, 29 Sep 2017 14:48:17 -0700
> Subject: [PATCH] bridge: check for attempt to forward STP PDU's with STP
> enabled
>
> Move validation into set function and do not allow
> configuring forwarding of STP packets if STP is enabled.
>
> Fixes: 5af48b59f35c ("net: bridge: add per-port group_fwd_mask with less restrictions")
What if the user requested explicitly to forward these frames from that port ?
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> net/bridge/br_if.c | 13 +++++++++++++
> net/bridge/br_netlink.c | 6 +++---
> net/bridge/br_private.h | 1 +
> net/bridge/br_sysfs_if.c | 6 +-----
> 4 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index f3aef22931ab..a30e12f76266 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -654,3 +654,16 @@ void br_port_flags_change(struct net_bridge_port *p, unsigned long mask)
> if (mask & BR_AUTO_MASK)
> nbp_update_port_count(br);
> }
> +
> +int br_set_group_fwd_mask(struct net_bridge_port *p, u16 fwd_mask)
> +{
> + if (fwd_mask & BR_GROUPFWD_MACPAUSE)
> + return -EINVAL;
> +
> + if ((fwd_mask & BR_GROUPFWD_STP) &&
> + (p->br->stp_enabled != BR_NO_STP))
> + return -EINVAL;
> +
> + p->group_fwd_mask = fwd_mask;
> + return 0;
> +}
> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
> index dea88a255d26..7b16819ecb59 100644
> --- a/net/bridge/br_netlink.c
> +++ b/net/bridge/br_netlink.c
> @@ -780,9 +780,9 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
> if (tb[IFLA_BRPORT_GROUP_FWD_MASK]) {
> u16 fwd_mask = nla_get_u16(tb[IFLA_BRPORT_GROUP_FWD_MASK]);
>
> - if (fwd_mask & BR_GROUPFWD_MACPAUSE)
> - return -EINVAL;
> - p->group_fwd_mask = fwd_mask;
> + err = br_set_group_fwd_mask(p, fwd_mask);
> + if (err)
> + return err;
> }
>
> br_port_flags_change(p, old_flags ^ p->flags);
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 020c709a017f..734933bebb08 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -573,6 +573,7 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
> netdev_features_t features);
> void br_port_flags_change(struct net_bridge_port *port, unsigned long mask);
> void br_manage_promisc(struct net_bridge *br);
> +int br_set_group_fwd_mask(struct net_bridge_port *p, u16 mask);
>
> /* br_input.c */
> int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb);
> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> index 9110d5e56085..f5871be10b24 100644
> --- a/net/bridge/br_sysfs_if.c
> +++ b/net/bridge/br_sysfs_if.c
> @@ -173,11 +173,7 @@ static ssize_t show_group_fwd_mask(struct net_bridge_port *p, char *buf)
> static int store_group_fwd_mask(struct net_bridge_port *p,
> unsigned long v)
> {
> - if (v & BR_GROUPFWD_MACPAUSE)
> - return -EINVAL;
> - p->group_fwd_mask = v;
> -
> - return 0;
> + return br_set_group_fwd_mask(p, v);
> }
> static BRPORT_ATTR(group_fwd_mask, S_IRUGO | S_IWUSR, show_group_fwd_mask,
> store_group_fwd_mask);
>
^ permalink raw reply
* Re: [PATCH next] bonding: speed/duplex update at NETDEV_UP event
From: Mahesh Bandewar (महेश बंडेवार) @ 2017-09-29 22:02 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Mahesh Bandewar, Jay Vosburgh, Andy Gospodarek, Veaceslav Falico,
David Miller, Netdev
In-Reply-To: <20170929130819.74879e99@xeon-e3>
On Fri, Sep 29, 2017 at 1:08 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Wed, 27 Sep 2017 18:03:49 -0700
> Mahesh Bandewar <mahesh@bandewar.net> wrote:
>
>> From: Mahesh Bandewar <maheshb@google.com>
>>
>> Some NIC drivers don't have correct speed/duplex settings at the
>> time they send NETDEV_UP notification and that messes up the
>> bonding state. Especially 802.3ad mode which is very sensitive
>> to these settings. In the current implementation we invoke
>> bond_update_speed_duplex() when we receive NETDEV_UP, however,
>> ignore the return value. If the values we get are invalid
>> (UNKNOWN), then slave gets removed from the aggregator with
>> speed and duplex set to UNKNOWN while link is still marked as UP.
>>
>> This patch fixes this scenario. Also 802.3ad mode is sensitive to
>> these conditions while other modes are not, so making sure that it
>> doesn't change the behavior for other modes.
>>
>> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>> ---
>> drivers/net/bonding/bond_main.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index b7313c1d9dcd..177be373966b 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -3076,7 +3076,16 @@ static int bond_slave_netdev_event(unsigned long event,
>> break;
>> case NETDEV_UP:
>> case NETDEV_CHANGE:
>> - bond_update_speed_duplex(slave);
>> + /* For 802.3ad mode only:
>> + * Getting invalid Speed/Duplex values here will put slave
>> + * in weird state. So mark it as link-down for the time
>> + * being and let link-monitoring (miimon) set it right when
>> + * correct speeds/duplex are available.
>> + */
>> + if (bond_update_speed_duplex(slave) &&
>> + BOND_MODE(bond) == BOND_MODE_8023AD)
>> + slave->link = BOND_LINK_DOWN;
>> +
>> if (BOND_MODE(bond) == BOND_MODE_8023AD)
>> bond_3ad_adapter_speed_duplex_changed(slave);
>> /* Fallthrough */
>
> Then fix the drivers. Trying to workaround it here isn't helping.
>
This is not a workaround. It avoids bonding state being weird.
> The problem is that miimon is not required. Bonding can be purely
> event driven.
>
really? Here is a code-snippet from bonding itself -
/* reset values for 802.3ad/TLB/ALB */
if (!bond_mode_uses_arp(bond_mode)) {
if (!miimon) {
pr_warn("Warning: miimon must be specified,
otherwise bonding will not detect link failure, speed and duplex which
are essential for 802.3ad operation\n");
pr_warn("Forcing miimon to 100msec\n");
miimon = BOND_DEFAULT_MIIMON;
}
}
^ permalink raw reply
* Re: [PATCH net-next] net: bridge: add per-port group_fwd_mask with less restrictions
From: Stephen Hemminger @ 2017-09-29 21:51 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: netdev, roopa, bridge
In-Reply-To: <e9a70870-9b6b-0ec4-de1e-d41da80fd10f@cumulusnetworks.com>
On Sat, 30 Sep 2017 00:01:24 +0300
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> On 29/09/17 18:14, Stephen Hemminger wrote:
> > On Wed, 27 Sep 2017 16:12:44 +0300
> > Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> >
> >> We need to be able to transparently forward most link-local frames via
> >> tunnels (e.g. vxlan, qinq). Currently the bridge's group_fwd_mask has a
> >> mask which restricts the forwarding of STP and LACP, but we need to be able
> >> to forward these over tunnels and control that forwarding on a per-port
> >> basis thus add a new per-port group_fwd_mask option which only disallows
> >> mac pause frames to be forwarded (they're always dropped anyway).
> >> The patch does not change the current default situation - all of the others
> >> are still restricted unless configured for forwarding.
> >> We have successfully tested this patch with LACP and STP forwarding over
> >> VxLAN and qinq tunnels.
> >>
> >> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> >
> > LACP is fine, but STP must not be forwarded if STP in user or kernel
> > mode is enabled.
> >
> > Please update this patch or revert it.
> >
>
> The default has not changed, STP is still _not_ forwarded. It can be only if explicitly
> requested by the user and that means the port might be participating in STP but not
> the bridge's STP, that is explicitly forward all STP frames from that port.
> I don't think we have to change anything.
>
You need to fail if user does something stupid. And DNR.
From 16c42db4852234c13288922c1bffbf7650e08434 Mon Sep 17 00:00:00 2001
From: Stephen Hemminger <sthemmin@microsoft.com>
Date: Fri, 29 Sep 2017 14:48:17 -0700
Subject: [PATCH] bridge: check for attempt to forward STP PDU's with STP
enabled
Move validation into set function and do not allow
configuring forwarding of STP packets if STP is enabled.
Fixes: 5af48b59f35c ("net: bridge: add per-port group_fwd_mask with less restrictions")
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
net/bridge/br_if.c | 13 +++++++++++++
net/bridge/br_netlink.c | 6 +++---
net/bridge/br_private.h | 1 +
net/bridge/br_sysfs_if.c | 6 +-----
4 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index f3aef22931ab..a30e12f76266 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -654,3 +654,16 @@ void br_port_flags_change(struct net_bridge_port *p, unsigned long mask)
if (mask & BR_AUTO_MASK)
nbp_update_port_count(br);
}
+
+int br_set_group_fwd_mask(struct net_bridge_port *p, u16 fwd_mask)
+{
+ if (fwd_mask & BR_GROUPFWD_MACPAUSE)
+ return -EINVAL;
+
+ if ((fwd_mask & BR_GROUPFWD_STP) &&
+ (p->br->stp_enabled != BR_NO_STP))
+ return -EINVAL;
+
+ p->group_fwd_mask = fwd_mask;
+ return 0;
+}
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index dea88a255d26..7b16819ecb59 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -780,9 +780,9 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
if (tb[IFLA_BRPORT_GROUP_FWD_MASK]) {
u16 fwd_mask = nla_get_u16(tb[IFLA_BRPORT_GROUP_FWD_MASK]);
- if (fwd_mask & BR_GROUPFWD_MACPAUSE)
- return -EINVAL;
- p->group_fwd_mask = fwd_mask;
+ err = br_set_group_fwd_mask(p, fwd_mask);
+ if (err)
+ return err;
}
br_port_flags_change(p, old_flags ^ p->flags);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 020c709a017f..734933bebb08 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -573,6 +573,7 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
netdev_features_t features);
void br_port_flags_change(struct net_bridge_port *port, unsigned long mask);
void br_manage_promisc(struct net_bridge *br);
+int br_set_group_fwd_mask(struct net_bridge_port *p, u16 mask);
/* br_input.c */
int br_handle_frame_finish(struct net *net, struct sock *sk, struct sk_buff *skb);
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 9110d5e56085..f5871be10b24 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -173,11 +173,7 @@ static ssize_t show_group_fwd_mask(struct net_bridge_port *p, char *buf)
static int store_group_fwd_mask(struct net_bridge_port *p,
unsigned long v)
{
- if (v & BR_GROUPFWD_MACPAUSE)
- return -EINVAL;
- p->group_fwd_mask = v;
-
- return 0;
+ return br_set_group_fwd_mask(p, v);
}
static BRPORT_ATTR(group_fwd_mask, S_IRUGO | S_IWUSR, show_group_fwd_mask,
store_group_fwd_mask);
--
2.11.0
^ permalink raw reply related
* [PATCH net-next v2 3/7] net: dsa: use temporary dsa_device_ops variable
From: Vivien Didelot @ 2017-09-29 21:19 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Vivien Didelot
In-Reply-To: <20170929211921.5571-1-vivien.didelot@savoirfairelinux.com>
When resolving the DSA tagging protocol used by a CPU switch, use a
temporary "tag_ops" variable to store the dsa_device_ops instead of
using directly dst->tag_ops. This will make the future patches moving
this pointer around easier to read.
There is no functional changes.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
net/dsa/dsa2.c | 8 +++++---
net/dsa/legacy.c | 8 +++++---
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index dcccaebde708..6a10c5c1639f 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -485,6 +485,7 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index,
struct dsa_switch_tree *dst,
struct dsa_switch *ds)
{
+ const struct dsa_device_ops *tag_ops;
enum dsa_tag_protocol tag_protocol;
struct net_device *ethernet_dev;
struct device_node *ethernet;
@@ -514,13 +515,14 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index,
ds->cpu_port_mask |= BIT(index);
tag_protocol = ds->ops->get_tag_protocol(ds);
- dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
- if (IS_ERR(dst->tag_ops)) {
+ tag_ops = dsa_resolve_tag_protocol(tag_protocol);
+ if (IS_ERR(tag_ops)) {
dev_warn(ds->dev, "No tagger for this switch\n");
ds->cpu_port_mask &= ~BIT(index);
- return PTR_ERR(dst->tag_ops);
+ return PTR_ERR(tag_ops);
}
+ dst->tag_ops = tag_ops;
dst->rcv = dst->tag_ops->rcv;
return 0;
diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index ae505d8e4417..8e849013f69d 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -144,13 +144,15 @@ static int dsa_switch_setup_one(struct dsa_switch *ds,
* switch.
*/
if (dst->cpu_dp->ds == ds) {
+ const struct dsa_device_ops *tag_ops;
enum dsa_tag_protocol tag_protocol;
tag_protocol = ops->get_tag_protocol(ds);
- dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
- if (IS_ERR(dst->tag_ops))
- return PTR_ERR(dst->tag_ops);
+ tag_ops = dsa_resolve_tag_protocol(tag_protocol);
+ if (IS_ERR(tag_ops))
+ return PTR_ERR(tag_ops);
+ dst->tag_ops = tag_ops;
dst->rcv = dst->tag_ops->rcv;
}
--
2.14.1
^ permalink raw reply related
* [PATCH net-next v2 7/7] net: dsa: remove tag ops from the switch tree
From: Vivien Didelot @ 2017-09-29 21:19 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, kernel, David S. Miller, Florian Fainelli,
Andrew Lunn, Vivien Didelot
In-Reply-To: <20170929211921.5571-1-vivien.didelot@savoirfairelinux.com>
Now that the dsa_ptr is a dsa_port instance, there is no need to keep
the tag operations in the dsa_switch_tree structure. Remove it.
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
include/net/dsa.h | 11 -----------
net/dsa/dsa2.c | 2 --
net/dsa/legacy.c | 2 --
3 files changed, 15 deletions(-)
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 6bda01fa5747..10dceccd9ce8 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -130,11 +130,6 @@ struct dsa_switch_tree {
*/
struct dsa_platform_data *pd;
- /* Copy of tag_ops->rcv for faster access in hot path */
- struct sk_buff * (*rcv)(struct sk_buff *skb,
- struct net_device *dev,
- struct packet_type *pt);
-
/*
* The switch port to which the CPU is attached.
*/
@@ -144,12 +139,6 @@ struct dsa_switch_tree {
* Data for the individual switch chips.
*/
struct dsa_switch *ds[DSA_MAX_SWITCHES];
-
- /*
- * Tagging protocol operations for adding and removing an
- * encapsulation tag.
- */
- const struct dsa_device_ops *tag_ops;
};
/* TC matchall action types, only mirroring for now */
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 62302558f38c..54ed054777bd 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -523,11 +523,9 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index,
}
dst->cpu_dp->tag_ops = tag_ops;
- dst->tag_ops = tag_ops;
/* Make a few copies for faster access in master receive hot path */
dst->cpu_dp->rcv = dst->cpu_dp->tag_ops->rcv;
- dst->rcv = dst->tag_ops->rcv;
dst->cpu_dp->dst = dst;
return 0;
diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index 71917505a5cc..19ff6e0a21dc 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -153,11 +153,9 @@ static int dsa_switch_setup_one(struct dsa_switch *ds,
return PTR_ERR(tag_ops);
dst->cpu_dp->tag_ops = tag_ops;
- dst->tag_ops = tag_ops;
/* Few copies for faster access in master receive hot path */
dst->cpu_dp->rcv = dst->cpu_dp->tag_ops->rcv;
- dst->rcv = dst->tag_ops->rcv;
dst->cpu_dp->dst = dst;
}
--
2.14.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox