[PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Dave Jiang]

Remove devm_* allocation of memory of 'struct device' objects.
The devm_* lifetime is incompatible with device->release() lifetime.
Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
functions for each component in order to free the allocated memory at
the appropriate time. Each component such as wq, engine, and group now
needs to be allocated individually in order to setup the lifetime properly.

Reported-by: Jason Gunthorpe <jgg@nvidia.com>
Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/dma/idxd/device.c |   24 +++---
 drivers/dma/idxd/idxd.h   |    6 +-
 drivers/dma/idxd/init.c   |  173 +++++++++++++++++++++++++++++++++++++--------
 drivers/dma/idxd/irq.c    |    6 +-
 drivers/dma/idxd/sysfs.c  |   69 ++++++++++++------
 5 files changed, 206 insertions(+), 72 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 205156afeb54..39cb311ad333 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -541,7 +541,7 @@ void idxd_device_wqs_clear_state(struct idxd_device *idxd)
 	lockdep_assert_held(&idxd->dev_lock);
 
 	for (i = 0; i < idxd->max_wqs; i++) {
-		struct idxd_wq *wq = &idxd->wqs[i];
+		struct idxd_wq *wq = idxd->wqs[i];
 
 		if (wq->state == IDXD_WQ_ENABLED) {
 			idxd_wq_disable_cleanup(wq);
@@ -721,7 +721,7 @@ static int idxd_groups_config_write(struct idxd_device *idxd)
 		ioread32(idxd->reg_base + IDXD_GENCFG_OFFSET));
 
 	for (i = 0; i < idxd->max_groups; i++) {
-		struct idxd_group *group = &idxd->groups[i];
+		struct idxd_group *group = idxd->groups[i];
 
 		idxd_group_config_write(group);
 	}
@@ -793,7 +793,7 @@ static int idxd_wqs_config_write(struct idxd_device *idxd)
 	int i, rc;
 
 	for (i = 0; i < idxd->max_wqs; i++) {
-		struct idxd_wq *wq = &idxd->wqs[i];
+		struct idxd_wq *wq = idxd->wqs[i];
 
 		rc = idxd_wq_config_write(wq);
 		if (rc < 0)
@@ -809,7 +809,7 @@ static void idxd_group_flags_setup(struct idxd_device *idxd)
 
 	/* TC-A 0 and TC-B 1 should be defaults */
 	for (i = 0; i < idxd->max_groups; i++) {
-		struct idxd_group *group = &idxd->groups[i];
+		struct idxd_group *group = idxd->groups[i];
 
 		if (group->tc_a == -1)
 			group->tc_a = group->grpcfg.flags.tc_a = 0;
@@ -836,12 +836,12 @@ static int idxd_engines_setup(struct idxd_device *idxd)
 	struct idxd_group *group;
 
 	for (i = 0; i < idxd->max_groups; i++) {
-		group = &idxd->groups[i];
+		group = idxd->groups[i];
 		group->grpcfg.engines = 0;
 	}
 
 	for (i = 0; i < idxd->max_engines; i++) {
-		eng = &idxd->engines[i];
+		eng = idxd->engines[i];
 		group = eng->group;
 
 		if (!group)
@@ -865,13 +865,13 @@ static int idxd_wqs_setup(struct idxd_device *idxd)
 	struct device *dev = &idxd->pdev->dev;
 
 	for (i = 0; i < idxd->max_groups; i++) {
-		group = &idxd->groups[i];
+		group = idxd->groups[i];
 		for (j = 0; j < 4; j++)
 			group->grpcfg.wqs[j] = 0;
 	}
 
 	for (i = 0; i < idxd->max_wqs; i++) {
-		wq = &idxd->wqs[i];
+		wq = idxd->wqs[i];
 		group = wq->group;
 
 		if (!wq->group)
@@ -982,7 +982,7 @@ static void idxd_group_load_config(struct idxd_group *group)
 
 			/* Set group assignment for wq if wq bit is set */
 			if (group->grpcfg.wqs[i] & BIT(j)) {
-				wq = &idxd->wqs[id];
+				wq = idxd->wqs[id];
 				wq->group = group;
 			}
 		}
@@ -999,7 +999,7 @@ static void idxd_group_load_config(struct idxd_group *group)
 			break;
 
 		if (group->grpcfg.engines & BIT(i)) {
-			struct idxd_engine *engine = &idxd->engines[i];
+			struct idxd_engine *engine = idxd->engines[i];
 
 			engine->group = group;
 		}
@@ -1020,13 +1020,13 @@ int idxd_device_load_config(struct idxd_device *idxd)
 	idxd->token_limit = reg.token_limit;
 
 	for (i = 0; i < idxd->max_groups; i++) {
-		struct idxd_group *group = &idxd->groups[i];
+		struct idxd_group *group = idxd->groups[i];
 
 		idxd_group_load_config(group);
 	}
 
 	for (i = 0; i < idxd->max_wqs; i++) {
-		struct idxd_wq *wq = &idxd->wqs[i];
+		struct idxd_wq *wq = idxd->wqs[i];
 
 		rc = idxd_wq_load_config(wq);
 		if (rc < 0)
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index a9386a66ab72..c2e89b3e3828 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -181,9 +181,9 @@ struct idxd_device {
 
 	spinlock_t dev_lock;	/* spinlock for device */
 	struct completion *cmd_done;
-	struct idxd_group *groups;
-	struct idxd_wq *wqs;
-	struct idxd_engine *engines;
+	struct idxd_group **groups;
+	struct idxd_wq **wqs;
+	struct idxd_engine **engines;
 
 	struct iommu_sva *sva;
 	unsigned int pasid;
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 0bd7b33b436a..bac7bb6be9b9 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -178,10 +178,97 @@ static int idxd_setup_interrupts(struct idxd_device *idxd)
 	return rc;
 }
 
+static int idxd_allocate_wqs(struct idxd_device *idxd)
+{
+	struct device *dev = &idxd->pdev->dev;
+	struct idxd_wq *wq;
+	int i, rc;
+
+	idxd->wqs = devm_kcalloc(dev, idxd->max_wqs, sizeof(struct idxd_wq *),
+				 GFP_KERNEL);
+	if (!idxd->wqs)
+		return -ENOMEM;
+
+	for (i = 0; i < idxd->max_wqs; i++) {
+		wq = kzalloc_node(sizeof(*wq), GFP_KERNEL, dev_to_node(dev));
+		if (!wq) {
+			rc = -ENOMEM;
+			goto err;
+		}
+
+		idxd->wqs[i] = wq;
+	}
+
+	return 0;
+
+ err:
+	while (--i)
+		kfree(idxd->wqs[i]);
+	return rc;
+}
+
+static int idxd_allocate_engines(struct idxd_device *idxd)
+{
+	struct idxd_engine *engine;
+	struct device *dev = &idxd->pdev->dev;
+	int i, rc;
+
+	idxd->engines = devm_kcalloc(dev, idxd->max_engines, sizeof(struct idxd_engine *),
+				     GFP_KERNEL);
+	if (!idxd->engines)
+		return -ENOMEM;
+
+	for (i = 0; i < idxd->max_engines; i++) {
+		engine = kzalloc_node(sizeof(*engine), GFP_KERNEL, dev_to_node(dev));
+		if (!engine) {
+			rc = -ENOMEM;
+			goto err;
+		}
+
+		idxd->engines[i] = engine;
+	}
+
+	return 0;
+
+ err:
+	while (--i)
+		kfree(idxd->engines[i]);
+	return rc;
+}
+
+static int idxd_allocate_groups(struct idxd_device *idxd)
+{
+	struct device *dev = &idxd->pdev->dev;
+	struct idxd_group *group;
+	int i, rc;
+
+	idxd->groups = devm_kcalloc(dev, idxd->max_groups, sizeof(struct idxd_group *),
+				    GFP_KERNEL);
+	if (!idxd->groups)
+		return -ENOMEM;
+
+	for (i = 0; i < idxd->max_groups; i++) {
+		group = kzalloc_node(sizeof(*group), GFP_KERNEL, dev_to_node(dev));
+		if (!group) {
+			rc = -ENOMEM;
+			goto err;
+		}
+
+		idxd->groups[i] = group;
+	}
+
+	return 0;
+
+ err:
+	while (--i)
+		kfree(idxd->groups[i]);
+	return rc;
+}
+
 static int idxd_setup_internals(struct idxd_device *idxd)
 {
 	struct device *dev = &idxd->pdev->dev;
-	int i;
+	int i, rc;
 
 	init_waitqueue_head(&idxd->cmd_waitq);
 
@@ -191,30 +278,25 @@ static int idxd_setup_internals(struct idxd_device *idxd)
 			return -ENOMEM;
 	}
 
-	idxd->groups = devm_kcalloc(dev, idxd->max_groups,
-				    sizeof(struct idxd_group), GFP_KERNEL);
-	if (!idxd->groups)
-		return -ENOMEM;
+	rc = idxd_allocate_groups(idxd);
+	if (rc < 0)
+		return rc;
 
 	for (i = 0; i < idxd->max_groups; i++) {
-		idxd->groups[i].idxd = idxd;
-		idxd->groups[i].id = i;
-		idxd->groups[i].tc_a = -1;
-		idxd->groups[i].tc_b = -1;
-	}
+		struct idxd_group *group = idxd->groups[i];
 
-	idxd->wqs = devm_kcalloc(dev, idxd->max_wqs, sizeof(struct idxd_wq),
-				 GFP_KERNEL);
-	if (!idxd->wqs)
-		return -ENOMEM;
+		group->idxd = idxd;
+		group->id = i;
+		group->tc_a = -1;
+		group->tc_b = -1;
+	}
 
-	idxd->engines = devm_kcalloc(dev, idxd->max_engines,
-				     sizeof(struct idxd_engine), GFP_KERNEL);
-	if (!idxd->engines)
-		return -ENOMEM;
+	rc = idxd_allocate_wqs(idxd);
+	if (rc < 0)
+		return rc;
 
 	for (i = 0; i < idxd->max_wqs; i++) {
-		struct idxd_wq *wq = &idxd->wqs[i];
+		struct idxd_wq *wq = idxd->wqs[i];
 
 		wq->id = i;
 		wq->idxd = idxd;
@@ -228,9 +310,15 @@ static int idxd_setup_internals(struct idxd_device *idxd)
 		init_completion(&wq->wq_dead);
 	}
 
+	rc = idxd_allocate_engines(idxd);
+	if (rc < 0)
+		return rc;
+
 	for (i = 0; i < idxd->max_engines; i++) {
-		idxd->engines[i].idxd = idxd;
-		idxd->engines[i].id = i;
+		struct idxd_engine *engine = idxd->engines[i];
+
+		engine->idxd = idxd;
+		engine->id = i;
 	}
 
 	idxd->wq = create_workqueue(dev_name(dev));
@@ -318,7 +406,7 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev)
 	struct device *dev = &pdev->dev;
 	struct idxd_device *idxd;
 
-	idxd = devm_kzalloc(dev, sizeof(struct idxd_device), GFP_KERNEL);
+	idxd = kzalloc_node(sizeof(*idxd), GFP_KERNEL, dev_to_node(dev));
 	if (!idxd)
 		return NULL;
 
@@ -436,6 +524,19 @@ static void idxd_type_init(struct idxd_device *idxd)
 		idxd->compl_size = sizeof(struct iax_completion_record);
 }
 
+static void idxd_free(struct idxd_device *idxd)
+{
+	int i;
+
+	for (i = 0; i < idxd->max_wqs; i++)
+		kfree(idxd->wqs[i]);
+	for (i = 0; i < idxd->max_engines; i++)
+		kfree(idxd->engines[i]);
+	for (i = 0; i < idxd->max_groups; i++)
+		kfree(idxd->groups[i]);
+	kfree(idxd);
+}
+
 static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct device *dev = &pdev->dev;
@@ -453,21 +554,23 @@ static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	dev_dbg(dev, "Mapping BARs\n");
 	idxd->reg_base = pcim_iomap(pdev, IDXD_MMIO_BAR, 0);
-	if (!idxd->reg_base)
-		return -ENOMEM;
+	if (!idxd->reg_base) {
+		rc = -ENOMEM;
+		goto err;
+	}
 
 	dev_dbg(dev, "Set DMA masks\n");
 	rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
 	if (rc)
 		rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
 	if (rc)
-		return rc;
+		goto err;
 
 	rc = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
 	if (rc)
 		rc = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32));
 	if (rc)
-		return rc;
+		goto err;
 
 	idxd_set_type(idxd);
 
@@ -481,13 +584,15 @@ static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	rc = idxd_probe(idxd);
 	if (rc) {
 		dev_err(dev, "Intel(R) IDXD DMA Engine init failed\n");
-		return -ENODEV;
+		rc = -ENODEV;
+		goto err;
 	}
 
 	rc = idxd_setup_sysfs(idxd);
 	if (rc) {
 		dev_err(dev, "IDXD sysfs setup failed\n");
-		return -ENODEV;
+		rc = -ENODEV;
+		goto err;
 	}
 
 	idxd->state = IDXD_DEV_CONF_READY;
@@ -496,6 +601,10 @@ static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		 idxd->hw.version);
 
 	return 0;
+
+ err:
+	idxd_free(idxd);
+	return rc;
 }
 
 static void idxd_flush_pending_llist(struct idxd_irq_entry *ie)
@@ -530,7 +639,7 @@ static void idxd_wqs_quiesce(struct idxd_device *idxd)
 	int i;
 
 	for (i = 0; i < idxd->max_wqs; i++) {
-		wq = &idxd->wqs[i];
+		wq = idxd->wqs[i];
 		if (wq->state == IDXD_WQ_ENABLED && wq->type == IDXD_WQT_KERNEL)
 			idxd_wq_quiesce(wq);
 	}
@@ -586,14 +695,16 @@ static void idxd_shutdown(struct pci_dev *pdev)
 static void idxd_remove(struct pci_dev *pdev)
 {
 	struct idxd_device *idxd = pci_get_drvdata(pdev);
+	int id = idxd->id;
+	enum idxd_type type = idxd->type;
 
 	dev_dbg(&pdev->dev, "%s called\n", __func__);
-	idxd_cleanup_sysfs(idxd);
 	idxd_shutdown(pdev);
 	if (device_pasid_enabled(idxd))
 		idxd_disable_system_pasid(idxd);
+	idxd_cleanup_sysfs(idxd);
 	mutex_lock(&idxd_idr_lock);
-	idr_remove(&idxd_idrs[idxd->type], idxd->id);
+	idr_remove(&idxd_idrs[type], id);
 	mutex_unlock(&idxd_idr_lock);
 }
 
diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index f1463fc58112..7b0181532f77 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -45,7 +45,7 @@ static void idxd_device_reinit(struct work_struct *work)
 		goto out;
 
 	for (i = 0; i < idxd->max_wqs; i++) {
-		struct idxd_wq *wq = &idxd->wqs[i];
+		struct idxd_wq *wq = idxd->wqs[i];
 
 		if (wq->state == IDXD_WQ_ENABLED) {
 			rc = idxd_wq_enable(wq);
@@ -130,7 +130,7 @@ static int process_misc_interrupts(struct idxd_device *idxd, u32 cause)
 
 		if (idxd->sw_err.valid && idxd->sw_err.wq_idx_valid) {
 			int id = idxd->sw_err.wq_idx;
-			struct idxd_wq *wq = &idxd->wqs[id];
+			struct idxd_wq *wq = idxd->wqs[id];
 
 			if (wq->type == IDXD_WQT_USER)
 				wake_up_interruptible(&wq->idxd_cdev.err_queue);
@@ -138,7 +138,7 @@ static int process_misc_interrupts(struct idxd_device *idxd, u32 cause)
 			int i;
 
 			for (i = 0; i < idxd->max_wqs; i++) {
-				struct idxd_wq *wq = &idxd->wqs[i];
+				struct idxd_wq *wq = idxd->wqs[i];
 
 				if (wq->type == IDXD_WQT_USER)
 					wake_up_interruptible(&wq->idxd_cdev.err_queue);
diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
index 6bf27529464c..1dc0d3548fba 100644
--- a/drivers/dma/idxd/sysfs.c
+++ b/drivers/dma/idxd/sysfs.c
@@ -16,26 +16,49 @@ static char *idxd_wq_type_names[] = {
 	[IDXD_WQT_USER]		= "user",
 };
 
-static void idxd_conf_device_release(struct device *dev)
+static void idxd_conf_group_release(struct device *dev)
 {
-	dev_dbg(dev, "%s for %s\n", __func__, dev_name(dev));
+	struct idxd_group *group = container_of(dev, struct idxd_group, conf_dev);
+
+	kfree(group);
 }
 
 static struct device_type idxd_group_device_type = {
 	.name = "group",
-	.release = idxd_conf_device_release,
+	.release = idxd_conf_group_release,
 };
 
+static void idxd_conf_wq_release(struct device *dev)
+{
+	struct idxd_wq *wq = container_of(dev, struct idxd_wq, conf_dev);
+
+	kfree(wq);
+}
+
 static struct device_type idxd_wq_device_type = {
 	.name = "wq",
-	.release = idxd_conf_device_release,
+	.release = idxd_conf_wq_release,
 };
 
+static void idxd_conf_engine_release(struct device *dev)
+{
+	struct idxd_engine *engine = container_of(dev, struct idxd_engine, conf_dev);
+
+	kfree(engine);
+}
+
 static struct device_type idxd_engine_device_type = {
 	.name = "engine",
-	.release = idxd_conf_device_release,
+	.release = idxd_conf_engine_release,
 };
 
+static void idxd_conf_device_release(struct device *dev)
+{
+	struct idxd_device *idxd = container_of(dev, struct idxd_device, conf_dev);
+
+	kfree(idxd);
+}
+
 static struct device_type dsa_device_type = {
 	.name = "dsa",
 	.release = idxd_conf_device_release,
@@ -346,7 +369,7 @@ static int idxd_config_bus_remove(struct device *dev)
 		dev_dbg(dev, "%s removing dev %s\n", __func__,
 			dev_name(&idxd->conf_dev));
 		for (i = 0; i < idxd->max_wqs; i++) {
-			struct idxd_wq *wq = &idxd->wqs[i];
+			struct idxd_wq *wq = idxd->wqs[i];
 
 			if (wq->state == IDXD_WQ_DISABLED)
 				continue;
@@ -359,7 +382,7 @@ static int idxd_config_bus_remove(struct device *dev)
 		rc = idxd_device_disable(idxd);
 		if (test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags)) {
 			for (i = 0; i < idxd->max_wqs; i++) {
-				struct idxd_wq *wq = &idxd->wqs[i];
+				struct idxd_wq *wq = idxd->wqs[i];
 
 				mutex_lock(&wq->wq_lock);
 				idxd_wq_disable_cleanup(wq);
@@ -514,7 +537,7 @@ static ssize_t engine_group_id_store(struct device *dev,
 
 	if (prevg)
 		prevg->num_engines--;
-	engine->group = &idxd->groups[id];
+	engine->group = idxd->groups[id];
 	engine->group->num_engines++;
 
 	return count;
@@ -545,7 +568,7 @@ static void idxd_set_free_tokens(struct idxd_device *idxd)
 	int i, tokens;
 
 	for (i = 0, tokens = 0; i < idxd->max_groups; i++) {
-		struct idxd_group *g = &idxd->groups[i];
+		struct idxd_group *g = idxd->groups[i];
 
 		tokens += g->tokens_reserved;
 	}
@@ -699,7 +722,7 @@ static ssize_t group_engines_show(struct device *dev,
 	struct idxd_device *idxd = group->idxd;
 
 	for (i = 0; i < idxd->max_engines; i++) {
-		struct idxd_engine *engine = &idxd->engines[i];
+		struct idxd_engine *engine = idxd->engines[i];
 
 		if (!engine->group)
 			continue;
@@ -726,7 +749,7 @@ static ssize_t group_work_queues_show(struct device *dev,
 	struct idxd_device *idxd = group->idxd;
 
 	for (i = 0; i < idxd->max_wqs; i++) {
-		struct idxd_wq *wq = &idxd->wqs[i];
+		struct idxd_wq *wq = idxd->wqs[i];
 
 		if (!wq->group)
 			continue;
@@ -918,7 +941,7 @@ static ssize_t wq_group_id_store(struct device *dev,
 		return count;
 	}
 
-	group = &idxd->groups[id];
+	group = idxd->groups[id];
 	prevg = wq->group;
 
 	if (prevg)
@@ -981,7 +1004,7 @@ static int total_claimed_wq_size(struct idxd_device *idxd)
 	int wq_size = 0;
 
 	for (i = 0; i < idxd->max_wqs; i++) {
-		struct idxd_wq *wq = &idxd->wqs[i];
+		struct idxd_wq *wq = idxd->wqs[i];
 
 		wq_size += wq->size;
 	}
@@ -1496,7 +1519,7 @@ static ssize_t clients_show(struct device *dev,
 
 	spin_lock_irqsave(&idxd->dev_lock, flags);
 	for (i = 0; i < idxd->max_wqs; i++) {
-		struct idxd_wq *wq = &idxd->wqs[i];
+		struct idxd_wq *wq = idxd->wqs[i];
 
 		count += wq->client_count;
 	}
@@ -1660,7 +1683,7 @@ static int idxd_setup_engine_sysfs(struct idxd_device *idxd)
 	int i, rc;
 
 	for (i = 0; i < idxd->max_engines; i++) {
-		struct idxd_engine *engine = &idxd->engines[i];
+		struct idxd_engine *engine = idxd->engines[i];
 
 		engine->conf_dev.parent = &idxd->conf_dev;
 		dev_set_name(&engine->conf_dev, "engine%d.%d",
@@ -1681,7 +1704,7 @@ static int idxd_setup_engine_sysfs(struct idxd_device *idxd)
 
 cleanup:
 	while (i--) {
-		struct idxd_engine *engine = &idxd->engines[i];
+		struct idxd_engine *engine = idxd->engines[i];
 
 		device_unregister(&engine->conf_dev);
 	}
@@ -1694,7 +1717,7 @@ static int idxd_setup_group_sysfs(struct idxd_device *idxd)
 	int i, rc;
 
 	for (i = 0; i < idxd->max_groups; i++) {
-		struct idxd_group *group = &idxd->groups[i];
+		struct idxd_group *group = idxd->groups[i];
 
 		group->conf_dev.parent = &idxd->conf_dev;
 		dev_set_name(&group->conf_dev, "group%d.%d",
@@ -1715,7 +1738,7 @@ static int idxd_setup_group_sysfs(struct idxd_device *idxd)
 
 cleanup:
 	while (i--) {
-		struct idxd_group *group = &idxd->groups[i];
+		struct idxd_group *group = idxd->groups[i];
 
 		device_unregister(&group->conf_dev);
 	}
@@ -1728,7 +1751,7 @@ static int idxd_setup_wq_sysfs(struct idxd_device *idxd)
 	int i, rc;
 
 	for (i = 0; i < idxd->max_wqs; i++) {
-		struct idxd_wq *wq = &idxd->wqs[i];
+		struct idxd_wq *wq = idxd->wqs[i];
 
 		wq->conf_dev.parent = &idxd->conf_dev;
 		dev_set_name(&wq->conf_dev, "wq%d.%d", idxd->id, wq->id);
@@ -1748,7 +1771,7 @@ static int idxd_setup_wq_sysfs(struct idxd_device *idxd)
 
 cleanup:
 	while (i--) {
-		struct idxd_wq *wq = &idxd->wqs[i];
+		struct idxd_wq *wq = idxd->wqs[i];
 
 		device_unregister(&wq->conf_dev);
 	}
@@ -1818,19 +1841,19 @@ void idxd_cleanup_sysfs(struct idxd_device *idxd)
 	int i;
 
 	for (i = 0; i < idxd->max_wqs; i++) {
-		struct idxd_wq *wq = &idxd->wqs[i];
+		struct idxd_wq *wq = idxd->wqs[i];
 
 		device_unregister(&wq->conf_dev);
 	}
 
 	for (i = 0; i < idxd->max_engines; i++) {
-		struct idxd_engine *engine = &idxd->engines[i];
+		struct idxd_engine *engine = idxd->engines[i];
 
 		device_unregister(&engine->conf_dev);
 	}
 
 	for (i = 0; i < idxd->max_groups; i++) {
-		struct idxd_group *group = &idxd->groups[i];
+		struct idxd_group *group = idxd->groups[i];
 
 		device_unregister(&group->conf_dev);
 	}

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Jason Gunthorpe]

On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
> Remove devm_* allocation of memory of 'struct device' objects.
> The devm_* lifetime is incompatible with device->release() lifetime.
> Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
> functions for each component in order to free the allocated memory at
> the appropriate time. Each component such as wq, engine, and group now
> needs to be allocated individually in order to setup the lifetime properly.

I really don't understand why idxd has so many struct device objects.

Typically I expect a simple driver to have exactly one, usually
provided by its subsystem.

What is the purpose?

And it is still messed up because it has:

struct idxd_device {
        enum idxd_type type;
        struct device conf_dev; <-- This is a kref

        struct dma_device dma_dev; <-- And this is also a kref
}

The first kref does kfree() and the second does
idxd_conf_device_release() which does nothing - this is obviously
wrong too.

> +static int idxd_allocate_wqs(struct idxd_device *idxd)
> +{
> +	struct device *dev = &idxd->pdev->dev;
> +	struct idxd_wq *wq;
> +	int i, rc;
> +
> +	idxd->wqs = devm_kcalloc(dev, idxd->max_wqs, sizeof(struct idxd_wq *),
> +				 GFP_KERNEL);

Memory stored in the idxd_device should be freed by the release
function, not by devm.

And since the sub objects already have a pointer to the idxd_device,
I'd keep all the types the same but have the sub structs carry a kref
on the idxd_device, so their release function is just kref_put

Would be much less code

But even better would be to get rid of the struct device embedded in
the sub objects

Jason

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Dave Jiang]

On 2/23/2021 5:59 AM, Jason Gunthorpe wrote:
> On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
>> Remove devm_* allocation of memory of 'struct device' objects.
>> The devm_* lifetime is incompatible with device->release() lifetime.
>> Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
>> functions for each component in order to free the allocated memory at
>> the appropriate time. Each component such as wq, engine, and group now
>> needs to be allocated individually in order to setup the lifetime properly.
> I really don't understand why idxd has so many struct device objects.
>
> Typically I expect a simple driver to have exactly one, usually
> provided by its subsystem.
>
> What is the purpose?

When we initially designed this, Dan suggested to tie the device and 
workqueue enabling to the Linux device model so that the 
enabling/disabling can be done via bind/unbind of the sub drivers. So 
that's how we ended up with all the 'struct device' under idxd and one 
for each configurable component of the hardware device.


>
> And it is still messed up because it has:
>
> struct idxd_device {
>          enum idxd_type type;
>          struct device conf_dev; <-- This is a kref
>
>          struct dma_device dma_dev; <-- And this is also a kref
> }
>
> The first kref does kfree() and the second does
> idxd_conf_device_release() which does nothing - this is obviously
> wrong too.

Yes I need to fix this. I forgot about the dma_dev.


>> +static int idxd_allocate_wqs(struct idxd_device *idxd)
>> +{
>> +	struct device *dev = &idxd->pdev->dev;
>> +	struct idxd_wq *wq;
>> +	int i, rc;
>> +
>> +	idxd->wqs = devm_kcalloc(dev, idxd->max_wqs, sizeof(struct idxd_wq *),
>> +				 GFP_KERNEL);
> Memory stored in the idxd_device should be freed by the release
> function, not by devm.

These memory are not tied to 'struct device' and should be able to 
managed by devm right?


>
> And since the sub objects already have a pointer to the idxd_device,
> I'd keep all the types the same but have the sub structs carry a kref
> on the idxd_device, so their release function is just kref_put
>
> Would be much less code

I think I see what you are saying. Let me have another go at it.


>
> But even better would be to get rid of the struct device embedded in
> the sub objects
>
> Jason

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Dan Williams]

On Tue, Feb 23, 2021 at 5:00 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
> > Remove devm_* allocation of memory of 'struct device' objects.
> > The devm_* lifetime is incompatible with device->release() lifetime.
> > Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
> > functions for each component in order to free the allocated memory at
> > the appropriate time. Each component such as wq, engine, and group now
> > needs to be allocated individually in order to setup the lifetime properly.
>
> I really don't understand why idxd has so many struct device objects.
>
> Typically I expect a simple driver to have exactly one, usually
> provided by its subsystem.
>
> What is the purpose?
>
> And it is still messed up because it has:
>
> struct idxd_device {
>         enum idxd_type type;
>         struct device conf_dev; <-- This is a kref
>
>         struct dma_device dma_dev; <-- And this is also a kref
> }
>
> The first kref does kfree() and the second does
> idxd_conf_device_release() which does nothing - this is obviously
> wrong too.

Obviously... I apologize I flubbed this and assumed the devm
conversions also moved these to pointers.

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Jason Gunthorpe]

On Tue, Feb 23, 2021 at 08:27:46AM -0700, Dave Jiang wrote:
> 
> On 2/23/2021 5:59 AM, Jason Gunthorpe wrote:
> > On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
> > > Remove devm_* allocation of memory of 'struct device' objects.
> > > The devm_* lifetime is incompatible with device->release() lifetime.
> > > Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
> > > functions for each component in order to free the allocated memory at
> > > the appropriate time. Each component such as wq, engine, and group now
> > > needs to be allocated individually in order to setup the lifetime properly.
> > I really don't understand why idxd has so many struct device objects.
> > 
> > Typically I expect a simple driver to have exactly one, usually
> > provided by its subsystem.
> > 
> > What is the purpose?
> 
> When we initially designed this, Dan suggested to tie the device and
> workqueue enabling to the Linux device model so that the enabling/disabling
> can be done via bind/unbind of the sub drivers. So that's how we ended up
> with all the 'struct device' under idxd and one for each configurable
> component of the hardware device.

IDXD created its own little bus just for that?? :\

It is really weird that something called a workqueue would show up in
the driver model at all

> > > +static int idxd_allocate_wqs(struct idxd_device *idxd)
> > > +{
> > > +	struct device *dev = &idxd->pdev->dev;
> > > +	struct idxd_wq *wq;
> > > +	int i, rc;
> > > +
> > > +	idxd->wqs = devm_kcalloc(dev, idxd->max_wqs, sizeof(struct idxd_wq *),
> > > +				 GFP_KERNEL);
> > Memory stored in the idxd_device should be freed by the release
> > function, not by devm.
> 
> These memory are not tied to 'struct device' and should be able to managed
> by devm right?

If it is a pointer inside the kref'd struct then it should be free'd
in the kref's release function.

Those should not use devm as they already have clearly defined
lifecycles based on the kref.

Either holding the kref says the struct and everything in it/pointed
to by it is valid, or you have an inscrutable mess.

Jason

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Dan Williams]

On Tue, Feb 23, 2021 at 9:09 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Tue, Feb 23, 2021 at 08:27:46AM -0700, Dave Jiang wrote:
> >
> > On 2/23/2021 5:59 AM, Jason Gunthorpe wrote:
> > > On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
> > > > Remove devm_* allocation of memory of 'struct device' objects.
> > > > The devm_* lifetime is incompatible with device->release() lifetime.
> > > > Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
> > > > functions for each component in order to free the allocated memory at
> > > > the appropriate time. Each component such as wq, engine, and group now
> > > > needs to be allocated individually in order to setup the lifetime properly.
> > > I really don't understand why idxd has so many struct device objects.
> > >
> > > Typically I expect a simple driver to have exactly one, usually
> > > provided by its subsystem.
> > >
> > > What is the purpose?
> >
> > When we initially designed this, Dan suggested to tie the device and
> > workqueue enabling to the Linux device model so that the enabling/disabling
> > can be done via bind/unbind of the sub drivers. So that's how we ended up
> > with all the 'struct device' under idxd and one for each configurable
> > component of the hardware device.
>
> IDXD created its own little bus just for that?? :\

Yes, for the dynamic configurability of the queues and engines it was
either a pile of ioctls, configfs, or a dynamic sysfs organization. I
did dynamic sysfs for libnvdimm and suggested idxd could use the same
model.

> It is really weird that something called a workqueue would show up in
> the driver model at all.

It's a partition of the hardware functionality.

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Jason Gunthorpe]

On Tue, Feb 23, 2021 at 10:05:58AM -0800, Dan Williams wrote:
> On Tue, Feb 23, 2021 at 9:09 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Tue, Feb 23, 2021 at 08:27:46AM -0700, Dave Jiang wrote:
> > >
> > > On 2/23/2021 5:59 AM, Jason Gunthorpe wrote:
> > > > On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
> > > > > Remove devm_* allocation of memory of 'struct device' objects.
> > > > > The devm_* lifetime is incompatible with device->release() lifetime.
> > > > > Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
> > > > > functions for each component in order to free the allocated memory at
> > > > > the appropriate time. Each component such as wq, engine, and group now
> > > > > needs to be allocated individually in order to setup the lifetime properly.
> > > > I really don't understand why idxd has so many struct device objects.
> > > >
> > > > Typically I expect a simple driver to have exactly one, usually
> > > > provided by its subsystem.
> > > >
> > > > What is the purpose?
> > >
> > > When we initially designed this, Dan suggested to tie the device and
> > > workqueue enabling to the Linux device model so that the enabling/disabling
> > > can be done via bind/unbind of the sub drivers. So that's how we ended up
> > > with all the 'struct device' under idxd and one for each configurable
> > > component of the hardware device.
> >
> > IDXD created its own little bus just for that?? :\
> 
> Yes, for the dynamic configurability of the queues and engines it was
> either a pile of ioctls, configfs, or a dynamic sysfs organization. I
> did dynamic sysfs for libnvdimm and suggested idxd could use the same
> model.
> 
> > It is really weird that something called a workqueue would show up in
> > the driver model at all.
> 
> It's a partition of the hardware functionality.

But to what end? What else are you going to do with a slice of the
IDXD device other than assign it to the IDXD driver?

Is it for vfio? If so then why doesn't the vfio just bind to the WQ -
why does it have an aux device??

Jason

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Dave Jiang]

On 2/23/2021 11:10 AM, Jason Gunthorpe wrote:
> On Tue, Feb 23, 2021 at 10:05:58AM -0800, Dan Williams wrote:
>> On Tue, Feb 23, 2021 at 9:09 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>>> On Tue, Feb 23, 2021 at 08:27:46AM -0700, Dave Jiang wrote:
>>>> On 2/23/2021 5:59 AM, Jason Gunthorpe wrote:
>>>>> On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
>>>>>> Remove devm_* allocation of memory of 'struct device' objects.
>>>>>> The devm_* lifetime is incompatible with device->release() lifetime.
>>>>>> Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
>>>>>> functions for each component in order to free the allocated memory at
>>>>>> the appropriate time. Each component such as wq, engine, and group now
>>>>>> needs to be allocated individually in order to setup the lifetime properly.
>>>>> I really don't understand why idxd has so many struct device objects.
>>>>>
>>>>> Typically I expect a simple driver to have exactly one, usually
>>>>> provided by its subsystem.
>>>>>
>>>>> What is the purpose?
>>>> When we initially designed this, Dan suggested to tie the device and
>>>> workqueue enabling to the Linux device model so that the enabling/disabling
>>>> can be done via bind/unbind of the sub drivers. So that's how we ended up
>>>> with all the 'struct device' under idxd and one for each configurable
>>>> component of the hardware device.
>>> IDXD created its own little bus just for that?? :\
>> Yes, for the dynamic configurability of the queues and engines it was
>> either a pile of ioctls, configfs, or a dynamic sysfs organization. I
>> did dynamic sysfs for libnvdimm and suggested idxd could use the same
>> model.
>>
>>> It is really weird that something called a workqueue would show up in
>>> the driver model at all.
>> It's a partition of the hardware functionality.
> But to what end? What else are you going to do with a slice of the
> IDXD device other than assign it to the IDXD driver?
>
> Is it for vfio? If so then why doesn't the vfio just bind to the WQ -
> why does it have an aux device??
No this is unrelated to vfio. It's for general configuration of the device.

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Dan Williams]

On Tue, Feb 23, 2021 at 10:11 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Tue, Feb 23, 2021 at 10:05:58AM -0800, Dan Williams wrote:
> > On Tue, Feb 23, 2021 at 9:09 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > >
> > > On Tue, Feb 23, 2021 at 08:27:46AM -0700, Dave Jiang wrote:
> > > >
> > > > On 2/23/2021 5:59 AM, Jason Gunthorpe wrote:
> > > > > On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
> > > > > > Remove devm_* allocation of memory of 'struct device' objects.
> > > > > > The devm_* lifetime is incompatible with device->release() lifetime.
> > > > > > Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
> > > > > > functions for each component in order to free the allocated memory at
> > > > > > the appropriate time. Each component such as wq, engine, and group now
> > > > > > needs to be allocated individually in order to setup the lifetime properly.
> > > > > I really don't understand why idxd has so many struct device objects.
> > > > >
> > > > > Typically I expect a simple driver to have exactly one, usually
> > > > > provided by its subsystem.
> > > > >
> > > > > What is the purpose?
> > > >
> > > > When we initially designed this, Dan suggested to tie the device and
> > > > workqueue enabling to the Linux device model so that the enabling/disabling
> > > > can be done via bind/unbind of the sub drivers. So that's how we ended up
> > > > with all the 'struct device' under idxd and one for each configurable
> > > > component of the hardware device.
> > >
> > > IDXD created its own little bus just for that?? :\
> >
> > Yes, for the dynamic configurability of the queues and engines it was
> > either a pile of ioctls, configfs, or a dynamic sysfs organization. I
> > did dynamic sysfs for libnvdimm and suggested idxd could use the same
> > model.
> >
> > > It is really weird that something called a workqueue would show up in
> > > the driver model at all.
> >
> > It's a partition of the hardware functionality.
>
> But to what end? What else are you going to do with a slice of the
> IDXD device other than assign it to the IDXD driver?

idxd, unlike other dmaengines, has a dynamic relationship between
backend hardware engines and frontend submission queues. The
assignment of resources to queues is managed via sysfs. I think this
would be clearer if there were some more upstream usage examples
beyond dmaengine. However, consider one exploratory usage of
offloading memory copies in the pmem driver. Each pmem device could be
given a submission queue even if all pmem devices shared an engine on
the backend.

> Is it for vfio? If so then why doesn't the vfio just bind to the WQ -
> why does it have an aux device??

Hmm, Dave? Couldn't there be an alternate queue driver that attached
vfio? At least that's how libnvdimm and dax devices change
personality, they just load a different driver for the same device.

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Jason Gunthorpe]

On Tue, Feb 23, 2021 at 10:30:14AM -0800, Dan Williams wrote:
> On Tue, Feb 23, 2021 at 10:11 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Tue, Feb 23, 2021 at 10:05:58AM -0800, Dan Williams wrote:
> > > On Tue, Feb 23, 2021 at 9:09 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > >
> > > > On Tue, Feb 23, 2021 at 08:27:46AM -0700, Dave Jiang wrote:
> > > > >
> > > > > On 2/23/2021 5:59 AM, Jason Gunthorpe wrote:
> > > > > > On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
> > > > > > > Remove devm_* allocation of memory of 'struct device' objects.
> > > > > > > The devm_* lifetime is incompatible with device->release() lifetime.
> > > > > > > Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
> > > > > > > functions for each component in order to free the allocated memory at
> > > > > > > the appropriate time. Each component such as wq, engine, and group now
> > > > > > > needs to be allocated individually in order to setup the lifetime properly.
> > > > > > I really don't understand why idxd has so many struct device objects.
> > > > > >
> > > > > > Typically I expect a simple driver to have exactly one, usually
> > > > > > provided by its subsystem.
> > > > > >
> > > > > > What is the purpose?
> > > > >
> > > > > When we initially designed this, Dan suggested to tie the device and
> > > > > workqueue enabling to the Linux device model so that the enabling/disabling
> > > > > can be done via bind/unbind of the sub drivers. So that's how we ended up
> > > > > with all the 'struct device' under idxd and one for each configurable
> > > > > component of the hardware device.
> > > >
> > > > IDXD created its own little bus just for that?? :\
> > >
> > > Yes, for the dynamic configurability of the queues and engines it was
> > > either a pile of ioctls, configfs, or a dynamic sysfs organization. I
> > > did dynamic sysfs for libnvdimm and suggested idxd could use the same
> > > model.
> > >
> > > > It is really weird that something called a workqueue would show up in
> > > > the driver model at all.
> > >
> > > It's a partition of the hardware functionality.
> >
> > But to what end? What else are you going to do with a slice of the
> > IDXD device other than assign it to the IDXD driver?
> 
> idxd, unlike other dmaengines, has a dynamic relationship between
> backend hardware engines and frontend submission queues. The
> assignment of resources to queues is managed via sysfs. I think this
> would be clearer if there were some more upstream usage examples
> beyond dmaengine. However, consider one exploratory usage of
> offloading memory copies in the pmem driver. Each pmem device could be
> given a submission queue even if all pmem devices shared an engine on
> the backend.

But there are no other idxd_device_driver's in the tree, so this is
all some future imaginings?

Jason

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Dave Jiang]

On 2/23/2021 11:30 AM, Dan Williams wrote:
> On Tue, Feb 23, 2021 at 10:11 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>> On Tue, Feb 23, 2021 at 10:05:58AM -0800, Dan Williams wrote:
>>> On Tue, Feb 23, 2021 at 9:09 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>>>> On Tue, Feb 23, 2021 at 08:27:46AM -0700, Dave Jiang wrote:
>>>>> On 2/23/2021 5:59 AM, Jason Gunthorpe wrote:
>>>>>> On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
>>>>>>> Remove devm_* allocation of memory of 'struct device' objects.
>>>>>>> The devm_* lifetime is incompatible with device->release() lifetime.
>>>>>>> Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
>>>>>>> functions for each component in order to free the allocated memory at
>>>>>>> the appropriate time. Each component such as wq, engine, and group now
>>>>>>> needs to be allocated individually in order to setup the lifetime properly.
>>>>>> I really don't understand why idxd has so many struct device objects.
>>>>>>
>>>>>> Typically I expect a simple driver to have exactly one, usually
>>>>>> provided by its subsystem.
>>>>>>
>>>>>> What is the purpose?
>>>>> When we initially designed this, Dan suggested to tie the device and
>>>>> workqueue enabling to the Linux device model so that the enabling/disabling
>>>>> can be done via bind/unbind of the sub drivers. So that's how we ended up
>>>>> with all the 'struct device' under idxd and one for each configurable
>>>>> component of the hardware device.
>>>> IDXD created its own little bus just for that?? :\
>>> Yes, for the dynamic configurability of the queues and engines it was
>>> either a pile of ioctls, configfs, or a dynamic sysfs organization. I
>>> did dynamic sysfs for libnvdimm and suggested idxd could use the same
>>> model.
>>>
>>>> It is really weird that something called a workqueue would show up in
>>>> the driver model at all.
>>> It's a partition of the hardware functionality.
>> But to what end? What else are you going to do with a slice of the
>> IDXD device other than assign it to the IDXD driver?
> idxd, unlike other dmaengines, has a dynamic relationship between
> backend hardware engines and frontend submission queues. The
> assignment of resources to queues is managed via sysfs. I think this
> would be clearer if there were some more upstream usage examples
> beyond dmaengine. However, consider one exploratory usage of
> offloading memory copies in the pmem driver. Each pmem device could be
> given a submission queue even if all pmem devices shared an engine on
> the backend.
>
>> Is it for vfio? If so then why doesn't the vfio just bind to the WQ -
>> why does it have an aux device??
> Hmm, Dave? Couldn't there be an alternate queue driver that attached
> vfio? At least that's how libnvdimm and dax devices change
> personality, they just load a different driver for the same device.

Would that work for a queue that's shared between multiple mdevs? And 
wasn't the main reason of pushing an aux dev under VFIO is so putting 
the code under the right maintainer for code review?

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Jason Gunthorpe]

On Tue, Feb 23, 2021 at 11:42:16AM -0700, Dave Jiang wrote:

> Would that work for a queue that's shared between multiple mdevs? And wasn't
> the main reason of pushing an aux dev under VFIO is so putting the code
> under the right maintainer for code review?

Huh? No matter what device_driver type you use, the VFIO part would
still live under drivers/vfio

Jason

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Dan Williams]

On Tue, Feb 23, 2021 at 10:42 AM Dave Jiang <dave.jiang@intel.com> wrote:
>
>
> On 2/23/2021 11:30 AM, Dan Williams wrote:
> > On Tue, Feb 23, 2021 at 10:11 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >> On Tue, Feb 23, 2021 at 10:05:58AM -0800, Dan Williams wrote:
> >>> On Tue, Feb 23, 2021 at 9:09 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >>>> On Tue, Feb 23, 2021 at 08:27:46AM -0700, Dave Jiang wrote:
> >>>>> On 2/23/2021 5:59 AM, Jason Gunthorpe wrote:
> >>>>>> On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
> >>>>>>> Remove devm_* allocation of memory of 'struct device' objects.
> >>>>>>> The devm_* lifetime is incompatible with device->release() lifetime.
> >>>>>>> Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
> >>>>>>> functions for each component in order to free the allocated memory at
> >>>>>>> the appropriate time. Each component such as wq, engine, and group now
> >>>>>>> needs to be allocated individually in order to setup the lifetime properly.
> >>>>>> I really don't understand why idxd has so many struct device objects.
> >>>>>>
> >>>>>> Typically I expect a simple driver to have exactly one, usually
> >>>>>> provided by its subsystem.
> >>>>>>
> >>>>>> What is the purpose?
> >>>>> When we initially designed this, Dan suggested to tie the device and
> >>>>> workqueue enabling to the Linux device model so that the enabling/disabling
> >>>>> can be done via bind/unbind of the sub drivers. So that's how we ended up
> >>>>> with all the 'struct device' under idxd and one for each configurable
> >>>>> component of the hardware device.
> >>>> IDXD created its own little bus just for that?? :\
> >>> Yes, for the dynamic configurability of the queues and engines it was
> >>> either a pile of ioctls, configfs, or a dynamic sysfs organization. I
> >>> did dynamic sysfs for libnvdimm and suggested idxd could use the same
> >>> model.
> >>>
> >>>> It is really weird that something called a workqueue would show up in
> >>>> the driver model at all.
> >>> It's a partition of the hardware functionality.
> >> But to what end? What else are you going to do with a slice of the
> >> IDXD device other than assign it to the IDXD driver?
> > idxd, unlike other dmaengines, has a dynamic relationship between
> > backend hardware engines and frontend submission queues. The
> > assignment of resources to queues is managed via sysfs. I think this
> > would be clearer if there were some more upstream usage examples
> > beyond dmaengine. However, consider one exploratory usage of
> > offloading memory copies in the pmem driver. Each pmem device could be
> > given a submission queue even if all pmem devices shared an engine on
> > the backend.
> >
> >> Is it for vfio? If so then why doesn't the vfio just bind to the WQ -
> >> why does it have an aux device??
> > Hmm, Dave? Couldn't there be an alternate queue driver that attached
> > vfio? At least that's how libnvdimm and dax devices change
> > personality, they just load a different driver for the same device.
>
> Would that work for a queue that's shared between multiple mdevs? And
> wasn't the main reason of pushing an aux dev under VFIO is so putting
> the code under the right maintainer for code review?

It's just a device with a vfio driver. Whether the device is idxd-dev
or aux-dev shouldn't matter. Just removes a level of indirection to
run vfio on idxd native devices since they're already there. I should
have made that connection myself.

That said, now that I look the bus probe arch for idxd does too much
in the core, I would expect much of that logic to be pushed out to the
per driver probe in the leaf drivers, and I don't understand why "iax"
and "dsa" need their own bus types?

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Dave Jiang]

On 2/23/2021 11:53 AM, Dan Williams wrote:
> On Tue, Feb 23, 2021 at 10:42 AM Dave Jiang <dave.jiang@intel.com> wrote:
>>
>> On 2/23/2021 11:30 AM, Dan Williams wrote:
>>> On Tue, Feb 23, 2021 at 10:11 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>>>> On Tue, Feb 23, 2021 at 10:05:58AM -0800, Dan Williams wrote:
>>>>> On Tue, Feb 23, 2021 at 9:09 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>>>>>> On Tue, Feb 23, 2021 at 08:27:46AM -0700, Dave Jiang wrote:
>>>>>>> On 2/23/2021 5:59 AM, Jason Gunthorpe wrote:
>>>>>>>> On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
>>>>>>>>> Remove devm_* allocation of memory of 'struct device' objects.
>>>>>>>>> The devm_* lifetime is incompatible with device->release() lifetime.
>>>>>>>>> Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
>>>>>>>>> functions for each component in order to free the allocated memory at
>>>>>>>>> the appropriate time. Each component such as wq, engine, and group now
>>>>>>>>> needs to be allocated individually in order to setup the lifetime properly.
>>>>>>>> I really don't understand why idxd has so many struct device objects.
>>>>>>>>
>>>>>>>> Typically I expect a simple driver to have exactly one, usually
>>>>>>>> provided by its subsystem.
>>>>>>>>
>>>>>>>> What is the purpose?
>>>>>>> When we initially designed this, Dan suggested to tie the device and
>>>>>>> workqueue enabling to the Linux device model so that the enabling/disabling
>>>>>>> can be done via bind/unbind of the sub drivers. So that's how we ended up
>>>>>>> with all the 'struct device' under idxd and one for each configurable
>>>>>>> component of the hardware device.
>>>>>> IDXD created its own little bus just for that?? :\
>>>>> Yes, for the dynamic configurability of the queues and engines it was
>>>>> either a pile of ioctls, configfs, or a dynamic sysfs organization. I
>>>>> did dynamic sysfs for libnvdimm and suggested idxd could use the same
>>>>> model.
>>>>>
>>>>>> It is really weird that something called a workqueue would show up in
>>>>>> the driver model at all.
>>>>> It's a partition of the hardware functionality.
>>>> But to what end? What else are you going to do with a slice of the
>>>> IDXD device other than assign it to the IDXD driver?
>>> idxd, unlike other dmaengines, has a dynamic relationship between
>>> backend hardware engines and frontend submission queues. The
>>> assignment of resources to queues is managed via sysfs. I think this
>>> would be clearer if there were some more upstream usage examples
>>> beyond dmaengine. However, consider one exploratory usage of
>>> offloading memory copies in the pmem driver. Each pmem device could be
>>> given a submission queue even if all pmem devices shared an engine on
>>> the backend.
>>>
>>>> Is it for vfio? If so then why doesn't the vfio just bind to the WQ -
>>>> why does it have an aux device??
>>> Hmm, Dave? Couldn't there be an alternate queue driver that attached
>>> vfio? At least that's how libnvdimm and dax devices change
>>> personality, they just load a different driver for the same device.
>> Would that work for a queue that's shared between multiple mdevs? And
>> wasn't the main reason of pushing an aux dev under VFIO is so putting
>> the code under the right maintainer for code review?
> It's just a device with a vfio driver. Whether the device is idxd-dev
> or aux-dev shouldn't matter. Just removes a level of indirection to
> run vfio on idxd native devices since they're already there. I should
> have made that connection myself.
>
> That said, now that I look the bus probe arch for idxd does too much
> in the core, I would expect much of that logic to be pushed out to the
> per driver probe in the leaf drivers, and I don't understand why "iax"
> and "dsa" need their own bus types?

I was wanting /sys/bus/dsa and /sys/bus/iax. I guess it's not necessary 
since /sys/bus/idxd is probably fine. Although now accel-config is 
depending on it. Do you think we should still change that?

Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Dan Williams]

On Tue, Feb 23, 2021 at 11:18 AM Dave Jiang <dave.jiang@intel.com> wrote:
>
>
> On 2/23/2021 11:53 AM, Dan Williams wrote:
> > On Tue, Feb 23, 2021 at 10:42 AM Dave Jiang <dave.jiang@intel.com> wrote:
> >>
> >> On 2/23/2021 11:30 AM, Dan Williams wrote:
> >>> On Tue, Feb 23, 2021 at 10:11 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >>>> On Tue, Feb 23, 2021 at 10:05:58AM -0800, Dan Williams wrote:
> >>>>> On Tue, Feb 23, 2021 at 9:09 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >>>>>> On Tue, Feb 23, 2021 at 08:27:46AM -0700, Dave Jiang wrote:
> >>>>>>> On 2/23/2021 5:59 AM, Jason Gunthorpe wrote:
> >>>>>>>> On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
> >>>>>>>>> Remove devm_* allocation of memory of 'struct device' objects.
> >>>>>>>>> The devm_* lifetime is incompatible with device->release() lifetime.
> >>>>>>>>> Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
> >>>>>>>>> functions for each component in order to free the allocated memory at
> >>>>>>>>> the appropriate time. Each component such as wq, engine, and group now
> >>>>>>>>> needs to be allocated individually in order to setup the lifetime properly.
> >>>>>>>> I really don't understand why idxd has so many struct device objects.
> >>>>>>>>
> >>>>>>>> Typically I expect a simple driver to have exactly one, usually
> >>>>>>>> provided by its subsystem.
> >>>>>>>>
> >>>>>>>> What is the purpose?
> >>>>>>> When we initially designed this, Dan suggested to tie the device and
> >>>>>>> workqueue enabling to the Linux device model so that the enabling/disabling
> >>>>>>> can be done via bind/unbind of the sub drivers. So that's how we ended up
> >>>>>>> with all the 'struct device' under idxd and one for each configurable
> >>>>>>> component of the hardware device.
> >>>>>> IDXD created its own little bus just for that?? :\
> >>>>> Yes, for the dynamic configurability of the queues and engines it was
> >>>>> either a pile of ioctls, configfs, or a dynamic sysfs organization. I
> >>>>> did dynamic sysfs for libnvdimm and suggested idxd could use the same
> >>>>> model.
> >>>>>
> >>>>>> It is really weird that something called a workqueue would show up in
> >>>>>> the driver model at all.
> >>>>> It's a partition of the hardware functionality.
> >>>> But to what end? What else are you going to do with a slice of the
> >>>> IDXD device other than assign it to the IDXD driver?
> >>> idxd, unlike other dmaengines, has a dynamic relationship between
> >>> backend hardware engines and frontend submission queues. The
> >>> assignment of resources to queues is managed via sysfs. I think this
> >>> would be clearer if there were some more upstream usage examples
> >>> beyond dmaengine. However, consider one exploratory usage of
> >>> offloading memory copies in the pmem driver. Each pmem device could be
> >>> given a submission queue even if all pmem devices shared an engine on
> >>> the backend.
> >>>
> >>>> Is it for vfio? If so then why doesn't the vfio just bind to the WQ -
> >>>> why does it have an aux device??
> >>> Hmm, Dave? Couldn't there be an alternate queue driver that attached
> >>> vfio? At least that's how libnvdimm and dax devices change
> >>> personality, they just load a different driver for the same device.
> >> Would that work for a queue that's shared between multiple mdevs? And
> >> wasn't the main reason of pushing an aux dev under VFIO is so putting
> >> the code under the right maintainer for code review?
> > It's just a device with a vfio driver. Whether the device is idxd-dev
> > or aux-dev shouldn't matter. Just removes a level of indirection to
> > run vfio on idxd native devices since they're already there. I should
> > have made that connection myself.
> >
> > That said, now that I look the bus probe arch for idxd does too much
> > in the core, I would expect much of that logic to be pushed out to the
> > per driver probe in the leaf drivers, and I don't understand why "iax"
> > and "dsa" need their own bus types?
>
> I was wanting /sys/bus/dsa and /sys/bus/iax. I guess it's not necessary
> since /sys/bus/idxd is probably fine. Although now accel-config is
> depending on it. Do you think we should still change that?
>

ABI is unfortunately forever. If there are end user deployments that
depend on this bus proliferation it will take time and compat
libraries to unwind it. For example the conversion of /sys/class/dax
to /sys/bus/dax is reaching the point (~4 years out) where
/sys/class/dax support can finally be removed. I would recommend
deprecating all but one bus for the idxd configuration interface and
try to repair this escape.