[PATCH v2 net-next 0/3] ionic: on-chip descriptors

[PATCH v2 net-next 0/3] ionic: on-chip descriptors

[Shannon Nelson]

We start with a couple of house-keeping patches that were
originally presented for 'net', then we add support for on-chip
descriptor rings.

v2: dropped the rx buffers patch

Shannon Nelson (3):
  ionic: remove unnecessary indirection
  ionic: remove unnecessary void casts
  ionic: add support for device Component Memory Buffers

 .../ethernet/pensando/ionic/ionic_bus_pci.c   |   7 +-
 .../net/ethernet/pensando/ionic/ionic_dev.c   |  71 ++++++++
 .../net/ethernet/pensando/ionic/ionic_dev.h   |  13 ++
 .../ethernet/pensando/ionic/ionic_ethtool.c   | 128 +++++++++++++-
 .../ethernet/pensando/ionic/ionic_ethtool.h   |   1 +
 .../net/ethernet/pensando/ionic/ionic_if.h    |   3 +-
 .../net/ethernet/pensando/ionic/ionic_lif.c   | 164 ++++++++++++++++--
 .../net/ethernet/pensando/ionic/ionic_lif.h   |  32 +++-
 .../net/ethernet/pensando/ionic/ionic_main.c  |   4 +-
 .../net/ethernet/pensando/ionic/ionic_phc.c   |   2 +-
 .../ethernet/pensando/ionic/ionic_rx_filter.c |   4 +-
 .../net/ethernet/pensando/ionic/ionic_txrx.c  |  22 ++-
 12 files changed, 422 insertions(+), 29 deletions(-)

-- 
2.17.1

[PATCH v2 net-next 1/3] ionic: remove unnecessary indirection

[Shannon Nelson]

We have the pointer already, don't need to go through the
lif struct for it.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_lif.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 4dd16c487f2b..8499165b1563 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -148,7 +148,7 @@ static void ionic_link_status_check(struct ionic_lif *lif)
 			mutex_lock(&lif->queue_lock);
 			err = ionic_start_queues(lif);
 			if (err && err != -EBUSY) {
-				netdev_err(lif->netdev,
+				netdev_err(netdev,
 					   "Failed to start queues: %d\n", err);
 				set_bit(IONIC_LIF_F_BROKEN, lif->state);
 				netif_carrier_off(lif->netdev);
@@ -2463,7 +2463,7 @@ static int ionic_set_vf_rate(struct net_device *netdev, int vf,
 
 		ret = ionic_set_vf_config(ionic, vf, &vfc);
 		if (!ret)
-			lif->ionic->vfs[vf].maxrate = cpu_to_le32(tx_max);
+			ionic->vfs[vf].maxrate = cpu_to_le32(tx_max);
 	}
 
 	up_write(&ionic->vf_op_lock);
-- 
2.17.1

[PATCH v2 net-next 2/3] ionic: remove unnecessary void casts

[Shannon Nelson]

Minor Code cleanup details.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
---
 drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c   | 4 ++--
 drivers/net/ethernet/pensando/ionic/ionic_main.c      | 4 ++--
 drivers/net/ethernet/pensando/ionic/ionic_phc.c       | 2 +-
 drivers/net/ethernet/pensando/ionic/ionic_rx_filter.c | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
index ce436e97324a..0eff78fa0565 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
@@ -121,7 +121,7 @@ static void ionic_vf_dealloc_locked(struct ionic *ionic)
 
 		if (v->stats_pa) {
 			vfc.stats_pa = 0;
-			(void)ionic_set_vf_config(ionic, i, &vfc);
+			ionic_set_vf_config(ionic, i, &vfc);
 			dma_unmap_single(ionic->dev, v->stats_pa,
 					 sizeof(v->stats), DMA_FROM_DEVICE);
 			v->stats_pa = 0;
@@ -169,7 +169,7 @@ static int ionic_vf_alloc(struct ionic *ionic, int num_vfs)
 
 		/* ignore failures from older FW, we just won't get stats */
 		vfc.stats_pa = cpu_to_le64(v->stats_pa);
-		(void)ionic_set_vf_config(ionic, i, &vfc);
+		ionic_set_vf_config(ionic, i, &vfc);
 	}
 
 out:
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_main.c b/drivers/net/ethernet/pensando/ionic/ionic_main.c
index a13530ec4dd8..79d4dfa9e07e 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_main.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_main.c
@@ -359,7 +359,7 @@ int ionic_adminq_wait(struct ionic_lif *lif, struct ionic_admin_ctx *ctx,
 			break;
 
 		/* force a check of FW status and break out if FW reset */
-		(void)ionic_heartbeat_check(lif->ionic);
+		ionic_heartbeat_check(lif->ionic);
 		if ((test_bit(IONIC_LIF_F_FW_RESET, lif->state) &&
 		     !lif->ionic->idev.fw_status_ready) ||
 		    test_bit(IONIC_LIF_F_FW_STOPPING, lif->state)) {
@@ -647,7 +647,7 @@ int ionic_port_init(struct ionic *ionic)
 	err = ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
 
 	ionic_dev_cmd_port_state(&ionic->idev, IONIC_PORT_ADMIN_STATE_UP);
-	(void)ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
+	ionic_dev_cmd_wait(ionic, DEVCMD_TIMEOUT);
 
 	mutex_unlock(&ionic->dev_cmd_lock);
 	if (err) {
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_phc.c b/drivers/net/ethernet/pensando/ionic/ionic_phc.c
index 887046838b3b..eac2f0e3576e 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_phc.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_phc.c
@@ -268,7 +268,7 @@ static u64 ionic_hwstamp_read(struct ionic *ionic,
 	u32 tick_high_before, tick_high, tick_low;
 
 	/* read and discard low part to defeat hw staging of high part */
-	(void)ioread32(&ionic->idev.hwstamp_regs->tick_low);
+	ioread32(&ionic->idev.hwstamp_regs->tick_low);
 
 	tick_high_before = ioread32(&ionic->idev.hwstamp_regs->tick_high);
 
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_rx_filter.c b/drivers/net/ethernet/pensando/ionic/ionic_rx_filter.c
index b7363376dfc8..1ee2f285cb42 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_rx_filter.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_rx_filter.c
@@ -604,14 +604,14 @@ void ionic_rx_filter_sync(struct ionic_lif *lif)
 	 * they can clear room for some new filters
 	 */
 	list_for_each_entry_safe(sync_item, spos, &sync_del_list, list) {
-		(void)ionic_lif_filter_del(lif, &sync_item->f.cmd);
+		ionic_lif_filter_del(lif, &sync_item->f.cmd);
 
 		list_del(&sync_item->list);
 		devm_kfree(dev, sync_item);
 	}
 
 	list_for_each_entry_safe(sync_item, spos, &sync_add_list, list) {
-		(void)ionic_lif_filter_add(lif, &sync_item->f.cmd);
+		ionic_lif_filter_add(lif, &sync_item->f.cmd);
 
 		list_del(&sync_item->list);
 		devm_kfree(dev, sync_item);
-- 
2.17.1

[PATCH v2 net-next 3/3] ionic: add support for device Component Memory Buffers

[Shannon Nelson]

The ionic device has on-board memory (CMB) that can be used
for descriptors as a way to speed descriptor access for faster
traffic processing.  It imposes a couple of restrictions so
is not on by default, but can be enabled through the ethtool
priv-flags.

Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 .../ethernet/pensando/ionic/ionic_bus_pci.c   |   3 +
 .../net/ethernet/pensando/ionic/ionic_dev.c   |  71 ++++++++
 .../net/ethernet/pensando/ionic/ionic_dev.h   |  13 ++
 .../ethernet/pensando/ionic/ionic_ethtool.c   | 128 +++++++++++++-
 .../ethernet/pensando/ionic/ionic_ethtool.h   |   1 +
 .../net/ethernet/pensando/ionic/ionic_if.h    |   3 +-
 .../net/ethernet/pensando/ionic/ionic_lif.c   | 160 ++++++++++++++++--
 .../net/ethernet/pensando/ionic/ionic_lif.h   |  32 +++-
 .../net/ethernet/pensando/ionic/ionic_txrx.c  |  22 ++-
 9 files changed, 413 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
index 0eff78fa0565..f62f11ba21c0 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
@@ -352,6 +352,7 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 err_out_reset:
 	ionic_reset(ionic);
 err_out_teardown:
+	ionic_dev_teardown(ionic);
 	pci_clear_master(pdev);
 	/* Don't fail the probe for these errors, keep
 	 * the hw interface around for inspection
@@ -369,6 +370,7 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 err_out_clear_drvdata:
 	mutex_destroy(&ionic->dev_cmd_lock);
 	ionic_devlink_free(ionic);
+	pci_set_drvdata(pdev, NULL);
 
 	return err;
 }
@@ -390,6 +392,7 @@ static void ionic_remove(struct pci_dev *pdev)
 
 	ionic_port_reset(ionic);
 	ionic_reset(ionic);
+	ionic_dev_teardown(ionic);
 	pci_clear_master(pdev);
 	ionic_unmap_bars(ionic);
 	pci_release_regions(pdev);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
index 626b9113e7c4..9b4bba2279ab 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
@@ -92,6 +92,7 @@ int ionic_dev_setup(struct ionic *ionic)
 	unsigned int num_bars = ionic->num_bars;
 	struct ionic_dev *idev = &ionic->idev;
 	struct device *dev = ionic->dev;
+	int size;
 	u32 sig;
 
 	/* BAR0: dev_cmd and interrupts */
@@ -133,9 +134,40 @@ int ionic_dev_setup(struct ionic *ionic)
 	idev->db_pages = bar->vaddr;
 	idev->phy_db_pages = bar->bus_addr;
 
+	/* BAR2: optional controller memory mapping */
+	bar++;
+	mutex_init(&idev->cmb_inuse_lock);
+	if (num_bars < 3 || !ionic->bars[IONIC_PCI_BAR_CMB].len) {
+		idev->cmb_inuse = NULL;
+		idev->phy_cmb_pages = 0;
+		idev->cmb_npages = 0;
+		return 0;
+	}
+
+	idev->phy_cmb_pages = bar->bus_addr;
+	idev->cmb_npages = bar->len / PAGE_SIZE;
+	size = BITS_TO_LONGS(idev->cmb_npages) * sizeof(long);
+	idev->cmb_inuse = kzalloc(size, GFP_KERNEL);
+	if (!idev->cmb_inuse) {
+		idev->phy_cmb_pages = 0;
+		idev->cmb_npages = 0;
+	}
+
 	return 0;
 }
 
+void ionic_dev_teardown(struct ionic *ionic)
+{
+	struct ionic_dev *idev = &ionic->idev;
+
+	kfree(idev->cmb_inuse);
+	idev->cmb_inuse = NULL;
+	idev->phy_cmb_pages = 0;
+	idev->cmb_npages = 0;
+
+	mutex_destroy(&idev->cmb_inuse_lock);
+}
+
 /* Devcmd Interface */
 bool ionic_is_fw_running(struct ionic_dev *idev)
 {
@@ -571,6 +603,33 @@ int ionic_db_page_num(struct ionic_lif *lif, int pid)
 	return (lif->hw_index * lif->dbid_count) + pid;
 }
 
+int ionic_get_cmb(struct ionic_lif *lif, u32 *pgid, phys_addr_t *pgaddr, int order)
+{
+	struct ionic_dev *idev = &lif->ionic->idev;
+	int ret;
+
+	mutex_lock(&idev->cmb_inuse_lock);
+	ret = bitmap_find_free_region(idev->cmb_inuse, idev->cmb_npages, order);
+	mutex_unlock(&idev->cmb_inuse_lock);
+
+	if (ret < 0)
+		return ret;
+
+	*pgid = (u32)ret;
+	*pgaddr = idev->phy_cmb_pages + ret * PAGE_SIZE;
+
+	return 0;
+}
+
+void ionic_put_cmb(struct ionic_lif *lif, u32 pgid, int order)
+{
+	struct ionic_dev *idev = &lif->ionic->idev;
+
+	mutex_lock(&idev->cmb_inuse_lock);
+	bitmap_release_region(idev->cmb_inuse, pgid, order);
+	mutex_unlock(&idev->cmb_inuse_lock);
+}
+
 int ionic_cq_init(struct ionic_lif *lif, struct ionic_cq *cq,
 		  struct ionic_intr_info *intr,
 		  unsigned int num_descs, size_t desc_size)
@@ -679,6 +738,18 @@ void ionic_q_map(struct ionic_queue *q, void *base, dma_addr_t base_pa)
 		cur->desc = base + (i * q->desc_size);
 }
 
+void ionic_q_cmb_map(struct ionic_queue *q, void __iomem *base, dma_addr_t base_pa)
+{
+	struct ionic_desc_info *cur;
+	unsigned int i;
+
+	q->cmb_base = base;
+	q->cmb_base_pa = base_pa;
+
+	for (i = 0, cur = q->info; i < q->num_descs; i++, cur++)
+		cur->cmb_desc = base + (i * q->desc_size);
+}
+
 void ionic_q_sg_map(struct ionic_queue *q, void *base, dma_addr_t base_pa)
 {
 	struct ionic_desc_info *cur;
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
index 2a1d7b9c07e7..a4a8802f3771 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
@@ -153,6 +153,11 @@ struct ionic_dev {
 	struct ionic_intr __iomem *intr_ctrl;
 	u64 __iomem *intr_status;
 
+	struct mutex cmb_inuse_lock; /* for cmb_inuse */
+	unsigned long *cmb_inuse;
+	dma_addr_t phy_cmb_pages;
+	u32 cmb_npages;
+
 	u32 port_info_sz;
 	struct ionic_port_info *port_info;
 	dma_addr_t port_info_pa;
@@ -197,6 +202,7 @@ struct ionic_desc_info {
 		struct ionic_rxq_desc *rxq_desc;
 		struct ionic_admin_cmd *adminq_desc;
 	};
+	void __iomem *cmb_desc;
 	union {
 		void *sg_desc;
 		struct ionic_txq_sg_desc *txq_sg_desc;
@@ -233,12 +239,14 @@ struct ionic_queue {
 		struct ionic_rxq_desc *rxq;
 		struct ionic_admin_cmd *adminq;
 	};
+	void __iomem *cmb_base;
 	union {
 		void *sg_base;
 		struct ionic_txq_sg_desc *txq_sgl;
 		struct ionic_rxq_sg_desc *rxq_sgl;
 	};
 	dma_addr_t base_pa;
+	dma_addr_t cmb_base_pa;
 	dma_addr_t sg_base_pa;
 	unsigned int desc_size;
 	unsigned int sg_desc_size;
@@ -301,6 +309,7 @@ static inline bool ionic_q_has_space(struct ionic_queue *q, unsigned int want)
 
 void ionic_init_devinfo(struct ionic *ionic);
 int ionic_dev_setup(struct ionic *ionic);
+void ionic_dev_teardown(struct ionic *ionic);
 
 void ionic_dev_cmd_go(struct ionic_dev *idev, union ionic_dev_cmd *cmd);
 u8 ionic_dev_cmd_status(struct ionic_dev *idev);
@@ -336,6 +345,9 @@ void ionic_dev_cmd_adminq_init(struct ionic_dev *idev, struct ionic_qcq *qcq,
 
 int ionic_db_page_num(struct ionic_lif *lif, int pid);
 
+int ionic_get_cmb(struct ionic_lif *lif, u32 *pgid, phys_addr_t *pgaddr, int order);
+void ionic_put_cmb(struct ionic_lif *lif, u32 pgid, int order);
+
 int ionic_cq_init(struct ionic_lif *lif, struct ionic_cq *cq,
 		  struct ionic_intr_info *intr,
 		  unsigned int num_descs, size_t desc_size);
@@ -352,6 +364,7 @@ int ionic_q_init(struct ionic_lif *lif, struct ionic_dev *idev,
 		 unsigned int num_descs, size_t desc_size,
 		 size_t sg_desc_size, unsigned int pid);
 void ionic_q_map(struct ionic_queue *q, void *base, dma_addr_t base_pa);
+void ionic_q_cmb_map(struct ionic_queue *q, void __iomem *base, dma_addr_t base_pa);
 void ionic_q_sg_map(struct ionic_queue *q, void *base, dma_addr_t base_pa);
 void ionic_q_post(struct ionic_queue *q, bool ring_doorbell, ionic_desc_cb cb,
 		  void *cb_arg);
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
index 01c22701482d..45e5dc400095 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
@@ -11,6 +11,38 @@
 #include "ionic_ethtool.h"
 #include "ionic_stats.h"
 
+static const char ionic_priv_flags_strings[][ETH_GSTRING_LEN] = {
+#define IONIC_PRIV_F_CMB_RINGS		BIT(0)
+	"cmb-rings",
+};
+
+#define IONIC_PRIV_FLAGS_COUNT ARRAY_SIZE(ionic_priv_flags_strings)
+
+static int ionic_validate_cmb_config(struct ionic_lif *lif,
+				     struct ionic_queue_params *qparam)
+{
+	int pages_have, pages_required = 0;
+	unsigned long sz;
+
+	if (!qparam->cmb_enabled)
+		return 0;
+
+	sz = sizeof(struct ionic_txq_desc) * qparam->ntxq_descs * qparam->nxqs;
+	pages_required += ALIGN(sz, PAGE_SIZE) / PAGE_SIZE;
+
+	sz = sizeof(struct ionic_rxq_desc) * qparam->nrxq_descs * qparam->nxqs;
+	pages_required += ALIGN(sz, PAGE_SIZE) / PAGE_SIZE;
+
+	pages_have = lif->ionic->bars[IONIC_PCI_BAR_CMB].len / PAGE_SIZE;
+	if (pages_required > pages_have) {
+		netdev_info(lif->netdev, "Not enough CMB pages for number of queues and size of descriptor rings, need %d have %d",
+			    pages_required, pages_have);
+		return -ENOMEM;
+	}
+
+	return pages_required;
+}
+
 static void ionic_get_stats_strings(struct ionic_lif *lif, u8 *buf)
 {
 	u32 i;
@@ -52,6 +84,9 @@ static int ionic_get_sset_count(struct net_device *netdev, int sset)
 	case ETH_SS_STATS:
 		count = ionic_get_stats_count(lif);
 		break;
+	case ETH_SS_PRIV_FLAGS:
+		count = IONIC_PRIV_FLAGS_COUNT;
+		break;
 	}
 	return count;
 }
@@ -65,6 +100,10 @@ static void ionic_get_strings(struct net_device *netdev,
 	case ETH_SS_STATS:
 		ionic_get_stats_strings(lif, buf);
 		break;
+	case ETH_SS_PRIV_FLAGS:
+		memcpy(buf, ionic_priv_flags_strings,
+		       IONIC_PRIV_FLAGS_COUNT * ETH_GSTRING_LEN);
+		break;
 	}
 }
 
@@ -554,6 +593,13 @@ static int ionic_set_ringparam(struct net_device *netdev,
 	    ring->rx_pending == lif->nrxq_descs)
 		return 0;
 
+	qparam.ntxq_descs = ring->tx_pending;
+	qparam.nrxq_descs = ring->rx_pending;
+
+	err = ionic_validate_cmb_config(lif, &qparam);
+	if (err < 0)
+		return err;
+
 	if (ring->tx_pending != lif->ntxq_descs)
 		netdev_info(netdev, "Changing Tx ring size from %d to %d\n",
 			    lif->ntxq_descs, ring->tx_pending);
@@ -569,9 +615,6 @@ static int ionic_set_ringparam(struct net_device *netdev,
 		return 0;
 	}
 
-	qparam.ntxq_descs = ring->tx_pending;
-	qparam.nrxq_descs = ring->rx_pending;
-
 	mutex_lock(&lif->queue_lock);
 	err = ionic_reconfigure_queues(lif, &qparam);
 	mutex_unlock(&lif->queue_lock);
@@ -638,7 +681,7 @@ static int ionic_set_channels(struct net_device *netdev,
 				    lif->nxqs, ch->combined_count);
 
 		qparam.nxqs = ch->combined_count;
-		qparam.intr_split = 0;
+		qparam.intr_split = false;
 	} else {
 		max_cnt /= 2;
 		if (ch->rx_count > max_cnt)
@@ -654,9 +697,13 @@ static int ionic_set_channels(struct net_device *netdev,
 				    lif->nxqs, ch->rx_count);
 
 		qparam.nxqs = ch->rx_count;
-		qparam.intr_split = 1;
+		qparam.intr_split = true;
 	}
 
+	err = ionic_validate_cmb_config(lif, &qparam);
+	if (err < 0)
+		return err;
+
 	/* if we're not running, just set the values and return */
 	if (!netif_running(lif->netdev)) {
 		lif->nxqs = qparam.nxqs;
@@ -699,6 +746,75 @@ static int ionic_get_rxnfc(struct net_device *netdev,
 	return err;
 }
 
+int ionic_cmb_pages_in_use(struct ionic_lif *lif)
+{
+	struct ionic_queue_params qparam;
+
+	ionic_init_queue_params(lif, &qparam);
+	return ionic_validate_cmb_config(lif, &qparam);
+}
+
+static int ionic_cmb_rings_toggle(struct ionic_lif *lif, bool cmb_enable)
+{
+	struct ionic_queue_params qparam;
+	int pages_used;
+
+	if (!(lif->qtype_info[IONIC_QTYPE_TXQ].features & IONIC_QIDENT_F_CMB) ||
+	    !(lif->qtype_info[IONIC_QTYPE_RXQ].features & IONIC_QIDENT_F_CMB) ||
+	    !lif->ionic->idev.cmb_npages) {
+		netdev_info(lif->netdev, "CMB rings are not supported on this device\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (netif_running(lif->netdev))
+		return -EBUSY;
+
+	ionic_init_queue_params(lif, &qparam);
+	qparam.cmb_enabled = cmb_enable;
+	pages_used = ionic_validate_cmb_config(lif, &qparam);
+	if (pages_used < 0)
+		return pages_used;
+
+	if (cmb_enable) {
+		netdev_info(lif->netdev, "Enabling CMB rings - %d pages\n",
+			    pages_used);
+		set_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
+	} else {
+		netdev_info(lif->netdev, "Disabling CMB rings\n");
+		clear_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
+	}
+
+	return 0;
+}
+
+static u32 ionic_get_priv_flags(struct net_device *netdev)
+{
+	struct ionic_lif *lif = netdev_priv(netdev);
+	u32 priv_flags = 0;
+
+	if (test_bit(IONIC_LIF_F_CMB_RINGS, lif->state))
+		priv_flags |= IONIC_PRIV_F_CMB_RINGS;
+
+	return priv_flags;
+}
+
+static int ionic_set_priv_flags(struct net_device *netdev, u32 priv_flags)
+{
+	struct ionic_lif *lif = netdev_priv(netdev);
+	bool cmb_now, cmb_req;
+	int ret;
+
+	cmb_now = test_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
+	cmb_req = !!(priv_flags & IONIC_PRIV_F_CMB_RINGS);
+	if (cmb_now != cmb_req) {
+		ret = ionic_cmb_rings_toggle(lif, cmb_req);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
 static u32 ionic_get_rxfh_indir_size(struct net_device *netdev)
 {
 	struct ionic_lif *lif = netdev_priv(netdev);
@@ -980,6 +1096,8 @@ static const struct ethtool_ops ionic_ethtool_ops = {
 	.get_strings		= ionic_get_strings,
 	.get_ethtool_stats	= ionic_get_stats,
 	.get_sset_count		= ionic_get_sset_count,
+	.get_priv_flags		= ionic_get_priv_flags,
+	.set_priv_flags		= ionic_set_priv_flags,
 	.get_rxnfc		= ionic_get_rxnfc,
 	.get_rxfh_indir_size	= ionic_get_rxfh_indir_size,
 	.get_rxfh_key_size	= ionic_get_rxfh_key_size,
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h
index 38b91b1d70ae..6bc9c177d14b 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h
@@ -4,6 +4,7 @@
 #ifndef _IONIC_ETHTOOL_H_
 #define _IONIC_ETHTOOL_H_
 
+int ionic_cmb_pages_in_use(struct ionic_lif *lif);
 void ionic_ethtool_set_ops(struct net_device *netdev);
 
 #endif /* _IONIC_ETHTOOL_H_ */
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_if.h b/drivers/net/ethernet/pensando/ionic/ionic_if.h
index eac09b2375b8..9a1825edf0d0 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_if.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_if.h
@@ -3073,9 +3073,10 @@ union ionic_adminq_comp {
 
 #define IONIC_BARS_MAX			6
 #define IONIC_PCI_BAR_DBELL		1
+#define IONIC_PCI_BAR_CMB		2
 
-/* BAR0 */
 #define IONIC_BAR0_SIZE				0x8000
+#define IONIC_BAR2_SIZE				0x800000
 
 #define IONIC_BAR0_DEV_INFO_REGS_OFFSET		0x0000
 #define IONIC_BAR0_DEV_CMD_REGS_OFFSET		0x0800
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
index 8499165b1563..e8884de83474 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -25,9 +25,12 @@
 static const u8 ionic_qtype_versions[IONIC_QTYPE_MAX] = {
 	[IONIC_QTYPE_ADMINQ]  = 0,   /* 0 = Base version with CQ support */
 	[IONIC_QTYPE_NOTIFYQ] = 0,   /* 0 = Base version */
-	[IONIC_QTYPE_RXQ]     = 0,   /* 0 = Base version with CQ+SG support */
-	[IONIC_QTYPE_TXQ]     = 1,   /* 0 = Base version with CQ+SG support
-				      * 1 =   ... with Tx SG version 1
+	[IONIC_QTYPE_RXQ]     = 2,   /* 0 = Base version with CQ+SG support
+				      * 2 =       ... with CMB rings
+				      */
+	[IONIC_QTYPE_TXQ]     = 3,   /* 0 = Base version with CQ+SG support
+				      * 1 =       ... with Tx SG version 1
+				      * 3 =       ... with CMB rings
 				      */
 };
 
@@ -379,6 +382,15 @@ static void ionic_qcq_free(struct ionic_lif *lif, struct ionic_qcq *qcq)
 		qcq->q_base_pa = 0;
 	}
 
+	if (qcq->cmb_q_base) {
+		iounmap(qcq->cmb_q_base);
+		ionic_put_cmb(lif, qcq->cmb_pgid, qcq->cmb_order);
+		qcq->cmb_pgid = 0;
+		qcq->cmb_order = 0;
+		qcq->cmb_q_base = NULL;
+		qcq->cmb_q_base_pa = 0;
+	}
+
 	if (qcq->cq_base) {
 		dma_free_coherent(dev, qcq->cq_size, qcq->cq_base, qcq->cq_base_pa);
 		qcq->cq_base = NULL;
@@ -587,6 +599,7 @@ static int ionic_qcq_alloc(struct ionic_lif *lif, unsigned int type,
 		ionic_cq_map(&new->cq, cq_base, cq_base_pa);
 		ionic_cq_bind(&new->cq, &new->q);
 	} else {
+		/* regular DMA q descriptors */
 		new->q_size = PAGE_SIZE + (num_descs * desc_size);
 		new->q_base = dma_alloc_coherent(dev, new->q_size, &new->q_base_pa,
 						 GFP_KERNEL);
@@ -599,6 +612,33 @@ static int ionic_qcq_alloc(struct ionic_lif *lif, unsigned int type,
 		q_base_pa = ALIGN(new->q_base_pa, PAGE_SIZE);
 		ionic_q_map(&new->q, q_base, q_base_pa);
 
+		if (flags & IONIC_QCQ_F_CMB_RINGS) {
+			/* on-chip CMB q descriptors */
+			new->cmb_q_size = num_descs * desc_size;
+			new->cmb_order = order_base_2(new->cmb_q_size / PAGE_SIZE);
+
+			err = ionic_get_cmb(lif, &new->cmb_pgid, &new->cmb_q_base_pa,
+					    new->cmb_order);
+			if (err) {
+				netdev_err(lif->netdev,
+					   "Cannot allocate queue order %d from cmb: err %d\n",
+					   new->cmb_order, err);
+				goto err_out_free_q;
+			}
+
+			new->cmb_q_base = ioremap_wc(new->cmb_q_base_pa, new->cmb_q_size);
+			if (!new->cmb_q_base) {
+				netdev_err(lif->netdev, "Cannot map queue from cmb\n");
+				ionic_put_cmb(lif, new->cmb_pgid, new->cmb_order);
+				err = -ENOMEM;
+				goto err_out_free_q;
+			}
+
+			new->cmb_q_base_pa -= idev->phy_cmb_pages;
+			ionic_q_cmb_map(&new->q, new->cmb_q_base, new->cmb_q_base_pa);
+		}
+
+		/* cq DMA descriptors */
 		new->cq_size = PAGE_SIZE + (num_descs * cq_desc_size);
 		new->cq_base = dma_alloc_coherent(dev, new->cq_size, &new->cq_base_pa,
 						  GFP_KERNEL);
@@ -637,6 +677,10 @@ static int ionic_qcq_alloc(struct ionic_lif *lif, unsigned int type,
 err_out_free_cq:
 	dma_free_coherent(dev, new->cq_size, new->cq_base, new->cq_base_pa);
 err_out_free_q:
+	if (new->cmb_q_base) {
+		iounmap(new->cmb_q_base);
+		ionic_put_cmb(lif, new->cmb_pgid, new->cmb_order);
+	}
 	dma_free_coherent(dev, new->q_size, new->q_base, new->q_base_pa);
 err_out_free_cq_info:
 	vfree(new->cq.info);
@@ -718,6 +762,8 @@ static void ionic_qcq_sanitize(struct ionic_qcq *qcq)
 	qcq->cq.tail_idx = 0;
 	qcq->cq.done_color = 1;
 	memset(qcq->q_base, 0, qcq->q_size);
+	if (qcq->cmb_q_base)
+		memset_io(qcq->cmb_q_base, 0, qcq->cmb_q_size);
 	memset(qcq->cq_base, 0, qcq->cq_size);
 	memset(qcq->sg_base, 0, qcq->sg_size);
 }
@@ -737,6 +783,7 @@ static int ionic_lif_txq_init(struct ionic_lif *lif, struct ionic_qcq *qcq)
 			.index = cpu_to_le32(q->index),
 			.flags = cpu_to_le16(IONIC_QINIT_F_IRQ |
 					     IONIC_QINIT_F_SG),
+			.intr_index = cpu_to_le16(qcq->intr.index),
 			.pid = cpu_to_le16(q->pid),
 			.ring_size = ilog2(q->num_descs),
 			.ring_base = cpu_to_le64(q->base_pa),
@@ -745,17 +792,19 @@ static int ionic_lif_txq_init(struct ionic_lif *lif, struct ionic_qcq *qcq)
 			.features = cpu_to_le64(q->features),
 		},
 	};
-	unsigned int intr_index;
 	int err;
 
-	intr_index = qcq->intr.index;
-
-	ctx.cmd.q_init.intr_index = cpu_to_le16(intr_index);
+	if (qcq->flags & IONIC_QCQ_F_CMB_RINGS) {
+		ctx.cmd.q_init.flags |= cpu_to_le16(IONIC_QINIT_F_CMB);
+		ctx.cmd.q_init.ring_base = cpu_to_le64(qcq->cmb_q_base_pa);
+	}
 
 	dev_dbg(dev, "txq_init.pid %d\n", ctx.cmd.q_init.pid);
 	dev_dbg(dev, "txq_init.index %d\n", ctx.cmd.q_init.index);
 	dev_dbg(dev, "txq_init.ring_base 0x%llx\n", ctx.cmd.q_init.ring_base);
 	dev_dbg(dev, "txq_init.ring_size %d\n", ctx.cmd.q_init.ring_size);
+	dev_dbg(dev, "txq_init.cq_ring_base 0x%llx\n", ctx.cmd.q_init.cq_ring_base);
+	dev_dbg(dev, "txq_init.sg_ring_base 0x%llx\n", ctx.cmd.q_init.sg_ring_base);
 	dev_dbg(dev, "txq_init.flags 0x%x\n", ctx.cmd.q_init.flags);
 	dev_dbg(dev, "txq_init.ver %d\n", ctx.cmd.q_init.ver);
 	dev_dbg(dev, "txq_init.intr_index %d\n", ctx.cmd.q_init.intr_index);
@@ -807,6 +856,11 @@ static int ionic_lif_rxq_init(struct ionic_lif *lif, struct ionic_qcq *qcq)
 	};
 	int err;
 
+	if (qcq->flags & IONIC_QCQ_F_CMB_RINGS) {
+		ctx.cmd.q_init.flags |= cpu_to_le16(IONIC_QINIT_F_CMB);
+		ctx.cmd.q_init.ring_base = cpu_to_le64(qcq->cmb_q_base_pa);
+	}
+
 	dev_dbg(dev, "rxq_init.pid %d\n", ctx.cmd.q_init.pid);
 	dev_dbg(dev, "rxq_init.index %d\n", ctx.cmd.q_init.index);
 	dev_dbg(dev, "rxq_init.ring_base 0x%llx\n", ctx.cmd.q_init.ring_base);
@@ -1966,8 +2020,13 @@ static int ionic_txrx_alloc(struct ionic_lif *lif)
 		sg_desc_sz = sizeof(struct ionic_txq_sg_desc);
 
 	flags = IONIC_QCQ_F_TX_STATS | IONIC_QCQ_F_SG;
+
+	if (test_bit(IONIC_LIF_F_CMB_RINGS, lif->state))
+		flags |= IONIC_QCQ_F_CMB_RINGS;
+
 	if (test_bit(IONIC_LIF_F_SPLIT_INTR, lif->state))
 		flags |= IONIC_QCQ_F_INTR;
+
 	for (i = 0; i < lif->nxqs; i++) {
 		err = ionic_qcq_alloc(lif, IONIC_QTYPE_TXQ, i, "tx", flags,
 				      num_desc, desc_sz, comp_sz, sg_desc_sz,
@@ -1988,6 +2047,9 @@ static int ionic_txrx_alloc(struct ionic_lif *lif)
 
 	flags = IONIC_QCQ_F_RX_STATS | IONIC_QCQ_F_SG | IONIC_QCQ_F_INTR;
 
+	if (test_bit(IONIC_LIF_F_CMB_RINGS, lif->state))
+		flags |= IONIC_QCQ_F_CMB_RINGS;
+
 	num_desc = lif->nrxq_descs;
 	desc_sz = sizeof(struct ionic_rxq_desc);
 	comp_sz = sizeof(struct ionic_rxq_comp);
@@ -2663,6 +2725,55 @@ static const struct net_device_ops ionic_netdev_ops = {
 	.ndo_get_vf_stats       = ionic_get_vf_stats,
 };
 
+static int ionic_cmb_reconfig(struct ionic_lif *lif,
+			      struct ionic_queue_params *qparam)
+{
+	struct ionic_queue_params start_qparams;
+	int err = 0;
+
+	/* When changing CMB queue parameters, we're using limited
+	 * on-device memory and don't have extra memory to use for
+	 * duplicate allocations, so we free it all first then
+	 * re-allocate with the new parameters.
+	 */
+
+	/* Checkpoint for possible unwind */
+	ionic_init_queue_params(lif, &start_qparams);
+
+	/* Stop and free the queues */
+	ionic_stop_queues_reconfig(lif);
+	ionic_txrx_free(lif);
+
+	/* Set up new qparams */
+	ionic_set_queue_params(lif, qparam);
+
+	if (netif_running(lif->netdev)) {
+		/* Alloc and start the new configuration */
+		err = ionic_txrx_alloc(lif);
+		if (err) {
+			dev_warn(lif->ionic->dev,
+				 "CMB reconfig failed, restoring values: %d\n", err);
+
+			/* Back out the changes */
+			ionic_set_queue_params(lif, &start_qparams);
+			err = ionic_txrx_alloc(lif);
+			if (err) {
+				dev_err(lif->ionic->dev,
+					"CMB restore failed: %d\n", err);
+				goto errout;
+			}
+		}
+
+		ionic_start_queues_reconfig(lif);
+	} else {
+		/* This was detached in ionic_stop_queues_reconfig() */
+		netif_device_attach(lif->netdev);
+	}
+
+errout:
+	return err;
+}
+
 static void ionic_swap_queues(struct ionic_qcq *a, struct ionic_qcq *b)
 {
 	/* only swapping the queues, not the napi, flags, or other stuff */
@@ -2705,6 +2816,10 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
 	unsigned int flags, i;
 	int err = 0;
 
+	/* Are we changing q params while CMB is on */
+	if (test_bit(IONIC_LIF_F_CMB_RINGS, lif->state) && qparam->cmb_enabled)
+		return ionic_cmb_reconfig(lif, qparam);
+
 	/* allocate temporary qcq arrays to hold new queue structs */
 	if (qparam->nxqs != lif->nxqs || qparam->ntxq_descs != lif->ntxq_descs) {
 		tx_qcqs = devm_kcalloc(lif->ionic->dev, lif->ionic->ntxqs_per_lif,
@@ -2741,6 +2856,16 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
 			sg_desc_sz = sizeof(struct ionic_txq_sg_desc);
 
 		for (i = 0; i < qparam->nxqs; i++) {
+			/* If missing, short placeholder qcq needed for swap */
+			if (!lif->txqcqs[i]) {
+				flags = IONIC_QCQ_F_TX_STATS | IONIC_QCQ_F_SG;
+				err = ionic_qcq_alloc(lif, IONIC_QTYPE_TXQ, i, "tx", flags,
+						      4, desc_sz, comp_sz, sg_desc_sz,
+						      lif->kern_pid, &lif->txqcqs[i]);
+				if (err)
+					goto err_out;
+			}
+
 			flags = lif->txqcqs[i]->flags & ~IONIC_QCQ_F_INTR;
 			err = ionic_qcq_alloc(lif, IONIC_QTYPE_TXQ, i, "tx", flags,
 					      num_desc, desc_sz, comp_sz, sg_desc_sz,
@@ -2760,6 +2885,16 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
 			comp_sz *= 2;
 
 		for (i = 0; i < qparam->nxqs; i++) {
+			/* If missing, short placeholder qcq needed for swap */
+			if (!lif->rxqcqs[i]) {
+				flags = IONIC_QCQ_F_RX_STATS | IONIC_QCQ_F_SG;
+				err = ionic_qcq_alloc(lif, IONIC_QTYPE_RXQ, i, "rx", flags,
+						      4, desc_sz, comp_sz, sg_desc_sz,
+						      lif->kern_pid, &lif->rxqcqs[i]);
+				if (err)
+					goto err_out;
+			}
+
 			flags = lif->rxqcqs[i]->flags & ~IONIC_QCQ_F_INTR;
 			err = ionic_qcq_alloc(lif, IONIC_QTYPE_RXQ, i, "rx", flags,
 					      num_desc, desc_sz, comp_sz, sg_desc_sz,
@@ -2809,10 +2944,15 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
 			lif->tx_coalesce_hw = lif->rx_coalesce_hw;
 		}
 
-		/* clear existing interrupt assignments */
+		/* Clear existing interrupt assignments.  We check for NULL here
+		 * because we're checking the whole array for potential qcqs, not
+		 * just those qcqs that have just been set up.
+		 */
 		for (i = 0; i < lif->ionic->ntxqs_per_lif; i++) {
-			ionic_qcq_intr_free(lif, lif->txqcqs[i]);
-			ionic_qcq_intr_free(lif, lif->rxqcqs[i]);
+			if (lif->txqcqs[i])
+				ionic_qcq_intr_free(lif, lif->txqcqs[i]);
+			if (lif->rxqcqs[i])
+				ionic_qcq_intr_free(lif, lif->rxqcqs[i]);
 		}
 
 		/* re-assign the interrupts */
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
index a53984bf3544..5425a8983ae0 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
@@ -59,6 +59,7 @@ struct ionic_rx_stats {
 #define IONIC_QCQ_F_TX_STATS		BIT(3)
 #define IONIC_QCQ_F_RX_STATS		BIT(4)
 #define IONIC_QCQ_F_NOTIFYQ		BIT(5)
+#define IONIC_QCQ_F_CMB_RINGS		BIT(6)
 
 struct ionic_qcq {
 	void *q_base;
@@ -70,6 +71,11 @@ struct ionic_qcq {
 	void *sg_base;
 	dma_addr_t sg_base_pa;
 	u32 sg_size;
+	void __iomem *cmb_q_base;
+	phys_addr_t cmb_q_base_pa;
+	u32 cmb_q_size;
+	u32 cmb_pgid;
+	u32 cmb_order;
 	struct dim dim;
 	struct ionic_queue q;
 	struct ionic_cq cq;
@@ -140,6 +146,7 @@ enum ionic_lif_state_flags {
 	IONIC_LIF_F_BROKEN,
 	IONIC_LIF_F_TX_DIM_INTR,
 	IONIC_LIF_F_RX_DIM_INTR,
+	IONIC_LIF_F_CMB_RINGS,
 
 	/* leave this as last */
 	IONIC_LIF_F_STATE_SIZE
@@ -243,8 +250,9 @@ struct ionic_queue_params {
 	unsigned int nxqs;
 	unsigned int ntxq_descs;
 	unsigned int nrxq_descs;
-	unsigned int intr_split;
 	u64 rxq_features;
+	bool intr_split;
+	bool cmb_enabled;
 };
 
 static inline void ionic_init_queue_params(struct ionic_lif *lif,
@@ -253,8 +261,28 @@ static inline void ionic_init_queue_params(struct ionic_lif *lif,
 	qparam->nxqs = lif->nxqs;
 	qparam->ntxq_descs = lif->ntxq_descs;
 	qparam->nrxq_descs = lif->nrxq_descs;
-	qparam->intr_split = test_bit(IONIC_LIF_F_SPLIT_INTR, lif->state);
 	qparam->rxq_features = lif->rxq_features;
+	qparam->intr_split = test_bit(IONIC_LIF_F_SPLIT_INTR, lif->state);
+	qparam->cmb_enabled = test_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
+}
+
+static inline void ionic_set_queue_params(struct ionic_lif *lif,
+					  struct ionic_queue_params *qparam)
+{
+	lif->nxqs = qparam->nxqs;
+	lif->ntxq_descs = qparam->ntxq_descs;
+	lif->nrxq_descs = qparam->nrxq_descs;
+	lif->rxq_features = qparam->rxq_features;
+
+	if (qparam->intr_split)
+		set_bit(IONIC_LIF_F_SPLIT_INTR, lif->state);
+	else
+		clear_bit(IONIC_LIF_F_SPLIT_INTR, lif->state);
+
+	if (qparam->cmb_enabled)
+		set_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
+	else
+		clear_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
 }
 
 static inline u32 ionic_coal_usec_to_hw(struct ionic *ionic, u32 usecs)
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
index 0c3977416cd1..0cb464931d3d 100644
--- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
@@ -419,6 +419,10 @@ void ionic_rx_fill(struct ionic_queue *q)
 					      IONIC_RXQ_DESC_OPCODE_SIMPLE;
 		desc_info->nbufs = nfrags;
 
+		/* commit CMB descriptor contents in one shot */
+		if (q_to_qcq(q)->flags & IONIC_QCQ_F_CMB_RINGS)
+			memcpy_toio(desc_info->cmb_desc, desc, q->desc_size);
+
 		ionic_rxq_post(q, false, ionic_rx_clean, NULL);
 	}
 
@@ -860,7 +864,8 @@ static int ionic_tx_tcp_pseudo_csum(struct sk_buff *skb)
 	return 0;
 }
 
-static void ionic_tx_tso_post(struct ionic_queue *q, struct ionic_txq_desc *desc,
+static void ionic_tx_tso_post(struct ionic_queue *q,
+			      struct ionic_desc_info *desc_info,
 			      struct sk_buff *skb,
 			      dma_addr_t addr, u8 nsge, u16 len,
 			      unsigned int hdrlen, unsigned int mss,
@@ -868,6 +873,7 @@ static void ionic_tx_tso_post(struct ionic_queue *q, struct ionic_txq_desc *desc
 			      u16 vlan_tci, bool has_vlan,
 			      bool start, bool done)
 {
+	struct ionic_txq_desc *desc = desc_info->desc;
 	u8 flags = 0;
 	u64 cmd;
 
@@ -883,6 +889,10 @@ static void ionic_tx_tso_post(struct ionic_queue *q, struct ionic_txq_desc *desc
 	desc->hdr_len = cpu_to_le16(hdrlen);
 	desc->mss = cpu_to_le16(mss);
 
+	/* commit CMB descriptor contents in one shot */
+	if (q_to_qcq(q)->flags & IONIC_QCQ_F_CMB_RINGS)
+		memcpy_toio(desc_info->cmb_desc, desc, q->desc_size);
+
 	if (start) {
 		skb_tx_timestamp(skb);
 		if (!unlikely(q->features & IONIC_TXQ_F_HWSTAMP))
@@ -1001,7 +1011,7 @@ static int ionic_tx_tso(struct ionic_queue *q, struct sk_buff *skb)
 		seg_rem = min(tso_rem, mss);
 		done = (tso_rem == 0);
 		/* post descriptor */
-		ionic_tx_tso_post(q, desc, skb,
+		ionic_tx_tso_post(q, desc_info, skb,
 				  desc_addr, desc_nsge, desc_len,
 				  hdrlen, mss, outer_csum, vlan_tci, has_vlan,
 				  start, done);
@@ -1050,6 +1060,10 @@ static void ionic_tx_calc_csum(struct ionic_queue *q, struct sk_buff *skb,
 	desc->csum_start = cpu_to_le16(skb_checksum_start_offset(skb));
 	desc->csum_offset = cpu_to_le16(skb->csum_offset);
 
+	/* commit descriptor contents in one shot */
+	if (q_to_qcq(q)->flags & IONIC_QCQ_F_CMB_RINGS)
+		memcpy_toio(desc_info->cmb_desc, desc, q->desc_size);
+
 	if (skb_csum_is_sctp(skb))
 		stats->crc32_csum++;
 	else
@@ -1087,6 +1101,10 @@ static void ionic_tx_calc_no_csum(struct ionic_queue *q, struct sk_buff *skb,
 	desc->csum_start = 0;
 	desc->csum_offset = 0;
 
+	/* commit descriptor contents in one shot */
+	if (q_to_qcq(q)->flags & IONIC_QCQ_F_CMB_RINGS)
+		memcpy_toio(desc_info->cmb_desc, desc, q->desc_size);
+
 	stats->csum_none++;
 }
 
-- 
2.17.1

Re: [PATCH v2 net-next 3/3] ionic: add support for device Component Memory Buffers

[Alexander H Duyck]

On Mon, 2023-02-06 at 10:16 -0800, Shannon Nelson wrote:
> The ionic device has on-board memory (CMB) that can be used
> for descriptors as a way to speed descriptor access for faster
> traffic processing.  It imposes a couple of restrictions so
> is not on by default, but can be enabled through the ethtool
> priv-flags.

For the purposes of patch review it might be convinent to call out what
those restrictions are as you enable the code below. I'm assuming it is
mostly just the amount of space you can use, but if there is something
else it would be useful to have that noted.

> 
> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
> ---
>  .../ethernet/pensando/ionic/ionic_bus_pci.c   |   3 +
>  .../net/ethernet/pensando/ionic/ionic_dev.c   |  71 ++++++++
>  .../net/ethernet/pensando/ionic/ionic_dev.h   |  13 ++
>  .../ethernet/pensando/ionic/ionic_ethtool.c   | 128 +++++++++++++-
>  .../ethernet/pensando/ionic/ionic_ethtool.h   |   1 +
>  .../net/ethernet/pensando/ionic/ionic_if.h    |   3 +-
>  .../net/ethernet/pensando/ionic/ionic_lif.c   | 160 ++++++++++++++++--
>  .../net/ethernet/pensando/ionic/ionic_lif.h   |  32 +++-
>  .../net/ethernet/pensando/ionic/ionic_txrx.c  |  22 ++-
>  9 files changed, 413 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> index 0eff78fa0565..f62f11ba21c0 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
> @@ -352,6 +352,7 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  err_out_reset:
>  	ionic_reset(ionic);
>  err_out_teardown:
> +	ionic_dev_teardown(ionic);
>  	pci_clear_master(pdev);
>  	/* Don't fail the probe for these errors, keep
>  	 * the hw interface around for inspection
> @@ -369,6 +370,7 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  err_out_clear_drvdata:
>  	mutex_destroy(&ionic->dev_cmd_lock);
>  	ionic_devlink_free(ionic);
> +	pci_set_drvdata(pdev, NULL);
>  
>  	return err;
>  }

This piece almost seems like it is unrelated. Perhaps it bled through
from some other work or cleanup?

> @@ -390,6 +392,7 @@ static void ionic_remove(struct pci_dev *pdev)
>  
>  	ionic_port_reset(ionic);
>  	ionic_reset(ionic);
> +	ionic_dev_teardown(ionic);
>  	pci_clear_master(pdev);
>  	ionic_unmap_bars(ionic);
>  	pci_release_regions(pdev);
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
> index 626b9113e7c4..9b4bba2279ab 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
> @@ -92,6 +92,7 @@ int ionic_dev_setup(struct ionic *ionic)
>  	unsigned int num_bars = ionic->num_bars;
>  	struct ionic_dev *idev = &ionic->idev;
>  	struct device *dev = ionic->dev;
> +	int size;
>  	u32 sig;
>  
>  	/* BAR0: dev_cmd and interrupts */
> @@ -133,9 +134,40 @@ int ionic_dev_setup(struct ionic *ionic)
>  	idev->db_pages = bar->vaddr;
>  	idev->phy_db_pages = bar->bus_addr;
>  
> +	/* BAR2: optional controller memory mapping */
> +	bar++;
> +	mutex_init(&idev->cmb_inuse_lock);
> +	if (num_bars < 3 || !ionic->bars[IONIC_PCI_BAR_CMB].len) {
> +		idev->cmb_inuse = NULL;
> +		idev->phy_cmb_pages = 0;
> +		idev->cmb_npages = 0;
> +		return 0;
> +	}
> +
> +	idev->phy_cmb_pages = bar->bus_addr;
> +	idev->cmb_npages = bar->len / PAGE_SIZE;
> +	size = BITS_TO_LONGS(idev->cmb_npages) * sizeof(long);
> +	idev->cmb_inuse = kzalloc(size, GFP_KERNEL);
> +	if (!idev->cmb_inuse) {
> +		idev->phy_cmb_pages = 0;
> +		idev->cmb_npages = 0;
> +	}
> +

Why not hold of on setting phy_cmb_pages and cmb_npages until after you
have allocated the pages rather then resetting them in the event of
failure?

Also is it really acceptable for this to fail silently?

>  	return 0;
>  }
>  
> +void ionic_dev_teardown(struct ionic *ionic)
> +{
> +	struct ionic_dev *idev = &ionic->idev;
> +
> +	kfree(idev->cmb_inuse);
> +	idev->cmb_inuse = NULL;
> +	idev->phy_cmb_pages = 0;
> +	idev->cmb_npages = 0;
> +
> +	mutex_destroy(&idev->cmb_inuse_lock);
> +}
> +
>  /* Devcmd Interface */
>  bool ionic_is_fw_running(struct ionic_dev *idev)
>  {
> @@ -571,6 +603,33 @@ int ionic_db_page_num(struct ionic_lif *lif, int pid)
>  	return (lif->hw_index * lif->dbid_count) + pid;
>  }
>  
> +int ionic_get_cmb(struct ionic_lif *lif, u32 *pgid, phys_addr_t *pgaddr, int order)
> +{
> +	struct ionic_dev *idev = &lif->ionic->idev;
> +	int ret;
> +
> +	mutex_lock(&idev->cmb_inuse_lock);
> +	ret = bitmap_find_free_region(idev->cmb_inuse, idev->cmb_npages, order);
> +	mutex_unlock(&idev->cmb_inuse_lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	*pgid = (u32)ret;

I'm not sure if the cast is necessary since we are guaranteed ret will
be unsigned based on the earlier check.

> +	*pgaddr = idev->phy_cmb_pages + ret * PAGE_SIZE;
> +
> +	return 0;
> +}
> +
> +void ionic_put_cmb(struct ionic_lif *lif, u32 pgid, int order)
> +{
> +	struct ionic_dev *idev = &lif->ionic->idev;
> +
> +	mutex_lock(&idev->cmb_inuse_lock);
> +	bitmap_release_region(idev->cmb_inuse, pgid, order);
> +	mutex_unlock(&idev->cmb_inuse_lock);
> +}
> +
>  int ionic_cq_init(struct ionic_lif *lif, struct ionic_cq *cq,
>  		  struct ionic_intr_info *intr,
>  		  unsigned int num_descs, size_t desc_size)
> @@ -679,6 +738,18 @@ void ionic_q_map(struct ionic_queue *q, void *base, dma_addr_t base_pa)
>  		cur->desc = base + (i * q->desc_size);
>  }
>  
> +void ionic_q_cmb_map(struct ionic_queue *q, void __iomem *base, dma_addr_t base_pa)
> +{
> +	struct ionic_desc_info *cur;
> +	unsigned int i;
> +
> +	q->cmb_base = base;
> +	q->cmb_base_pa = base_pa;
> +
> +	for (i = 0, cur = q->info; i < q->num_descs; i++, cur++)
> +		cur->cmb_desc = base + (i * q->desc_size);
> +}
> +
>  void ionic_q_sg_map(struct ionic_queue *q, void *base, dma_addr_t base_pa)
>  {
>  	struct ionic_desc_info *cur;
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
> index 2a1d7b9c07e7..a4a8802f3771 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
> @@ -153,6 +153,11 @@ struct ionic_dev {
>  	struct ionic_intr __iomem *intr_ctrl;
>  	u64 __iomem *intr_status;
>  
> +	struct mutex cmb_inuse_lock; /* for cmb_inuse */
> +	unsigned long *cmb_inuse;
> +	dma_addr_t phy_cmb_pages;
> +	u32 cmb_npages;
> +
>  	u32 port_info_sz;
>  	struct ionic_port_info *port_info;
>  	dma_addr_t port_info_pa;
> @@ -197,6 +202,7 @@ struct ionic_desc_info {
>  		struct ionic_rxq_desc *rxq_desc;
>  		struct ionic_admin_cmd *adminq_desc;
>  	};
> +	void __iomem *cmb_desc;
>  	union {
>  		void *sg_desc;
>  		struct ionic_txq_sg_desc *txq_sg_desc;
> @@ -233,12 +239,14 @@ struct ionic_queue {
>  		struct ionic_rxq_desc *rxq;
>  		struct ionic_admin_cmd *adminq;
>  	};
> +	void __iomem *cmb_base;
>  	union {
>  		void *sg_base;
>  		struct ionic_txq_sg_desc *txq_sgl;
>  		struct ionic_rxq_sg_desc *rxq_sgl;
>  	};
>  	dma_addr_t base_pa;
> +	dma_addr_t cmb_base_pa;
>  	dma_addr_t sg_base_pa;
>  	unsigned int desc_size;
>  	unsigned int sg_desc_size;
> @@ -301,6 +309,7 @@ static inline bool ionic_q_has_space(struct ionic_queue *q, unsigned int want)
>  
>  void ionic_init_devinfo(struct ionic *ionic);
>  int ionic_dev_setup(struct ionic *ionic);
> +void ionic_dev_teardown(struct ionic *ionic);
>  
>  void ionic_dev_cmd_go(struct ionic_dev *idev, union ionic_dev_cmd *cmd);
>  u8 ionic_dev_cmd_status(struct ionic_dev *idev);
> @@ -336,6 +345,9 @@ void ionic_dev_cmd_adminq_init(struct ionic_dev *idev, struct ionic_qcq *qcq,
>  
>  int ionic_db_page_num(struct ionic_lif *lif, int pid);
>  
> +int ionic_get_cmb(struct ionic_lif *lif, u32 *pgid, phys_addr_t *pgaddr, int order);
> +void ionic_put_cmb(struct ionic_lif *lif, u32 pgid, int order);
> +
>  int ionic_cq_init(struct ionic_lif *lif, struct ionic_cq *cq,
>  		  struct ionic_intr_info *intr,
>  		  unsigned int num_descs, size_t desc_size);
> @@ -352,6 +364,7 @@ int ionic_q_init(struct ionic_lif *lif, struct ionic_dev *idev,
>  		 unsigned int num_descs, size_t desc_size,
>  		 size_t sg_desc_size, unsigned int pid);
>  void ionic_q_map(struct ionic_queue *q, void *base, dma_addr_t base_pa);
> +void ionic_q_cmb_map(struct ionic_queue *q, void __iomem *base, dma_addr_t base_pa);
>  void ionic_q_sg_map(struct ionic_queue *q, void *base, dma_addr_t base_pa);
>  void ionic_q_post(struct ionic_queue *q, bool ring_doorbell, ionic_desc_cb cb,
>  		  void *cb_arg);
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> index 01c22701482d..45e5dc400095 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
> @@ -11,6 +11,38 @@
>  #include "ionic_ethtool.h"
>  #include "ionic_stats.h"
>  
> +static const char ionic_priv_flags_strings[][ETH_GSTRING_LEN] = {
> +#define IONIC_PRIV_F_CMB_RINGS		BIT(0)
> +	"cmb-rings",
> +};
> +
> +#define IONIC_PRIV_FLAGS_COUNT ARRAY_SIZE(ionic_priv_flags_strings)
> +
> +static int ionic_validate_cmb_config(struct ionic_lif *lif,
> +				     struct ionic_queue_params *qparam)
> +{
> +	int pages_have, pages_required = 0;
> +	unsigned long sz;
> +
> +	if (!qparam->cmb_enabled)
> +		return 0;
> +
> +	sz = sizeof(struct ionic_txq_desc) * qparam->ntxq_descs * qparam->nxqs;
> +	pages_required += ALIGN(sz, PAGE_SIZE) / PAGE_SIZE;
> +
> +	sz = sizeof(struct ionic_rxq_desc) * qparam->nrxq_descs * qparam->nxqs;
> +	pages_required += ALIGN(sz, PAGE_SIZE) / PAGE_SIZE;
> +
> +	pages_have = lif->ionic->bars[IONIC_PCI_BAR_CMB].len / PAGE_SIZE;
> +	if (pages_required > pages_have) {
> +		netdev_info(lif->netdev, "Not enough CMB pages for number of queues and size of descriptor rings, need %d have %d",
> +			    pages_required, pages_have);
> +		return -ENOMEM;
> +	}
> +
> +	return pages_required;
> +}
> +
>  static void ionic_get_stats_strings(struct ionic_lif *lif, u8 *buf)
>  {
>  	u32 i;
> @@ -52,6 +84,9 @@ static int ionic_get_sset_count(struct net_device *netdev, int sset)
>  	case ETH_SS_STATS:
>  		count = ionic_get_stats_count(lif);
>  		break;
> +	case ETH_SS_PRIV_FLAGS:
> +		count = IONIC_PRIV_FLAGS_COUNT;
> +		break;
>  	}
>  	return count;
>  }
> @@ -65,6 +100,10 @@ static void ionic_get_strings(struct net_device *netdev,
>  	case ETH_SS_STATS:
>  		ionic_get_stats_strings(lif, buf);
>  		break;
> +	case ETH_SS_PRIV_FLAGS:
> +		memcpy(buf, ionic_priv_flags_strings,
> +		       IONIC_PRIV_FLAGS_COUNT * ETH_GSTRING_LEN);
> +		break;
>  	}
>  }
>  
> @@ -554,6 +593,13 @@ static int ionic_set_ringparam(struct net_device *netdev,
>  	    ring->rx_pending == lif->nrxq_descs)
>  		return 0;
>  
> +	qparam.ntxq_descs = ring->tx_pending;
> +	qparam.nrxq_descs = ring->rx_pending;
> +
> +	err = ionic_validate_cmb_config(lif, &qparam);
> +	if (err < 0)
> +		return err;
> +
>  	if (ring->tx_pending != lif->ntxq_descs)
>  		netdev_info(netdev, "Changing Tx ring size from %d to %d\n",
>  			    lif->ntxq_descs, ring->tx_pending);
> @@ -569,9 +615,6 @@ static int ionic_set_ringparam(struct net_device *netdev,
>  		return 0;
>  	}
>  
> -	qparam.ntxq_descs = ring->tx_pending;
> -	qparam.nrxq_descs = ring->rx_pending;
> -
>  	mutex_lock(&lif->queue_lock);
>  	err = ionic_reconfigure_queues(lif, &qparam);
>  	mutex_unlock(&lif->queue_lock);
> @@ -638,7 +681,7 @@ static int ionic_set_channels(struct net_device *netdev,
>  				    lif->nxqs, ch->combined_count);
>  
>  		qparam.nxqs = ch->combined_count;
> -		qparam.intr_split = 0;
> +		qparam.intr_split = false;
>  	} else {
>  		max_cnt /= 2;
>  		if (ch->rx_count > max_cnt)
> @@ -654,9 +697,13 @@ static int ionic_set_channels(struct net_device *netdev,
>  				    lif->nxqs, ch->rx_count);
>  
>  		qparam.nxqs = ch->rx_count;
> -		qparam.intr_split = 1;
> +		qparam.intr_split = true;
>  	}
>  
> +	err = ionic_validate_cmb_config(lif, &qparam);
> +	if (err < 0)
> +		return err;
> +
>  	/* if we're not running, just set the values and return */
>  	if (!netif_running(lif->netdev)) {
>  		lif->nxqs = qparam.nxqs;
> @@ -699,6 +746,75 @@ static int ionic_get_rxnfc(struct net_device *netdev,
>  	return err;
>  }
>  
> +int ionic_cmb_pages_in_use(struct ionic_lif *lif)
> +{
> +	struct ionic_queue_params qparam;
> +
> +	ionic_init_queue_params(lif, &qparam);
> +	return ionic_validate_cmb_config(lif, &qparam);
> +}
> +
> +static int ionic_cmb_rings_toggle(struct ionic_lif *lif, bool cmb_enable)
> +{
> +	struct ionic_queue_params qparam;
> +	int pages_used;
> +
> +	if (!(lif->qtype_info[IONIC_QTYPE_TXQ].features & IONIC_QIDENT_F_CMB) ||
> +	    !(lif->qtype_info[IONIC_QTYPE_RXQ].features & IONIC_QIDENT_F_CMB) ||
> +	    !lif->ionic->idev.cmb_npages) {
> +		netdev_info(lif->netdev, "CMB rings are not supported on this device\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (netif_running(lif->netdev))
> +		return -EBUSY;
> +
> +	ionic_init_queue_params(lif, &qparam);
> +	qparam.cmb_enabled = cmb_enable;
> +	pages_used = ionic_validate_cmb_config(lif, &qparam);
> +	if (pages_used < 0)
> +		return pages_used;
> +
> +	if (cmb_enable) {
> +		netdev_info(lif->netdev, "Enabling CMB rings - %d pages\n",
> +			    pages_used);
> +		set_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
> +	} else {
> +		netdev_info(lif->netdev, "Disabling CMB rings\n");
> +		clear_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
> +	}
> +
> +	return 0;
> +}
> +
> +static u32 ionic_get_priv_flags(struct net_device *netdev)
> +{
> +	struct ionic_lif *lif = netdev_priv(netdev);
> +	u32 priv_flags = 0;
> +
> +	if (test_bit(IONIC_LIF_F_CMB_RINGS, lif->state))
> +		priv_flags |= IONIC_PRIV_F_CMB_RINGS;
> +
> +	return priv_flags;
> +}
> +
> +static int ionic_set_priv_flags(struct net_device *netdev, u32 priv_flags)
> +{
> +	struct ionic_lif *lif = netdev_priv(netdev);
> +	bool cmb_now, cmb_req;
> +	int ret;
> +
> +	cmb_now = test_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
> +	cmb_req = !!(priv_flags & IONIC_PRIV_F_CMB_RINGS);
> +	if (cmb_now != cmb_req) {
> +		ret = ionic_cmb_rings_toggle(lif, cmb_req);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static u32 ionic_get_rxfh_indir_size(struct net_device *netdev)
>  {
>  	struct ionic_lif *lif = netdev_priv(netdev);
> @@ -980,6 +1096,8 @@ static const struct ethtool_ops ionic_ethtool_ops = {
>  	.get_strings		= ionic_get_strings,
>  	.get_ethtool_stats	= ionic_get_stats,
>  	.get_sset_count		= ionic_get_sset_count,
> +	.get_priv_flags		= ionic_get_priv_flags,
> +	.set_priv_flags		= ionic_set_priv_flags,
>  	.get_rxnfc		= ionic_get_rxnfc,
>  	.get_rxfh_indir_size	= ionic_get_rxfh_indir_size,
>  	.get_rxfh_key_size	= ionic_get_rxfh_key_size,
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h
> index 38b91b1d70ae..6bc9c177d14b 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h
> @@ -4,6 +4,7 @@
>  #ifndef _IONIC_ETHTOOL_H_
>  #define _IONIC_ETHTOOL_H_
>  
> +int ionic_cmb_pages_in_use(struct ionic_lif *lif);
>  void ionic_ethtool_set_ops(struct net_device *netdev);
>  
>  #endif /* _IONIC_ETHTOOL_H_ */
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_if.h b/drivers/net/ethernet/pensando/ionic/ionic_if.h
> index eac09b2375b8..9a1825edf0d0 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_if.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_if.h
> @@ -3073,9 +3073,10 @@ union ionic_adminq_comp {
>  
>  #define IONIC_BARS_MAX			6
>  #define IONIC_PCI_BAR_DBELL		1
> +#define IONIC_PCI_BAR_CMB		2
>  
> -/* BAR0 */
>  #define IONIC_BAR0_SIZE				0x8000
> +#define IONIC_BAR2_SIZE				0x800000
>  
>  #define IONIC_BAR0_DEV_INFO_REGS_OFFSET		0x0000
>  #define IONIC_BAR0_DEV_CMD_REGS_OFFSET		0x0800
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> index 8499165b1563..e8884de83474 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> @@ -25,9 +25,12 @@
>  static const u8 ionic_qtype_versions[IONIC_QTYPE_MAX] = {
>  	[IONIC_QTYPE_ADMINQ]  = 0,   /* 0 = Base version with CQ support */
>  	[IONIC_QTYPE_NOTIFYQ] = 0,   /* 0 = Base version */
> -	[IONIC_QTYPE_RXQ]     = 0,   /* 0 = Base version with CQ+SG support */
> -	[IONIC_QTYPE_TXQ]     = 1,   /* 0 = Base version with CQ+SG support
> -				      * 1 =   ... with Tx SG version 1
> +	[IONIC_QTYPE_RXQ]     = 2,   /* 0 = Base version with CQ+SG support
> +				      * 2 =       ... with CMB rings
> +				      */
> +	[IONIC_QTYPE_TXQ]     = 3,   /* 0 = Base version with CQ+SG support
> +				      * 1 =       ... with Tx SG version 1
> +				      * 3 =       ... with CMB rings
>  				      */
>  };
>  
> @@ -379,6 +382,15 @@ static void ionic_qcq_free(struct ionic_lif *lif, struct ionic_qcq *qcq)
>  		qcq->q_base_pa = 0;
>  	}
>  
> +	if (qcq->cmb_q_base) {
> +		iounmap(qcq->cmb_q_base);
> +		ionic_put_cmb(lif, qcq->cmb_pgid, qcq->cmb_order);
> +		qcq->cmb_pgid = 0;
> +		qcq->cmb_order = 0;
> +		qcq->cmb_q_base = NULL;
> +		qcq->cmb_q_base_pa = 0;
> +	}
> +
>  	if (qcq->cq_base) {
>  		dma_free_coherent(dev, qcq->cq_size, qcq->cq_base, qcq->cq_base_pa);
>  		qcq->cq_base = NULL;
> @@ -587,6 +599,7 @@ static int ionic_qcq_alloc(struct ionic_lif *lif, unsigned int type,
>  		ionic_cq_map(&new->cq, cq_base, cq_base_pa);
>  		ionic_cq_bind(&new->cq, &new->q);
>  	} else {
> +		/* regular DMA q descriptors */
>  		new->q_size = PAGE_SIZE + (num_descs * desc_size);
>  		new->q_base = dma_alloc_coherent(dev, new->q_size, &new->q_base_pa,
>  						 GFP_KERNEL);
> @@ -599,6 +612,33 @@ static int ionic_qcq_alloc(struct ionic_lif *lif, unsigned int type,
>  		q_base_pa = ALIGN(new->q_base_pa, PAGE_SIZE);
>  		ionic_q_map(&new->q, q_base, q_base_pa);
>  
> +		if (flags & IONIC_QCQ_F_CMB_RINGS) {
> +			/* on-chip CMB q descriptors */
> +			new->cmb_q_size = num_descs * desc_size;
> +			new->cmb_order = order_base_2(new->cmb_q_size / PAGE_SIZE);
> +
> +			err = ionic_get_cmb(lif, &new->cmb_pgid, &new->cmb_q_base_pa,
> +					    new->cmb_order);
> +			if (err) {
> +				netdev_err(lif->netdev,
> +					   "Cannot allocate queue order %d from cmb: err %d\n",
> +					   new->cmb_order, err);
> +				goto err_out_free_q;
> +			}
> +
> +			new->cmb_q_base = ioremap_wc(new->cmb_q_base_pa, new->cmb_q_size);
> +			if (!new->cmb_q_base) {
> +				netdev_err(lif->netdev, "Cannot map queue from cmb\n");
> +				ionic_put_cmb(lif, new->cmb_pgid, new->cmb_order);
> +				err = -ENOMEM;
> +				goto err_out_free_q;
> +			}
> +
> +			new->cmb_q_base_pa -= idev->phy_cmb_pages;
> +			ionic_q_cmb_map(&new->q, new->cmb_q_base, new->cmb_q_base_pa);
> +		}
> +
> +		/* cq DMA descriptors */
>  		new->cq_size = PAGE_SIZE + (num_descs * cq_desc_size);
>  		new->cq_base = dma_alloc_coherent(dev, new->cq_size, &new->cq_base_pa,
>  						  GFP_KERNEL);
> @@ -637,6 +677,10 @@ static int ionic_qcq_alloc(struct ionic_lif *lif, unsigned int type,
>  err_out_free_cq:
>  	dma_free_coherent(dev, new->cq_size, new->cq_base, new->cq_base_pa);
>  err_out_free_q:
> +	if (new->cmb_q_base) {
> +		iounmap(new->cmb_q_base);
> +		ionic_put_cmb(lif, new->cmb_pgid, new->cmb_order);
> +	}
>  	dma_free_coherent(dev, new->q_size, new->q_base, new->q_base_pa);
>  err_out_free_cq_info:
>  	vfree(new->cq.info);
> @@ -718,6 +762,8 @@ static void ionic_qcq_sanitize(struct ionic_qcq *qcq)
>  	qcq->cq.tail_idx = 0;
>  	qcq->cq.done_color = 1;
>  	memset(qcq->q_base, 0, qcq->q_size);
> +	if (qcq->cmb_q_base)
> +		memset_io(qcq->cmb_q_base, 0, qcq->cmb_q_size);
>  	memset(qcq->cq_base, 0, qcq->cq_size);
>  	memset(qcq->sg_base, 0, qcq->sg_size);
>  }
> @@ -737,6 +783,7 @@ static int ionic_lif_txq_init(struct ionic_lif *lif, struct ionic_qcq *qcq)
>  			.index = cpu_to_le32(q->index),
>  			.flags = cpu_to_le16(IONIC_QINIT_F_IRQ |
>  					     IONIC_QINIT_F_SG),
> +			.intr_index = cpu_to_le16(qcq->intr.index),
>  			.pid = cpu_to_le16(q->pid),
>  			.ring_size = ilog2(q->num_descs),
>  			.ring_base = cpu_to_le64(q->base_pa),
> @@ -745,17 +792,19 @@ static int ionic_lif_txq_init(struct ionic_lif *lif, struct ionic_qcq *qcq)
>  			.features = cpu_to_le64(q->features),
>  		},
>  	};
> -	unsigned int intr_index;
>  	int err;
>  
> -	intr_index = qcq->intr.index;
> -
> -	ctx.cmd.q_init.intr_index = cpu_to_le16(intr_index);
> +	if (qcq->flags & IONIC_QCQ_F_CMB_RINGS) {
> +		ctx.cmd.q_init.flags |= cpu_to_le16(IONIC_QINIT_F_CMB);
> +		ctx.cmd.q_init.ring_base = cpu_to_le64(qcq->cmb_q_base_pa);
> +	}
>  
>  	dev_dbg(dev, "txq_init.pid %d\n", ctx.cmd.q_init.pid);
>  	dev_dbg(dev, "txq_init.index %d\n", ctx.cmd.q_init.index);
>  	dev_dbg(dev, "txq_init.ring_base 0x%llx\n", ctx.cmd.q_init.ring_base);
>  	dev_dbg(dev, "txq_init.ring_size %d\n", ctx.cmd.q_init.ring_size);
> +	dev_dbg(dev, "txq_init.cq_ring_base 0x%llx\n", ctx.cmd.q_init.cq_ring_base);
> +	dev_dbg(dev, "txq_init.sg_ring_base 0x%llx\n", ctx.cmd.q_init.sg_ring_base);
>  	dev_dbg(dev, "txq_init.flags 0x%x\n", ctx.cmd.q_init.flags);
>  	dev_dbg(dev, "txq_init.ver %d\n", ctx.cmd.q_init.ver);
>  	dev_dbg(dev, "txq_init.intr_index %d\n", ctx.cmd.q_init.intr_index);
> @@ -807,6 +856,11 @@ static int ionic_lif_rxq_init(struct ionic_lif *lif, struct ionic_qcq *qcq)
>  	};
>  	int err;
>  
> +	if (qcq->flags & IONIC_QCQ_F_CMB_RINGS) {
> +		ctx.cmd.q_init.flags |= cpu_to_le16(IONIC_QINIT_F_CMB);
> +		ctx.cmd.q_init.ring_base = cpu_to_le64(qcq->cmb_q_base_pa);
> +	}
> +
>  	dev_dbg(dev, "rxq_init.pid %d\n", ctx.cmd.q_init.pid);
>  	dev_dbg(dev, "rxq_init.index %d\n", ctx.cmd.q_init.index);
>  	dev_dbg(dev, "rxq_init.ring_base 0x%llx\n", ctx.cmd.q_init.ring_base);
> @@ -1966,8 +2020,13 @@ static int ionic_txrx_alloc(struct ionic_lif *lif)
>  		sg_desc_sz = sizeof(struct ionic_txq_sg_desc);
>  
>  	flags = IONIC_QCQ_F_TX_STATS | IONIC_QCQ_F_SG;
> +
> +	if (test_bit(IONIC_LIF_F_CMB_RINGS, lif->state))
> +		flags |= IONIC_QCQ_F_CMB_RINGS;
> +
>  	if (test_bit(IONIC_LIF_F_SPLIT_INTR, lif->state))
>  		flags |= IONIC_QCQ_F_INTR;
> +
>  	for (i = 0; i < lif->nxqs; i++) {
>  		err = ionic_qcq_alloc(lif, IONIC_QTYPE_TXQ, i, "tx", flags,
>  				      num_desc, desc_sz, comp_sz, sg_desc_sz,
> @@ -1988,6 +2047,9 @@ static int ionic_txrx_alloc(struct ionic_lif *lif)
>  
>  	flags = IONIC_QCQ_F_RX_STATS | IONIC_QCQ_F_SG | IONIC_QCQ_F_INTR;
>  
> +	if (test_bit(IONIC_LIF_F_CMB_RINGS, lif->state))
> +		flags |= IONIC_QCQ_F_CMB_RINGS;
> +
>  	num_desc = lif->nrxq_descs;
>  	desc_sz = sizeof(struct ionic_rxq_desc);
>  	comp_sz = sizeof(struct ionic_rxq_comp);
> @@ -2663,6 +2725,55 @@ static const struct net_device_ops ionic_netdev_ops = {
>  	.ndo_get_vf_stats       = ionic_get_vf_stats,
>  };
>  
> +static int ionic_cmb_reconfig(struct ionic_lif *lif,
> +			      struct ionic_queue_params *qparam)
> +{
> +	struct ionic_queue_params start_qparams;
> +	int err = 0;
> +
> +	/* When changing CMB queue parameters, we're using limited
> +	 * on-device memory and don't have extra memory to use for
> +	 * duplicate allocations, so we free it all first then
> +	 * re-allocate with the new parameters.
> +	 */
> +
> +	/* Checkpoint for possible unwind */
> +	ionic_init_queue_params(lif, &start_qparams);
> +
> +	/* Stop and free the queues */
> +	ionic_stop_queues_reconfig(lif);
> +	ionic_txrx_free(lif);
> +
> +	/* Set up new qparams */
> +	ionic_set_queue_params(lif, qparam);
> +
> +	if (netif_running(lif->netdev)) {
> +		/* Alloc and start the new configuration */
> +		err = ionic_txrx_alloc(lif);
> +		if (err) {
> +			dev_warn(lif->ionic->dev,
> +				 "CMB reconfig failed, restoring values: %d\n", err);
> +
> +			/* Back out the changes */
> +			ionic_set_queue_params(lif, &start_qparams);
> +			err = ionic_txrx_alloc(lif);
> +			if (err) {
> +				dev_err(lif->ionic->dev,
> +					"CMB restore failed: %d\n", err);
> +				goto errout;
> +			}
> +		}
> +
> +		ionic_start_queues_reconfig(lif);
> +	} else {
> +		/* This was detached in ionic_stop_queues_reconfig() */
> +		netif_device_attach(lif->netdev);
> +	}
> +
> +errout:
> +	return err;
> +}
> +
>  static void ionic_swap_queues(struct ionic_qcq *a, struct ionic_qcq *b)
>  {
>  	/* only swapping the queues, not the napi, flags, or other stuff */
> @@ -2705,6 +2816,10 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
>  	unsigned int flags, i;
>  	int err = 0;
>  
> +	/* Are we changing q params while CMB is on */
> +	if (test_bit(IONIC_LIF_F_CMB_RINGS, lif->state) && qparam->cmb_enabled)
> +		return ionic_cmb_reconfig(lif, qparam);
> +
>  	/* allocate temporary qcq arrays to hold new queue structs */
>  	if (qparam->nxqs != lif->nxqs || qparam->ntxq_descs != lif->ntxq_descs) {
>  		tx_qcqs = devm_kcalloc(lif->ionic->dev, lif->ionic->ntxqs_per_lif,
> @@ -2741,6 +2856,16 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
>  			sg_desc_sz = sizeof(struct ionic_txq_sg_desc);
>  
>  		for (i = 0; i < qparam->nxqs; i++) {
> +			/* If missing, short placeholder qcq needed for swap */
> +			if (!lif->txqcqs[i]) {
> +				flags = IONIC_QCQ_F_TX_STATS | IONIC_QCQ_F_SG;
> +				err = ionic_qcq_alloc(lif, IONIC_QTYPE_TXQ, i, "tx", flags,
> +						      4, desc_sz, comp_sz, sg_desc_sz,
> +						      lif->kern_pid, &lif->txqcqs[i]);
> +				if (err)
> +					goto err_out;
> +			}
> +
>  			flags = lif->txqcqs[i]->flags & ~IONIC_QCQ_F_INTR;
>  			err = ionic_qcq_alloc(lif, IONIC_QTYPE_TXQ, i, "tx", flags,
>  					      num_desc, desc_sz, comp_sz, sg_desc_sz,
> @@ -2760,6 +2885,16 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
>  			comp_sz *= 2;
>  
>  		for (i = 0; i < qparam->nxqs; i++) {
> +			/* If missing, short placeholder qcq needed for swap */
> +			if (!lif->rxqcqs[i]) {
> +				flags = IONIC_QCQ_F_RX_STATS | IONIC_QCQ_F_SG;
> +				err = ionic_qcq_alloc(lif, IONIC_QTYPE_RXQ, i, "rx", flags,
> +						      4, desc_sz, comp_sz, sg_desc_sz,
> +						      lif->kern_pid, &lif->rxqcqs[i]);
> +				if (err)
> +					goto err_out;
> +			}
> +
>  			flags = lif->rxqcqs[i]->flags & ~IONIC_QCQ_F_INTR;
>  			err = ionic_qcq_alloc(lif, IONIC_QTYPE_RXQ, i, "rx", flags,
>  					      num_desc, desc_sz, comp_sz, sg_desc_sz,
> @@ -2809,10 +2944,15 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
>  			lif->tx_coalesce_hw = lif->rx_coalesce_hw;
>  		}
>  
> -		/* clear existing interrupt assignments */
> +		/* Clear existing interrupt assignments.  We check for NULL here
> +		 * because we're checking the whole array for potential qcqs, not
> +		 * just those qcqs that have just been set up.
> +		 */
>  		for (i = 0; i < lif->ionic->ntxqs_per_lif; i++) {
> -			ionic_qcq_intr_free(lif, lif->txqcqs[i]);
> -			ionic_qcq_intr_free(lif, lif->rxqcqs[i]);
> +			if (lif->txqcqs[i])
> +				ionic_qcq_intr_free(lif, lif->txqcqs[i]);
> +			if (lif->rxqcqs[i])
> +				ionic_qcq_intr_free(lif, lif->rxqcqs[i]);
>  		}
>  
>  		/* re-assign the interrupts */
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> index a53984bf3544..5425a8983ae0 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> @@ -59,6 +59,7 @@ struct ionic_rx_stats {
>  #define IONIC_QCQ_F_TX_STATS		BIT(3)
>  #define IONIC_QCQ_F_RX_STATS		BIT(4)
>  #define IONIC_QCQ_F_NOTIFYQ		BIT(5)
> +#define IONIC_QCQ_F_CMB_RINGS		BIT(6)
>  
>  struct ionic_qcq {
>  	void *q_base;
> @@ -70,6 +71,11 @@ struct ionic_qcq {
>  	void *sg_base;
>  	dma_addr_t sg_base_pa;
>  	u32 sg_size;
> +	void __iomem *cmb_q_base;
> +	phys_addr_t cmb_q_base_pa;
> +	u32 cmb_q_size;
> +	u32 cmb_pgid;
> +	u32 cmb_order;
>  	struct dim dim;
>  	struct ionic_queue q;
>  	struct ionic_cq cq;
> @@ -140,6 +146,7 @@ enum ionic_lif_state_flags {
>  	IONIC_LIF_F_BROKEN,
>  	IONIC_LIF_F_TX_DIM_INTR,
>  	IONIC_LIF_F_RX_DIM_INTR,
> +	IONIC_LIF_F_CMB_RINGS,
>  
>  	/* leave this as last */
>  	IONIC_LIF_F_STATE_SIZE
> @@ -243,8 +250,9 @@ struct ionic_queue_params {
>  	unsigned int nxqs;
>  	unsigned int ntxq_descs;
>  	unsigned int nrxq_descs;
> -	unsigned int intr_split;
>  	u64 rxq_features;
> +	bool intr_split;
> +	bool cmb_enabled;

Use of bool in structs is frowned upon since bool doesn't have a
strictly defined size. It is usually preferred to use u8.

>  };
>  
>  static inline void ionic_init_queue_params(struct ionic_lif *lif,
> @@ -253,8 +261,28 @@ static inline void ionic_init_queue_params(struct ionic_lif *lif,
>  	qparam->nxqs = lif->nxqs;
>  	qparam->ntxq_descs = lif->ntxq_descs;
>  	qparam->nrxq_descs = lif->nrxq_descs;
> -	qparam->intr_split = test_bit(IONIC_LIF_F_SPLIT_INTR, lif->state);
>  	qparam->rxq_features = lif->rxq_features;
> +	qparam->intr_split = test_bit(IONIC_LIF_F_SPLIT_INTR, lif->state);
> +	qparam->cmb_enabled = test_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
> +}
> +
> +static inline void ionic_set_queue_params(struct ionic_lif *lif,
> +					  struct ionic_queue_params *qparam)
> +{
> +	lif->nxqs = qparam->nxqs;
> +	lif->ntxq_descs = qparam->ntxq_descs;
> +	lif->nrxq_descs = qparam->nrxq_descs;
> +	lif->rxq_features = qparam->rxq_features;
> +
> +	if (qparam->intr_split)
> +		set_bit(IONIC_LIF_F_SPLIT_INTR, lif->state);
> +	else
> +		clear_bit(IONIC_LIF_F_SPLIT_INTR, lif->state);
> +
> +	if (qparam->cmb_enabled)
> +		set_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
> +	else
> +		clear_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
>  }
>  
>  static inline u32 ionic_coal_usec_to_hw(struct ionic *ionic, u32 usecs)
> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> index 0c3977416cd1..0cb464931d3d 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
> @@ -419,6 +419,10 @@ void ionic_rx_fill(struct ionic_queue *q)
>  					      IONIC_RXQ_DESC_OPCODE_SIMPLE;
>  		desc_info->nbufs = nfrags;
>  
> +		/* commit CMB descriptor contents in one shot */
> +		if (q_to_qcq(q)->flags & IONIC_QCQ_F_CMB_RINGS)
> +			memcpy_toio(desc_info->cmb_desc, desc, q->desc_size);
> +
>  		ionic_rxq_post(q, false, ionic_rx_clean, NULL);
>  	}
>  

So this bit is repeated below and the comment is somewhat confusing as
I am assuming you are using some combination of write combining and the
memcpy to generate the "one shot" update you are referring to here. 

My suggestion would be to look at creating an inline function somewhere
above here that takes the q, desc_info and desc as arguments and can do
all this as a single function call. Then in that function you can add a
comment explaining how this is a "one shot" write.

> @@ -860,7 +864,8 @@ static int ionic_tx_tcp_pseudo_csum(struct sk_buff *skb)
>  	return 0;
>  }
>  
> -static void ionic_tx_tso_post(struct ionic_queue *q, struct ionic_txq_desc *desc,
> +static void ionic_tx_tso_post(struct ionic_queue *q,
> +			      struct ionic_desc_info *desc_info,
>  			      struct sk_buff *skb,
>  			      dma_addr_t addr, u8 nsge, u16 len,
>  			      unsigned int hdrlen, unsigned int mss,
> @@ -868,6 +873,7 @@ static void ionic_tx_tso_post(struct ionic_queue *q, struct ionic_txq_desc *desc
>  			      u16 vlan_tci, bool has_vlan,
>  			      bool start, bool done)
>  {
> +	struct ionic_txq_desc *desc = desc_info->desc;
>  	u8 flags = 0;
>  	u64 cmd;
>  
> @@ -883,6 +889,10 @@ static void ionic_tx_tso_post(struct ionic_queue *q, struct ionic_txq_desc *desc
>  	desc->hdr_len = cpu_to_le16(hdrlen);
>  	desc->mss = cpu_to_le16(mss);
>  
> +	/* commit CMB descriptor contents in one shot */
> +	if (q_to_qcq(q)->flags & IONIC_QCQ_F_CMB_RINGS)
> +		memcpy_toio(desc_info->cmb_desc, desc, q->desc_size);
> +
>  	if (start) {
>  		skb_tx_timestamp(skb);
>  		if (!unlikely(q->features & IONIC_TXQ_F_HWSTAMP))
> @@ -1001,7 +1011,7 @@ static int ionic_tx_tso(struct ionic_queue *q, struct sk_buff *skb)
>  		seg_rem = min(tso_rem, mss);
>  		done = (tso_rem == 0);
>  		/* post descriptor */
> -		ionic_tx_tso_post(q, desc, skb,
> +		ionic_tx_tso_post(q, desc_info, skb,
>  				  desc_addr, desc_nsge, desc_len,
>  				  hdrlen, mss, outer_csum, vlan_tci, has_vlan,
>  				  start, done);
> @@ -1050,6 +1060,10 @@ static void ionic_tx_calc_csum(struct ionic_queue *q, struct sk_buff *skb,
>  	desc->csum_start = cpu_to_le16(skb_checksum_start_offset(skb));
>  	desc->csum_offset = cpu_to_le16(skb->csum_offset);
>  
> +	/* commit descriptor contents in one shot */
> +	if (q_to_qcq(q)->flags & IONIC_QCQ_F_CMB_RINGS)
> +		memcpy_toio(desc_info->cmb_desc, desc, q->desc_size);
> +
>  	if (skb_csum_is_sctp(skb))
>  		stats->crc32_csum++;
>  	else
> @@ -1087,6 +1101,10 @@ static void ionic_tx_calc_no_csum(struct ionic_queue *q, struct sk_buff *skb,
>  	desc->csum_start = 0;
>  	desc->csum_offset = 0;
>  
> +	/* commit descriptor contents in one shot */
> +	if (q_to_qcq(q)->flags & IONIC_QCQ_F_CMB_RINGS)
> +		memcpy_toio(desc_info->cmb_desc, desc, q->desc_size);
> +
>  	stats->csum_none++;
>  }
>  

Re: [PATCH v2 net-next 3/3] ionic: add support for device Component Memory Buffers

[Shannon Nelson]

On 2/6/23 1:36 PM, Alexander H Duyck wrote:
> On Mon, 2023-02-06 at 10:16 -0800, Shannon Nelson wrote:
>> The ionic device has on-board memory (CMB) that can be used
>> for descriptors as a way to speed descriptor access for faster
>> traffic processing.  It imposes a couple of restrictions so
>> is not on by default, but can be enabled through the ethtool
>> priv-flags.
> 
> For the purposes of patch review it might be convinent to call out what
> those restrictions are as you enable the code below. I'm assuming it is
> mostly just the amount of space you can use, but if there is something
> else it would be useful to have that noted.
> 
>>
>> Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
>> ---
>>   .../ethernet/pensando/ionic/ionic_bus_pci.c   |   3 +
>>   .../net/ethernet/pensando/ionic/ionic_dev.c   |  71 ++++++++
>>   .../net/ethernet/pensando/ionic/ionic_dev.h   |  13 ++
>>   .../ethernet/pensando/ionic/ionic_ethtool.c   | 128 +++++++++++++-
>>   .../ethernet/pensando/ionic/ionic_ethtool.h   |   1 +
>>   .../net/ethernet/pensando/ionic/ionic_if.h    |   3 +-
>>   .../net/ethernet/pensando/ionic/ionic_lif.c   | 160 ++++++++++++++++--
>>   .../net/ethernet/pensando/ionic/ionic_lif.h   |  32 +++-
>>   .../net/ethernet/pensando/ionic/ionic_txrx.c  |  22 ++-
>>   9 files changed, 413 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> index 0eff78fa0565..f62f11ba21c0 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_bus_pci.c
>> @@ -352,6 +352,7 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   err_out_reset:
>>        ionic_reset(ionic);
>>   err_out_teardown:
>> +     ionic_dev_teardown(ionic);
>>        pci_clear_master(pdev);
>>        /* Don't fail the probe for these errors, keep
>>         * the hw interface around for inspection
>> @@ -369,6 +370,7 @@ static int ionic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   err_out_clear_drvdata:
>>        mutex_destroy(&ionic->dev_cmd_lock);
>>        ionic_devlink_free(ionic);
>> +     pci_set_drvdata(pdev, NULL);
>>
>>        return err;
>>   }
> 
> This piece almost seems like it is unrelated. Perhaps it bled through
> from some other work or cleanup?

I can pull that out.


> 
>> @@ -390,6 +392,7 @@ static void ionic_remove(struct pci_dev *pdev)
>>
>>        ionic_port_reset(ionic);
>>        ionic_reset(ionic);
>> +     ionic_dev_teardown(ionic);
>>        pci_clear_master(pdev);
>>        ionic_unmap_bars(ionic);
>>        pci_release_regions(pdev);
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>> index 626b9113e7c4..9b4bba2279ab 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>> @@ -92,6 +92,7 @@ int ionic_dev_setup(struct ionic *ionic)
>>        unsigned int num_bars = ionic->num_bars;
>>        struct ionic_dev *idev = &ionic->idev;
>>        struct device *dev = ionic->dev;
>> +     int size;
>>        u32 sig;
>>
>>        /* BAR0: dev_cmd and interrupts */
>> @@ -133,9 +134,40 @@ int ionic_dev_setup(struct ionic *ionic)
>>        idev->db_pages = bar->vaddr;
>>        idev->phy_db_pages = bar->bus_addr;
>>
>> +     /* BAR2: optional controller memory mapping */
>> +     bar++;
>> +     mutex_init(&idev->cmb_inuse_lock);
>> +     if (num_bars < 3 || !ionic->bars[IONIC_PCI_BAR_CMB].len) {
>> +             idev->cmb_inuse = NULL;
>> +             idev->phy_cmb_pages = 0;
>> +             idev->cmb_npages = 0;
>> +             return 0;
>> +     }
>> +
>> +     idev->phy_cmb_pages = bar->bus_addr;
>> +     idev->cmb_npages = bar->len / PAGE_SIZE;
>> +     size = BITS_TO_LONGS(idev->cmb_npages) * sizeof(long);
>> +     idev->cmb_inuse = kzalloc(size, GFP_KERNEL);
>> +     if (!idev->cmb_inuse) {
>> +             idev->phy_cmb_pages = 0;
>> +             idev->cmb_npages = 0;
>> +     }
>> +
> 
> Why not hold of on setting phy_cmb_pages and cmb_npages until after you
> have allocated the pages rather then resetting them in the event of
> failure?

I need the values anyway to determine size, and the fail is unlikely, so 
why bother with tmp variables in the middle?  Also, this clearly sets 
idev->cmb_npages to 0 which is used as an indicator in the ethtool 
handler that thm CMB pages feature is not available.

> 
> Also is it really acceptable for this to fail silently?

The fail would be from the memory allocation, which usually will already 
have printed some message, and we are usually discouraged from adding 
more "alloc failed" type messages.  Also, this is an optional feature, 
not needed for normal driver operations, so we don't need to kill the 
probe with an error code.  If the user tries to enable CMB, they will 
get a message that it is not available.

> 
>>        return 0;
>>   }
>>
>> +void ionic_dev_teardown(struct ionic *ionic)
>> +{
>> +     struct ionic_dev *idev = &ionic->idev;
>> +
>> +     kfree(idev->cmb_inuse);
>> +     idev->cmb_inuse = NULL;
>> +     idev->phy_cmb_pages = 0;
>> +     idev->cmb_npages = 0;
>> +
>> +     mutex_destroy(&idev->cmb_inuse_lock);
>> +}
>> +
>>   /* Devcmd Interface */
>>   bool ionic_is_fw_running(struct ionic_dev *idev)
>>   {
>> @@ -571,6 +603,33 @@ int ionic_db_page_num(struct ionic_lif *lif, int pid)
>>        return (lif->hw_index * lif->dbid_count) + pid;
>>   }
>>
>> +int ionic_get_cmb(struct ionic_lif *lif, u32 *pgid, phys_addr_t *pgaddr, int order)
>> +{
>> +     struct ionic_dev *idev = &lif->ionic->idev;
>> +     int ret;
>> +
>> +     mutex_lock(&idev->cmb_inuse_lock);
>> +     ret = bitmap_find_free_region(idev->cmb_inuse, idev->cmb_npages, order);
>> +     mutex_unlock(&idev->cmb_inuse_lock);
>> +
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     *pgid = (u32)ret;
> 
> I'm not sure if the cast is necessary since we are guaranteed ret will
> be unsigned based on the earlier check.

Belt and suspenders?  Sure, I can remove that.

> 
>> +     *pgaddr = idev->phy_cmb_pages + ret * PAGE_SIZE;
>> +
>> +     return 0;
>> +}
>> +
>> +void ionic_put_cmb(struct ionic_lif *lif, u32 pgid, int order)
>> +{
>> +     struct ionic_dev *idev = &lif->ionic->idev;
>> +
>> +     mutex_lock(&idev->cmb_inuse_lock);
>> +     bitmap_release_region(idev->cmb_inuse, pgid, order);
>> +     mutex_unlock(&idev->cmb_inuse_lock);
>> +}
>> +
>>   int ionic_cq_init(struct ionic_lif *lif, struct ionic_cq *cq,
>>                  struct ionic_intr_info *intr,
>>                  unsigned int num_descs, size_t desc_size)
>> @@ -679,6 +738,18 @@ void ionic_q_map(struct ionic_queue *q, void *base, dma_addr_t base_pa)
>>                cur->desc = base + (i * q->desc_size);
>>   }
>>
>> +void ionic_q_cmb_map(struct ionic_queue *q, void __iomem *base, dma_addr_t base_pa)
>> +{
>> +     struct ionic_desc_info *cur;
>> +     unsigned int i;
>> +
>> +     q->cmb_base = base;
>> +     q->cmb_base_pa = base_pa;
>> +
>> +     for (i = 0, cur = q->info; i < q->num_descs; i++, cur++)
>> +             cur->cmb_desc = base + (i * q->desc_size);
>> +}
>> +
>>   void ionic_q_sg_map(struct ionic_queue *q, void *base, dma_addr_t base_pa)
>>   {
>>        struct ionic_desc_info *cur;
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.h b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
>> index 2a1d7b9c07e7..a4a8802f3771 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.h
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.h
>> @@ -153,6 +153,11 @@ struct ionic_dev {
>>        struct ionic_intr __iomem *intr_ctrl;
>>        u64 __iomem *intr_status;
>>
>> +     struct mutex cmb_inuse_lock; /* for cmb_inuse */
>> +     unsigned long *cmb_inuse;
>> +     dma_addr_t phy_cmb_pages;
>> +     u32 cmb_npages;
>> +
>>        u32 port_info_sz;
>>        struct ionic_port_info *port_info;
>>        dma_addr_t port_info_pa;
>> @@ -197,6 +202,7 @@ struct ionic_desc_info {
>>                struct ionic_rxq_desc *rxq_desc;
>>                struct ionic_admin_cmd *adminq_desc;
>>        };
>> +     void __iomem *cmb_desc;
>>        union {
>>                void *sg_desc;
>>                struct ionic_txq_sg_desc *txq_sg_desc;
>> @@ -233,12 +239,14 @@ struct ionic_queue {
>>                struct ionic_rxq_desc *rxq;
>>                struct ionic_admin_cmd *adminq;
>>        };
>> +     void __iomem *cmb_base;
>>        union {
>>                void *sg_base;
>>                struct ionic_txq_sg_desc *txq_sgl;
>>                struct ionic_rxq_sg_desc *rxq_sgl;
>>        };
>>        dma_addr_t base_pa;
>> +     dma_addr_t cmb_base_pa;
>>        dma_addr_t sg_base_pa;
>>        unsigned int desc_size;
>>        unsigned int sg_desc_size;
>> @@ -301,6 +309,7 @@ static inline bool ionic_q_has_space(struct ionic_queue *q, unsigned int want)
>>
>>   void ionic_init_devinfo(struct ionic *ionic);
>>   int ionic_dev_setup(struct ionic *ionic);
>> +void ionic_dev_teardown(struct ionic *ionic);
>>
>>   void ionic_dev_cmd_go(struct ionic_dev *idev, union ionic_dev_cmd *cmd);
>>   u8 ionic_dev_cmd_status(struct ionic_dev *idev);
>> @@ -336,6 +345,9 @@ void ionic_dev_cmd_adminq_init(struct ionic_dev *idev, struct ionic_qcq *qcq,
>>
>>   int ionic_db_page_num(struct ionic_lif *lif, int pid);
>>
>> +int ionic_get_cmb(struct ionic_lif *lif, u32 *pgid, phys_addr_t *pgaddr, int order);
>> +void ionic_put_cmb(struct ionic_lif *lif, u32 pgid, int order);
>> +
>>   int ionic_cq_init(struct ionic_lif *lif, struct ionic_cq *cq,
>>                  struct ionic_intr_info *intr,
>>                  unsigned int num_descs, size_t desc_size);
>> @@ -352,6 +364,7 @@ int ionic_q_init(struct ionic_lif *lif, struct ionic_dev *idev,
>>                 unsigned int num_descs, size_t desc_size,
>>                 size_t sg_desc_size, unsigned int pid);
>>   void ionic_q_map(struct ionic_queue *q, void *base, dma_addr_t base_pa);
>> +void ionic_q_cmb_map(struct ionic_queue *q, void __iomem *base, dma_addr_t base_pa);
>>   void ionic_q_sg_map(struct ionic_queue *q, void *base, dma_addr_t base_pa);
>>   void ionic_q_post(struct ionic_queue *q, bool ring_doorbell, ionic_desc_cb cb,
>>                  void *cb_arg);
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>> index 01c22701482d..45e5dc400095 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.c
>> @@ -11,6 +11,38 @@
>>   #include "ionic_ethtool.h"
>>   #include "ionic_stats.h"
>>
>> +static const char ionic_priv_flags_strings[][ETH_GSTRING_LEN] = {
>> +#define IONIC_PRIV_F_CMB_RINGS               BIT(0)
>> +     "cmb-rings",
>> +};
>> +
>> +#define IONIC_PRIV_FLAGS_COUNT ARRAY_SIZE(ionic_priv_flags_strings)
>> +
>> +static int ionic_validate_cmb_config(struct ionic_lif *lif,
>> +                                  struct ionic_queue_params *qparam)
>> +{
>> +     int pages_have, pages_required = 0;
>> +     unsigned long sz;
>> +
>> +     if (!qparam->cmb_enabled)
>> +             return 0;
>> +
>> +     sz = sizeof(struct ionic_txq_desc) * qparam->ntxq_descs * qparam->nxqs;
>> +     pages_required += ALIGN(sz, PAGE_SIZE) / PAGE_SIZE;
>> +
>> +     sz = sizeof(struct ionic_rxq_desc) * qparam->nrxq_descs * qparam->nxqs;
>> +     pages_required += ALIGN(sz, PAGE_SIZE) / PAGE_SIZE;
>> +
>> +     pages_have = lif->ionic->bars[IONIC_PCI_BAR_CMB].len / PAGE_SIZE;
>> +     if (pages_required > pages_have) {
>> +             netdev_info(lif->netdev, "Not enough CMB pages for number of queues and size of descriptor rings, need %d have %d",
>> +                         pages_required, pages_have);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     return pages_required;
>> +}
>> +
>>   static void ionic_get_stats_strings(struct ionic_lif *lif, u8 *buf)
>>   {
>>        u32 i;
>> @@ -52,6 +84,9 @@ static int ionic_get_sset_count(struct net_device *netdev, int sset)
>>        case ETH_SS_STATS:
>>                count = ionic_get_stats_count(lif);
>>                break;
>> +     case ETH_SS_PRIV_FLAGS:
>> +             count = IONIC_PRIV_FLAGS_COUNT;
>> +             break;
>>        }
>>        return count;
>>   }
>> @@ -65,6 +100,10 @@ static void ionic_get_strings(struct net_device *netdev,
>>        case ETH_SS_STATS:
>>                ionic_get_stats_strings(lif, buf);
>>                break;
>> +     case ETH_SS_PRIV_FLAGS:
>> +             memcpy(buf, ionic_priv_flags_strings,
>> +                    IONIC_PRIV_FLAGS_COUNT * ETH_GSTRING_LEN);
>> +             break;
>>        }
>>   }
>>
>> @@ -554,6 +593,13 @@ static int ionic_set_ringparam(struct net_device *netdev,
>>            ring->rx_pending == lif->nrxq_descs)
>>                return 0;
>>
>> +     qparam.ntxq_descs = ring->tx_pending;
>> +     qparam.nrxq_descs = ring->rx_pending;
>> +
>> +     err = ionic_validate_cmb_config(lif, &qparam);
>> +     if (err < 0)
>> +             return err;
>> +
>>        if (ring->tx_pending != lif->ntxq_descs)
>>                netdev_info(netdev, "Changing Tx ring size from %d to %d\n",
>>                            lif->ntxq_descs, ring->tx_pending);
>> @@ -569,9 +615,6 @@ static int ionic_set_ringparam(struct net_device *netdev,
>>                return 0;
>>        }
>>
>> -     qparam.ntxq_descs = ring->tx_pending;
>> -     qparam.nrxq_descs = ring->rx_pending;
>> -
>>        mutex_lock(&lif->queue_lock);
>>        err = ionic_reconfigure_queues(lif, &qparam);
>>        mutex_unlock(&lif->queue_lock);
>> @@ -638,7 +681,7 @@ static int ionic_set_channels(struct net_device *netdev,
>>                                    lif->nxqs, ch->combined_count);
>>
>>                qparam.nxqs = ch->combined_count;
>> -             qparam.intr_split = 0;
>> +             qparam.intr_split = false;
>>        } else {
>>                max_cnt /= 2;
>>                if (ch->rx_count > max_cnt)
>> @@ -654,9 +697,13 @@ static int ionic_set_channels(struct net_device *netdev,
>>                                    lif->nxqs, ch->rx_count);
>>
>>                qparam.nxqs = ch->rx_count;
>> -             qparam.intr_split = 1;
>> +             qparam.intr_split = true;
>>        }
>>
>> +     err = ionic_validate_cmb_config(lif, &qparam);
>> +     if (err < 0)
>> +             return err;
>> +
>>        /* if we're not running, just set the values and return */
>>        if (!netif_running(lif->netdev)) {
>>                lif->nxqs = qparam.nxqs;
>> @@ -699,6 +746,75 @@ static int ionic_get_rxnfc(struct net_device *netdev,
>>        return err;
>>   }
>>
>> +int ionic_cmb_pages_in_use(struct ionic_lif *lif)
>> +{
>> +     struct ionic_queue_params qparam;
>> +
>> +     ionic_init_queue_params(lif, &qparam);
>> +     return ionic_validate_cmb_config(lif, &qparam);
>> +}
>> +
>> +static int ionic_cmb_rings_toggle(struct ionic_lif *lif, bool cmb_enable)
>> +{
>> +     struct ionic_queue_params qparam;
>> +     int pages_used;
>> +
>> +     if (!(lif->qtype_info[IONIC_QTYPE_TXQ].features & IONIC_QIDENT_F_CMB) ||
>> +         !(lif->qtype_info[IONIC_QTYPE_RXQ].features & IONIC_QIDENT_F_CMB) ||
>> +         !lif->ionic->idev.cmb_npages) {
>> +             netdev_info(lif->netdev, "CMB rings are not supported on this device\n");
>> +             return -EOPNOTSUPP;
>> +     }
>> +
>> +     if (netif_running(lif->netdev))
>> +             return -EBUSY;
>> +
>> +     ionic_init_queue_params(lif, &qparam);
>> +     qparam.cmb_enabled = cmb_enable;
>> +     pages_used = ionic_validate_cmb_config(lif, &qparam);
>> +     if (pages_used < 0)
>> +             return pages_used;
>> +
>> +     if (cmb_enable) {
>> +             netdev_info(lif->netdev, "Enabling CMB rings - %d pages\n",
>> +                         pages_used);
>> +             set_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
>> +     } else {
>> +             netdev_info(lif->netdev, "Disabling CMB rings\n");
>> +             clear_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static u32 ionic_get_priv_flags(struct net_device *netdev)
>> +{
>> +     struct ionic_lif *lif = netdev_priv(netdev);
>> +     u32 priv_flags = 0;
>> +
>> +     if (test_bit(IONIC_LIF_F_CMB_RINGS, lif->state))
>> +             priv_flags |= IONIC_PRIV_F_CMB_RINGS;
>> +
>> +     return priv_flags;
>> +}
>> +
>> +static int ionic_set_priv_flags(struct net_device *netdev, u32 priv_flags)
>> +{
>> +     struct ionic_lif *lif = netdev_priv(netdev);
>> +     bool cmb_now, cmb_req;
>> +     int ret;
>> +
>> +     cmb_now = test_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
>> +     cmb_req = !!(priv_flags & IONIC_PRIV_F_CMB_RINGS);
>> +     if (cmb_now != cmb_req) {
>> +             ret = ionic_cmb_rings_toggle(lif, cmb_req);
>> +             if (ret < 0)
>> +                     return ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>   static u32 ionic_get_rxfh_indir_size(struct net_device *netdev)
>>   {
>>        struct ionic_lif *lif = netdev_priv(netdev);
>> @@ -980,6 +1096,8 @@ static const struct ethtool_ops ionic_ethtool_ops = {
>>        .get_strings            = ionic_get_strings,
>>        .get_ethtool_stats      = ionic_get_stats,
>>        .get_sset_count         = ionic_get_sset_count,
>> +     .get_priv_flags         = ionic_get_priv_flags,
>> +     .set_priv_flags         = ionic_set_priv_flags,
>>        .get_rxnfc              = ionic_get_rxnfc,
>>        .get_rxfh_indir_size    = ionic_get_rxfh_indir_size,
>>        .get_rxfh_key_size      = ionic_get_rxfh_key_size,
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h
>> index 38b91b1d70ae..6bc9c177d14b 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_ethtool.h
>> @@ -4,6 +4,7 @@
>>   #ifndef _IONIC_ETHTOOL_H_
>>   #define _IONIC_ETHTOOL_H_
>>
>> +int ionic_cmb_pages_in_use(struct ionic_lif *lif);
>>   void ionic_ethtool_set_ops(struct net_device *netdev);
>>
>>   #endif /* _IONIC_ETHTOOL_H_ */
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_if.h b/drivers/net/ethernet/pensando/ionic/ionic_if.h
>> index eac09b2375b8..9a1825edf0d0 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_if.h
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_if.h
>> @@ -3073,9 +3073,10 @@ union ionic_adminq_comp {
>>
>>   #define IONIC_BARS_MAX                       6
>>   #define IONIC_PCI_BAR_DBELL          1
>> +#define IONIC_PCI_BAR_CMB            2
>>
>> -/* BAR0 */
>>   #define IONIC_BAR0_SIZE                              0x8000
>> +#define IONIC_BAR2_SIZE                              0x800000
>>
>>   #define IONIC_BAR0_DEV_INFO_REGS_OFFSET              0x0000
>>   #define IONIC_BAR0_DEV_CMD_REGS_OFFSET               0x0800
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> index 8499165b1563..e8884de83474 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
>> @@ -25,9 +25,12 @@
>>   static const u8 ionic_qtype_versions[IONIC_QTYPE_MAX] = {
>>        [IONIC_QTYPE_ADMINQ]  = 0,   /* 0 = Base version with CQ support */
>>        [IONIC_QTYPE_NOTIFYQ] = 0,   /* 0 = Base version */
>> -     [IONIC_QTYPE_RXQ]     = 0,   /* 0 = Base version with CQ+SG support */
>> -     [IONIC_QTYPE_TXQ]     = 1,   /* 0 = Base version with CQ+SG support
>> -                                   * 1 =   ... with Tx SG version 1
>> +     [IONIC_QTYPE_RXQ]     = 2,   /* 0 = Base version with CQ+SG support
>> +                                   * 2 =       ... with CMB rings
>> +                                   */
>> +     [IONIC_QTYPE_TXQ]     = 3,   /* 0 = Base version with CQ+SG support
>> +                                   * 1 =       ... with Tx SG version 1
>> +                                   * 3 =       ... with CMB rings
>>                                      */
>>   };
>>
>> @@ -379,6 +382,15 @@ static void ionic_qcq_free(struct ionic_lif *lif, struct ionic_qcq *qcq)
>>                qcq->q_base_pa = 0;
>>        }
>>
>> +     if (qcq->cmb_q_base) {
>> +             iounmap(qcq->cmb_q_base);
>> +             ionic_put_cmb(lif, qcq->cmb_pgid, qcq->cmb_order);
>> +             qcq->cmb_pgid = 0;
>> +             qcq->cmb_order = 0;
>> +             qcq->cmb_q_base = NULL;
>> +             qcq->cmb_q_base_pa = 0;
>> +     }
>> +
>>        if (qcq->cq_base) {
>>                dma_free_coherent(dev, qcq->cq_size, qcq->cq_base, qcq->cq_base_pa);
>>                qcq->cq_base = NULL;
>> @@ -587,6 +599,7 @@ static int ionic_qcq_alloc(struct ionic_lif *lif, unsigned int type,
>>                ionic_cq_map(&new->cq, cq_base, cq_base_pa);
>>                ionic_cq_bind(&new->cq, &new->q);
>>        } else {
>> +             /* regular DMA q descriptors */
>>                new->q_size = PAGE_SIZE + (num_descs * desc_size);
>>                new->q_base = dma_alloc_coherent(dev, new->q_size, &new->q_base_pa,
>>                                                 GFP_KERNEL);
>> @@ -599,6 +612,33 @@ static int ionic_qcq_alloc(struct ionic_lif *lif, unsigned int type,
>>                q_base_pa = ALIGN(new->q_base_pa, PAGE_SIZE);
>>                ionic_q_map(&new->q, q_base, q_base_pa);
>>
>> +             if (flags & IONIC_QCQ_F_CMB_RINGS) {
>> +                     /* on-chip CMB q descriptors */
>> +                     new->cmb_q_size = num_descs * desc_size;
>> +                     new->cmb_order = order_base_2(new->cmb_q_size / PAGE_SIZE);
>> +
>> +                     err = ionic_get_cmb(lif, &new->cmb_pgid, &new->cmb_q_base_pa,
>> +                                         new->cmb_order);
>> +                     if (err) {
>> +                             netdev_err(lif->netdev,
>> +                                        "Cannot allocate queue order %d from cmb: err %d\n",
>> +                                        new->cmb_order, err);
>> +                             goto err_out_free_q;
>> +                     }
>> +
>> +                     new->cmb_q_base = ioremap_wc(new->cmb_q_base_pa, new->cmb_q_size);
>> +                     if (!new->cmb_q_base) {
>> +                             netdev_err(lif->netdev, "Cannot map queue from cmb\n");
>> +                             ionic_put_cmb(lif, new->cmb_pgid, new->cmb_order);
>> +                             err = -ENOMEM;
>> +                             goto err_out_free_q;
>> +                     }
>> +
>> +                     new->cmb_q_base_pa -= idev->phy_cmb_pages;
>> +                     ionic_q_cmb_map(&new->q, new->cmb_q_base, new->cmb_q_base_pa);
>> +             }
>> +
>> +             /* cq DMA descriptors */
>>                new->cq_size = PAGE_SIZE + (num_descs * cq_desc_size);
>>                new->cq_base = dma_alloc_coherent(dev, new->cq_size, &new->cq_base_pa,
>>                                                  GFP_KERNEL);
>> @@ -637,6 +677,10 @@ static int ionic_qcq_alloc(struct ionic_lif *lif, unsigned int type,
>>   err_out_free_cq:
>>        dma_free_coherent(dev, new->cq_size, new->cq_base, new->cq_base_pa);
>>   err_out_free_q:
>> +     if (new->cmb_q_base) {
>> +             iounmap(new->cmb_q_base);
>> +             ionic_put_cmb(lif, new->cmb_pgid, new->cmb_order);
>> +     }
>>        dma_free_coherent(dev, new->q_size, new->q_base, new->q_base_pa);
>>   err_out_free_cq_info:
>>        vfree(new->cq.info);
>> @@ -718,6 +762,8 @@ static void ionic_qcq_sanitize(struct ionic_qcq *qcq)
>>        qcq->cq.tail_idx = 0;
>>        qcq->cq.done_color = 1;
>>        memset(qcq->q_base, 0, qcq->q_size);
>> +     if (qcq->cmb_q_base)
>> +             memset_io(qcq->cmb_q_base, 0, qcq->cmb_q_size);
>>        memset(qcq->cq_base, 0, qcq->cq_size);
>>        memset(qcq->sg_base, 0, qcq->sg_size);
>>   }
>> @@ -737,6 +783,7 @@ static int ionic_lif_txq_init(struct ionic_lif *lif, struct ionic_qcq *qcq)
>>                        .index = cpu_to_le32(q->index),
>>                        .flags = cpu_to_le16(IONIC_QINIT_F_IRQ |
>>                                             IONIC_QINIT_F_SG),
>> +                     .intr_index = cpu_to_le16(qcq->intr.index),
>>                        .pid = cpu_to_le16(q->pid),
>>                        .ring_size = ilog2(q->num_descs),
>>                        .ring_base = cpu_to_le64(q->base_pa),
>> @@ -745,17 +792,19 @@ static int ionic_lif_txq_init(struct ionic_lif *lif, struct ionic_qcq *qcq)
>>                        .features = cpu_to_le64(q->features),
>>                },
>>        };
>> -     unsigned int intr_index;
>>        int err;
>>
>> -     intr_index = qcq->intr.index;
>> -
>> -     ctx.cmd.q_init.intr_index = cpu_to_le16(intr_index);
>> +     if (qcq->flags & IONIC_QCQ_F_CMB_RINGS) {
>> +             ctx.cmd.q_init.flags |= cpu_to_le16(IONIC_QINIT_F_CMB);
>> +             ctx.cmd.q_init.ring_base = cpu_to_le64(qcq->cmb_q_base_pa);
>> +     }
>>
>>        dev_dbg(dev, "txq_init.pid %d\n", ctx.cmd.q_init.pid);
>>        dev_dbg(dev, "txq_init.index %d\n", ctx.cmd.q_init.index);
>>        dev_dbg(dev, "txq_init.ring_base 0x%llx\n", ctx.cmd.q_init.ring_base);
>>        dev_dbg(dev, "txq_init.ring_size %d\n", ctx.cmd.q_init.ring_size);
>> +     dev_dbg(dev, "txq_init.cq_ring_base 0x%llx\n", ctx.cmd.q_init.cq_ring_base);
>> +     dev_dbg(dev, "txq_init.sg_ring_base 0x%llx\n", ctx.cmd.q_init.sg_ring_base);
>>        dev_dbg(dev, "txq_init.flags 0x%x\n", ctx.cmd.q_init.flags);
>>        dev_dbg(dev, "txq_init.ver %d\n", ctx.cmd.q_init.ver);
>>        dev_dbg(dev, "txq_init.intr_index %d\n", ctx.cmd.q_init.intr_index);
>> @@ -807,6 +856,11 @@ static int ionic_lif_rxq_init(struct ionic_lif *lif, struct ionic_qcq *qcq)
>>        };
>>        int err;
>>
>> +     if (qcq->flags & IONIC_QCQ_F_CMB_RINGS) {
>> +             ctx.cmd.q_init.flags |= cpu_to_le16(IONIC_QINIT_F_CMB);
>> +             ctx.cmd.q_init.ring_base = cpu_to_le64(qcq->cmb_q_base_pa);
>> +     }
>> +
>>        dev_dbg(dev, "rxq_init.pid %d\n", ctx.cmd.q_init.pid);
>>        dev_dbg(dev, "rxq_init.index %d\n", ctx.cmd.q_init.index);
>>        dev_dbg(dev, "rxq_init.ring_base 0x%llx\n", ctx.cmd.q_init.ring_base);
>> @@ -1966,8 +2020,13 @@ static int ionic_txrx_alloc(struct ionic_lif *lif)
>>                sg_desc_sz = sizeof(struct ionic_txq_sg_desc);
>>
>>        flags = IONIC_QCQ_F_TX_STATS | IONIC_QCQ_F_SG;
>> +
>> +     if (test_bit(IONIC_LIF_F_CMB_RINGS, lif->state))
>> +             flags |= IONIC_QCQ_F_CMB_RINGS;
>> +
>>        if (test_bit(IONIC_LIF_F_SPLIT_INTR, lif->state))
>>                flags |= IONIC_QCQ_F_INTR;
>> +
>>        for (i = 0; i < lif->nxqs; i++) {
>>                err = ionic_qcq_alloc(lif, IONIC_QTYPE_TXQ, i, "tx", flags,
>>                                      num_desc, desc_sz, comp_sz, sg_desc_sz,
>> @@ -1988,6 +2047,9 @@ static int ionic_txrx_alloc(struct ionic_lif *lif)
>>
>>        flags = IONIC_QCQ_F_RX_STATS | IONIC_QCQ_F_SG | IONIC_QCQ_F_INTR;
>>
>> +     if (test_bit(IONIC_LIF_F_CMB_RINGS, lif->state))
>> +             flags |= IONIC_QCQ_F_CMB_RINGS;
>> +
>>        num_desc = lif->nrxq_descs;
>>        desc_sz = sizeof(struct ionic_rxq_desc);
>>        comp_sz = sizeof(struct ionic_rxq_comp);
>> @@ -2663,6 +2725,55 @@ static const struct net_device_ops ionic_netdev_ops = {
>>        .ndo_get_vf_stats       = ionic_get_vf_stats,
>>   };
>>
>> +static int ionic_cmb_reconfig(struct ionic_lif *lif,
>> +                           struct ionic_queue_params *qparam)
>> +{
>> +     struct ionic_queue_params start_qparams;
>> +     int err = 0;
>> +
>> +     /* When changing CMB queue parameters, we're using limited
>> +      * on-device memory and don't have extra memory to use for
>> +      * duplicate allocations, so we free it all first then
>> +      * re-allocate with the new parameters.
>> +      */
>> +
>> +     /* Checkpoint for possible unwind */
>> +     ionic_init_queue_params(lif, &start_qparams);
>> +
>> +     /* Stop and free the queues */
>> +     ionic_stop_queues_reconfig(lif);
>> +     ionic_txrx_free(lif);
>> +
>> +     /* Set up new qparams */
>> +     ionic_set_queue_params(lif, qparam);
>> +
>> +     if (netif_running(lif->netdev)) {
>> +             /* Alloc and start the new configuration */
>> +             err = ionic_txrx_alloc(lif);
>> +             if (err) {
>> +                     dev_warn(lif->ionic->dev,
>> +                              "CMB reconfig failed, restoring values: %d\n", err);
>> +
>> +                     /* Back out the changes */
>> +                     ionic_set_queue_params(lif, &start_qparams);
>> +                     err = ionic_txrx_alloc(lif);
>> +                     if (err) {
>> +                             dev_err(lif->ionic->dev,
>> +                                     "CMB restore failed: %d\n", err);
>> +                             goto errout;
>> +                     }
>> +             }
>> +
>> +             ionic_start_queues_reconfig(lif);
>> +     } else {
>> +             /* This was detached in ionic_stop_queues_reconfig() */
>> +             netif_device_attach(lif->netdev);
>> +     }
>> +
>> +errout:
>> +     return err;
>> +}
>> +
>>   static void ionic_swap_queues(struct ionic_qcq *a, struct ionic_qcq *b)
>>   {
>>        /* only swapping the queues, not the napi, flags, or other stuff */
>> @@ -2705,6 +2816,10 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
>>        unsigned int flags, i;
>>        int err = 0;
>>
>> +     /* Are we changing q params while CMB is on */
>> +     if (test_bit(IONIC_LIF_F_CMB_RINGS, lif->state) && qparam->cmb_enabled)
>> +             return ionic_cmb_reconfig(lif, qparam);
>> +
>>        /* allocate temporary qcq arrays to hold new queue structs */
>>        if (qparam->nxqs != lif->nxqs || qparam->ntxq_descs != lif->ntxq_descs) {
>>                tx_qcqs = devm_kcalloc(lif->ionic->dev, lif->ionic->ntxqs_per_lif,
>> @@ -2741,6 +2856,16 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
>>                        sg_desc_sz = sizeof(struct ionic_txq_sg_desc);
>>
>>                for (i = 0; i < qparam->nxqs; i++) {
>> +                     /* If missing, short placeholder qcq needed for swap */
>> +                     if (!lif->txqcqs[i]) {
>> +                             flags = IONIC_QCQ_F_TX_STATS | IONIC_QCQ_F_SG;
>> +                             err = ionic_qcq_alloc(lif, IONIC_QTYPE_TXQ, i, "tx", flags,
>> +                                                   4, desc_sz, comp_sz, sg_desc_sz,
>> +                                                   lif->kern_pid, &lif->txqcqs[i]);
>> +                             if (err)
>> +                                     goto err_out;
>> +                     }
>> +
>>                        flags = lif->txqcqs[i]->flags & ~IONIC_QCQ_F_INTR;
>>                        err = ionic_qcq_alloc(lif, IONIC_QTYPE_TXQ, i, "tx", flags,
>>                                              num_desc, desc_sz, comp_sz, sg_desc_sz,
>> @@ -2760,6 +2885,16 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
>>                        comp_sz *= 2;
>>
>>                for (i = 0; i < qparam->nxqs; i++) {
>> +                     /* If missing, short placeholder qcq needed for swap */
>> +                     if (!lif->rxqcqs[i]) {
>> +                             flags = IONIC_QCQ_F_RX_STATS | IONIC_QCQ_F_SG;
>> +                             err = ionic_qcq_alloc(lif, IONIC_QTYPE_RXQ, i, "rx", flags,
>> +                                                   4, desc_sz, comp_sz, sg_desc_sz,
>> +                                                   lif->kern_pid, &lif->rxqcqs[i]);
>> +                             if (err)
>> +                                     goto err_out;
>> +                     }
>> +
>>                        flags = lif->rxqcqs[i]->flags & ~IONIC_QCQ_F_INTR;
>>                        err = ionic_qcq_alloc(lif, IONIC_QTYPE_RXQ, i, "rx", flags,
>>                                              num_desc, desc_sz, comp_sz, sg_desc_sz,
>> @@ -2809,10 +2944,15 @@ int ionic_reconfigure_queues(struct ionic_lif *lif,
>>                        lif->tx_coalesce_hw = lif->rx_coalesce_hw;
>>                }
>>
>> -             /* clear existing interrupt assignments */
>> +             /* Clear existing interrupt assignments.  We check for NULL here
>> +              * because we're checking the whole array for potential qcqs, not
>> +              * just those qcqs that have just been set up.
>> +              */
>>                for (i = 0; i < lif->ionic->ntxqs_per_lif; i++) {
>> -                     ionic_qcq_intr_free(lif, lif->txqcqs[i]);
>> -                     ionic_qcq_intr_free(lif, lif->rxqcqs[i]);
>> +                     if (lif->txqcqs[i])
>> +                             ionic_qcq_intr_free(lif, lif->txqcqs[i]);
>> +                     if (lif->rxqcqs[i])
>> +                             ionic_qcq_intr_free(lif, lif->rxqcqs[i]);
>>                }
>>
>>                /* re-assign the interrupts */
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.h b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> index a53984bf3544..5425a8983ae0 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
>> @@ -59,6 +59,7 @@ struct ionic_rx_stats {
>>   #define IONIC_QCQ_F_TX_STATS         BIT(3)
>>   #define IONIC_QCQ_F_RX_STATS         BIT(4)
>>   #define IONIC_QCQ_F_NOTIFYQ          BIT(5)
>> +#define IONIC_QCQ_F_CMB_RINGS                BIT(6)
>>
>>   struct ionic_qcq {
>>        void *q_base;
>> @@ -70,6 +71,11 @@ struct ionic_qcq {
>>        void *sg_base;
>>        dma_addr_t sg_base_pa;
>>        u32 sg_size;
>> +     void __iomem *cmb_q_base;
>> +     phys_addr_t cmb_q_base_pa;
>> +     u32 cmb_q_size;
>> +     u32 cmb_pgid;
>> +     u32 cmb_order;
>>        struct dim dim;
>>        struct ionic_queue q;
>>        struct ionic_cq cq;
>> @@ -140,6 +146,7 @@ enum ionic_lif_state_flags {
>>        IONIC_LIF_F_BROKEN,
>>        IONIC_LIF_F_TX_DIM_INTR,
>>        IONIC_LIF_F_RX_DIM_INTR,
>> +     IONIC_LIF_F_CMB_RINGS,
>>
>>        /* leave this as last */
>>        IONIC_LIF_F_STATE_SIZE
>> @@ -243,8 +250,9 @@ struct ionic_queue_params {
>>        unsigned int nxqs;
>>        unsigned int ntxq_descs;
>>        unsigned int nrxq_descs;
>> -     unsigned int intr_split;
>>        u64 rxq_features;
>> +     bool intr_split;
>> +     bool cmb_enabled;
> 
> Use of bool in structs is frowned upon since bool doesn't have a
> strictly defined size. It is usually preferred to use u8.

I understand this to be the case for structs that care about cache size 
and placement (as mentioned in coding-style.rst), but this is not the 
case here, and I think these tend towards better readability.

> 
>>   };
>>
>>   static inline void ionic_init_queue_params(struct ionic_lif *lif,
>> @@ -253,8 +261,28 @@ static inline void ionic_init_queue_params(struct ionic_lif *lif,
>>        qparam->nxqs = lif->nxqs;
>>        qparam->ntxq_descs = lif->ntxq_descs;
>>        qparam->nrxq_descs = lif->nrxq_descs;
>> -     qparam->intr_split = test_bit(IONIC_LIF_F_SPLIT_INTR, lif->state);
>>        qparam->rxq_features = lif->rxq_features;
>> +     qparam->intr_split = test_bit(IONIC_LIF_F_SPLIT_INTR, lif->state);
>> +     qparam->cmb_enabled = test_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
>> +}
>> +
>> +static inline void ionic_set_queue_params(struct ionic_lif *lif,
>> +                                       struct ionic_queue_params *qparam)
>> +{
>> +     lif->nxqs = qparam->nxqs;
>> +     lif->ntxq_descs = qparam->ntxq_descs;
>> +     lif->nrxq_descs = qparam->nrxq_descs;
>> +     lif->rxq_features = qparam->rxq_features;
>> +
>> +     if (qparam->intr_split)
>> +             set_bit(IONIC_LIF_F_SPLIT_INTR, lif->state);
>> +     else
>> +             clear_bit(IONIC_LIF_F_SPLIT_INTR, lif->state);
>> +
>> +     if (qparam->cmb_enabled)
>> +             set_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
>> +     else
>> +             clear_bit(IONIC_LIF_F_CMB_RINGS, lif->state);
>>   }
>>
>>   static inline u32 ionic_coal_usec_to_hw(struct ionic *ionic, u32 usecs)
>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> index 0c3977416cd1..0cb464931d3d 100644
>> --- a/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_txrx.c
>> @@ -419,6 +419,10 @@ void ionic_rx_fill(struct ionic_queue *q)
>>                                              IONIC_RXQ_DESC_OPCODE_SIMPLE;
>>                desc_info->nbufs = nfrags;
>>
>> +             /* commit CMB descriptor contents in one shot */
>> +             if (q_to_qcq(q)->flags & IONIC_QCQ_F_CMB_RINGS)
>> +                     memcpy_toio(desc_info->cmb_desc, desc, q->desc_size);
>> +
>>                ionic_rxq_post(q, false, ionic_rx_clean, NULL);
>>        }
>>
> 
> So this bit is repeated below and the comment is somewhat confusing as
> I am assuming you are using some combination of write combining and the
> memcpy to generate the "one shot" update you are referring to here.
> 
> My suggestion would be to look at creating an inline function somewhere
> above here that takes the q, desc_info and desc as arguments and can do
> all this as a single function call. Then in that function you can add a
> comment explaining how this is a "one shot" write.

Sure, I can replace the 4 appearances of this with a function.
(An early draft of this code wrote the descriptor fields into CMB 
individually, rather than a single copy action.  The comment might now 
be more distracting than necessary.)

Thank you Alex for your review comments and time, much appreciated.
sln

> 
>> @@ -860,7 +864,8 @@ static int ionic_tx_tcp_pseudo_csum(struct sk_buff *skb)
>>        return 0;
>>   }
>>
>> -static void ionic_tx_tso_post(struct ionic_queue *q, struct ionic_txq_desc *desc,
>> +static void ionic_tx_tso_post(struct ionic_queue *q,
>> +                           struct ionic_desc_info *desc_info,
>>                              struct sk_buff *skb,
>>                              dma_addr_t addr, u8 nsge, u16 len,
>>                              unsigned int hdrlen, unsigned int mss,
>> @@ -868,6 +873,7 @@ static void ionic_tx_tso_post(struct ionic_queue *q, struct ionic_txq_desc *desc
>>                              u16 vlan_tci, bool has_vlan,
>>                              bool start, bool done)
>>   {
>> +     struct ionic_txq_desc *desc = desc_info->desc;
>>        u8 flags = 0;
>>        u64 cmd;
>>
>> @@ -883,6 +889,10 @@ static void ionic_tx_tso_post(struct ionic_queue *q, struct ionic_txq_desc *desc
>>        desc->hdr_len = cpu_to_le16(hdrlen);
>>        desc->mss = cpu_to_le16(mss);
>>
>> +     /* commit CMB descriptor contents in one shot */
>> +     if (q_to_qcq(q)->flags & IONIC_QCQ_F_CMB_RINGS)
>> +             memcpy_toio(desc_info->cmb_desc, desc, q->desc_size);
>> +
>>        if (start) {
>>                skb_tx_timestamp(skb);
>>                if (!unlikely(q->features & IONIC_TXQ_F_HWSTAMP))
>> @@ -1001,7 +1011,7 @@ static int ionic_tx_tso(struct ionic_queue *q, struct sk_buff *skb)
>>                seg_rem = min(tso_rem, mss);
>>                done = (tso_rem == 0);
>>                /* post descriptor */
>> -             ionic_tx_tso_post(q, desc, skb,
>> +             ionic_tx_tso_post(q, desc_info, skb,
>>                                  desc_addr, desc_nsge, desc_len,
>>                                  hdrlen, mss, outer_csum, vlan_tci, has_vlan,
>>                                  start, done);
>> @@ -1050,6 +1060,10 @@ static void ionic_tx_calc_csum(struct ionic_queue *q, struct sk_buff *skb,
>>        desc->csum_start = cpu_to_le16(skb_checksum_start_offset(skb));
>>        desc->csum_offset = cpu_to_le16(skb->csum_offset);
>>
>> +     /* commit descriptor contents in one shot */
>> +     if (q_to_qcq(q)->flags & IONIC_QCQ_F_CMB_RINGS)
>> +             memcpy_toio(desc_info->cmb_desc, desc, q->desc_size);
>> +
>>        if (skb_csum_is_sctp(skb))
>>                stats->crc32_csum++;
>>        else
>> @@ -1087,6 +1101,10 @@ static void ionic_tx_calc_no_csum(struct ionic_queue *q, struct sk_buff *skb,
>>        desc->csum_start = 0;
>>        desc->csum_offset = 0;
>>
>> +     /* commit descriptor contents in one shot */
>> +     if (q_to_qcq(q)->flags & IONIC_QCQ_F_CMB_RINGS)
>> +             memcpy_toio(desc_info->cmb_desc, desc, q->desc_size);
>> +
>>        stats->csum_none++;
>>   }
>>
> 

Re: [PATCH v2 net-next 3/3] ionic: add support for device Component Memory Buffers

[Alexander Duyck]

On Tue, Feb 7, 2023 at 10:24 AM Shannon Nelson <shannon.nelson@amd.com> wrote:
>
> On 2/6/23 1:36 PM, Alexander H Duyck wrote:
> > On Mon, 2023-02-06 at 10:16 -0800, Shannon Nelson wrote:
> >> The ionic device has on-board memory (CMB) that can be used
> >> for descriptors as a way to speed descriptor access for faster
> >> traffic processing.  It imposes a couple of restrictions so
> >> is not on by default, but can be enabled through the ethtool
> >> priv-flags.
> >
> > For the purposes of patch review it might be convinent to call out what
> > those restrictions are as you enable the code below. I'm assuming it is
> > mostly just the amount of space you can use, but if there is something
> > else it would be useful to have that noted.

The big thing for me is to make sure you call out your limitations. I
just want to make sure as the reviewer we know what to watch out for.
My main concern is wanting to see it documented somewhere as I am
assuming that it is mostly related to your MMIO size limitations.
However I have concerns that there may be other items such as the use
of write combining that would be nice to see called out somewhere as I
assume that is only needed for performance and not some writeback
limitation of the hardware.

> >
> >> @@ -390,6 +392,7 @@ static void ionic_remove(struct pci_dev *pdev)
> >>
> >>        ionic_port_reset(ionic);
> >>        ionic_reset(ionic);
> >> +     ionic_dev_teardown(ionic);
> >>        pci_clear_master(pdev);
> >>        ionic_unmap_bars(ionic);
> >>        pci_release_regions(pdev);
> >> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
> >> index 626b9113e7c4..9b4bba2279ab 100644
> >> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
> >> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
> >> @@ -92,6 +92,7 @@ int ionic_dev_setup(struct ionic *ionic)
> >>        unsigned int num_bars = ionic->num_bars;
> >>        struct ionic_dev *idev = &ionic->idev;
> >>        struct device *dev = ionic->dev;
> >> +     int size;
> >>        u32 sig;
> >>
> >>        /* BAR0: dev_cmd and interrupts */
> >> @@ -133,9 +134,40 @@ int ionic_dev_setup(struct ionic *ionic)
> >>        idev->db_pages = bar->vaddr;
> >>        idev->phy_db_pages = bar->bus_addr;
> >>
> >> +     /* BAR2: optional controller memory mapping */
> >> +     bar++;
> >> +     mutex_init(&idev->cmb_inuse_lock);
> >> +     if (num_bars < 3 || !ionic->bars[IONIC_PCI_BAR_CMB].len) {
> >> +             idev->cmb_inuse = NULL;
> >> +             idev->phy_cmb_pages = 0;
> >> +             idev->cmb_npages = 0;
> >> +             return 0;
> >> +     }
> >> +
> >> +     idev->phy_cmb_pages = bar->bus_addr;
> >> +     idev->cmb_npages = bar->len / PAGE_SIZE;
> >> +     size = BITS_TO_LONGS(idev->cmb_npages) * sizeof(long);
> >> +     idev->cmb_inuse = kzalloc(size, GFP_KERNEL);
> >> +     if (!idev->cmb_inuse) {
> >> +             idev->phy_cmb_pages = 0;
> >> +             idev->cmb_npages = 0;
> >> +     }
> >> +
> >
> > Why not hold of on setting phy_cmb_pages and cmb_npages until after you
> > have allocated the pages rather then resetting them in the event of
> > failure?
>
> I need the values anyway to determine size, and the fail is unlikely, so
> why bother with tmp variables in the middle?  Also, this clearly sets
> idev->cmb_npages to 0 which is used as an indicator in the ethtool
> handler that thm CMB pages feature is not available.

Everything seems to be based on cmb_inuse being set anyway. You could
probably just leave the values set and not bother with zeroing them
since cmb_inuse would be NULL in case of the failure.

> >
> > Also is it really acceptable for this to fail silently?
>
> The fail would be from the memory allocation, which usually will already
> have printed some message, and we are usually discouraged from adding
> more "alloc failed" type messages.  Also, this is an optional feature,
> not needed for normal driver operations, so we don't need to kill the
> probe with an error code.  If the user tries to enable CMB, they will
> get a message that it is not available.

The error I would be looking for would be specific to the feature
failing to be enabled. The memory allocation would be harder to track
down since you would have to pick it out of a backtrace. If we had a
one line message about it failing to initialize that might be useful
in debugging should an attempt to enable it fail.

Re: [PATCH v2 net-next 3/3] ionic: add support for device Component Memory Buffers

[Shannon Nelson]

On 2/7/23 10:45 AM, Alexander Duyck wrote:
> On Tue, Feb 7, 2023 at 10:24 AM Shannon Nelson <shannon.nelson@amd.com> wrote:
>>
>> On 2/6/23 1:36 PM, Alexander H Duyck wrote:
>>> On Mon, 2023-02-06 at 10:16 -0800, Shannon Nelson wrote:
>>>> The ionic device has on-board memory (CMB) that can be used
>>>> for descriptors as a way to speed descriptor access for faster
>>>> traffic processing.  It imposes a couple of restrictions so
>>>> is not on by default, but can be enabled through the ethtool
>>>> priv-flags.
>>>
>>> For the purposes of patch review it might be convinent to call out what
>>> those restrictions are as you enable the code below. I'm assuming it is
>>> mostly just the amount of space you can use, but if there is something
>>> else it would be useful to have that noted.
> 
> The big thing for me is to make sure you call out your limitations. I
> just want to make sure as the reviewer we know what to watch out for.
> My main concern is wanting to see it documented somewhere as I am
> assuming that it is mostly related to your MMIO size limitations.
> However I have concerns that there may be other items such as the use
> of write combining that would be nice to see called out somewhere as I
> assume that is only needed for performance and not some writeback
> limitation of the hardware.

Oh, ouch, in concentrating on the other comments, I missed responding to 
this one.

That "restrictions" note I think is over-stated here and left over from 
an earlier draft where queue/ring/mtu size changes were disallowed while 
CMB was running.  The main restriction now is just that the interface 
must be 'down' before enabling/disabing CMB.

I'll fix up the commit comment to be a little less alarming.

> 
>>>
>>>> @@ -390,6 +392,7 @@ static void ionic_remove(struct pci_dev *pdev)
>>>>
>>>>         ionic_port_reset(ionic);
>>>>         ionic_reset(ionic);
>>>> +     ionic_dev_teardown(ionic);
>>>>         pci_clear_master(pdev);
>>>>         ionic_unmap_bars(ionic);
>>>>         pci_release_regions(pdev);
>>>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>>>> index 626b9113e7c4..9b4bba2279ab 100644
>>>> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>>>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>>>> @@ -92,6 +92,7 @@ int ionic_dev_setup(struct ionic *ionic)
>>>>         unsigned int num_bars = ionic->num_bars;
>>>>         struct ionic_dev *idev = &ionic->idev;
>>>>         struct device *dev = ionic->dev;
>>>> +     int size;
>>>>         u32 sig;
>>>>
>>>>         /* BAR0: dev_cmd and interrupts */
>>>> @@ -133,9 +134,40 @@ int ionic_dev_setup(struct ionic *ionic)
>>>>         idev->db_pages = bar->vaddr;
>>>>         idev->phy_db_pages = bar->bus_addr;
>>>>
>>>> +     /* BAR2: optional controller memory mapping */
>>>> +     bar++;
>>>> +     mutex_init(&idev->cmb_inuse_lock);
>>>> +     if (num_bars < 3 || !ionic->bars[IONIC_PCI_BAR_CMB].len) {
>>>> +             idev->cmb_inuse = NULL;
>>>> +             idev->phy_cmb_pages = 0;
>>>> +             idev->cmb_npages = 0;
>>>> +             return 0;
>>>> +     }
>>>> +
>>>> +     idev->phy_cmb_pages = bar->bus_addr;
>>>> +     idev->cmb_npages = bar->len / PAGE_SIZE;
>>>> +     size = BITS_TO_LONGS(idev->cmb_npages) * sizeof(long);
>>>> +     idev->cmb_inuse = kzalloc(size, GFP_KERNEL);
>>>> +     if (!idev->cmb_inuse) {
>>>> +             idev->phy_cmb_pages = 0;
>>>> +             idev->cmb_npages = 0;
>>>> +     }
>>>> +
>>>
>>> Why not hold of on setting phy_cmb_pages and cmb_npages until after you
>>> have allocated the pages rather then resetting them in the event of
>>> failure?
>>
>> I need the values anyway to determine size, and the fail is unlikely, so
>> why bother with tmp variables in the middle?  Also, this clearly sets
>> idev->cmb_npages to 0 which is used as an indicator in the ethtool
>> handler that thm CMB pages feature is not available.
> 
> Everything seems to be based on cmb_inuse being set anyway. You could
> probably just leave the values set and not bother with zeroing them
> since cmb_inuse would be NULL in case of the failure.

I should be able to change ionic_cmb_rings_toggle() to look at cmb_inuse 
rather than cmb_npages.


> 
>>>
>>> Also is it really acceptable for this to fail silently?
>>
>> The fail would be from the memory allocation, which usually will already
>> have printed some message, and we are usually discouraged from adding
>> more "alloc failed" type messages.  Also, this is an optional feature,
>> not needed for normal driver operations, so we don't need to kill the
>> probe with an error code.  If the user tries to enable CMB, they will
>> get a message that it is not available.
> 
> The error I would be looking for would be specific to the feature
> failing to be enabled. The memory allocation would be harder to track
> down since you would have to pick it out of a backtrace. If we had a
> one line message about it failing to initialize that might be useful
> in debugging should an attempt to enable it fail.

Sure, I can add that.

Thanks,
sln