Skip to content

Commit

Permalink
LF-12807-2 net: enetc: fix the issues of XDP_REDIRECT feature
Browse files Browse the repository at this point in the history 
When testing the XDP_REDIRECT function on the LS1028A platform, we
found a very easy-to-reproduce issue that causes Tx frames cannot
be sent out anymore, even if XDP_REDIRECT is turned off. Specially,
when XDP_REDIRECT is turned on if there is a lot of traffic on Rx,
the console may display some warnings like "timeout for tx ring #6
clear", and all redirected frames will be dropped, the detaild log
is as follows.

root@ls1028ardb:~# ./xdp-bench redirect eno0 eno2
Redirecting from eno0 (ifindex 3; driver fsl_enetc) to eno2 (ifindex 4; driver fsl_enetc)
[203.849809] fsl_enetc 0000:00:00.2 eno2: timeout for tx ring #5 clear
[204.006051] fsl_enetc 0000:00:00.2 eno2: timeout for tx ring #6 clear
[204.161944] fsl_enetc 0000:00:00.2 eno2: timeout for tx ring #7 clear
eno0->eno2     1420505 rx/s       1420590 err,drop/s      0 xmit/s
  xmit eno0->eno2    0 xmit/s     1420590 drop/s     0 drv_err/s     15.71 bulk-avg
eno0->eno2     1420484 rx/s       1420485 err,drop/s      0 xmit/s
  xmit eno0->eno2    0 xmit/s     1420485 drop/s     0 drv_err/s     15.71 bulk-avg

By analyzing the XDP_REDIRECT implementation of enetc driver, we
found two problems. First, enetc driver will reconfigure Tx and
Rx BD rings when a bpf program is installed or uninstalled, but
there is no mechanisms to block the redirected frames when enetc
driver reconfigures BD rings. So introduce ENETC_TX_DOWN flag to
prevent the redirected frames to be attached to Tx BD rings.

Second, Tx BD rings are disabled first in enetc_stop() and then
wait for empty. This operation is not safe while the Tx BD ring
is actively transmitting frames, and will cause the ring to not
be empty and hardware exception. As described in the block guide
of LS1028A NETC, software should only disable an active ring after
all pending ring entries have been consumed (i.e. when PI = CI).
Disabling a transmit ring that is actively processing BDs risks
a HW-SW race hazard whereby a hardware resource becomes assigned
to work on one or more ring entries only to have those entries be
removed due to the ring becoming disabled. So the correct approach
is that the software stops putting frames on the Tx BD rings (this
is what ENETC_TX_DOWN does), then waits for the Tx BD rings to be
empty, and finally disables the Tx BD rings.

Fixes: c33bfaf ("net: enetc: set up XDP program under enetc_reconfigure()")
Signed-off-by: Wei Fang <wei.fang@nxp.com>
Acked-by: Claudiu Manoil <claudiu.manoil@nxp.com>
  • Loading branch information
Wei Fang committed Jul 26, 2024
1 parent 14a3bba commit 8ad8246
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 9 deletions.
43 changes: 34 additions & 9 deletions drivers/net/ethernet/freescale/enetc/enetc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1272,6 +1272,7 @@ static bool enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int napi_budget)

if (unlikely(tx_frm_cnt && netif_carrier_ok(ndev) &&
__netif_subqueue_stopped(ndev, tx_ring->index) &&
!test_bit(ENETC_TX_DOWN, &priv->flags) &&
(enetc_bd_unused(tx_ring) >= ENETC_TX_STOP_THRESHOLD))) {
netif_wake_subqueue(ndev, tx_ring->index);
}
Expand Down Expand Up @@ -1752,6 +1753,9 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
int xdp_tx_bd_cnt, i, k;
int xdp_tx_frm_cnt = 0;

if (unlikely(test_bit(ENETC_TX_DOWN, &priv->flags)))
return -ENETDOWN;

enetc_lock_mdio();

tx_ring = priv->xdp_tx_ring[smp_processor_id()];
Expand Down Expand Up @@ -2668,18 +2672,24 @@ static void enetc_enable_rxbdr(struct enetc_hw *hw, struct enetc_bdr *rx_ring)
enetc_rxbdr_wr(hw, idx, ENETC_RBMR, rbmr);
}

static void enetc_enable_bdrs(struct enetc_ndev_priv *priv)
static void enetc_enable_rx_bdrs(struct enetc_ndev_priv *priv)
{
struct enetc_hw *hw = &priv->si->hw;
int i;

for (i = 0; i < priv->num_tx_rings; i++)
enetc_enable_txbdr(hw, priv->tx_ring[i]);

for (i = 0; i < priv->num_rx_rings; i++)
enetc_enable_rxbdr(hw, priv->rx_ring[i]);
}

static void enetc_enable_tx_bdrs(struct enetc_ndev_priv *priv)
{
struct enetc_hw *hw = &priv->si->hw;
int i;

for (i = 0; i < priv->num_tx_rings; i++)
enetc_enable_txbdr(hw, priv->tx_ring[i]);
}

static void enetc_disable_rxbdr(struct enetc_hw *hw, struct enetc_bdr *rx_ring)
{
int idx = rx_ring->index;
Expand All @@ -2696,16 +2706,23 @@ static void enetc_disable_txbdr(struct enetc_hw *hw, struct enetc_bdr *rx_ring)
enetc_txbdr_wr(hw, idx, ENETC_TBMR, 0);
}

static void enetc_disable_bdrs(struct enetc_ndev_priv *priv)
static void enetc_disable_rx_bdrs(struct enetc_ndev_priv *priv)
{
struct enetc_hw *hw = &priv->si->hw;
int i;

for (i = 0; i < priv->num_rx_rings; i++)
enetc_disable_rxbdr(hw, priv->rx_ring[i]);
}

static void enetc_disable_tx_bdrs(struct enetc_ndev_priv *priv)
{
struct enetc_hw *hw = &priv->si->hw;
int i;

for (i = 0; i < priv->num_tx_rings; i++)
enetc_disable_txbdr(hw, priv->tx_ring[i]);

for (i = 0; i < priv->num_rx_rings; i++)
enetc_disable_rxbdr(hw, priv->rx_ring[i]);
}

static void enetc_wait_txbdr(struct enetc_hw *hw, struct enetc_bdr *tx_ring)
Expand Down Expand Up @@ -2922,6 +2939,8 @@ void enetc_start(struct net_device *ndev)

enetc_setup_interrupts(priv);

enetc_enable_tx_bdrs(priv);

for (i = 0; i < priv->bdr_int_num; i++) {
int irq = pci_irq_vector(priv->si->pdev,
ENETC_BDR_INT_BASE_IDX + i);
Expand All @@ -2930,9 +2949,11 @@ void enetc_start(struct net_device *ndev)
enable_irq(irq);
}

enetc_enable_bdrs(priv);
enetc_enable_rx_bdrs(priv);

netif_tx_start_all_queues(ndev);

clear_bit(ENETC_TX_DOWN, &priv->flags);
}
EXPORT_SYMBOL_GPL(enetc_start);

Expand Down Expand Up @@ -3007,9 +3028,11 @@ void enetc_stop(struct net_device *ndev)
struct enetc_ndev_priv *priv = netdev_priv(ndev);
int i;

set_bit(ENETC_TX_DOWN, &priv->flags);

netif_tx_stop_all_queues(ndev);

enetc_disable_bdrs(priv);
enetc_disable_rx_bdrs(priv);

for (i = 0; i < priv->bdr_int_num; i++) {
int irq = pci_irq_vector(priv->si->pdev,
Expand All @@ -3022,6 +3045,8 @@ void enetc_stop(struct net_device *ndev)

enetc_wait_bdrs(priv);

enetc_disable_tx_bdrs(priv);

enetc_clear_interrupts(priv);
}
EXPORT_SYMBOL_GPL(enetc_stop);
Expand Down
1 change: 1 addition & 0 deletions drivers/net/ethernet/freescale/enetc/enetc.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,7 @@ enum enetc_active_offloads {

enum enetc_flags_bit {
ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS = 0,
ENETC_TX_DOWN,
};

/* interrupt coalescing modes */
Expand Down

0 comments on commit 8ad8246

Please sign in to comment.