path: root/sys/dev/ixl/ixl_txrx.c
diff options
authorEric Joyner <erj@FreeBSD.org>2019-01-24 01:03:00 +0000
committerEric Joyner <erj@FreeBSD.org>2019-01-24 01:03:00 +0000
commit088a0b27adb406c4ca6141145332145ed1765ff0 (patch)
tree042341b724cda440e39d9430b272beb44a0032a2 /sys/dev/ixl/ixl_txrx.c
parente190da54d3d3674806cb8831a62bebf7a2643e75 (diff)
intel iflib drivers: correct initialization of tx_cidx_processed
From Jake: In r341156 ("Fix first-packet completion", 2018-11-28) a hack to work around a delta calculation determining how many descriptors were used was added to ixl_isc_tx_credits_update_dwb. The same fix was also applied to the em and igb drivers in r340310, and to ix in r341156. The hack checked the case where prev and cur were equal, and then added one. This works, because by the time we do the delta check, we already know there is at least one packet available, so the delta should be at least one. However, it's not a complete fix, and as indicated by the comment is really a hack to work around the real bug. The real problem is that the first time that we transmit a packet, tx_cidx_processed will be set to point to the start of the ring. Ultimately, the credits_update function expects it to point to the *last* descriptor that was processed. Since we haven't yet processed any descriptors, pointing it to 0 results in this incorrect calculation. Fix the initialization code to have it point to the end of the ring instead. One way to think about this, is that we are setting the value to be one prior to the first available descriptor. Doing so, corrects the delta calculation in all cases. The original fix only works if the first packet has exactly one descriptor. Otherwise, we will report 1 less than the correct value. As part of this fix, also update the MPASS assertions to match the real expectations. First, ensure that prev is not equal to cur, since this should never happen. Second, remove the assertion about prev==0 || delta != 0. It looks like that originated from when the em driver was converted to iflib. It seems like it was supposed to ensure that delta was non-zero. However, because we originally returned 0 delta for the first calculation, the "prev == 0" was tacked on. Instead, replace this with a check that delta is greater than zero, after the correction necessary when the ring pointers wrap around. This new solution should fix the same bug as r341156 did, but in a more robust way. Submitted by: Jacob Keller <jacob.e.keller@intel.com> Reviewed by: shurd@ Sponsored by: Intel Corporation Differential Revision: https://reviews.freebsd.org/D18545
Notes: svn path=/head/; revision=343369
Diffstat (limited to 'sys/dev/ixl/ixl_txrx.c')
1 files changed, 12 insertions, 9 deletions
diff --git a/sys/dev/ixl/ixl_txrx.c b/sys/dev/ixl/ixl_txrx.c
index ea215f272d9f..a705bfba4022 100644
--- a/sys/dev/ixl/ixl_txrx.c
+++ b/sys/dev/ixl/ixl_txrx.c
@@ -515,16 +515,11 @@ ixl_isc_txd_credits_update_dwb(void *arg, uint16_t txqid, bool clear)
prev = txr->tx_cidx_processed;
ntxd = scctx->isc_ntxd[0];
do {
+ MPASS(prev != cur);
delta = (int32_t)cur - (int32_t)prev;
- /*
- * XXX This appears to be a hack for first-packet.
- * A correct fix would prevent prev == cur in the first place.
- */
- MPASS(prev == 0 || delta != 0);
- if (prev == 0 && cur == 0)
- delta += 1;
if (delta < 0)
delta += ntxd;
+ MPASS(delta > 0);
#if 0
"%s: (q%d) cidx_processed=%u cur=%u clear=%d delta=%d\n",
@@ -793,7 +788,14 @@ ixl_init_tx_rsqs(struct ixl_vsi *vsi)
for (i = 0, tx_que = vsi->tx_queues; i < vsi->num_tx_queues; i++, tx_que++) {
struct tx_ring *txr = &tx_que->txr;
- txr->tx_rs_cidx = txr->tx_rs_pidx = txr->tx_cidx_processed = 0;
+ txr->tx_rs_cidx = txr->tx_rs_pidx;
+ /* Initialize the last processed descriptor to be the end of
+ * the ring, rather than the start, so that we avoid an
+ * off-by-one error when calculating how many descriptors are
+ * done in the credits_update function.
+ */
+ txr->tx_cidx_processed = scctx->isc_ntxd[0] - 1;
for (j = 0; j < scctx->isc_ntxd[0]; j++)
txr->tx_rsq[j] = QIDX_INVALID;
@@ -803,13 +805,14 @@ ixl_init_tx_rsqs(struct ixl_vsi *vsi)
ixl_init_tx_cidx(struct ixl_vsi *vsi)
+ if_softc_ctx_t scctx = vsi->shared;
struct ixl_tx_queue *tx_que;
int i;
for (i = 0, tx_que = vsi->tx_queues; i < vsi->num_tx_queues; i++, tx_que++) {
struct tx_ring *txr = &tx_que->txr;
- txr->tx_cidx_processed = 0;
+ txr->tx_cidx_processed = scctx->isc_ntxd[0] - 1;