[PATCH net-next 0/8] ionic: rework fix for doorbell miss

[PATCH net-next 0/8] ionic: rework fix for doorbell miss

[Shannon Nelson]

A latency test in a scaled out setting (many VMs with many queues)
has uncovered an issue with our missed doorbell fix from
commit b69585bfcece ("ionic: missed doorbell workaround")

As a refresher, the Elba ASIC has an issue where once in a blue
moon it might miss/drop a queue doorbell notification from
the driver.  This can result in Tx timeouts and potential Rx
buffer misses.

The basic problem with the original solution is that
we're delaying things with a timer for every single queue,
periodically using mod_timer() to reset to reset the alarm, and
mod_timer() becomes a more and more expensive thing as there
are more and more VFs and queues each with their own timer.
A ping-pong latency test tends to exacerbate the effect such
that every napi is doing a mod_timer() in every cycle.

An alternative has been worked out to replace this using
periodic workqueue items outside the napi cycle to request a
napi_schedule driven by a single delayed-workqueue per device
rather than a timer for every queue.  Also, now that newer
firmware is actually reporting its ASIC type, we can restrict
this to the appropriate chip.

The testing scenario used 128 VFs in UP state, 16 queues per
VF, and latency tests were done using TCP_RR with adaptive
interrupt coalescing enabled, running on 1 VF.  We would see
99th percentile latencies of up to 900us range, with some max
fliers as much as 4ms.

With these fixes the 99th percentile latencies are typically well
under 50us with the occasional max under 500us.

Brett Creeley (3):
  ionic: Keep interrupt affinity up to date
  ionic: Use an u16 for rx_copybreak
  ionic: Only run the doorbell workaround for certain asic_type

Shannon Nelson (5):
  ionic: remove missed doorbell per-queue timer
  ionic: add private workqueue per-device
  ionic: add work item for missed-doorbell check
  ionic: add per-queue napi_schedule for doorbell check
  ionic: check for queue deadline in doorbell_napi_work

 drivers/net/ethernet/pensando/ionic/ionic.h   |   7 +
 .../ethernet/pensando/ionic/ionic_bus_pci.c   |   1 +
 .../net/ethernet/pensando/ionic/ionic_dev.c   | 128 ++++++++++++++-
 .../net/ethernet/pensando/ionic/ionic_dev.h   |   8 +-
 .../ethernet/pensando/ionic/ionic_ethtool.c   |  10 +-
 .../net/ethernet/pensando/ionic/ionic_lif.c   | 146 ++++++++++++------
 .../net/ethernet/pensando/ionic/ionic_lif.h   |   8 +-
 .../net/ethernet/pensando/ionic/ionic_main.c  |   2 +-
 .../net/ethernet/pensando/ionic/ionic_txrx.c  |  24 ++-
 9 files changed, 260 insertions(+), 74 deletions(-)

-- 
2.17.1

[PATCH net-next 1/8] ionic: remove missed doorbell per-queue timer

[Shannon Nelson]

Remove the timer-per-queue mechanics from the missed doorbell
check in preparation for the new missed doorbell fix.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 .../net/ethernet/pensando/ionic/ionic_dev.c   |  4 ---
 .../net/ethernet/pensando/ionic/ionic_lif.c   | 36 ++++---------------
 .../net/ethernet/pensando/ionic/ionic_lif.h   |  2 --
 .../net/ethernet/pensando/ionic/ionic_txrx.c  | 22 +++++-------
 4 files changed, 15 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
index 874499337132..89b4310f244c 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
@@ -703,10 +703,6 @@ void ionic_q_post(struct ionic_queue *q, bool ring_doorbell)
 				 q->dbval | q->head_idx);
 
 		q->dbell_jiffies = jiffies;
-
-		if (q_to_qcq(q)->napi_qcq)
-			mod_timer(&q_to_qcq(q)->napi_qcq->napi_deadline,
-				  jiffies + IONIC_NAPI_DEADLINE);
 	}
 }
 
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 23e1f6638b38..48b2b150fbcc 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -213,13 +213,6 @@ void ionic_link_status_check_request(struct ionic_lif *lif, bool can_sleep)
 	}
 }
 
-static void ionic_napi_deadline(struct timer_list *timer)
-{
-	struct ionic_qcq *qcq = container_of(timer, struct ionic_qcq, napi_deadline);
-
-	napi_schedule(&qcq->napi);
-}
-
 static irqreturn_t ionic_isr(int irq, void *data)
 {
 	struct napi_struct *napi = data;
@@ -345,7 +338,6 @@ static int ionic_qcq_disable(struct ionic_lif *lif, struct ionic_qcq *qcq, int f
 		synchronize_irq(qcq->intr.vector);
 		irq_set_affinity_hint(qcq->intr.vector, NULL);
 		napi_disable(&qcq->napi);
-		del_timer_sync(&qcq->napi_deadline);
 	}
 
 	/* If there was a previous fw communcation error, don't bother with
@@ -480,7 +472,6 @@ static void ionic_link_qcq_interrupts(struct ionic_qcq *src_qcq,
 {
 	n_qcq->intr.vector = src_qcq->intr.vector;
 	n_qcq->intr.index = src_qcq->intr.index;
-	n_qcq->napi_qcq = src_qcq->napi_qcq;
 }
 
 static int ionic_alloc_qcq_interrupt(struct ionic_lif *lif, struct ionic_qcq *qcq)
@@ -834,11 +825,8 @@ static int ionic_lif_txq_init(struct ionic_lif *lif, struct ionic_qcq *qcq)
 	q->dbell_deadline = IONIC_TX_DOORBELL_DEADLINE;
 	q->dbell_jiffies = jiffies;
 
-	if (test_bit(IONIC_LIF_F_SPLIT_INTR, lif->state)) {
+	if (test_bit(IONIC_LIF_F_SPLIT_INTR, lif->state))
 		netif_napi_add(lif->netdev, &qcq->napi, ionic_tx_napi);
-		qcq->napi_qcq = qcq;
-		timer_setup(&qcq->napi_deadline, ionic_napi_deadline, 0);
-	}
 
 	qcq->flags |= IONIC_QCQ_F_INITED;
 
@@ -911,9 +899,6 @@ static int ionic_lif_rxq_init(struct ionic_lif *lif, struct ionic_qcq *qcq)
 	else
 		netif_napi_add(lif->netdev, &qcq->napi, ionic_txrx_napi);
 
-	qcq->napi_qcq = qcq;
-	timer_setup(&qcq->napi_deadline, ionic_napi_deadline, 0);
-
 	qcq->flags |= IONIC_QCQ_F_INITED;
 
 	return 0;
@@ -1168,7 +1153,6 @@ static int ionic_adminq_napi(struct napi_struct *napi, int budget)
 	struct ionic_dev *idev = &lif->ionic->idev;
 	unsigned long irqflags;
 	unsigned int flags = 0;
-	bool resched = false;
 	int rx_work = 0;
 	int tx_work = 0;
 	int n_work = 0;
@@ -1205,15 +1189,12 @@ static int ionic_adminq_napi(struct napi_struct *napi, int budget)
 		ionic_intr_credits(idev->intr_ctrl, intr->index, credits, flags);
 	}
 
-	if (!a_work && ionic_adminq_poke_doorbell(&lif->adminqcq->q))
-		resched = true;
-	if (lif->hwstamp_rxq && !rx_work && ionic_rxq_poke_doorbell(&lif->hwstamp_rxq->q))
-		resched = true;
-	if (lif->hwstamp_txq && !tx_work && ionic_txq_poke_doorbell(&lif->hwstamp_txq->q))
-		resched = true;
-	if (resched)
-		mod_timer(&lif->adminqcq->napi_deadline,
-			  jiffies + IONIC_NAPI_DEADLINE);
+	if (!a_work)
+		ionic_adminq_poke_doorbell(&lif->adminqcq->q);
+	if (lif->hwstamp_rxq && !rx_work)
+		ionic_rxq_poke_doorbell(&lif->hwstamp_rxq->q);
+	if (lif->hwstamp_txq && !tx_work)
+		ionic_txq_poke_doorbell(&lif->hwstamp_txq->q);
 
 	return work_done;
 }
@@ -3504,9 +3485,6 @@ static int ionic_lif_adminq_init(struct ionic_lif *lif)
 
 	netif_napi_add(lif->netdev, &qcq->napi, ionic_adminq_napi);
 
-	qcq->napi_qcq = qcq;
-	timer_setup(&qcq->napi_deadline, ionic_napi_deadline, 0);
-
 	napi_enable(&qcq->napi);
 
 	if (qcq->flags & IONIC_QCQ_F_INTR) {
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
index 08f4266fe2aa..a029206c0bc8 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
@@ -84,11 +84,9 @@ struct ionic_qcq {
 	u32 cmb_pgid;
 	u32 cmb_order;
 	struct dim dim;
-	struct timer_list napi_deadline;
 	struct ionic_queue q;
 	struct ionic_cq cq;
 	struct napi_struct napi;
-	struct ionic_qcq *napi_qcq;
 	struct ionic_intr_info intr;
 	struct dentry *dentry;
 };
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
index c3a6c4af52f1..3066eb4788f9 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
@@ -867,9 +867,6 @@ void ionic_rx_fill(struct ionic_queue *q)
 
 	q->dbell_deadline = IONIC_RX_MIN_DOORBELL_DEADLINE;
 	q->dbell_jiffies = jiffies;
-
-	mod_timer(&q_to_qcq(q)->napi_qcq->napi_deadline,
-		  jiffies + IONIC_NAPI_DEADLINE);
 }
 
 void ionic_rx_empty(struct ionic_queue *q)
@@ -952,8 +949,8 @@ int ionic_tx_napi(struct napi_struct *napi, int budget)
 				   work_done, flags);
 	}
 
-	if (!work_done && ionic_txq_poke_doorbell(&qcq->q))
-		mod_timer(&qcq->napi_deadline, jiffies + IONIC_NAPI_DEADLINE);
+	if (!work_done)
+		ionic_txq_poke_doorbell(&qcq->q);
 
 	return work_done;
 }
@@ -995,8 +992,8 @@ int ionic_rx_napi(struct napi_struct *napi, int budget)
 				   work_done, flags);
 	}
 
-	if (!work_done && ionic_rxq_poke_doorbell(&qcq->q))
-		mod_timer(&qcq->napi_deadline, jiffies + IONIC_NAPI_DEADLINE);
+	if (!work_done)
+		ionic_rxq_poke_doorbell(&qcq->q);
 
 	return work_done;
 }
@@ -1009,7 +1006,6 @@ int ionic_txrx_napi(struct napi_struct *napi, int budget)
 	struct ionic_qcq *txqcq;
 	struct ionic_lif *lif;
 	struct ionic_cq *txcq;
-	bool resched = false;
 	u32 rx_work_done = 0;
 	u32 tx_work_done = 0;
 	u32 flags = 0;
@@ -1041,12 +1037,10 @@ int ionic_txrx_napi(struct napi_struct *napi, int budget)
 				   tx_work_done + rx_work_done, flags);
 	}
 
-	if (!rx_work_done && ionic_rxq_poke_doorbell(&rxqcq->q))
-		resched = true;
-	if (!tx_work_done && ionic_txq_poke_doorbell(&txqcq->q))
-		resched = true;
-	if (resched)
-		mod_timer(&rxqcq->napi_deadline, jiffies + IONIC_NAPI_DEADLINE);
+	if (!rx_work_done)
+		ionic_rxq_poke_doorbell(&rxqcq->q);
+	if (!tx_work_done)
+		ionic_txq_poke_doorbell(&txqcq->q);
 
 	return rx_work_done;
 }
-- 
2.17.1

[PATCH net-next 2/8] ionic: Keep interrupt affinity up to date

[Shannon Nelson]

From: Brett Creeley <brett.creeley@amd.com>

Currently the driver either sets the initial interrupt affinity for its
adminq and tx/rx queues on probe or resets it on various
down/up/reconfigure flows. If any user and/or user process
(i.e. irqbalance) changes IRQ affinity for any of the driver's interrupts
that will be reset to driver defaults whenever any down/up/reconfigure
operation happens. This is incorrect and is fixed by making 2 changes:

1. Allocate an array of cpumasks that's only allocated on probe and
   destroyed on remove.
2. Update the cpumask(s) for interrupts that are in use by registering
   for affinity notifiers.

Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic.h   |  1 +
 .../net/ethernet/pensando/ionic/ionic_dev.h   |  4 +-
 .../net/ethernet/pensando/ionic/ionic_lif.c   | 85 +++++++++++++++++--
 3 files changed, 81 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h
index 2ccc2c2a06e3..438172cfb170 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic.h
@@ -54,6 +54,7 @@ struct ionic {
 	unsigned int nrxqs_per_lif;
 	unsigned int nintrs;
 	DECLARE_BITMAP(intrs, IONIC_INTR_CTRL_REGS_MAX);
+	cpumask_var_t *affinity_masks;
 	struct work_struct nb_work;
 	struct notifier_block nb;
 	struct rw_semaphore vf_op_lock;	/* lock for VF operations */
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
index f30eee4a5a80..7dbd3b8b0e36 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
@@ -280,9 +280,9 @@ struct ionic_intr_info {
 	u64 rearm_count;
 	unsigned int index;
 	unsigned int vector;
-	unsigned int cpu;
 	u32 dim_coal_hw;
-	cpumask_t affinity_mask;
+	cpumask_var_t *affinity_mask;
+	struct irq_affinity_notify aff_notify;
 };
 
 struct ionic_cq {
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 48b2b150fbcc..ff6a7e86254c 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -265,6 +265,18 @@ static void ionic_intr_free(struct ionic *ionic, int index)
 		clear_bit(index, ionic->intrs);
 }
 
+static void ionic_irq_aff_notify(struct irq_affinity_notify *notify,
+				 const cpumask_t *mask)
+{
+	struct ionic_intr_info *intr = container_of(notify, struct ionic_intr_info, aff_notify);
+
+	cpumask_copy(*intr->affinity_mask, mask);
+}
+
+static void ionic_irq_aff_release(struct kref __always_unused *ref)
+{
+}
+
 static int ionic_qcq_enable(struct ionic_qcq *qcq)
 {
 	struct ionic_queue *q = &qcq->q;
@@ -301,8 +313,10 @@ static int ionic_qcq_enable(struct ionic_qcq *qcq)
 		napi_enable(&qcq->napi);
 
 	if (qcq->flags & IONIC_QCQ_F_INTR) {
+		irq_set_affinity_notifier(qcq->intr.vector,
+					  &qcq->intr.aff_notify);
 		irq_set_affinity_hint(qcq->intr.vector,
-				      &qcq->intr.affinity_mask);
+				      *qcq->intr.affinity_mask);
 		ionic_intr_mask(idev->intr_ctrl, qcq->intr.index,
 				IONIC_INTR_MASK_CLEAR);
 	}
@@ -336,6 +350,7 @@ static int ionic_qcq_disable(struct ionic_lif *lif, struct ionic_qcq *qcq, int f
 		ionic_intr_mask(idev->intr_ctrl, qcq->intr.index,
 				IONIC_INTR_MASK_SET);
 		synchronize_irq(qcq->intr.vector);
+		irq_set_affinity_notifier(qcq->intr.vector, NULL);
 		irq_set_affinity_hint(qcq->intr.vector, NULL);
 		napi_disable(&qcq->napi);
 	}
@@ -476,6 +491,7 @@ static void ionic_link_qcq_interrupts(struct ionic_qcq *src_qcq,
 
 static int ionic_alloc_qcq_interrupt(struct ionic_lif *lif, struct ionic_qcq *qcq)
 {
+	cpumask_var_t *affinity_mask;
 	int err;
 
 	if (!(qcq->flags & IONIC_QCQ_F_INTR)) {
@@ -507,10 +523,19 @@ static int ionic_alloc_qcq_interrupt(struct ionic_lif *lif, struct ionic_qcq *qc
 	}
 
 	/* try to get the irq on the local numa node first */
-	qcq->intr.cpu = cpumask_local_spread(qcq->intr.index,
-					     dev_to_node(lif->ionic->dev));
-	if (qcq->intr.cpu != -1)
-		cpumask_set_cpu(qcq->intr.cpu, &qcq->intr.affinity_mask);
+	affinity_mask = &lif->ionic->affinity_masks[qcq->intr.index];
+	if (cpumask_empty(*affinity_mask)) {
+		unsigned int cpu;
+
+		cpu = cpumask_local_spread(qcq->intr.index,
+					   dev_to_node(lif->ionic->dev));
+		if (cpu != -1)
+			cpumask_set_cpu(cpu, *affinity_mask);
+	}
+
+	qcq->intr.affinity_mask = affinity_mask;
+	qcq->intr.aff_notify.notify = ionic_irq_aff_notify;
+	qcq->intr.aff_notify.release = ionic_irq_aff_release;
 
 	netdev_dbg(lif->netdev, "%s: Interrupt index %d\n", qcq->q.name, qcq->intr.index);
 	return 0;
@@ -3122,6 +3147,44 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
 	return err;
 }
 
+static int ionic_affinity_masks_alloc(struct ionic *ionic)
+{
+	cpumask_var_t *affinity_masks;
+	int nintrs = ionic->nintrs;
+	int i;
+
+	affinity_masks = kcalloc(nintrs, sizeof(cpumask_var_t), GFP_KERNEL);
+	if (!affinity_masks)
+		return	-ENOMEM;
+
+	for (i = 0; i < nintrs; i++) {
+		if (!zalloc_cpumask_var_node(&affinity_masks[i], GFP_KERNEL,
+					     dev_to_node(ionic->dev)))
+			goto err_out;
+	}
+
+	ionic->affinity_masks = affinity_masks;
+
+	return 0;
+
+err_out:
+	for (--i; i >= 0; i--)
+		free_cpumask_var(affinity_masks[i]);
+	kfree(affinity_masks);
+
+	return -ENOMEM;
+}
+
+static void ionic_affinity_masks_free(struct ionic *ionic)
+{
+	int i;
+
+	for (i = 0; i < ionic->nintrs; i++)
+		free_cpumask_var(ionic->affinity_masks[i]);
+	kfree(ionic->affinity_masks);
+	ionic->affinity_masks = NULL;
+}
+
 int ionic_lif_alloc(struct ionic *ionic)
 {
 	struct device *dev = ionic->dev;
@@ -3213,11 +3276,15 @@ int ionic_lif_alloc(struct ionic *ionic)
 
 	ionic_debugfs_add_lif(lif);
 
+	err = ionic_affinity_masks_alloc(ionic);
+	if (err)
+		goto err_out_free_lif_info;
+
 	/* allocate control queues and txrx queue arrays */
 	ionic_lif_queue_identify(lif);
 	err = ionic_qcqs_alloc(lif);
 	if (err)
-		goto err_out_free_lif_info;
+		goto err_out_free_affinity_masks;
 
 	/* allocate rss indirection table */
 	tbl_sz = le16_to_cpu(lif->ionic->ident.lif.eth.rss_ind_tbl_sz);
@@ -3239,6 +3306,8 @@ int ionic_lif_alloc(struct ionic *ionic)
 
 err_out_free_qcqs:
 	ionic_qcqs_free(lif);
+err_out_free_affinity_masks:
+	ionic_affinity_masks_free(lif->ionic);
 err_out_free_lif_info:
 	dma_free_coherent(dev, lif->info_sz, lif->info, lif->info_pa);
 	lif->info = NULL;
@@ -3412,6 +3481,8 @@ void ionic_lif_free(struct ionic_lif *lif)
 	if (!test_bit(IONIC_LIF_F_FW_RESET, lif->state))
 		ionic_lif_reset(lif);
 
+	ionic_affinity_masks_free(lif->ionic);
+
 	/* free lif info */
 	kfree(lif->identity);
 	dma_free_coherent(dev, lif->info_sz, lif->info, lif->info_pa);
@@ -3489,7 +3560,7 @@ static int ionic_lif_adminq_init(struct ionic_lif *lif)
 
 	if (qcq->flags & IONIC_QCQ_F_INTR) {
 		irq_set_affinity_hint(qcq->intr.vector,
-				      &qcq->intr.affinity_mask);
+				      *qcq->intr.affinity_mask);
 		ionic_intr_mask(idev->intr_ctrl, qcq->intr.index,
 				IONIC_INTR_MASK_CLEAR);
 	}
-- 
2.17.1

[PATCH net-next 3/8] ionic: add private workqueue per-device

[Shannon Nelson]

Instead of using the system's default workqueue,
add a private workqueue for the device to use for
its little jobs.

Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic.h   |  1 +
 .../net/ethernet/pensando/ionic/ionic_dev.c   | 21 +++++++++++++++----
 .../net/ethernet/pensando/ionic/ionic_lif.c   | 14 ++++++-------
 .../net/ethernet/pensando/ionic/ionic_lif.h   |  2 +-
 .../net/ethernet/pensando/ionic/ionic_main.c  |  2 +-
 5 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h
index 438172cfb170..df29c977a702 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic.h
@@ -47,6 +47,7 @@ struct ionic {
 	struct ionic_dev_bar bars[IONIC_BARS_MAX];
 	unsigned int num_bars;
 	struct ionic_identity ident;
+	struct workqueue_struct *wq;
 	struct ionic_lif *lif;
 	unsigned int nnqs_per_lif;
 	unsigned int neqs_per_lif;
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
index 89b4310f244c..342863fd0b16 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
@@ -43,11 +43,11 @@ static void ionic_watchdog_cb(struct timer_list *t)
 
 		work->type = IONIC_DW_TYPE_RX_MODE;
 		netdev_dbg(lif->netdev, "deferred: rx_mode\n");
-		ionic_lif_deferred_enqueue(&lif->deferred, work);
+		ionic_lif_deferred_enqueue(lif, work);
 	}
 }
 
-static void ionic_watchdog_init(struct ionic *ionic)
+static int ionic_watchdog_init(struct ionic *ionic)
 {
 	struct ionic_dev *idev = &ionic->idev;
 
@@ -63,6 +63,15 @@ static void ionic_watchdog_init(struct ionic *ionic)
 	idev->fw_status_ready = true;
 	idev->fw_generation = IONIC_FW_STS_F_GENERATION &
 			      ioread8(&idev->dev_info_regs->fw_status);
+
+	ionic->wq = alloc_workqueue("%s-wq", WQ_UNBOUND, 0,
+				    dev_name(ionic->dev));
+	if (!ionic->wq) {
+		dev_err(ionic->dev, "alloc_workqueue failed");
+		return -ENOMEM;
+	}
+
+	return 0;
 }
 
 void ionic_init_devinfo(struct ionic *ionic)
@@ -94,6 +103,7 @@ int ionic_dev_setup(struct ionic *ionic)
 	struct device *dev = ionic->dev;
 	int size;
 	u32 sig;
+	int err;
 
 	/* BAR0: dev_cmd and interrupts */
 	if (num_bars < 1) {
@@ -129,7 +139,9 @@ int ionic_dev_setup(struct ionic *ionic)
 		return -EFAULT;
 	}
 
-	ionic_watchdog_init(ionic);
+	err = ionic_watchdog_init(ionic);
+	if (err)
+		return err;
 
 	idev->db_pages = bar->vaddr;
 	idev->phy_db_pages = bar->bus_addr;
@@ -161,6 +173,7 @@ void ionic_dev_teardown(struct ionic *ionic)
 	idev->phy_cmb_pages = 0;
 	idev->cmb_npages = 0;
 
+	destroy_workqueue(ionic->wq);
 	mutex_destroy(&idev->cmb_inuse_lock);
 }
 
@@ -273,7 +286,7 @@ int ionic_heartbeat_check(struct ionic *ionic)
 			if (work) {
 				work->type = IONIC_DW_TYPE_LIF_RESET;
 				work->fw_status = fw_status_ready;
-				ionic_lif_deferred_enqueue(&lif->deferred, work);
+				ionic_lif_deferred_enqueue(lif, work);
 			}
 		}
 	}
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index ff6a7e86254c..af269a198d5d 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -126,13 +126,13 @@ static void ionic_lif_deferred_work(struct work_struct *work)
 	} while (true);
 }
 
-void ionic_lif_deferred_enqueue(struct ionic_deferred *def,
+void ionic_lif_deferred_enqueue(struct ionic_lif *lif,
 				struct ionic_deferred_work *work)
 {
-	spin_lock_bh(&def->lock);
-	list_add_tail(&work->list, &def->list);
-	spin_unlock_bh(&def->lock);
-	schedule_work(&def->work);
+	spin_lock_bh(&lif->deferred.lock);
+	list_add_tail(&work->list, &lif->deferred.list);
+	spin_unlock_bh(&lif->deferred.lock);
+	queue_work(lif->ionic->wq, &lif->deferred.work);
 }
 
 static void ionic_link_status_check(struct ionic_lif *lif)
@@ -207,7 +207,7 @@ void ionic_link_status_check_request(struct ionic_lif *lif, bool can_sleep)
 		}
 
 		work->type = IONIC_DW_TYPE_LINK_STATUS;
-		ionic_lif_deferred_enqueue(&lif->deferred, work);
+		ionic_lif_deferred_enqueue(lif, work);
 	} else {
 		ionic_link_status_check(lif);
 	}
@@ -1391,7 +1391,7 @@ static void ionic_ndo_set_rx_mode(struct net_device *netdev)
 	}
 	work->type = IONIC_DW_TYPE_RX_MODE;
 	netdev_dbg(lif->netdev, "deferred: rx_mode\n");
-	ionic_lif_deferred_enqueue(&lif->deferred, work);
+	ionic_lif_deferred_enqueue(lif, work);
 }
 
 static __le64 ionic_netdev_features_to_nic(netdev_features_t features)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
index a029206c0bc8..e4a5ae70793e 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
@@ -331,7 +331,7 @@ static inline bool ionic_txq_hwstamp_enabled(struct ionic_queue *q)
 void ionic_link_status_check_request(struct ionic_lif *lif, bool can_sleep);
 void ionic_get_stats64(struct net_device *netdev,
 		       struct rtnl_link_stats64 *ns);
-void ionic_lif_deferred_enqueue(struct ionic_deferred *def,
+void ionic_lif_deferred_enqueue(struct ionic_lif *lif,
 				struct ionic_deferred_work *work);
 int ionic_lif_alloc(struct ionic *ionic);
 int ionic_lif_init(struct ionic_lif *lif);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
index c1259324b0be..0f817c3f92d8 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
@@ -287,7 +287,7 @@ bool ionic_notifyq_service(struct ionic_cq *cq)
 				clear_bit(IONIC_LIF_F_FW_STOPPING, lif->state);
 			} else {
 				work->type = IONIC_DW_TYPE_LIF_RESET;
-				ionic_lif_deferred_enqueue(&lif->deferred, work);
+				ionic_lif_deferred_enqueue(lif, work);
 			}
 		}
 		break;
-- 
2.17.1

[PATCH net-next 4/8] ionic: add work item for missed-doorbell check

[Shannon Nelson]

Add the first queued work for checking on the missed doorbell.
This is a delayed work item that reschedules itself every cycle
starting at probe.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic.h   |  1 +
 .../ethernet/pensando/ionic/ionic_bus_pci.c   |  1 +
 .../net/ethernet/pensando/ionic/ionic_dev.c   | 65 +++++++++++++++++++
 .../net/ethernet/pensando/ionic/ionic_dev.h   |  3 +-
 .../net/ethernet/pensando/ionic/ionic_lif.c   |  3 +
 5 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h
index df29c977a702..106ee5b2ceff 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic.h
@@ -56,6 +56,7 @@ struct ionic {
 	unsigned int nintrs;
 	DECLARE_BITMAP(intrs, IONIC_INTR_CTRL_REGS_MAX);
 	cpumask_var_t *affinity_masks;
+	struct delayed_work doorbell_check_dwork;
 	struct work_struct nb_work;
 	struct notifier_block nb;
 	struct rw_semaphore vf_op_lock;	/* lock for VF operations */
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
index 6ba8d4aca0a0..a6a6df6b7304 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
@@ -372,6 +372,7 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	mod_timer(&ionic->watchdog_timer,
 		  round_jiffies(jiffies + ionic->watchdog_period));
+	ionic_queue_doorbell_check(ionic, IONIC_NAPI_DEADLINE);
 
 	return 0;
 
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
index 342863fd0b16..30185ac324ff 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
@@ -47,6 +47,60 @@ static void ionic_watchdog_cb(struct timer_list *t)
 	}
 }
 
+static void ionic_napi_schedule_do_softirq(struct napi_struct *napi)
+{
+	if (napi_schedule_prep(napi)) {
+		local_bh_disable();
+		__napi_schedule(napi);
+		local_bh_enable();
+	}
+}
+
+static int ionic_get_preferred_cpu(struct ionic *ionic,
+				   struct ionic_intr_info *intr)
+{
+	int cpu;
+
+	cpu = cpumask_first_and(*intr->affinity_mask, cpu_online_mask);
+	if (cpu >= nr_cpu_ids)
+		cpu = cpumask_local_spread(0, dev_to_node(ionic->dev));
+
+	return cpu;
+}
+
+static void ionic_doorbell_check_dwork(struct work_struct *work)
+{
+	struct ionic *ionic = container_of(work, struct ionic,
+					   doorbell_check_dwork.work);
+	struct ionic_lif *lif = ionic->lif;
+
+	if (test_bit(IONIC_LIF_F_FW_STOPPING, lif->state) ||
+	    test_bit(IONIC_LIF_F_FW_RESET, lif->state))
+		return;
+
+	mutex_lock(&lif->queue_lock);
+	ionic_napi_schedule_do_softirq(&lif->adminqcq->napi);
+
+	if (test_bit(IONIC_LIF_F_UP, lif->state)) {
+		int i;
+
+		for (i = 0; i < lif->nxqs; i++) {
+			ionic_napi_schedule_do_softirq(&lif->txqcqs[i]->napi);
+			ionic_napi_schedule_do_softirq(&lif->rxqcqs[i]->napi);
+		}
+
+		if (lif->hwstamp_txq &&
+		    lif->hwstamp_txq->flags & IONIC_QCQ_F_INTR)
+			ionic_napi_schedule_do_softirq(&lif->hwstamp_txq->napi);
+		if (lif->hwstamp_rxq &&
+		    lif->hwstamp_rxq->flags & IONIC_QCQ_F_INTR)
+			ionic_napi_schedule_do_softirq(&lif->hwstamp_rxq->napi);
+	}
+	mutex_unlock(&lif->queue_lock);
+
+	ionic_queue_doorbell_check(ionic, IONIC_NAPI_DEADLINE);
+}
+
 static int ionic_watchdog_init(struct ionic *ionic)
 {
 	struct ionic_dev *idev = &ionic->idev;
@@ -70,10 +124,21 @@ static int ionic_watchdog_init(struct ionic *ionic)
 		dev_err(ionic->dev, "alloc_workqueue failed");
 		return -ENOMEM;
 	}
+	INIT_DELAYED_WORK(&ionic->doorbell_check_dwork,
+			  ionic_doorbell_check_dwork);
 
 	return 0;
 }
 
+void ionic_queue_doorbell_check(struct ionic *ionic, int delay)
+{
+	int cpu;
+
+	cpu = ionic_get_preferred_cpu(ionic, &ionic->lif->adminqcq->intr);
+	queue_delayed_work_on(cpu, ionic->wq, &ionic->doorbell_check_dwork,
+			      delay);
+}
+
 void ionic_init_devinfo(struct ionic *ionic)
 {
 	struct ionic_dev *idev = &ionic->idev;
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
index 7dbd3b8b0e36..d87e6020cfb1 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
@@ -28,7 +28,7 @@
 #define IONIC_DEV_INFO_REG_COUNT	32
 #define IONIC_DEV_CMD_REG_COUNT		32
 
-#define IONIC_NAPI_DEADLINE		(HZ / 200)	/* 5ms */
+#define IONIC_NAPI_DEADLINE		(HZ)		/* 1 sec */
 #define IONIC_ADMIN_DOORBELL_DEADLINE	(HZ / 2)	/* 500ms */
 #define IONIC_TX_DOORBELL_DEADLINE	(HZ / 100)	/* 10ms */
 #define IONIC_RX_MIN_DOORBELL_DEADLINE	(HZ / 100)	/* 10ms */
@@ -386,6 +386,7 @@ bool ionic_q_is_posted(struct ionic_queue *q, unsigned int pos);
 
 int ionic_heartbeat_check(struct ionic *ionic);
 bool ionic_is_fw_running(struct ionic_dev *idev);
+void ionic_queue_doorbell_check(struct ionic *ionic, int delay);
 
 bool ionic_adminq_poke_doorbell(struct ionic_queue *q);
 bool ionic_txq_poke_doorbell(struct ionic_queue *q);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index af269a198d5d..a04d09b8189f 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -1193,6 +1193,7 @@ static int ionic_adminq_napi(struct napi_struct *napi, int budget)
 	if (lif->adminqcq && lif->adminqcq->flags & IONIC_QCQ_F_INITED)
 		a_work = ionic_cq_service(&lif->adminqcq->cq, budget,
 					  ionic_adminq_service, NULL, NULL);
+
 	spin_unlock_irqrestore(&lif->adminq_lock, irqflags);
 
 	if (lif->hwstamp_rxq)
@@ -3408,6 +3409,7 @@ int ionic_restart_lif(struct ionic_lif *lif)
 	clear_bit(IONIC_LIF_F_FW_RESET, lif->state);
 	ionic_link_status_check_request(lif, CAN_SLEEP);
 	netif_device_attach(lif->netdev);
+	ionic_queue_doorbell_check(ionic, IONIC_NAPI_DEADLINE);
 
 	return 0;
 
@@ -3506,6 +3508,7 @@ void ionic_lif_deinit(struct ionic_lif *lif)
 	if (!test_and_clear_bit(IONIC_LIF_F_INITED, lif->state))
 		return;
 
+	cancel_delayed_work_sync(&lif->ionic->doorbell_check_dwork);
 	if (!test_bit(IONIC_LIF_F_FW_RESET, lif->state)) {
 		cancel_work_sync(&lif->deferred.work);
 		cancel_work_sync(&lif->tx_timeout_work);
-- 
2.17.1

[PATCH net-next 5/8] ionic: add per-queue napi_schedule for doorbell check

[Shannon Nelson]

Add a work item for each queue that will be run on the queue's
preferred cpu and will schedule another napi.  This napi is
run in case the device missed a doorbell and didn't process
a packet.  This is a problem for the Elba asic that happens
very rarely.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 .../net/ethernet/pensando/ionic/ionic_dev.c   | 23 +++++++++++++++++--
 .../net/ethernet/pensando/ionic/ionic_dev.h   |  1 +
 .../net/ethernet/pensando/ionic/ionic_lif.c   |  2 ++
 .../net/ethernet/pensando/ionic/ionic_lif.h   |  1 +
 4 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
index 30185ac324ff..a5a4dc21b9f3 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
@@ -56,6 +56,13 @@ static void ionic_napi_schedule_do_softirq(struct napi_struct *napi)
 	}
 }
 
+void ionic_doorbell_napi_work(struct work_struct *work)
+{
+	struct ionic_qcq *qcq = container_of(work, struct ionic_qcq,
+					     doorbell_napi_work);
+	ionic_napi_schedule_do_softirq(&qcq->napi);
+}
+
 static int ionic_get_preferred_cpu(struct ionic *ionic,
 				   struct ionic_intr_info *intr)
 {
@@ -68,6 +75,18 @@ static int ionic_get_preferred_cpu(struct ionic *ionic,
 	return cpu;
 }
 
+static void ionic_queue_dbell_napi_work(struct ionic *ionic,
+					struct ionic_qcq *qcq)
+{
+	int cpu;
+
+	if (!(qcq->flags & IONIC_QCQ_F_INTR))
+		return;
+
+	cpu = ionic_get_preferred_cpu(ionic, &qcq->intr);
+	queue_work_on(cpu, ionic->wq, &qcq->doorbell_napi_work);
+}
+
 static void ionic_doorbell_check_dwork(struct work_struct *work)
 {
 	struct ionic *ionic = container_of(work, struct ionic,
@@ -85,8 +104,8 @@ static void ionic_doorbell_check_dwork(struct work_struct *work)
 		int i;
 
 		for (i = 0; i < lif->nxqs; i++) {
-			ionic_napi_schedule_do_softirq(&lif->txqcqs[i]->napi);
-			ionic_napi_schedule_do_softirq(&lif->rxqcqs[i]->napi);
+			ionic_queue_dbell_napi_work(ionic, lif->txqcqs[i]);
+			ionic_queue_dbell_napi_work(ionic, lif->rxqcqs[i]);
 		}
 
 		if (lif->hwstamp_txq &&
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
index d87e6020cfb1..92f16b6c5662 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
@@ -386,6 +386,7 @@ bool ionic_q_is_posted(struct ionic_queue *q, unsigned int pos);
 
 int ionic_heartbeat_check(struct ionic *ionic);
 bool ionic_is_fw_running(struct ionic_dev *idev);
+void ionic_doorbell_napi_work(struct work_struct *work);
 void ionic_queue_doorbell_check(struct ionic *ionic, int delay);
 
 bool ionic_adminq_poke_doorbell(struct ionic_queue *q);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index a04d09b8189f..be601183deff 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -346,6 +346,7 @@ static int ionic_qcq_disable(struct ionic_lif *lif, struct ionic_qcq *qcq, int f
 	if (qcq->flags & IONIC_QCQ_F_INTR) {
 		struct ionic_dev *idev = &lif->ionic->idev;
 
+		cancel_work_sync(&qcq->doorbell_napi_work);
 		cancel_work_sync(&qcq->dim.work);
 		ionic_intr_mask(idev->intr_ctrl, qcq->intr.index,
 				IONIC_INTR_MASK_SET);
@@ -692,6 +693,7 @@ static int ionic_qcq_alloc(struct ionic_lif *lif, unsigned int type,
 
 	INIT_WORK(&new->dim.work, ionic_dim_work);
 	new->dim.mode = DIM_CQ_PERIOD_MODE_START_FROM_CQE;
+	INIT_WORK(&new->doorbell_napi_work, ionic_doorbell_napi_work);
 
 	*qcq = new;
 
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
index e4a5ae70793e..40b28d0b858f 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
@@ -88,6 +88,7 @@ struct ionic_qcq {
 	struct ionic_cq cq;
 	struct napi_struct napi;
 	struct ionic_intr_info intr;
+	struct work_struct doorbell_napi_work;
 	struct dentry *dentry;
 };
 
-- 
2.17.1

[PATCH net-next 6/8] ionic: check for queue deadline in doorbell_napi_work

[Shannon Nelson]

Check the deadline against the last time run and only
schedule a new napi if we haven't been run recently.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_dev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
index a5a4dc21b9f3..f3f603c90c94 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
@@ -60,7 +60,14 @@ void ionic_doorbell_napi_work(struct work_struct *work)
 {
 	struct ionic_qcq *qcq = container_of(work, struct ionic_qcq,
 					     doorbell_napi_work);
-	ionic_napi_schedule_do_softirq(&qcq->napi);
+	unsigned long now, then, dif;
+
+	now = READ_ONCE(jiffies);
+	then = qcq->q.dbell_jiffies;
+	dif = now - then;
+
+	if (dif > qcq->q.dbell_deadline)
+		ionic_napi_schedule_do_softirq(&qcq->napi);
 }
 
 static int ionic_get_preferred_cpu(struct ionic *ionic,
-- 
2.17.1

[PATCH net-next 7/8] ionic: Use an u16 for rx_copybreak

[Shannon Nelson]

From: Brett Creeley <brett.creeley@amd.com>

To make space for other data members on the first cache line reduce
rx_copybreak from an u32 to u16.  The max Rx buffer size we support
is (u16)-1 anyway so this makes sense.

Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 10 +++++++++-
 drivers/net/ethernet/pensando/ionic/ionic_lif.h     |  2 +-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index 91183965a6b7..26acd82cf6bc 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -872,10 +872,18 @@ static int ionic_set_tunable(struct net_device *dev,
 			     const void *data)
 {
 	struct ionic_lif *lif = netdev_priv(dev);
+	u32 rx_copybreak, max_rx_copybreak;
 
 	switch (tuna->id) {
 	case ETHTOOL_RX_COPYBREAK:
-		lif->rx_copybreak = *(u32 *)data;
+		rx_copybreak = *(u32 *)data;
+		max_rx_copybreak = min_t(u32, U16_MAX, IONIC_MAX_BUF_LEN);
+		if (rx_copybreak > max_rx_copybreak) {
+			netdev_err(dev, "Max supported rx_copybreak size: %u\n",
+				   max_rx_copybreak);
+			return -EINVAL;
+		}
+		lif->rx_copybreak = (u16)rx_copybreak;
 		break;
 	default:
 		return -EOPNOTSUPP;
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
index 40b28d0b858f..50fda9bdc4b8 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
@@ -206,7 +206,7 @@ struct ionic_lif {
 	unsigned int nxqs;
 	unsigned int ntxq_descs;
 	unsigned int nrxq_descs;
-	u32 rx_copybreak;
+	u16 rx_copybreak;
 	u64 rxq_features;
 	u16 rx_mode;
 	u64 hw_features;
-- 
2.17.1

[PATCH net-next 8/8] ionic: Only run the doorbell workaround for certain asic_type

[Shannon Nelson]

From: Brett Creeley <brett.creeley@amd.com>

If the doorbell workaround isn't required for a certain
asic_type then there is no need to run the associated
code. Since newer FW versions are finally reporting their
asic_type we can use a flag to determine whether or not to
do the workaround.

Signed-off-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/pensando/ionic/ionic.h   |  4 ++++
 .../net/ethernet/pensando/ionic/ionic_dev.c   | 16 +++++++++++--
 .../net/ethernet/pensando/ionic/ionic_lif.c   | 24 ++++++++++++-------
 .../net/ethernet/pensando/ionic/ionic_lif.h   |  1 +
 .../net/ethernet/pensando/ionic/ionic_txrx.c  | 14 ++++++-----
 5 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic.h b/drivers/net/ethernet/pensando/ionic/ionic.h
index 106ee5b2ceff..1c61390677f7 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic.h
@@ -18,6 +18,8 @@ struct ionic_lif;
 #define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_PF	0x1002
 #define PCI_DEVICE_ID_PENSANDO_IONIC_ETH_VF	0x1003
 
+#define IONIC_ASIC_TYPE_ELBA	2
+
 #define DEVCMD_TIMEOUT			5
 #define IONIC_ADMINQ_TIME_SLICE		msecs_to_jiffies(100)
 
@@ -96,4 +98,6 @@ int ionic_port_identify(struct ionic *ionic);
 int ionic_port_init(struct ionic *ionic);
 int ionic_port_reset(struct ionic *ionic);
 
+bool ionic_doorbell_wa(struct ionic *ionic);
+
 #endif /* _IONIC_H_ */
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
index f3f603c90c94..d18ea1b4a8b9 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
@@ -127,6 +127,13 @@ static void ionic_doorbell_check_dwork(struct work_struct *work)
 	ionic_queue_doorbell_check(ionic, IONIC_NAPI_DEADLINE);
 }
 
+bool ionic_doorbell_wa(struct ionic *ionic)
+{
+	u8 asic_type = ionic->idev.dev_info.asic_type;
+
+	return !asic_type || asic_type == IONIC_ASIC_TYPE_ELBA;
+}
+
 static int ionic_watchdog_init(struct ionic *ionic)
 {
 	struct ionic_dev *idev = &ionic->idev;
@@ -150,8 +157,10 @@ static int ionic_watchdog_init(struct ionic *ionic)
 		dev_err(ionic->dev, "alloc_workqueue failed");
 		return -ENOMEM;
 	}
-	INIT_DELAYED_WORK(&ionic->doorbell_check_dwork,
-			  ionic_doorbell_check_dwork);
+
+	if (ionic_doorbell_wa(ionic))
+		INIT_DELAYED_WORK(&ionic->doorbell_check_dwork,
+				  ionic_doorbell_check_dwork);
 
 	return 0;
 }
@@ -160,6 +169,9 @@ void ionic_queue_doorbell_check(struct ionic *ionic, int delay)
 {
 	int cpu;
 
+	if (!ionic->lif->doorbell_wa)
+		return;
+
 	cpu = ionic_get_preferred_cpu(ionic, &ionic->lif->adminqcq->intr);
 	queue_delayed_work_on(cpu, ionic->wq, &ionic->doorbell_check_dwork,
 			      delay);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index be601183deff..b8813809bb75 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -346,7 +346,8 @@ static int ionic_qcq_disable(struct ionic_lif *lif, struct ionic_qcq *qcq, int f
 	if (qcq->flags & IONIC_QCQ_F_INTR) {
 		struct ionic_dev *idev = &lif->ionic->idev;
 
-		cancel_work_sync(&qcq->doorbell_napi_work);
+		if (lif->doorbell_wa)
+			cancel_work_sync(&qcq->doorbell_napi_work);
 		cancel_work_sync(&qcq->dim.work);
 		ionic_intr_mask(idev->intr_ctrl, qcq->intr.index,
 				IONIC_INTR_MASK_SET);
@@ -693,7 +694,8 @@ static int ionic_qcq_alloc(struct ionic_lif *lif, unsigned int type,
 
 	INIT_WORK(&new->dim.work, ionic_dim_work);
 	new->dim.mode = DIM_CQ_PERIOD_MODE_START_FROM_CQE;
-	INIT_WORK(&new->doorbell_napi_work, ionic_doorbell_napi_work);
+	if (lif->doorbell_wa)
+		INIT_WORK(&new->doorbell_napi_work, ionic_doorbell_napi_work);
 
 	*qcq = new;
 
@@ -1217,12 +1219,14 @@ static int ionic_adminq_napi(struct napi_struct *napi, int budget)
 		ionic_intr_credits(idev->intr_ctrl, intr->index, credits, flags);
 	}
 
-	if (!a_work)
-		ionic_adminq_poke_doorbell(&lif->adminqcq->q);
-	if (lif->hwstamp_rxq && !rx_work)
-		ionic_rxq_poke_doorbell(&lif->hwstamp_rxq->q);
-	if (lif->hwstamp_txq && !tx_work)
-		ionic_txq_poke_doorbell(&lif->hwstamp_txq->q);
+	if (lif->doorbell_wa) {
+		if (!a_work)
+			ionic_adminq_poke_doorbell(&lif->adminqcq->q);
+		if (lif->hwstamp_rxq && !rx_work)
+			ionic_rxq_poke_doorbell(&lif->hwstamp_rxq->q);
+		if (lif->hwstamp_txq && !tx_work)
+			ionic_txq_poke_doorbell(&lif->hwstamp_txq->q);
+	}
 
 	return work_done;
 }
@@ -3510,7 +3514,8 @@ void ionic_lif_deinit(struct ionic_lif *lif)
 	if (!test_and_clear_bit(IONIC_LIF_F_INITED, lif->state))
 		return;
 
-	cancel_delayed_work_sync(&lif->ionic->doorbell_check_dwork);
+	if (lif->doorbell_wa)
+		cancel_delayed_work_sync(&lif->ionic->doorbell_check_dwork);
 	if (!test_bit(IONIC_LIF_F_FW_RESET, lif->state)) {
 		cancel_work_sync(&lif->deferred.work);
 		cancel_work_sync(&lif->tx_timeout_work);
@@ -3752,6 +3757,7 @@ int ionic_lif_init(struct ionic_lif *lif)
 		goto err_out_notifyq_deinit;
 
 	lif->rx_copybreak = IONIC_RX_COPYBREAK_DEFAULT;
+	lif->doorbell_wa = ionic_doorbell_wa(lif->ionic);
 
 	set_bit(IONIC_LIF_F_INITED, lif->state);
 
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
index 50fda9bdc4b8..e5bcdd2cf86a 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
@@ -207,6 +207,7 @@ struct ionic_lif {
 	unsigned int ntxq_descs;
 	unsigned int nrxq_descs;
 	u16 rx_copybreak;
+	u8 doorbell_wa:1;
 	u64 rxq_features;
 	u16 rx_mode;
 	u64 hw_features;
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
index 3066eb4788f9..b381863b54ef 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
@@ -949,7 +949,7 @@ int ionic_tx_napi(struct napi_struct *napi, int budget)
 				   work_done, flags);
 	}
 
-	if (!work_done)
+	if (!work_done && cq->bound_q->lif->doorbell_wa)
 		ionic_txq_poke_doorbell(&qcq->q);
 
 	return work_done;
@@ -992,7 +992,7 @@ int ionic_rx_napi(struct napi_struct *napi, int budget)
 				   work_done, flags);
 	}
 
-	if (!work_done)
+	if (!work_done && cq->bound_q->lif->doorbell_wa)
 		ionic_rxq_poke_doorbell(&qcq->q);
 
 	return work_done;
@@ -1037,10 +1037,12 @@ int ionic_txrx_napi(struct napi_struct *napi, int budget)
 				   tx_work_done + rx_work_done, flags);
 	}
 
-	if (!rx_work_done)
-		ionic_rxq_poke_doorbell(&rxqcq->q);
-	if (!tx_work_done)
-		ionic_txq_poke_doorbell(&txqcq->q);
+	if (lif->doorbell_wa) {
+		if (!rx_work_done)
+			ionic_rxq_poke_doorbell(&rxqcq->q);
+		if (!tx_work_done)
+			ionic_txq_poke_doorbell(&txqcq->q);
+	}
 
 	return rx_work_done;
 }
-- 
2.17.1

Re: [PATCH net-next 3/8] ionic: add private workqueue per-device

[Jakub Kicinski]

On Mon, 10 Jun 2024 16:07:01 -0700 Shannon Nelson wrote:
> Instead of using the system's default workqueue,
> add a private workqueue for the device to use for
> its little jobs.

little jobs little point of having your own wq, no?
At this point of reading the series its a bit unclear why 
the wq separation is needed.

Re: [PATCH net-next 4/8] ionic: add work item for missed-doorbell check

[Jakub Kicinski]

On Mon, 10 Jun 2024 16:07:02 -0700 Shannon Nelson wrote:
> +static void ionic_napi_schedule_do_softirq(struct napi_struct *napi)
> +{
> +	if (napi_schedule_prep(napi)) {
> +		local_bh_disable();
> +		__napi_schedule(napi);
> +		local_bh_enable();

No need to open code napi_schedule()

	local_bh_disable();
	napi_schedule(napi);
	local_bh_enable();

is a fairly well-established pattern

> +	}
> +}

> +static void ionic_doorbell_check_dwork(struct work_struct *work)
> +{
> +	struct ionic *ionic = container_of(work, struct ionic,
> +					   doorbell_check_dwork.work);
> +	struct ionic_lif *lif = ionic->lif;
> +
> +	if (test_bit(IONIC_LIF_F_FW_STOPPING, lif->state) ||
> +	    test_bit(IONIC_LIF_F_FW_RESET, lif->state))
> +		return;
> +
> +	mutex_lock(&lif->queue_lock);

This will deadlock under very inopportune circumstances, no?

The best way of implementing periodic checks using a workqueue is to
only cancel it sync from the .remove callback, before you free the
netdev. Otherwise cancel it non-sync or don't cancel at all, and once
it takes the lock double check the device is still actually running.

Re: [PATCH net-next 3/8] ionic: add private workqueue per-device

[Nelson, Shannon]

On 6/12/2024 6:08 PM, Jakub Kicinski wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> On Mon, 10 Jun 2024 16:07:01 -0700 Shannon Nelson wrote:
>> Instead of using the system's default workqueue,
>> add a private workqueue for the device to use for
>> its little jobs.
> 
> little jobs little point of having your own wq, no?
> At this point of reading the series its a bit unclear why
> the wq separation is needed.

Yes, when using only a single PF or two this doesn't look so bad to be 
left on the system workqueue.  But we have a couple customers that want 
to scale out to lots of VFs with multiple queues per VF, which 
multiplies out 100's of queues getting workitems.  We thought that 
instead of firebombing the system workqueue with a lot of little jobs, 
we would give the scheduler a chance to work with our stuff separately, 
and setting it up by device seemed like an easy enough way to partition 
the work.  Other options might be one ionic wq used by all devices, or 
maybe a wq per PF family?  Do you have a preference, or do you still 
think that the system wq is enough?

Thanks,
sln

Re: [PATCH net-next 4/8] ionic: add work item for missed-doorbell check

[Nelson, Shannon]

On 6/12/2024 6:19 PM, Jakub Kicinski wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Mon, 10 Jun 2024 16:07:02 -0700 Shannon Nelson wrote:
>> +static void ionic_napi_schedule_do_softirq(struct napi_struct *napi)
>> +{
>> +     if (napi_schedule_prep(napi)) {
>> +             local_bh_disable();
>> +             __napi_schedule(napi);
>> +             local_bh_enable();
> 
> No need to open code napi_schedule()
> 
>          local_bh_disable();
>          napi_schedule(napi);
>          local_bh_enable();
> 
> is a fairly well-established pattern

Sure, we can do that.

> 
>> +     }
>> +}
> 
>> +static void ionic_doorbell_check_dwork(struct work_struct *work)
>> +{
>> +     struct ionic *ionic = container_of(work, struct ionic,
>> +                                        doorbell_check_dwork.work);
>> +     struct ionic_lif *lif = ionic->lif;
>> +
>> +     if (test_bit(IONIC_LIF_F_FW_STOPPING, lif->state) ||
>> +         test_bit(IONIC_LIF_F_FW_RESET, lif->state))
>> +             return;
>> +
>> +     mutex_lock(&lif->queue_lock);
> 
> This will deadlock under very inopportune circumstances, no?
> 
> The best way of implementing periodic checks using a workqueue is to
> only cancel it sync from the .remove callback, before you free the
> netdev. Otherwise cancel it non-sync or don't cancel at all, and once
> it takes the lock double check the device is still actually running.

Hmmm... we'll dig a little more on this.

Thanks,
sln

Re: [PATCH net-next 3/8] ionic: add private workqueue per-device

[Jakub Kicinski]

On Thu, 13 Jun 2024 13:38:18 -0700 Nelson, Shannon wrote:
> > little jobs little point of having your own wq, no?
> > At this point of reading the series its a bit unclear why
> > the wq separation is needed.  
> 
> Yes, when using only a single PF or two this doesn't look so bad to be 
> left on the system workqueue.  But we have a couple customers that want 
> to scale out to lots of VFs with multiple queues per VF, which 
> multiplies out 100's of queues getting workitems.  We thought that 
> instead of firebombing the system workqueue with a lot of little jobs, 
> we would give the scheduler a chance to work with our stuff separately, 
> and setting it up by device seemed like an easy enough way to partition 
> the work.  Other options might be one ionic wq used by all devices, or 
> maybe a wq per PF family?  Do you have a preference, or do you still 
> think that the system wq is enough?

No, no, code is fine. I was just complaining about the commit message :)
The math for how many work items you can see in reasonable scenarios
would be helpful to see when judging the need.

Re: [PATCH net-next 3/8] ionic: add private workqueue per-device

[Nelson, Shannon]

On 6/13/2024 3:19 PM, Jakub Kicinski wrote:
> 
> On Thu, 13 Jun 2024 13:38:18 -0700 Nelson, Shannon wrote:
>>> little jobs little point of having your own wq, no?
>>> At this point of reading the series its a bit unclear why
>>> the wq separation is needed.
>>
>> Yes, when using only a single PF or two this doesn't look so bad to be
>> left on the system workqueue.  But we have a couple customers that want
>> to scale out to lots of VFs with multiple queues per VF, which
>> multiplies out 100's of queues getting workitems.  We thought that
>> instead of firebombing the system workqueue with a lot of little jobs,
>> we would give the scheduler a chance to work with our stuff separately,
>> and setting it up by device seemed like an easy enough way to partition
>> the work.  Other options might be one ionic wq used by all devices, or
>> maybe a wq per PF family?  Do you have a preference, or do you still
>> think that the system wq is enough?
> 
> No, no, code is fine. I was just complaining about the commit message :)
> The math for how many work items you can see in reasonable scenarios
> would be helpful to see when judging the need.

Sure, I can add to the description - thanks.
sln

Re: [PATCH net-next 4/8] ionic: add work item for missed-doorbell check

[Przemek Kitszel]

On 6/13/24 22:38, Nelson, Shannon wrote:
> 
> 
> On 6/12/2024 6:19 PM, Jakub Kicinski wrote:
>> Caution: This message originated from an External Source. Use proper 
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> On Mon, 10 Jun 2024 16:07:02 -0700 Shannon Nelson wrote:
>>> +static void ionic_napi_schedule_do_softirq(struct napi_struct *napi)
>>> +{
>>> +     if (napi_schedule_prep(napi)) {
>>> +             local_bh_disable();
>>> +             __napi_schedule(napi);
>>> +             local_bh_enable();
>>
>> No need to open code napi_schedule()
>>
>>          local_bh_disable();
>>          napi_schedule(napi);
>>          local_bh_enable();
>>
>> is a fairly well-established pattern
> 
> Sure, we can do that.
> 
>>
>>> +     }
>>> +}
>>
>>> +static void ionic_doorbell_check_dwork(struct work_struct *work)
>>> +{
>>> +     struct ionic *ionic = container_of(work, struct ionic,
>>> +                                        doorbell_check_dwork.work);
>>> +     struct ionic_lif *lif = ionic->lif;
>>> +
>>> +     if (test_bit(IONIC_LIF_F_FW_STOPPING, lif->state) ||
>>> +         test_bit(IONIC_LIF_F_FW_RESET, lif->state))
>>> +             return;
>>> +
>>> +     mutex_lock(&lif->queue_lock);
>>
>> This will deadlock under very inopportune circumstances, no?
>>
>> The best way of implementing periodic checks using a workqueue is to
>> only cancel it sync from the .remove callback, before you free the
>> netdev. Otherwise cancel it non-sync or don't cancel at all, and once
>> it takes the lock double check the device is still actually running.
> 
> Hmmm... we'll dig a little more on this.
> 
> Thanks,
> sln

We had a very similar error (with stopping a VF, IIRC); it's easiest to
repro on RT kernels

RE: [PATCH net-next 7/8] ionic: Use an u16 for rx_copybreak

[David Laight]

From: Shannon Nelson
> Sent: 11 June 2024 00:07
> 
> From: Brett Creeley <brett.creeley@amd.com>
> 
> To make space for other data members on the first cache line reduce
> rx_copybreak from an u32 to u16.  The max Rx buffer size we support
> is (u16)-1 anyway so this makes sense.
> 
> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 10 +++++++++-
>  drivers/net/ethernet/pensando/ionic/ionic_lif.h     |  2 +-
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> index 91183965a6b7..26acd82cf6bc 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> @@ -872,10 +872,18 @@ static int ionic_set_tunable(struct net_device *dev,
>  			     const void *data)
>  {
>  	struct ionic_lif *lif = netdev_priv(dev);
> +	u32 rx_copybreak, max_rx_copybreak;
> 
>  	switch (tuna->id) {
>  	case ETHTOOL_RX_COPYBREAK:
> -		lif->rx_copybreak = *(u32 *)data;
> +		rx_copybreak = *(u32 *)data;
> +		max_rx_copybreak = min_t(u32, U16_MAX, IONIC_MAX_BUF_LEN);

I doubt that needs to be min_t() or that you really need the temporary.

> +		if (rx_copybreak > max_rx_copybreak) {
> +			netdev_err(dev, "Max supported rx_copybreak size: %u\n",
> +				   max_rx_copybreak);
> +			return -EINVAL;
> +		}
> +		lif->rx_copybreak = (u16)rx_copybreak;
>  		break;
>  	default:
>  		return -EOPNOTSUPP;
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> index 40b28d0b858f..50fda9bdc4b8 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> @@ -206,7 +206,7 @@ struct ionic_lif {
>  	unsigned int nxqs;
>  	unsigned int ntxq_descs;
>  	unsigned int nrxq_descs;
> -	u32 rx_copybreak;
> +	u16 rx_copybreak;
>  	u64 rxq_features;
>  	u16 rx_mode;

There seem to be 6 pad bytes here - why not just use them??

>  	u64 hw_features;

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH net-next 7/8] ionic: Use an u16 for rx_copybreak

[Andrew Lunn]

> > --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> > +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> > @@ -206,7 +206,7 @@ struct ionic_lif {
> >  	unsigned int nxqs;
> >  	unsigned int ntxq_descs;
> >  	unsigned int nrxq_descs;
> > -	u32 rx_copybreak;
> > +	u16 rx_copybreak;
> >  	u64 rxq_features;
> >  	u16 rx_mode;
> 
> There seem to be 6 pad bytes here - why not just use them??

Or at least move rx_copybreak next to rx_mode so the compiler can pack
them together.

It would be good to include some output from pahole in the commit
message to show the goal of this patch has actually been reached.

     Andrew

RE: [PATCH net-next 7/8] ionic: Use an u16 for rx_copybreak

[David Laight]

From: Andrew Lunn
> Sent: 16 June 2024 02:29
> 
> > > --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> > > +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> > > @@ -206,7 +206,7 @@ struct ionic_lif {
> > >  	unsigned int nxqs;
> > >  	unsigned int ntxq_descs;
> > >  	unsigned int nrxq_descs;
> > > -	u32 rx_copybreak;
> > > +	u16 rx_copybreak;
> > >  	u64 rxq_features;
> > >  	u16 rx_mode;
> >
> > There seem to be 6 pad bytes here - why not just use them??
> 
> Or at least move rx_copybreak next to rx_mode so the compiler can pack
> them together.
> 
> It would be good to include some output from pahole in the commit
> message to show the goal of this patch has actually been reached.

And then start asking whether the fields are grouped for cache usage at all.
And whether the structure itself is allocated by kmalloc() or is nested
in something else.

You might worry about structure holes because they make the structure
larger - but that only matters if the allocator rounds it up to a
bigger size. And that only really matters if you allocate lots of them.

So the dominant part of this change is probably the extra code.

When you add the extra flag as 'uint flag:1' you also generate worse
code that just using a 'u8' - so not worth it unless you need to pack
multiple flags into a word.

Of course, slight re-ordering to avoid holes is usually fine.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH net-next 7/8] ionic: Use an u16 for rx_copybreak

[Brett Creeley]

On 6/15/2024 2:20 PM, David Laight wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> From: Shannon Nelson
>> Sent: 11 June 2024 00:07
>>
>> From: Brett Creeley <brett.creeley@amd.com>
>>
>> To make space for other data members on the first cache line reduce
>> rx_copybreak from an u32 to u16.  The max Rx buffer size we support
>> is (u16)-1 anyway so this makes sense.
>>
>> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> ---
>>   drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 10 +++++++++-
>>   drivers/net/ethernet/pensando/ionic/ionic_lif.h     |  2 +-
>>   2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>> b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>> index 91183965a6b7..26acd82cf6bc 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>> @@ -872,10 +872,18 @@ static int ionic_set_tunable(struct net_device *dev,
>>                             const void *data)
>>   {
>>        struct ionic_lif *lif = netdev_priv(dev);
>> +     u32 rx_copybreak, max_rx_copybreak;
>>
>>        switch (tuna->id) {
>>        case ETHTOOL_RX_COPYBREAK:
>> -             lif->rx_copybreak = *(u32 *)data;
>> +             rx_copybreak = *(u32 *)data;
>> +             max_rx_copybreak = min_t(u32, U16_MAX, IONIC_MAX_BUF_LEN);
> 
> I doubt that needs to be min_t() or that you really need the temporary.

IMHO the temporary variable here makes it more readable than comparing 
directly to the casted/de-referenced opaque data pointer and then 
assigning to the rx_copybreak member if it's a valid value.

We can double check that min_t() is required.

> 
>> +             if (rx_copybreak > max_rx_copybreak) {
>> +                     netdev_err(dev, "Max supported rx_copybreak size: %u\n",
>> +                                max_rx_copybreak);
>> +                     return -EINVAL;
>> +             }
>> +             lif->rx_copybreak = (u16)rx_copybreak;
>>                break;
>>        default:
>>                return -EOPNOTSUPP;
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> index 40b28d0b858f..50fda9bdc4b8 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> @@ -206,7 +206,7 @@ struct ionic_lif {
>>        unsigned int nxqs;
>>        unsigned int ntxq_descs;
>>        unsigned int nrxq_descs;
>> -     u32 rx_copybreak;
>> +     u16 rx_copybreak;
>>        u64 rxq_features;
>>        u16 rx_mode;
> 
> There seem to be 6 pad bytes here - why not just use them??

Thanks for pointing this out. It looks like I missed the differences 
between the OOT and upstream ionic_lif structure when reviewing this 
patch. We will take another look.

Thanks for the review,

Brett
> 
>>        u64 hw_features;
> 
>          David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

RE: [PATCH net-next 7/8] ionic: Use an u16 for rx_copybreak

[David Laight]

From: Brett Creeley
> Sent: 17 June 2024 17:25
> 
> On 6/15/2024 2:20 PM, David Laight wrote:
> >
> > From: Shannon Nelson
> >> Sent: 11 June 2024 00:07
> >>
> >> From: Brett Creeley <brett.creeley@amd.com>
> >>
> >> To make space for other data members on the first cache line reduce
> >> rx_copybreak from an u32 to u16.  The max Rx buffer size we support
> >> is (u16)-1 anyway so this makes sense.
> >>
> >> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
> >> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> >> ---
> >>   drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 10 +++++++++-
> >>   drivers/net/ethernet/pensando/ionic/ionic_lif.h     |  2 +-
> >>   2 files changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> >> b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> >> index 91183965a6b7..26acd82cf6bc 100644
> >> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> >> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> >> @@ -872,10 +872,18 @@ static int ionic_set_tunable(struct net_device *dev,
> >>                             const void *data)
> >>   {
> >>        struct ionic_lif *lif = netdev_priv(dev);
> >> +     u32 rx_copybreak, max_rx_copybreak;
> >>
> >>        switch (tuna->id) {
> >>        case ETHTOOL_RX_COPYBREAK:
> >> -             lif->rx_copybreak = *(u32 *)data;
> >> +             rx_copybreak = *(u32 *)data;
> >> +             max_rx_copybreak = min_t(u32, U16_MAX, IONIC_MAX_BUF_LEN);
> >
> > I doubt that needs to be min_t() or that you really need the temporary.
> 
> IMHO the temporary variable here makes it more readable than comparing
> directly to the casted/de-referenced opaque data pointer and then
> assigning to the rx_copybreak member if it's a valid value.
...

I was thinking of the temporary for the result of min().

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH net-next 7/8] ionic: Use an u16 for rx_copybreak

[Brett Creeley]

On 6/18/2024 1:16 AM, David Laight wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> From: Brett Creeley
>> Sent: 17 June 2024 17:25
>>
>> On 6/15/2024 2:20 PM, David Laight wrote:
>>>
>>> From: Shannon Nelson
>>>> Sent: 11 June 2024 00:07
>>>>
>>>> From: Brett Creeley <brett.creeley@amd.com>
>>>>
>>>> To make space for other data members on the first cache line reduce
>>>> rx_copybreak from an u32 to u16.  The max Rx buffer size we support
>>>> is (u16)-1 anyway so this makes sense.
>>>>
>>>> Signed-off-by: Brett Creeley <brett.creeley@amd.com>
>>>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>>>> ---
>>>>    drivers/net/ethernet/pensando/ionic/ionic_ethtool.c | 10 +++++++++-
>>>>    drivers/net/ethernet/pensando/ionic/ionic_lif.h     |  2 +-
>>>>    2 files changed, 10 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>>>> b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>>>> index 91183965a6b7..26acd82cf6bc 100644
>>>> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>>>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>>>> @@ -872,10 +872,18 @@ static int ionic_set_tunable(struct net_device *dev,
>>>>                              const void *data)
>>>>    {
>>>>         struct ionic_lif *lif = netdev_priv(dev);
>>>> +     u32 rx_copybreak, max_rx_copybreak;
>>>>
>>>>         switch (tuna->id) {
>>>>         case ETHTOOL_RX_COPYBREAK:
>>>> -             lif->rx_copybreak = *(u32 *)data;
>>>> +             rx_copybreak = *(u32 *)data;
>>>> +             max_rx_copybreak = min_t(u32, U16_MAX, IONIC_MAX_BUF_LEN);
>>>
>>> I doubt that needs to be min_t() or that you really need the temporary.
>>
>> IMHO the temporary variable here makes it more readable than comparing
>> directly to the casted/de-referenced opaque data pointer and then
>> assigning to the rx_copybreak member if it's a valid value.
> ...
> 
> I was thinking of the temporary for the result of min().

Yeah. I think a #define would be better yet. Thanks for pointing this out.

Brett

> 
>          David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)