[PATCH v7 0/8] idxd 'struct device' lifetime handling fixes

[PATCH v7 0/8] idxd 'struct device' lifetime handling fixes

[Dave Jiang]

v7:
- Fix up the 'struct device' setup in char device code (Jason)
- Split out the char dev fixes (Jason)
- Split out the DMA dev fixes (Dan)
- Split out the each of the conf_dev fixes
- Split out removal of the pcim_* calls
- Split out removal of the devm_* calls
- Split out the fixes for interrupt config calls
- Reviewed by Dan.

v6:
- Fix char dev initialization issues (Jason)
- Fix other 'struct device' initialization issues.

v5:
- Rebased against 5.12-rc dmaengine/fixes
v4:
- fix up the life time of cdev creation/destruction (Jason)
- Tested with KASAN and other memory allocation leak detections. (Jason)

v3:
- Remove devm_* for irq request and cleanup related bits (Jason)
v2:
- Remove all devm_* alloc for idxd_device (Jason)
- Add kref dep for dma_dev (Jason)

Vinod,
The series fixes the various 'struct device' lifetime handling in the
idxd driver. The devm managed lifetime is incompatible with 'struct device'
objects that resides in the idxd context. Tested with
CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that.

Please consider for damengine/fixes for the 5.12-rc.

---

Dave Jiang (8):
      dmaengine: idxd: fix dma device lifetime
      dmaengine: idxd: cleanup pci interrupt vector allocation management
      dmaengine: idxd: removal of pcim managed mmio mapping
      dmaengine: idxd: fix idxd conf_dev 'struct device' lifetime
      dmaengine: idxd: fix wq conf_dev 'struct device' lifetime
      dmaengine: idxd: fix engine conf_dev lifetime
      dmaengine: idxd: fix group conf_dev lifetime
      dmaengine: idxd: fix cdev setup and free device lifetime issues


 drivers/dma/idxd/cdev.c   | 131 +++++++-----------
 drivers/dma/idxd/device.c |  20 +--
 drivers/dma/idxd/idxd.h   |  54 ++++++--
 drivers/dma/idxd/init.c   | 284 +++++++++++++++++++++++++++-----------
 drivers/dma/idxd/irq.c    |  10 +-
 drivers/dma/idxd/sysfs.c  | 239 ++++++++++++++++----------------
 6 files changed, 437 insertions(+), 301 deletions(-)

--

[PATCH v7 1/8] dmaengine: idxd: fix dma device lifetime

[Dave Jiang]

The devm managed lifetime is incompatible with 'struct device' objects that
resides in idxd context. This is one of the series that clean up the idxd
driver 'struct device' lifetime. Remove embedding of dma_device and dma_chan
in idxd since it's not the only interface that idxd will use. The freeing of
the dma_device will be managed by the ->release() function.

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 |    2 +
 drivers/dma/idxd/dma.c    |   61 +++++++++++++++++++++++++++++++++++++--------
 drivers/dma/idxd/idxd.h   |   17 +++++++++++--
 3 files changed, 66 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 84a6ea60ecf0..971a10a60093 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -186,7 +186,7 @@ int idxd_wq_alloc_resources(struct idxd_wq *wq)
 		desc->id = i;
 		desc->wq = wq;
 		desc->cpu = -1;
-		dma_async_tx_descriptor_init(&desc->txd, &wq->dma_chan);
+		dma_async_tx_descriptor_init(&desc->txd, &wq->idxd_chan->chan);
 		desc->txd.tx_submit = idxd_dma_tx_submit;
 	}
 
diff --git a/drivers/dma/idxd/dma.c b/drivers/dma/idxd/dma.c
index a15e50126434..8cea87f0d42d 100644
--- a/drivers/dma/idxd/dma.c
+++ b/drivers/dma/idxd/dma.c
@@ -14,7 +14,10 @@
 
 static inline struct idxd_wq *to_idxd_wq(struct dma_chan *c)
 {
-	return container_of(c, struct idxd_wq, dma_chan);
+	struct idxd_dma_chan *idxd_chan;
+
+	idxd_chan = container_of(c, struct idxd_dma_chan, chan);
+	return idxd_chan->wq;
 }
 
 void idxd_dma_complete_txd(struct idxd_desc *desc,
@@ -156,14 +159,25 @@ dma_cookie_t idxd_dma_tx_submit(struct dma_async_tx_descriptor *tx)
 
 static void idxd_dma_release(struct dma_device *device)
 {
+	struct idxd_dma_dev *idxd_dma = container_of(device, struct idxd_dma_dev, dma);
+
+	kfree(idxd_dma);
 }
 
 int idxd_register_dma_device(struct idxd_device *idxd)
 {
-	struct dma_device *dma = &idxd->dma_dev;
+	struct idxd_dma_dev *idxd_dma;
+	struct dma_device *dma;
+	struct device *dev = &idxd->pdev->dev;
+	int rc;
+
+	idxd_dma = kzalloc_node(sizeof(*idxd_dma), GFP_KERNEL, dev_to_node(dev));
+	if (!idxd_dma)
+		return -ENOMEM;
 
+	dma = &idxd_dma->dma;
 	INIT_LIST_HEAD(&dma->channels);
-	dma->dev = &idxd->pdev->dev;
+	dma->dev = dev;
 
 	dma_cap_set(DMA_PRIVATE, dma->cap_mask);
 	dma_cap_set(DMA_COMPLETION_NO_ORDER, dma->cap_mask);
@@ -179,35 +193,60 @@ int idxd_register_dma_device(struct idxd_device *idxd)
 	dma->device_alloc_chan_resources = idxd_dma_alloc_chan_resources;
 	dma->device_free_chan_resources = idxd_dma_free_chan_resources;
 
-	return dma_async_device_register(&idxd->dma_dev);
+	rc = dma_async_device_register(dma);
+	if (rc < 0) {
+		kfree(idxd_dma);
+		return rc;
+	}
+
+	idxd_dma->idxd = idxd;
+	idxd->idxd_dma = idxd_dma;
+	return 0;
 }
 
 void idxd_unregister_dma_device(struct idxd_device *idxd)
 {
-	dma_async_device_unregister(&idxd->dma_dev);
+	dma_async_device_unregister(&idxd->idxd_dma->dma);
 }
 
 int idxd_register_dma_channel(struct idxd_wq *wq)
 {
 	struct idxd_device *idxd = wq->idxd;
-	struct dma_device *dma = &idxd->dma_dev;
-	struct dma_chan *chan = &wq->dma_chan;
+	struct dma_device *dma = &idxd->idxd_dma->dma;
+	struct device *dev = &idxd->pdev->dev;
+	struct idxd_dma_chan *idxd_chan;
+	struct dma_chan *chan;
 	int rc;
 
-	memset(&wq->dma_chan, 0, sizeof(struct dma_chan));
+	idxd_chan = kzalloc_node(sizeof(*idxd_chan), GFP_KERNEL, dev_to_node(dev));
+	if (!idxd_chan)
+		return -ENOMEM;
+
+	chan = &idxd_chan->chan;
 	chan->device = dma;
 	list_add_tail(&chan->device_node, &dma->channels);
 	rc = dma_async_device_channel_register(dma, chan);
-	if (rc < 0)
+	if (rc < 0) {
+		kfree(idxd_chan);
 		return rc;
+	}
+
+	wq->idxd_chan = idxd_chan;
+	idxd_chan->wq = wq;
+	get_device(&wq->conf_dev);
 
 	return 0;
 }
 
 void idxd_unregister_dma_channel(struct idxd_wq *wq)
 {
-	struct dma_chan *chan = &wq->dma_chan;
+	struct idxd_dma_chan *idxd_chan = wq->idxd_chan;
+	struct dma_chan *chan = &idxd_chan->chan;
+	struct idxd_dma_dev *idxd_dma = wq->idxd->idxd_dma;
 
-	dma_async_device_channel_unregister(&wq->idxd->dma_dev, chan);
+	dma_async_device_channel_unregister(&idxd_dma->dma, chan);
 	list_del(&chan->device_node);
+	kfree(wq->idxd_chan);
+	wq->idxd_chan = NULL;
+	put_device(&wq->conf_dev);
 }
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 81a0e65fd316..40001c46d95c 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -14,6 +14,9 @@
 
 extern struct kmem_cache *idxd_desc_pool;
 
+struct idxd_device;
+struct idxd_wq;
+
 #define IDXD_REG_TIMEOUT	50
 #define IDXD_DRAIN_TIMEOUT	5000
 
@@ -96,6 +99,11 @@ enum idxd_complete_type {
 	IDXD_COMPLETE_DEV_FAIL,
 };
 
+struct idxd_dma_chan {
+	struct dma_chan chan;
+	struct idxd_wq *wq;
+};
+
 struct idxd_wq {
 	void __iomem *portal;
 	struct device conf_dev;
@@ -125,7 +133,7 @@ struct idxd_wq {
 	int compls_size;
 	struct idxd_desc **descs;
 	struct sbitmap_queue sbq;
-	struct dma_chan dma_chan;
+	struct idxd_dma_chan *idxd_chan;
 	char name[WQ_NAME_SIZE + 1];
 	u64 max_xfer_bytes;
 	u32 max_batch_size;
@@ -162,6 +170,11 @@ enum idxd_device_flag {
 	IDXD_FLAG_PASID_ENABLED,
 };
 
+struct idxd_dma_dev {
+	struct idxd_device *idxd;
+	struct dma_device dma;
+};
+
 struct idxd_device {
 	enum idxd_type type;
 	struct device conf_dev;
@@ -210,7 +223,7 @@ struct idxd_device {
 	int num_wq_irqs;
 	struct idxd_irq_entry *irq_entries;
 
-	struct dma_device dma_dev;
+	struct idxd_dma_dev *idxd_dma;
 	struct workqueue_struct *wq;
 	struct work_struct work;
 };

[PATCH v7 2/8] dmaengine: idxd: cleanup pci interrupt vector allocation management

[Dave Jiang]

The devm managed lifetime is incompatible with 'struct device' objects that
resides in idxd context. This is one of the series that clean up the idxd
driver 'struct device' lifetime. Remove devm managed pci interrupt vectors
and replace with unmanged allocators.

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 |    4 +--
 drivers/dma/idxd/idxd.h   |    2 +
 drivers/dma/idxd/init.c   |   64 +++++++++++++++++++--------------------------
 3 files changed, 30 insertions(+), 40 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 971a10a60093..a56af1d8b091 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -19,7 +19,7 @@ static void idxd_cmd_exec(struct idxd_device *idxd, int cmd_code, u32 operand,
 /* Interrupt control bits */
 void idxd_mask_msix_vector(struct idxd_device *idxd, int vec_id)
 {
-	struct irq_data *data = irq_get_irq_data(idxd->msix_entries[vec_id].vector);
+	struct irq_data *data = irq_get_irq_data(idxd->irq_entries[vec_id].vector);
 
 	pci_msi_mask_irq(data);
 }
@@ -36,7 +36,7 @@ void idxd_mask_msix_vectors(struct idxd_device *idxd)
 
 void idxd_unmask_msix_vector(struct idxd_device *idxd, int vec_id)
 {
-	struct irq_data *data = irq_get_irq_data(idxd->msix_entries[vec_id].vector);
+	struct irq_data *data = irq_get_irq_data(idxd->irq_entries[vec_id].vector);
 
 	pci_msi_unmask_irq(data);
 }
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 40001c46d95c..3409a84b8674 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -36,6 +36,7 @@ struct idxd_device_driver {
 struct idxd_irq_entry {
 	struct idxd_device *idxd;
 	int id;
+	int vector;
 	struct llist_head pending_llist;
 	struct list_head work_list;
 	/*
@@ -219,7 +220,6 @@ struct idxd_device {
 
 	union sw_err_reg sw_err;
 	wait_queue_head_t cmd_waitq;
-	struct msix_entry *msix_entries;
 	int num_wq_irqs;
 	struct idxd_irq_entry *irq_entries;
 
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 085a0c3b62c6..72f319dd52fc 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -61,7 +61,6 @@ static int idxd_setup_interrupts(struct idxd_device *idxd)
 {
 	struct pci_dev *pdev = idxd->pdev;
 	struct device *dev = &pdev->dev;
-	struct msix_entry *msix;
 	struct idxd_irq_entry *irq_entry;
 	int i, msixcnt;
 	int rc = 0;
@@ -70,23 +69,13 @@ static int idxd_setup_interrupts(struct idxd_device *idxd)
 	msixcnt = pci_msix_vec_count(pdev);
 	if (msixcnt < 0) {
 		dev_err(dev, "Not MSI-X interrupt capable.\n");
-		goto err_no_irq;
+		return -ENOSPC;
 	}
 
-	idxd->msix_entries = devm_kzalloc(dev, sizeof(struct msix_entry) *
-			msixcnt, GFP_KERNEL);
-	if (!idxd->msix_entries) {
-		rc = -ENOMEM;
-		goto err_no_irq;
-	}
-
-	for (i = 0; i < msixcnt; i++)
-		idxd->msix_entries[i].entry = i;
-
-	rc = pci_enable_msix_exact(pdev, idxd->msix_entries, msixcnt);
-	if (rc) {
-		dev_err(dev, "Failed enabling %d MSIX entries.\n", msixcnt);
-		goto err_no_irq;
+	rc = pci_alloc_irq_vectors(pdev, msixcnt, msixcnt, PCI_IRQ_MSIX);
+	if (rc != msixcnt) {
+		dev_err(dev, "Failed enabling %d MSIX entries: %d\n", msixcnt, rc);
+		return -ENOSPC;
 	}
 	dev_dbg(dev, "Enabled %d msix vectors\n", msixcnt);
 
@@ -99,48 +88,41 @@ static int idxd_setup_interrupts(struct idxd_device *idxd)
 					 GFP_KERNEL);
 	if (!idxd->irq_entries) {
 		rc = -ENOMEM;
-		goto err_no_irq;
+		goto err_irq_entries;
 	}
 
 	for (i = 0; i < msixcnt; i++) {
 		idxd->irq_entries[i].id = i;
 		idxd->irq_entries[i].idxd = idxd;
+		idxd->irq_entries[i].vector = pci_irq_vector(pdev, i);
 		spin_lock_init(&idxd->irq_entries[i].list_lock);
 	}
 
-	msix = &idxd->msix_entries[0];
 	irq_entry = &idxd->irq_entries[0];
-	rc = devm_request_threaded_irq(dev, msix->vector, idxd_irq_handler,
-				       idxd_misc_thread, 0, "idxd-misc",
-				       irq_entry);
+	rc = request_threaded_irq(irq_entry->vector, idxd_irq_handler, idxd_misc_thread,
+				  0, "idxd-misc", irq_entry);
 	if (rc < 0) {
 		dev_err(dev, "Failed to allocate misc interrupt.\n");
-		goto err_no_irq;
+		goto err_misc_irq;
 	}
 
-	dev_dbg(dev, "Allocated idxd-misc handler on msix vector %d\n",
-		msix->vector);
+	dev_dbg(dev, "Allocated idxd-misc handler on msix vector %d\n", irq_entry->vector);
 
 	/* first MSI-X entry is not for wq interrupts */
 	idxd->num_wq_irqs = msixcnt - 1;
 
 	for (i = 1; i < msixcnt; i++) {
-		msix = &idxd->msix_entries[i];
 		irq_entry = &idxd->irq_entries[i];
 
 		init_llist_head(&idxd->irq_entries[i].pending_llist);
 		INIT_LIST_HEAD(&idxd->irq_entries[i].work_list);
-		rc = devm_request_threaded_irq(dev, msix->vector,
-					       idxd_irq_handler,
-					       idxd_wq_thread, 0,
-					       "idxd-portal", irq_entry);
+		rc = request_threaded_irq(irq_entry->vector, idxd_irq_handler,
+					  idxd_wq_thread, 0, "idxd-portal", irq_entry);
 		if (rc < 0) {
-			dev_err(dev, "Failed to allocate irq %d.\n",
-				msix->vector);
-			goto err_no_irq;
+			dev_err(dev, "Failed to allocate irq %d.\n", irq_entry->vector);
+			goto err_wq_irqs;
 		}
-		dev_dbg(dev, "Allocated idxd-msix %d for vector %d\n",
-			i, msix->vector);
+		dev_dbg(dev, "Allocated idxd-msix %d for vector %d\n", i, irq_entry->vector);
 	}
 
 	idxd_unmask_error_interrupts(idxd);
@@ -154,10 +136,16 @@ static int idxd_setup_interrupts(struct idxd_device *idxd)
 
 	return 0;
 
- err_no_irq:
+ err_wq_irqs:
+	while (--i) {
+		irq_entry = &idxd->irq_entries[i];
+		free_irq(irq_entry->vector, irq_entry);
+	}
+ err_misc_irq:
 	/* Disable error interrupt generation */
 	idxd_mask_error_interrupts(idxd);
-	pci_disable_msix(pdev);
+ err_irq_entries:
+	pci_free_irq_vectors(pdev);
 	dev_err(dev, "No usable interrupts\n");
 	return rc;
 }
@@ -503,13 +491,15 @@ static void idxd_shutdown(struct pci_dev *pdev)
 
 	for (i = 0; i < msixcnt; i++) {
 		irq_entry = &idxd->irq_entries[i];
-		synchronize_irq(idxd->msix_entries[i].vector);
+		synchronize_irq(irq_entry->vector);
+		free_irq(irq_entry->vector, irq_entry);
 		if (i == 0)
 			continue;
 		idxd_flush_pending_llist(irq_entry);
 		idxd_flush_work_list(irq_entry);
 	}
 
+	pci_free_irq_vectors(pdev);
 	destroy_workqueue(idxd->wq);
 }
 

[PATCH v7 3/8] dmaengine: idxd: removal of pcim managed mmio mapping

[Dave Jiang]

The devm managed lifetime is incompatible with 'struct device' objects that
resides in idxd context. This is one of the series that clean up the idxd
driver 'struct device' lifetime. Remove pcim_* management of the PCI device
and the ioremap of MMIO BAR and replace with unmanaged versions. This is
for consistency of removing all the pcim/devm based calls.

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/init.c |   33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 72f319dd52fc..080d31bf5b33 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -392,32 +392,36 @@ static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct idxd_device *idxd;
 	int rc;
 
-	rc = pcim_enable_device(pdev);
+	rc = pci_enable_device(pdev);
 	if (rc)
 		return rc;
 
 	dev_dbg(dev, "Alloc IDXD context\n");
 	idxd = idxd_alloc(pdev);
-	if (!idxd)
-		return -ENOMEM;
+	if (!idxd) {
+		rc = -ENOMEM;
+		goto err_idxd_alloc;
+	}
 
 	dev_dbg(dev, "Mapping BARs\n");
-	idxd->reg_base = pcim_iomap(pdev, IDXD_MMIO_BAR, 0);
-	if (!idxd->reg_base)
-		return -ENOMEM;
+	idxd->reg_base = pci_iomap(pdev, IDXD_MMIO_BAR, 0);
+	if (!idxd->reg_base) {
+		rc = -ENOMEM;
+		goto err_iomap;
+	}
 
 	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);
 
@@ -431,13 +435,13 @@ 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;
+		goto err;
 	}
 
 	rc = idxd_setup_sysfs(idxd);
 	if (rc) {
 		dev_err(dev, "IDXD sysfs setup failed\n");
-		return -ENODEV;
+		goto err;
 	}
 
 	idxd->state = IDXD_DEV_CONF_READY;
@@ -446,6 +450,13 @@ static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		 idxd->hw.version);
 
 	return 0;
+
+ err:
+	pci_iounmap(pdev, idxd->reg_base);
+ err_iomap:
+ err_idxd_alloc:
+	pci_disable_device(pdev);
+	return rc;
 }
 
 static void idxd_flush_pending_llist(struct idxd_irq_entry *ie)
@@ -500,6 +511,8 @@ static void idxd_shutdown(struct pci_dev *pdev)
 	}
 
 	pci_free_irq_vectors(pdev);
+	pci_iounmap(pdev, idxd->reg_base);
+	pci_disable_device(pdev);
 	destroy_workqueue(idxd->wq);
 }
 

[PATCH v7 4/8] dmaengine: idxd: fix idxd conf_dev 'struct device' lifetime

[Dave Jiang]

The devm managed lifetime is incompatible with 'struct device' objects that
resides in idxd context. This is one of the series that clean up the idxd
driver 'struct device' lifetime. Fix idxd->conf_dev 'struct device'
lifetime. Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE.
Add release functions in order to free the allocated memory at the
appropriate time.

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/idxd.h  |   21 ++++++++++++-
 drivers/dma/idxd/init.c  |   22 ++++++++++----
 drivers/dma/idxd/sysfs.c |   72 +++++++++++++++++++++++-----------------------
 3 files changed, 70 insertions(+), 45 deletions(-)

diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 3409a84b8674..f3893a07f6d5 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -255,6 +255,23 @@ extern struct bus_type dsa_bus_type;
 extern struct bus_type iax_bus_type;
 
 extern bool support_enqcmd;
+extern struct device_type dsa_device_type;
+extern struct device_type iax_device_type;
+
+static inline bool is_dsa_dev(struct device *dev)
+{
+	return dev->type == &dsa_device_type;
+}
+
+static inline bool is_iax_dev(struct device *dev)
+{
+	return dev->type == &iax_device_type;
+}
+
+static inline bool is_idxd_dev(struct device *dev)
+{
+	return is_dsa_dev(dev) || is_iax_dev(dev);
+}
 
 static inline bool wq_dedicated(struct idxd_wq *wq)
 {
@@ -322,8 +339,8 @@ static inline int idxd_wq_refcount(struct idxd_wq *wq)
 const char *idxd_get_dev_name(struct idxd_device *idxd);
 int idxd_register_bus_type(void);
 void idxd_unregister_bus_type(void);
-int idxd_setup_sysfs(struct idxd_device *idxd);
-void idxd_cleanup_sysfs(struct idxd_device *idxd);
+int idxd_register_devices(struct idxd_device *idxd);
+void idxd_unregister_devices(struct idxd_device *idxd);
 int idxd_register_driver(void);
 void idxd_unregister_driver(void);
 struct bus_type *idxd_get_bus_type(struct idxd_device *idxd);
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 080d31bf5b33..158f9e0158a3 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -83,9 +83,8 @@ static int idxd_setup_interrupts(struct idxd_device *idxd)
 	 * We implement 1 completion list per MSI-X entry except for
 	 * entry 0, which is for errors and others.
 	 */
-	idxd->irq_entries = devm_kcalloc(dev, msixcnt,
-					 sizeof(struct idxd_irq_entry),
-					 GFP_KERNEL);
+	idxd->irq_entries = kcalloc_node(msixcnt, sizeof(struct idxd_irq_entry),
+					 GFP_KERNEL, dev_to_node(dev));
 	if (!idxd->irq_entries) {
 		rc = -ENOMEM;
 		goto err_irq_entries;
@@ -144,6 +143,7 @@ static int idxd_setup_interrupts(struct idxd_device *idxd)
  err_misc_irq:
 	/* Disable error interrupt generation */
 	idxd_mask_error_interrupts(idxd);
+	kfree(idxd->irq_entries);
  err_irq_entries:
 	pci_free_irq_vectors(pdev);
 	dev_err(dev, "No usable interrupts\n");
@@ -276,7 +276,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;
 
@@ -320,6 +320,11 @@ static void idxd_disable_system_pasid(struct idxd_device *idxd)
 	idxd->sva = NULL;
 }
 
+static void idxd_free(struct idxd_device *idxd)
+{
+	kfree(idxd);
+}
+
 static int idxd_probe(struct idxd_device *idxd)
 {
 	struct pci_dev *pdev = idxd->pdev;
@@ -438,7 +443,7 @@ static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto err;
 	}
 
-	rc = idxd_setup_sysfs(idxd);
+	rc = idxd_register_devices(idxd);
 	if (rc) {
 		dev_err(dev, "IDXD sysfs setup failed\n");
 		goto err;
@@ -454,6 +459,7 @@ static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
  err:
 	pci_iounmap(pdev, idxd->reg_base);
  err_iomap:
+	idxd_free(idxd);
  err_idxd_alloc:
 	pci_disable_device(pdev);
 	return rc;
@@ -519,15 +525,17 @@ 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);
 	mutex_lock(&idxd_idr_lock);
-	idr_remove(&idxd_idrs[idxd->type], idxd->id);
+	idr_remove(&idxd_idrs[type], id);
 	mutex_unlock(&idxd_idr_lock);
+	idxd_unregister_devices(idxd);
 }
 
 static struct pci_driver idxd_pci_driver = {
diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
index 4dbb03c545e4..55a92dfe0fad 100644
--- a/drivers/dma/idxd/sysfs.c
+++ b/drivers/dma/idxd/sysfs.c
@@ -16,51 +16,26 @@ static char *idxd_wq_type_names[] = {
 	[IDXD_WQT_USER]		= "user",
 };
 
-static void idxd_conf_device_release(struct device *dev)
+static void idxd_conf_sub_device_release(struct device *dev)
 {
 	dev_dbg(dev, "%s for %s\n", __func__, dev_name(dev));
 }
 
 static struct device_type idxd_group_device_type = {
 	.name = "group",
-	.release = idxd_conf_device_release,
+	.release = idxd_conf_sub_device_release,
 };
 
 static struct device_type idxd_wq_device_type = {
 	.name = "wq",
-	.release = idxd_conf_device_release,
+	.release = idxd_conf_sub_device_release,
 };
 
 static struct device_type idxd_engine_device_type = {
 	.name = "engine",
-	.release = idxd_conf_device_release,
-};
-
-static struct device_type dsa_device_type = {
-	.name = "dsa",
-	.release = idxd_conf_device_release,
-};
-
-static struct device_type iax_device_type = {
-	.name = "iax",
-	.release = idxd_conf_device_release,
+	.release = idxd_conf_sub_device_release,
 };
 
-static inline bool is_dsa_dev(struct device *dev)
-{
-	return dev ? dev->type == &dsa_device_type : false;
-}
-
-static inline bool is_iax_dev(struct device *dev)
-{
-	return dev ? dev->type == &iax_device_type : false;
-}
-
-static inline bool is_idxd_dev(struct device *dev)
-{
-	return is_dsa_dev(dev) || is_iax_dev(dev);
-}
-
 static inline bool is_idxd_wq_dev(struct device *dev)
 {
 	return dev ? dev->type == &idxd_wq_device_type : false;
@@ -1643,6 +1618,29 @@ static const struct attribute_group *idxd_attribute_groups[] = {
 	NULL,
 };
 
+static void idxd_conf_device_release(struct device *dev)
+{
+	struct idxd_device *idxd = container_of(dev, struct idxd_device, conf_dev);
+
+	kfree(idxd->groups);
+	kfree(idxd->wqs);
+	kfree(idxd->engines);
+	kfree(idxd->irq_entries);
+	kfree(idxd);
+}
+
+struct device_type dsa_device_type = {
+	.name = "dsa",
+	.release = idxd_conf_device_release,
+	.groups = idxd_attribute_groups,
+};
+
+struct device_type iax_device_type = {
+	.name = "iax",
+	.release = idxd_conf_device_release,
+	.groups = idxd_attribute_groups,
+};
+
 static int idxd_setup_engine_sysfs(struct idxd_device *idxd)
 {
 	struct device *dev = &idxd->pdev->dev;
@@ -1748,17 +1746,19 @@ static int idxd_setup_device_sysfs(struct idxd_device *idxd)
 {
 	struct device *dev = &idxd->pdev->dev;
 	int rc;
-	char devname[IDXD_NAME_SIZE];
 
-	sprintf(devname, "%s%d", idxd_get_dev_name(idxd), idxd->id);
+	device_initialize(&idxd->conf_dev);
 	idxd->conf_dev.parent = dev;
-	dev_set_name(&idxd->conf_dev, "%s", devname);
 	idxd->conf_dev.bus = idxd_get_bus_type(idxd);
-	idxd->conf_dev.groups = idxd_attribute_groups;
 	idxd->conf_dev.type = idxd_get_device_type(idxd);
+	rc = dev_set_name(&idxd->conf_dev, "%s%d", idxd_get_dev_name(idxd), idxd->id);
+	if (rc < 0) {
+		put_device(&idxd->conf_dev);
+		return rc;
+	}
 
 	dev_dbg(dev, "IDXD device register: %s\n", dev_name(&idxd->conf_dev));
-	rc = device_register(&idxd->conf_dev);
+	rc = device_add(&idxd->conf_dev);
 	if (rc < 0) {
 		put_device(&idxd->conf_dev);
 		return rc;
@@ -1767,7 +1767,7 @@ static int idxd_setup_device_sysfs(struct idxd_device *idxd)
 	return 0;
 }
 
-int idxd_setup_sysfs(struct idxd_device *idxd)
+int idxd_register_devices(struct idxd_device *idxd)
 {
 	struct device *dev = &idxd->pdev->dev;
 	int rc;
@@ -1802,7 +1802,7 @@ int idxd_setup_sysfs(struct idxd_device *idxd)
 	return 0;
 }
 
-void idxd_cleanup_sysfs(struct idxd_device *idxd)
+void idxd_unregister_devices(struct idxd_device *idxd)
 {
 	int i;
 

[PATCH v7 5/8] dmaengine: idxd: fix wq conf_dev 'struct device' lifetime

[Dave Jiang]

The devm managed lifetime is incompatible with 'struct device' objects that
resides in idxd context. This is one of the series that clean up the idxd
driver 'struct device' lifetime. Fix wq->conf_dev 'struct device'
lifetime. Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE.
Add release functions in order to free the allocated memory at the
appropriate time.

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 |    6 ++--
 drivers/dma/idxd/idxd.h   |   20 ++++++++++++-
 drivers/dma/idxd/init.c   |   69 ++++++++++++++++++++++++++++++++++++--------
 drivers/dma/idxd/irq.c    |    6 ++--
 drivers/dma/idxd/sysfs.c  |   70 +++++++++++++++++++--------------------------
 5 files changed, 111 insertions(+), 60 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index a56af1d8b091..2292d7bfef58 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -515,7 +515,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);
@@ -696,7 +696,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)
@@ -774,7 +774,7 @@ static int idxd_wqs_setup(struct idxd_device *idxd)
 	}
 
 	for (i = 0; i < idxd->max_wqs; i++) {
-		wq = &idxd->wqs[i];
+		wq = idxd->wqs[i];
 		group = wq->group;
 
 		if (!wq->group)
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index f3893a07f6d5..cd41baeb2bdf 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -193,7 +193,7 @@ struct idxd_device {
 	spinlock_t dev_lock;	/* spinlock for device */
 	struct completion *cmd_done;
 	struct idxd_group *groups;
-	struct idxd_wq *wqs;
+	struct idxd_wq **wqs;
 	struct idxd_engine *engines;
 
 	struct iommu_sva *sva;
@@ -257,6 +257,7 @@ extern struct bus_type iax_bus_type;
 extern bool support_enqcmd;
 extern struct device_type dsa_device_type;
 extern struct device_type iax_device_type;
+extern struct device_type idxd_wq_device_type;
 
 static inline bool is_dsa_dev(struct device *dev)
 {
@@ -273,6 +274,23 @@ static inline bool is_idxd_dev(struct device *dev)
 	return is_dsa_dev(dev) || is_iax_dev(dev);
 }
 
+static inline bool is_idxd_wq_dev(struct device *dev)
+{
+	        return dev->type == &idxd_wq_device_type;
+}
+
+static inline bool is_idxd_wq_dmaengine(struct idxd_wq *wq)
+{
+	if (wq->type == IDXD_WQT_KERNEL && strcmp(wq->name, "dmaengine") == 0)
+		return true;
+	return false;
+}
+
+static inline bool is_idxd_wq_cdev(struct idxd_wq *wq)
+{
+	return wq->type == IDXD_WQT_USER;
+}
+
 static inline bool wq_dedicated(struct idxd_wq *wq)
 {
 	return test_bit(WQ_FLAG_DEDICATED, &wq->flags);
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 158f9e0158a3..2aceb99ef9d1 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -150,10 +150,41 @@ 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 = kcalloc_node(idxd->max_wqs, sizeof(struct idxd_wq *),
+				 GFP_KERNEL, dev_to_node(dev));
+	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]);
+	kfree(idxd->wqs);
+	idxd->wqs = NULL;
+	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);
 	idxd->groups = devm_kcalloc(dev, idxd->max_groups,
@@ -168,18 +199,12 @@ static int idxd_setup_internals(struct idxd_device *idxd)
 		idxd->groups[i].tc_b = -1;
 	}
 
-	idxd->wqs = devm_kcalloc(dev, idxd->max_wqs, sizeof(struct idxd_wq),
-				 GFP_KERNEL);
-	if (!idxd->wqs)
-		return -ENOMEM;
-
-	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;
@@ -187,11 +212,17 @@ static int idxd_setup_internals(struct idxd_device *idxd)
 		wq->idxd_cdev.minor = -1;
 		wq->max_xfer_bytes = idxd->max_xfer_bytes;
 		wq->max_batch_size = idxd->max_batch_size;
-		wq->wqcfg = devm_kzalloc(dev, idxd->wqcfg_size, GFP_KERNEL);
+		wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
 		if (!wq->wqcfg)
 			return -ENOMEM;
 	}
 
+	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++) {
 		idxd->engines[i].idxd = idxd;
 		idxd->engines[i].id = i;
@@ -322,6 +353,16 @@ static void idxd_disable_system_pasid(struct idxd_device *idxd)
 
 static void idxd_free(struct idxd_device *idxd)
 {
+	int i;
+
+	if (idxd->wqs) {
+		for (i = 0; i < idxd->max_wqs; i++) {
+			kfree(idxd->wqs[i]->wqcfg);
+			kfree(idxd->wqs[i]);
+		}
+		kfree(idxd->wqs);
+	}
+
 	kfree(idxd);
 }
 
@@ -357,7 +398,7 @@ static int idxd_probe(struct idxd_device *idxd)
 
 	rc = idxd_setup_interrupts(idxd);
 	if (rc)
-		goto err_setup;
+		goto err_int;
 
 	dev_dbg(dev, "IDXD interrupt setup complete.\n");
 
@@ -377,6 +418,8 @@ static int idxd_probe(struct idxd_device *idxd)
  err_idr_fail:
 	idxd_mask_error_interrupts(idxd);
 	idxd_mask_msix_vectors(idxd);
+ err_int:
+	idxd_free(idxd);
  err_setup:
 	if (device_pasid_enabled(idxd))
 		idxd_disable_system_pasid(idxd);
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 55a92dfe0fad..10ce9b5f2951 100644
--- a/drivers/dma/idxd/sysfs.c
+++ b/drivers/dma/idxd/sysfs.c
@@ -26,34 +26,11 @@ static struct device_type idxd_group_device_type = {
 	.release = idxd_conf_sub_device_release,
 };
 
-static struct device_type idxd_wq_device_type = {
-	.name = "wq",
-	.release = idxd_conf_sub_device_release,
-};
-
 static struct device_type idxd_engine_device_type = {
 	.name = "engine",
 	.release = idxd_conf_sub_device_release,
 };
 
-static inline bool is_idxd_wq_dev(struct device *dev)
-{
-	return dev ? dev->type == &idxd_wq_device_type : false;
-}
-
-static inline bool is_idxd_wq_dmaengine(struct idxd_wq *wq)
-{
-	if (wq->type == IDXD_WQT_KERNEL &&
-	    strcmp(wq->name, "dmaengine") == 0)
-		return true;
-	return false;
-}
-
-static inline bool is_idxd_wq_cdev(struct idxd_wq *wq)
-{
-	return wq->type == IDXD_WQT_USER;
-}
-
 static int idxd_config_bus_match(struct device *dev,
 				 struct device_driver *drv)
 {
@@ -302,7 +279,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;
@@ -314,7 +291,7 @@ static int idxd_config_bus_remove(struct device *dev)
 		idxd_unregister_dma_device(idxd);
 		rc = idxd_device_disable(idxd);
 		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);
@@ -683,7 +660,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;
@@ -940,7 +917,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;
 	}
@@ -1336,6 +1313,20 @@ static const struct attribute_group *idxd_wq_attribute_groups[] = {
 	NULL,
 };
 
+static void idxd_conf_wq_release(struct device *dev)
+{
+	struct idxd_wq *wq = container_of(dev, struct idxd_wq, conf_dev);
+
+	kfree(wq->wqcfg);
+	kfree(wq);
+}
+
+struct device_type idxd_wq_device_type = {
+	.name = "wq",
+	.release = idxd_conf_wq_release,
+	.groups = idxd_wq_attribute_groups,
+};
+
 /* IDXD device attribs */
 static ssize_t version_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
@@ -1460,7 +1451,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;
 	}
@@ -1715,29 +1706,28 @@ 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];
 
+		device_initialize(&wq->conf_dev);
 		wq->conf_dev.parent = &idxd->conf_dev;
-		dev_set_name(&wq->conf_dev, "wq%d.%d", idxd->id, wq->id);
 		wq->conf_dev.bus = idxd_get_bus_type(idxd);
-		wq->conf_dev.groups = idxd_wq_attribute_groups;
 		wq->conf_dev.type = &idxd_wq_device_type;
-		dev_dbg(dev, "WQ device register: %s\n",
-			dev_name(&wq->conf_dev));
-		rc = device_register(&wq->conf_dev);
-		if (rc < 0) {
-			put_device(&wq->conf_dev);
+		rc = dev_set_name(&wq->conf_dev, "wq%d.%d", idxd->id, wq->id);
+		if (rc < 0)
+			goto cleanup;
+		dev_dbg(dev, "WQ device register: %s\n", dev_name(&wq->conf_dev));
+		rc = device_add(&wq->conf_dev);
+		if (rc < 0)
 			goto cleanup;
-		}
 	}
 
 	return 0;
 
 cleanup:
 	while (i--) {
-		struct idxd_wq *wq = &idxd->wqs[i];
+		struct idxd_wq *wq = idxd->wqs[i];
 
-		device_unregister(&wq->conf_dev);
+		put_device(&wq->conf_dev);
 	}
 	return rc;
 }
@@ -1807,7 +1797,7 @@ void idxd_unregister_devices(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);
 	}

[PATCH v7 6/8] dmaengine: idxd: fix engine conf_dev lifetime

[Dave Jiang]

The devm managed lifetime is incompatible with 'struct device' objects that
resides in idxd context. This is one of the series that clean up the idxd
driver 'struct device' lifetime. Fix engine->conf_dev 'struct device'
lifetime. Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE.
Add release functions in order to free the allocated memory at the
appropriate time.

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 |    2 +-
 drivers/dma/idxd/idxd.h   |    2 +-
 drivers/dma/idxd/init.c   |   51 +++++++++++++++++++++++++++++++++++++++------
 drivers/dma/idxd/sysfs.c  |   44 ++++++++++++++++++++++-----------------
 4 files changed, 71 insertions(+), 28 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index 2292d7bfef58..d4fa9d2472c1 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -744,7 +744,7 @@ static int idxd_engines_setup(struct idxd_device *idxd)
 	}
 
 	for (i = 0; i < idxd->max_engines; i++) {
-		eng = &idxd->engines[i];
+		eng = idxd->engines[i];
 		group = eng->group;
 
 		if (!group)
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index cd41baeb2bdf..6c217cab383f 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -194,7 +194,7 @@ struct idxd_device {
 	struct completion *cmd_done;
 	struct idxd_group *groups;
 	struct idxd_wq **wqs;
-	struct idxd_engine *engines;
+	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 2aceb99ef9d1..6af896c6909b 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -181,6 +181,37 @@ static int idxd_allocate_wqs(struct idxd_device *idxd)
 	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 = kcalloc_node(idxd->max_engines, sizeof(struct idxd_engine *),
+				     GFP_KERNEL, dev_to_node(dev));
+	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]);
+	kfree(idxd->engines);
+	idxd->engines = NULL;
+	return rc;
+}
+
 static int idxd_setup_internals(struct idxd_device *idxd)
 {
 	struct device *dev = &idxd->pdev->dev;
@@ -217,15 +248,15 @@ static int idxd_setup_internals(struct idxd_device *idxd)
 			return -ENOMEM;
 	}
 
-	idxd->engines = devm_kcalloc(dev, idxd->max_engines,
-				     sizeof(struct idxd_engine), GFP_KERNEL);
-	if (!idxd->engines)
-		return -ENOMEM;
-
+	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));
@@ -363,6 +394,12 @@ static void idxd_free(struct idxd_device *idxd)
 		kfree(idxd->wqs);
 	}
 
+	if (idxd->engines) {
+		for (i = 0; i < idxd->max_engines; i++)
+			kfree(idxd->engines[i]);
+		kfree(idxd->engines);
+	}
+
 	kfree(idxd);
 }
 
diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
index 10ce9b5f2951..021da1752f2a 100644
--- a/drivers/dma/idxd/sysfs.c
+++ b/drivers/dma/idxd/sysfs.c
@@ -26,11 +26,6 @@ static struct device_type idxd_group_device_type = {
 	.release = idxd_conf_sub_device_release,
 };
 
-static struct device_type idxd_engine_device_type = {
-	.name = "engine",
-	.release = idxd_conf_sub_device_release,
-};
-
 static int idxd_config_bus_match(struct device *dev,
 				 struct device_driver *drv)
 {
@@ -469,6 +464,19 @@ static const struct attribute_group *idxd_engine_attribute_groups[] = {
 	NULL,
 };
 
+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_engine_release,
+	.groups = idxd_engine_attribute_groups,
+};
+
 /* Group attributes */
 
 static void idxd_set_free_tokens(struct idxd_device *idxd)
@@ -631,7 +639,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;
@@ -1638,30 +1646,28 @@ 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];
 
+		device_initialize(&engine->conf_dev);
 		engine->conf_dev.parent = &idxd->conf_dev;
-		dev_set_name(&engine->conf_dev, "engine%d.%d",
-			     idxd->id, engine->id);
 		engine->conf_dev.bus = idxd_get_bus_type(idxd);
-		engine->conf_dev.groups = idxd_engine_attribute_groups;
 		engine->conf_dev.type = &idxd_engine_device_type;
-		dev_dbg(dev, "Engine device register: %s\n",
-			dev_name(&engine->conf_dev));
-		rc = device_register(&engine->conf_dev);
-		if (rc < 0) {
-			put_device(&engine->conf_dev);
+		rc = dev_set_name(&engine->conf_dev, "engine%d.%d", idxd->id, engine->id);
+		if (rc < 0)
+			goto cleanup;
+		dev_dbg(dev, "Engine device register: %s\n", dev_name(&engine->conf_dev));
+		rc = device_add(&engine->conf_dev);
+		if (rc < 0)
 			goto cleanup;
-		}
 	}
 
 	return 0;
 
 cleanup:
 	while (i--) {
-		struct idxd_engine *engine = &idxd->engines[i];
+		struct idxd_engine *engine = idxd->engines[i];
 
-		device_unregister(&engine->conf_dev);
+		put_device(&engine->conf_dev);
 	}
 	return rc;
 }
@@ -1803,7 +1809,7 @@ void idxd_unregister_devices(struct idxd_device *idxd)
 	}
 
 	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);
 	}

[PATCH v7 7/8] dmaengine: idxd: fix group conf_dev lifetime

[Dave Jiang]

The devm managed lifetime is incompatible with 'struct device' objects that
resides in idxd context. This is one of the series that clean up the idxd
driver 'struct device' lifetime. Fix group->conf_dev 'struct device'
lifetime. Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE.
Add release functions in order to free the allocated memory at the
appropriate time.

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 |    8 +++----
 drivers/dma/idxd/idxd.h   |    2 +-
 drivers/dma/idxd/init.c   |   55 ++++++++++++++++++++++++++++++++++++++-------
 drivers/dma/idxd/sysfs.c  |   53 ++++++++++++++++++++++---------------------
 4 files changed, 79 insertions(+), 39 deletions(-)

diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
index d4fa9d2472c1..1e5380b2a88c 100644
--- a/drivers/dma/idxd/device.c
+++ b/drivers/dma/idxd/device.c
@@ -624,7 +624,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);
 	}
@@ -712,7 +712,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;
@@ -739,7 +739,7 @@ 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;
 	}
 
@@ -768,7 +768,7 @@ 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;
 	}
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 6c217cab383f..35bb616f04d5 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -192,7 +192,7 @@ struct idxd_device {
 
 	spinlock_t dev_lock;	/* spinlock for device */
 	struct completion *cmd_done;
-	struct idxd_group *groups;
+	struct idxd_group **groups;
 	struct idxd_wq **wqs;
 	struct idxd_engine **engines;
 
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 6af896c6909b..72b859c33937 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -212,22 +212,55 @@ static int idxd_allocate_engines(struct idxd_device *idxd)
 	return rc;
 }
 
-static int idxd_setup_internals(struct idxd_device *idxd)
+static int idxd_allocate_groups(struct idxd_device *idxd)
 {
 	struct device *dev = &idxd->pdev->dev;
+	struct idxd_group *group;
 	int i, rc;
 
-	init_waitqueue_head(&idxd->cmd_waitq);
-	idxd->groups = devm_kcalloc(dev, idxd->max_groups,
-				    sizeof(struct idxd_group), GFP_KERNEL);
+	idxd->groups = kcalloc_node(idxd->max_groups, sizeof(struct idxd_group *),
+				    GFP_KERNEL, dev_to_node(dev));
 	if (!idxd->groups)
 		return -ENOMEM;
 
 	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;
+		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]);
+	kfree(idxd->groups);
+	idxd->groups = NULL;
+	return rc;
+}
+
+static int idxd_setup_internals(struct idxd_device *idxd)
+{
+	struct device *dev = &idxd->pdev->dev;
+	int i, rc;
+
+	init_waitqueue_head(&idxd->cmd_waitq);
+
+	rc = idxd_allocate_groups(idxd);
+	if (rc < 0)
+		return rc;
+
+	for (i = 0; i < idxd->max_groups; i++) {
+		struct idxd_group *group = idxd->groups[i];
+
+		group->idxd = idxd;
+		group->id = i;
+		group->tc_a = -1;
+		group->tc_b = -1;
 	}
 
 	rc = idxd_allocate_wqs(idxd);
@@ -400,6 +433,12 @@ static void idxd_free(struct idxd_device *idxd)
 		kfree(idxd->engines);
 	}
 
+	if (idxd->groups) {
+		for (i = 0; i < idxd->max_groups; i++)
+			kfree(idxd->groups[i]);
+		kfree(idxd->groups);
+	}
+
 	kfree(idxd);
 }
 
diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
index 021da1752f2a..03079ff54889 100644
--- a/drivers/dma/idxd/sysfs.c
+++ b/drivers/dma/idxd/sysfs.c
@@ -16,16 +16,6 @@ static char *idxd_wq_type_names[] = {
 	[IDXD_WQT_USER]		= "user",
 };
 
-static void idxd_conf_sub_device_release(struct device *dev)
-{
-	dev_dbg(dev, "%s for %s\n", __func__, dev_name(dev));
-}
-
-static struct device_type idxd_group_device_type = {
-	.name = "group",
-	.release = idxd_conf_sub_device_release,
-};
-
 static int idxd_config_bus_match(struct device *dev,
 				 struct device_driver *drv)
 {
@@ -440,7 +430,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;
@@ -484,7 +474,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;
 	}
@@ -789,6 +779,19 @@ static const struct attribute_group *idxd_group_attribute_groups[] = {
 	NULL,
 };
 
+static void idxd_conf_group_release(struct device *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_group_release,
+	.groups = idxd_group_attribute_groups,
+};
+
 /* IDXD work queue attribs */
 static ssize_t wq_clients_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
@@ -861,7 +864,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)
@@ -1678,30 +1681,28 @@ 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];
 
+		device_initialize(&group->conf_dev);
 		group->conf_dev.parent = &idxd->conf_dev;
-		dev_set_name(&group->conf_dev, "group%d.%d",
-			     idxd->id, group->id);
 		group->conf_dev.bus = idxd_get_bus_type(idxd);
-		group->conf_dev.groups = idxd_group_attribute_groups;
 		group->conf_dev.type = &idxd_group_device_type;
-		dev_dbg(dev, "Group device register: %s\n",
-			dev_name(&group->conf_dev));
-		rc = device_register(&group->conf_dev);
-		if (rc < 0) {
-			put_device(&group->conf_dev);
+		rc = dev_set_name(&group->conf_dev, "group%d.%d", idxd->id, group->id);
+		if (rc < 0)
+			goto cleanup;
+		dev_dbg(dev, "Group device register: %s\n", dev_name(&group->conf_dev));
+		rc = device_add(&group->conf_dev);
+		if (rc < 0)
 			goto cleanup;
-		}
 	}
 
 	return 0;
 
 cleanup:
 	while (i--) {
-		struct idxd_group *group = &idxd->groups[i];
+		struct idxd_group *group = idxd->groups[i];
 
-		device_unregister(&group->conf_dev);
+		put_device(&group->conf_dev);
 	}
 	return rc;
 }
@@ -1815,7 +1816,7 @@ void idxd_unregister_devices(struct idxd_device *idxd)
 	}
 
 	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);
 	}

[PATCH v7 8/8] dmaengine: idxd: fix cdev setup and free device lifetime issues

[Dave Jiang]

The char device setup and cleanup has device lifetime issues regarding when
parts are initialized and cleaned up. The initialization of struct device is
done incorrectly. device_initialize() needs to be called on the 'struct
device' and then additional changes can be added. The ->release() function
needs to be setup via device_type before dev_set_name() to allow proper
cleanup. The change re-parents the cdev under the wq->conf_dev to get
natural reference inheritance. No known dependency on the old device path exists.

Reported-by: Jason Gunthorpe <jgg@nvidia.com>
Fixes: 42d279f9137a ("dmaengine: idxd: add char driver to expose submission portal to userland")
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/dma/idxd/cdev.c  |  131 +++++++++++++++++-----------------------------
 drivers/dma/idxd/idxd.h  |    7 +-
 drivers/dma/idxd/init.c  |    2 -
 drivers/dma/idxd/irq.c   |    4 +
 drivers/dma/idxd/sysfs.c |    8 ++-
 5 files changed, 63 insertions(+), 89 deletions(-)

diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
index 0db9b82ed8cf..b4ff97f93f13 100644
--- a/drivers/dma/idxd/cdev.c
+++ b/drivers/dma/idxd/cdev.c
@@ -39,15 +39,15 @@ struct idxd_user_context {
 	struct iommu_sva *sva;
 };
 
-enum idxd_cdev_cleanup {
-	CDEV_NORMAL = 0,
-	CDEV_FAILED,
-};
-
 static void idxd_cdev_dev_release(struct device *dev)
 {
-	dev_dbg(dev, "releasing cdev device\n");
-	kfree(dev);
+	struct idxd_cdev *idxd_cdev = container_of(dev, struct idxd_cdev, dev);
+	struct idxd_cdev_context *cdev_ctx;
+	struct idxd_wq *wq = idxd_cdev->wq;
+
+	cdev_ctx = &ictx[wq->idxd->type];
+	ida_simple_remove(&cdev_ctx->minor_ida, idxd_cdev->minor);
+	kfree(idxd_cdev);
 }
 
 static struct device_type idxd_cdev_device_type = {
@@ -62,14 +62,11 @@ static inline struct idxd_cdev *inode_idxd_cdev(struct inode *inode)
 	return container_of(cdev, struct idxd_cdev, cdev);
 }
 
-static inline struct idxd_wq *idxd_cdev_wq(struct idxd_cdev *idxd_cdev)
-{
-	return container_of(idxd_cdev, struct idxd_wq, idxd_cdev);
-}
-
 static inline struct idxd_wq *inode_wq(struct inode *inode)
 {
-	return idxd_cdev_wq(inode_idxd_cdev(inode));
+	struct idxd_cdev *idxd_cdev = inode_idxd_cdev(inode);
+
+	return idxd_cdev->wq;
 }
 
 static int idxd_cdev_open(struct inode *inode, struct file *filp)
@@ -220,11 +217,10 @@ static __poll_t idxd_cdev_poll(struct file *filp,
 	struct idxd_user_context *ctx = filp->private_data;
 	struct idxd_wq *wq = ctx->wq;
 	struct idxd_device *idxd = wq->idxd;
-	struct idxd_cdev *idxd_cdev = &wq->idxd_cdev;
 	unsigned long flags;
 	__poll_t out = 0;
 
-	poll_wait(filp, &idxd_cdev->err_queue, wait);
+	poll_wait(filp, &wq->err_queue, wait);
 	spin_lock_irqsave(&idxd->dev_lock, flags);
 	if (idxd->sw_err.valid)
 		out = EPOLLIN | EPOLLRDNORM;
@@ -246,98 +242,69 @@ int idxd_cdev_get_major(struct idxd_device *idxd)
 	return MAJOR(ictx[idxd->type].devt);
 }
 
-static int idxd_wq_cdev_dev_setup(struct idxd_wq *wq)
+int idxd_wq_add_cdev(struct idxd_wq *wq)
 {
 	struct idxd_device *idxd = wq->idxd;
-	struct idxd_cdev *idxd_cdev = &wq->idxd_cdev;
-	struct idxd_cdev_context *cdev_ctx;
+	struct idxd_cdev *idxd_cdev;
+	struct cdev *cdev;
 	struct device *dev;
-	int minor, rc;
+	struct idxd_cdev_context *cdev_ctx;
+	int rc, minor;
 
-	idxd_cdev->dev = kzalloc(sizeof(*idxd_cdev->dev), GFP_KERNEL);
-	if (!idxd_cdev->dev)
+	idxd_cdev = kzalloc(sizeof(*idxd_cdev), GFP_KERNEL);
+	if (!idxd_cdev)
 		return -ENOMEM;
 
-	dev = idxd_cdev->dev;
-	dev->parent = &idxd->pdev->dev;
-	dev_set_name(dev, "%s/wq%u.%u", idxd_get_dev_name(idxd),
-		     idxd->id, wq->id);
-	dev->bus = idxd_get_bus_type(idxd);
-
+	idxd_cdev->wq = wq;
+	cdev = &idxd_cdev->cdev;
+	dev = &idxd_cdev->dev;
 	cdev_ctx = &ictx[wq->idxd->type];
 	minor = ida_simple_get(&cdev_ctx->minor_ida, 0, MINORMASK, GFP_KERNEL);
 	if (minor < 0) {
-		rc = minor;
-		kfree(dev);
-		goto ida_err;
-	}
-
-	dev->devt = MKDEV(MAJOR(cdev_ctx->devt), minor);
-	dev->type = &idxd_cdev_device_type;
-	rc = device_register(dev);
-	if (rc < 0) {
-		dev_err(&idxd->pdev->dev, "device register failed\n");
-		goto dev_reg_err;
+		kfree(idxd_cdev);
+		return minor;
 	}
 	idxd_cdev->minor = minor;
 
-	return 0;
-
- dev_reg_err:
-	ida_simple_remove(&cdev_ctx->minor_ida, MINOR(dev->devt));
-	put_device(dev);
- ida_err:
-	idxd_cdev->dev = NULL;
-	return rc;
-}
-
-static void idxd_wq_cdev_cleanup(struct idxd_wq *wq,
-				 enum idxd_cdev_cleanup cdev_state)
-{
-	struct idxd_cdev *idxd_cdev = &wq->idxd_cdev;
-	struct idxd_cdev_context *cdev_ctx;
-
-	cdev_ctx = &ictx[wq->idxd->type];
-	if (cdev_state == CDEV_NORMAL)
-		cdev_del(&idxd_cdev->cdev);
-	device_unregister(idxd_cdev->dev);
-	/*
-	 * The device_type->release() will be called on the device and free
-	 * the allocated struct device. We can just forget it.
-	 */
-	ida_simple_remove(&cdev_ctx->minor_ida, idxd_cdev->minor);
-	idxd_cdev->dev = NULL;
-	idxd_cdev->minor = -1;
-}
-
-int idxd_wq_add_cdev(struct idxd_wq *wq)
-{
-	struct idxd_cdev *idxd_cdev = &wq->idxd_cdev;
-	struct cdev *cdev = &idxd_cdev->cdev;
-	struct device *dev;
-	int rc;
+	device_initialize(dev);
+	dev->parent = &wq->conf_dev;
+	dev->bus = idxd_get_bus_type(idxd);
+	dev->type = &idxd_cdev_device_type;
+	dev->devt = MKDEV(MAJOR(cdev_ctx->devt), minor);
 
-	rc = idxd_wq_cdev_dev_setup(wq);
+	rc = dev_set_name(dev, "%s/wq%u.%u", idxd_get_dev_name(idxd),
+			  idxd->id, wq->id);
 	if (rc < 0)
-		return rc;
+		goto err;
 
-	dev = idxd_cdev->dev;
+	wq->idxd_cdev = idxd_cdev;
 	cdev_init(cdev, &idxd_cdev_fops);
-	cdev_set_parent(cdev, &dev->kobj);
-	rc = cdev_add(cdev, dev->devt, 1);
+	rc = cdev_device_add(cdev, dev);
 	if (rc) {
 		dev_dbg(&wq->idxd->pdev->dev, "cdev_add failed: %d\n", rc);
-		idxd_wq_cdev_cleanup(wq, CDEV_FAILED);
-		return rc;
+		goto err;
 	}
 
-	init_waitqueue_head(&idxd_cdev->err_queue);
 	return 0;
+
+ err:
+	put_device(dev);
+	wq->idxd_cdev = NULL;
+	return rc;
 }
 
 void idxd_wq_del_cdev(struct idxd_wq *wq)
 {
-	idxd_wq_cdev_cleanup(wq, CDEV_NORMAL);
+	struct idxd_cdev *idxd_cdev;
+	struct idxd_cdev_context *cdev_ctx;
+
+	cdev_ctx = &ictx[wq->idxd->type];
+	mutex_lock(&wq->wq_lock);
+	idxd_cdev = wq->idxd_cdev;
+	wq->idxd_cdev = NULL;
+	mutex_unlock(&wq->wq_lock);
+	cdev_device_del(&idxd_cdev->cdev, &idxd_cdev->dev);
+	put_device(&idxd_cdev->dev);
 }
 
 int idxd_cdev_register(void)
diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index 35bb616f04d5..acb70c3b7e36 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -79,10 +79,10 @@ enum idxd_wq_type {
 };
 
 struct idxd_cdev {
+	struct idxd_wq *wq;
 	struct cdev cdev;
-	struct device *dev;
+	struct device dev;
 	int minor;
-	struct wait_queue_head err_queue;
 };
 
 #define IDXD_ALLOCATED_BATCH_SIZE	128U
@@ -108,7 +108,8 @@ struct idxd_dma_chan {
 struct idxd_wq {
 	void __iomem *portal;
 	struct device conf_dev;
-	struct idxd_cdev idxd_cdev;
+	struct idxd_cdev *idxd_cdev;
+	struct wait_queue_head err_queue;
 	struct idxd_device *idxd;
 	int id;
 	enum idxd_wq_type type;
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 72b859c33937..7847daa51d4c 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -273,7 +273,7 @@ static int idxd_setup_internals(struct idxd_device *idxd)
 		wq->id = i;
 		wq->idxd = idxd;
 		mutex_init(&wq->wq_lock);
-		wq->idxd_cdev.minor = -1;
+		init_waitqueue_head(&wq->err_queue);
 		wq->max_xfer_bytes = idxd->max_xfer_bytes;
 		wq->max_batch_size = idxd->max_batch_size;
 		wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
index 7b0181532f77..fc0781e3f36d 100644
--- a/drivers/dma/idxd/irq.c
+++ b/drivers/dma/idxd/irq.c
@@ -133,7 +133,7 @@ static int process_misc_interrupts(struct idxd_device *idxd, u32 cause)
 			struct idxd_wq *wq = idxd->wqs[id];
 
 			if (wq->type == IDXD_WQT_USER)
-				wake_up_interruptible(&wq->idxd_cdev.err_queue);
+				wake_up_interruptible(&wq->err_queue);
 		} else {
 			int i;
 
@@ -141,7 +141,7 @@ static int process_misc_interrupts(struct idxd_device *idxd, u32 cause)
 				struct idxd_wq *wq = idxd->wqs[i];
 
 				if (wq->type == IDXD_WQT_USER)
-					wake_up_interruptible(&wq->idxd_cdev.err_queue);
+					wake_up_interruptible(&wq->err_queue);
 			}
 		}
 
diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
index 03079ff54889..8a08988ea9d1 100644
--- a/drivers/dma/idxd/sysfs.c
+++ b/drivers/dma/idxd/sysfs.c
@@ -1174,8 +1174,14 @@ static ssize_t wq_cdev_minor_show(struct device *dev,
 				  struct device_attribute *attr, char *buf)
 {
 	struct idxd_wq *wq = container_of(dev, struct idxd_wq, conf_dev);
+	int minor = -1;
 
-	return sprintf(buf, "%d\n", wq->idxd_cdev.minor);
+	mutex_lock(&wq->wq_lock);
+	if (wq->idxd_cdev)
+		minor = wq->idxd_cdev->minor;
+	mutex_unlock(&wq->wq_lock);
+
+	return sprintf(buf, "%d\n", minor);
 }
 
 static struct device_attribute dev_attr_wq_cdev_minor =

Re: [PATCH v7 8/8] dmaengine: idxd: fix cdev setup and free device lifetime issues

[Dan Williams]

On Mon, Mar 22, 2021 at 4:32 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
> The char device setup and cleanup has device lifetime issues regarding when
> parts are initialized and cleaned up. The initialization of struct device is
> done incorrectly. device_initialize() needs to be called on the 'struct
> device' and then additional changes can be added. The ->release() function
> needs to be setup via device_type before dev_set_name() to allow proper
> cleanup. The change re-parents the cdev under the wq->conf_dev to get
> natural reference inheritance. No known dependency on the old device path exists.
>
> Reported-by: Jason Gunthorpe <jgg@nvidia.com>
> Fixes: 42d279f9137a ("dmaengine: idxd: add char driver to expose submission portal to userland")
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
[..]
> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
> index 03079ff54889..8a08988ea9d1 100644
> --- a/drivers/dma/idxd/sysfs.c
> +++ b/drivers/dma/idxd/sysfs.c
> @@ -1174,8 +1174,14 @@ static ssize_t wq_cdev_minor_show(struct device *dev,
>                                   struct device_attribute *attr, char *buf)
>  {
>         struct idxd_wq *wq = container_of(dev, struct idxd_wq, conf_dev);
> +       int minor = -1;
>
> -       return sprintf(buf, "%d\n", wq->idxd_cdev.minor);
> +       mutex_lock(&wq->wq_lock);
> +       if (wq->idxd_cdev)
> +               minor = wq->idxd_cdev->minor;
> +       mutex_unlock(&wq->wq_lock);
> +
> +       return sprintf(buf, "%d\n", minor);

As I mentioned, let's not emit a negative value here. ...not that
userspace should be using this awkward recreation of the existing core
'dev' attribute anyway.

if (minor == -1)
    return -ENXIO;
return sprintf(buf, "%d\n", minor);

Re: [PATCH v7 8/8] dmaengine: idxd: fix cdev setup and free device lifetime issues

[Dave Jiang]

On 3/22/2021 4:38 PM, Dan Williams wrote:
> On Mon, Mar 22, 2021 at 4:32 PM Dave Jiang <dave.jiang@intel.com> wrote:
>> The char device setup and cleanup has device lifetime issues regarding when
>> parts are initialized and cleaned up. The initialization of struct device is
>> done incorrectly. device_initialize() needs to be called on the 'struct
>> device' and then additional changes can be added. The ->release() function
>> needs to be setup via device_type before dev_set_name() to allow proper
>> cleanup. The change re-parents the cdev under the wq->conf_dev to get
>> natural reference inheritance. No known dependency on the old device path exists.
>>
>> Reported-by: Jason Gunthorpe <jgg@nvidia.com>
>> Fixes: 42d279f9137a ("dmaengine: idxd: add char driver to expose submission portal to userland")
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> [..]
>> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
>> index 03079ff54889..8a08988ea9d1 100644
>> --- a/drivers/dma/idxd/sysfs.c
>> +++ b/drivers/dma/idxd/sysfs.c
>> @@ -1174,8 +1174,14 @@ static ssize_t wq_cdev_minor_show(struct device *dev,
>>                                    struct device_attribute *attr, char *buf)
>>   {
>>          struct idxd_wq *wq = container_of(dev, struct idxd_wq, conf_dev);
>> +       int minor = -1;
>>
>> -       return sprintf(buf, "%d\n", wq->idxd_cdev.minor);
>> +       mutex_lock(&wq->wq_lock);
>> +       if (wq->idxd_cdev)
>> +               minor = wq->idxd_cdev->minor;
>> +       mutex_unlock(&wq->wq_lock);
>> +
>> +       return sprintf(buf, "%d\n", minor);
> As I mentioned, let's not emit a negative value here. ...not that
> userspace should be using this awkward recreation of the existing core
> 'dev' attribute anyway.
>
> if (minor == -1)
>      return -ENXIO;
> return sprintf(buf, "%d\n", minor);
Ok I'll update. This will go away when we convert to UACCE based driver.

Re: [PATCH v7 8/8] dmaengine: idxd: fix cdev setup and free device lifetime issues

[Jason Gunthorpe]

On Mon, Mar 22, 2021 at 04:44:24PM -0700, Dave Jiang wrote:
> 
> On 3/22/2021 4:38 PM, Dan Williams wrote:
> > On Mon, Mar 22, 2021 at 4:32 PM Dave Jiang <dave.jiang@intel.com> wrote:
> > > The char device setup and cleanup has device lifetime issues regarding when
> > > parts are initialized and cleaned up. The initialization of struct device is
> > > done incorrectly. device_initialize() needs to be called on the 'struct
> > > device' and then additional changes can be added. The ->release() function
> > > needs to be setup via device_type before dev_set_name() to allow proper
> > > cleanup. The change re-parents the cdev under the wq->conf_dev to get
> > > natural reference inheritance. No known dependency on the old device path exists.
> > > 
> > > Reported-by: Jason Gunthorpe <jgg@nvidia.com>
> > > Fixes: 42d279f9137a ("dmaengine: idxd: add char driver to expose submission portal to userland")
> > > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > [..]
> > > diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
> > > index 03079ff54889..8a08988ea9d1 100644
> > > +++ b/drivers/dma/idxd/sysfs.c
> > > @@ -1174,8 +1174,14 @@ static ssize_t wq_cdev_minor_show(struct device *dev,
> > >                                    struct device_attribute *attr, char *buf)
> > >   {
> > >          struct idxd_wq *wq = container_of(dev, struct idxd_wq, conf_dev);
> > > +       int minor = -1;
> > > 
> > > -       return sprintf(buf, "%d\n", wq->idxd_cdev.minor);
> > > +       mutex_lock(&wq->wq_lock);
> > > +       if (wq->idxd_cdev)
> > > +               minor = wq->idxd_cdev->minor;
> > > +       mutex_unlock(&wq->wq_lock);
> > > +
> > > +       return sprintf(buf, "%d\n", minor);
> > As I mentioned, let's not emit a negative value here. ...not that
> > userspace should be using this awkward recreation of the existing core
> > 'dev' attribute anyway.
> > 
> > if (minor == -1)
> >      return -ENXIO;
> > return sprintf(buf, "%d\n", minor);
> Ok I'll update. This will go away when we convert to UACCE based driver.

Also ensure all new syfs callbacks are using sysfs_emit()

Jason

Re: [PATCH v7 0/8] idxd 'struct device' lifetime handling fixes

[Vinod Koul]

Hi Dave,

On 22-03-21, 16:31, Dave Jiang wrote:
> v7:
> - Fix up the 'struct device' setup in char device code (Jason)
> - Split out the char dev fixes (Jason)
> - Split out the DMA dev fixes (Dan)
> - Split out the each of the conf_dev fixes
> - Split out removal of the pcim_* calls
> - Split out removal of the devm_* calls
> - Split out the fixes for interrupt config calls
> - Reviewed by Dan.
> 
> v6:
> - Fix char dev initialization issues (Jason)
> - Fix other 'struct device' initialization issues.
> 
> v5:
> - Rebased against 5.12-rc dmaengine/fixes
> v4:
> - fix up the life time of cdev creation/destruction (Jason)
> - Tested with KASAN and other memory allocation leak detections. (Jason)
> 
> v3:
> - Remove devm_* for irq request and cleanup related bits (Jason)
> v2:
> - Remove all devm_* alloc for idxd_device (Jason)
> - Add kref dep for dma_dev (Jason)
> 
> Vinod,
> The series fixes the various 'struct device' lifetime handling in the
> idxd driver. The devm managed lifetime is incompatible with 'struct device'
> objects that resides in the idxd context. Tested with
> CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that.

Sorry for not looking into this earlier.. But I would like to check on
the direction idxd is taking. Somehow I get the feel the dmaengine is
not the right place for this. Considering that now we have auxdev merged
in, should the idxd be spilt into smaller function and no dmaengine
parts moved away from dmaengine... I think we do lack a subsystem for
all things accelerator in kernel atm...

Dan what do you think about splitting idxd?

Thanks
-- 
~Vinod

Re: [PATCH v7 0/8] idxd 'struct device' lifetime handling fixes

[Jason Gunthorpe]

On Tue, Mar 23, 2021 at 05:15:30PM +0530, Vinod Koul wrote:
> Hi Dave,
> 
> On 22-03-21, 16:31, Dave Jiang wrote:
> > v7:
> > - Fix up the 'struct device' setup in char device code (Jason)
> > - Split out the char dev fixes (Jason)
> > - Split out the DMA dev fixes (Dan)
> > - Split out the each of the conf_dev fixes
> > - Split out removal of the pcim_* calls
> > - Split out removal of the devm_* calls
> > - Split out the fixes for interrupt config calls
> > - Reviewed by Dan.
> > 
> > v6:
> > - Fix char dev initialization issues (Jason)
> > - Fix other 'struct device' initialization issues.
> > 
> > v5:
> > - Rebased against 5.12-rc dmaengine/fixes
> > v4:
> > - fix up the life time of cdev creation/destruction (Jason)
> > - Tested with KASAN and other memory allocation leak detections. (Jason)
> > 
> > v3:
> > - Remove devm_* for irq request and cleanup related bits (Jason)
> > v2:
> > - Remove all devm_* alloc for idxd_device (Jason)
> > - Add kref dep for dma_dev (Jason)
> > 
> > Vinod,
> > The series fixes the various 'struct device' lifetime handling in the
> > idxd driver. The devm managed lifetime is incompatible with 'struct device'
> > objects that resides in the idxd context. Tested with
> > CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that.
> 
> Sorry for not looking into this earlier.. But I would like to check on
> the direction idxd is taking. Somehow I get the feel the dmaengine is
> not the right place for this. Considering that now we have auxdev merged
> in, should the idxd be spilt into smaller function and no dmaengine
> parts moved away from dmaengine... I think we do lack a subsystem for
> all things accelerator in kernel atm...

auxdev shouldn't be over-used IMHO.

If the main purpose of the driver is dma engine then it is OK if the
"core" part lives there too.

Jason

Re: [PATCH v7 8/8] dmaengine: idxd: fix cdev setup and free device lifetime issues

[Dave Jiang]

On 3/22/2021 5:02 PM, Jason Gunthorpe wrote:
> On Mon, Mar 22, 2021 at 04:44:24PM -0700, Dave Jiang wrote:
>> On 3/22/2021 4:38 PM, Dan Williams wrote:
>>> On Mon, Mar 22, 2021 at 4:32 PM Dave Jiang <dave.jiang@intel.com> wrote:
>>>> The char device setup and cleanup has device lifetime issues regarding when
>>>> parts are initialized and cleaned up. The initialization of struct device is
>>>> done incorrectly. device_initialize() needs to be called on the 'struct
>>>> device' and then additional changes can be added. The ->release() function
>>>> needs to be setup via device_type before dev_set_name() to allow proper
>>>> cleanup. The change re-parents the cdev under the wq->conf_dev to get
>>>> natural reference inheritance. No known dependency on the old device path exists.
>>>>
>>>> Reported-by: Jason Gunthorpe <jgg@nvidia.com>
>>>> Fixes: 42d279f9137a ("dmaengine: idxd: add char driver to expose submission portal to userland")
>>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>>> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
>>> [..]
>>>> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
>>>> index 03079ff54889..8a08988ea9d1 100644
>>>> +++ b/drivers/dma/idxd/sysfs.c
>>>> @@ -1174,8 +1174,14 @@ static ssize_t wq_cdev_minor_show(struct device *dev,
>>>>                                     struct device_attribute *attr, char *buf)
>>>>    {
>>>>           struct idxd_wq *wq = container_of(dev, struct idxd_wq, conf_dev);
>>>> +       int minor = -1;
>>>>
>>>> -       return sprintf(buf, "%d\n", wq->idxd_cdev.minor);
>>>> +       mutex_lock(&wq->wq_lock);
>>>> +       if (wq->idxd_cdev)
>>>> +               minor = wq->idxd_cdev->minor;
>>>> +       mutex_unlock(&wq->wq_lock);
>>>> +
>>>> +       return sprintf(buf, "%d\n", minor);
>>> As I mentioned, let's not emit a negative value here. ...not that
>>> userspace should be using this awkward recreation of the existing core
>>> 'dev' attribute anyway.
>>>
>>> if (minor == -1)
>>>       return -ENXIO;
>>> return sprintf(buf, "%d\n", minor);
>> Ok I'll update. This will go away when we convert to UACCE based driver.
> Also ensure all new syfs callbacks are using sysfs_emit()
>
Thanks Jason! I have posted a patch that fixes all the instances [1]. 
But I can fix it locally here as well.


[1]: 
https://lore.kernel.org/dmaengine/161495936863.573579.5804720281778882171.stgit@djiang5-desk3.ch.intel.com/

Re: [PATCH v7 0/8] idxd 'struct device' lifetime handling fixes

[Dave Jiang]

On 3/23/2021 4:56 AM, Jason Gunthorpe wrote:
> On Tue, Mar 23, 2021 at 05:15:30PM +0530, Vinod Koul wrote:
>> Hi Dave,
>>
>> On 22-03-21, 16:31, Dave Jiang wrote:
>>> v7:
>>> - Fix up the 'struct device' setup in char device code (Jason)
>>> - Split out the char dev fixes (Jason)
>>> - Split out the DMA dev fixes (Dan)
>>> - Split out the each of the conf_dev fixes
>>> - Split out removal of the pcim_* calls
>>> - Split out removal of the devm_* calls
>>> - Split out the fixes for interrupt config calls
>>> - Reviewed by Dan.
>>>
>>> v6:
>>> - Fix char dev initialization issues (Jason)
>>> - Fix other 'struct device' initialization issues.
>>>
>>> v5:
>>> - Rebased against 5.12-rc dmaengine/fixes
>>> v4:
>>> - fix up the life time of cdev creation/destruction (Jason)
>>> - Tested with KASAN and other memory allocation leak detections. (Jason)
>>>
>>> v3:
>>> - Remove devm_* for irq request and cleanup related bits (Jason)
>>> v2:
>>> - Remove all devm_* alloc for idxd_device (Jason)
>>> - Add kref dep for dma_dev (Jason)
>>>
>>> Vinod,
>>> The series fixes the various 'struct device' lifetime handling in the
>>> idxd driver. The devm managed lifetime is incompatible with 'struct device'
>>> objects that resides in the idxd context. Tested with
>>> CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that.
>> Sorry for not looking into this earlier.. But I would like to check on
>> the direction idxd is taking. Somehow I get the feel the dmaengine is
>> not the right place for this. Considering that now we have auxdev merged
>> in, should the idxd be spilt into smaller function and no dmaengine
>> parts moved away from dmaengine... I think we do lack a subsystem for
>> all things accelerator in kernel atm...
> auxdev shouldn't be over-used IMHO.
>
> If the main purpose of the driver is dma engine then it is OK if the
> "core" part lives there too.

Hi Vinod,

So this patch series serves as the basis of getting the idxd 
dsa_bus_type related bits fixed up so that auxdev is not necessary. When 
Jason reviewed previous iterations of the mdev series, he noted that the 
mdev driver needs to go under VFIO. After the auxdev conversion of the 
mdev series, Jason and Dan after further review suggested that given 
there's an internal bus in idxd driver already (dsa_bus_type), that can 
be used to load drivers rather than needing to rely on auxiliary bus. 
But the implementation of the dsa_bus_type needs some fixes. After this 
series, I have another series that's going through internal review right 
now that will fixup the driver setup and initialization of dsa_bus_type 
and allow us to load external drivers for the wq. The in kernel use 
cases for DSA is still valid under dmaengine so the core parts remains 
valid for dmaengine. The plan going forward is after getting all the 
fixups in we are planning to:

1. Introduce UACCE framework support for idxd and have a wq driver 
resides under drivers/misc/uacce/idxd to support the char device 
operations and deprecate the current custom char dev in idxd. This 
should remove the burden on you to deal with the char device.

2. Resubmit the mdev driver under drivers/vfio/mdev/idxd after Jason's 
latest VFIO refactoring is done.

3. Introduce new kernel use cases with dmanegine API support for SVA 
operations.

Let me know if this sounds ok to you. Thanks!

Re: [PATCH v7 0/8] idxd 'struct device' lifetime handling fixes

[Jason Gunthorpe]

On Tue, Mar 23, 2021 at 08:44:29AM -0700, Dave Jiang wrote:

> 1. Introduce UACCE framework support for idxd and have a wq driver resides
> under drivers/misc/uacce/idxd to support the char device operations and
> deprecate the current custom char dev in idxd. This should remove the burden
> on you to deal with the char device.

Gah, I feel I already complained at Intel for cramming their own
private char devices into subsystems! *subsystems* define the user
API, not random drivers in them.

uacce is a reasonable place to put something like this if there isn't
a multi-driver standard

If this is the plan we should block of the char dev under
CONFIG_EXPERIMENTAL or something to discourage people using the uAPI
we are planning to delete

Jason

Re: [PATCH v7 0/8] idxd 'struct device' lifetime handling fixes

[Dave Jiang]

On 3/24/2021 6:56 AM, Jason Gunthorpe wrote:
> On Tue, Mar 23, 2021 at 08:44:29AM -0700, Dave Jiang wrote:
>
>> 1. Introduce UACCE framework support for idxd and have a wq driver resides
>> under drivers/misc/uacce/idxd to support the char device operations and
>> deprecate the current custom char dev in idxd. This should remove the burden
>> on you to deal with the char device.
> Gah, I feel I already complained at Intel for cramming their own
> private char devices into subsystems! *subsystems* define the user
> API, not random drivers in them.
>
> uacce is a reasonable place to put something like this if there isn't
> a multi-driver standard
>
> If this is the plan we should block of the char dev under
> CONFIG_EXPERIMENTAL or something to discourage people using the uAPI
> we are planning to delete
The whole reason to move to UACCE is to relieve Vinod the burden of 
having to review that code under dmaengine. It was unfortunate that 
UACCE showed up a kernel version later after the idxd driver was 
accepted. Do you have a better suggestion?

Re: [PATCH v7 0/8] idxd 'struct device' lifetime handling fixes

[Jason Gunthorpe]

On Wed, Mar 24, 2021 at 07:57:57AM -0700, Dave Jiang wrote:
> 
> On 3/24/2021 6:56 AM, Jason Gunthorpe wrote:
> > On Tue, Mar 23, 2021 at 08:44:29AM -0700, Dave Jiang wrote:
> > 
> > > 1. Introduce UACCE framework support for idxd and have a wq driver resides
> > > under drivers/misc/uacce/idxd to support the char device operations and
> > > deprecate the current custom char dev in idxd. This should remove the burden
> > > on you to deal with the char device.
> > Gah, I feel I already complained at Intel for cramming their own
> > private char devices into subsystems! *subsystems* define the user
> > API, not random drivers in them.
> > 
> > uacce is a reasonable place to put something like this if there isn't
> > a multi-driver standard
> > 
> > If this is the plan we should block of the char dev under
> > CONFIG_EXPERIMENTAL or something to discourage people using the uAPI
> > we are planning to delete
> The whole reason to move to UACCE is to relieve Vinod the burden of having
> to review that code under dmaengine. It was unfortunate that UACCE showed up
> a kernel version later after the idxd driver was accepted. Do you have a
> better suggestion?

No, I said it is a reasonable thing to do

Jason

Re: [PATCH v7 0/8] idxd 'struct device' lifetime handling fixes

[Vinod Koul]

On 23-03-21, 08:44, Dave Jiang wrote:
> On 3/23/2021 4:56 AM, Jason Gunthorpe wrote:
> > On Tue, Mar 23, 2021 at 05:15:30PM +0530, Vinod Koul wrote:

> > > > Vinod,
> > > > The series fixes the various 'struct device' lifetime handling in the
> > > > idxd driver. The devm managed lifetime is incompatible with 'struct device'
> > > > objects that resides in the idxd context. Tested with
> > > > CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that.
> > > Sorry for not looking into this earlier.. But I would like to check on
> > > the direction idxd is taking. Somehow I get the feel the dmaengine is
> > > not the right place for this. Considering that now we have auxdev merged
> > > in, should the idxd be spilt into smaller function and no dmaengine
> > > parts moved away from dmaengine... I think we do lack a subsystem for
> > > all things accelerator in kernel atm...
> > auxdev shouldn't be over-used IMHO.
> > 
> > If the main purpose of the driver is dma engine then it is OK if the
> > "core" part lives there too.
> 
> Hi Vinod,
> 
> So this patch series serves as the basis of getting the idxd dsa_bus_type
> related bits fixed up so that auxdev is not necessary. When Jason reviewed
> previous iterations of the mdev series, he noted that the mdev driver needs
> to go under VFIO. After the auxdev conversion of the mdev series, Jason and
> Dan after further review suggested that given there's an internal bus in
> idxd driver already (dsa_bus_type), that can be used to load drivers rather
> than needing to rely on auxiliary bus. But the implementation of the
> dsa_bus_type needs some fixes. After this series, I have another series
> that's going through internal review right now that will fixup the driver
> setup and initialization of dsa_bus_type and allow us to load external
> drivers for the wq. The in kernel use cases for DSA is still valid under
> dmaengine so the core parts remains valid for dmaengine. The plan going
> forward is after getting all the fixups in we are planning to:
> 
> 1. Introduce UACCE framework support for idxd and have a wq driver resides
> under drivers/misc/uacce/idxd to support the char device operations and
> deprecate the current custom char dev in idxd. This should remove the burden
> on you to deal with the char device.
> 
> 2. Resubmit the mdev driver under drivers/vfio/mdev/idxd after Jason's
> latest VFIO refactoring is done.
> 
> 3. Introduce new kernel use cases with dmanegine API support for SVA
> operations.
> 
> Let me know if this sounds ok to you. Thanks!

Yes that does sound reasonable to me, when should I expect this move to
show up on list?

-- 
~Vinod

Re: [PATCH v7 0/8] idxd 'struct device' lifetime handling fixes

[Dave Jiang]

On 3/24/2021 11:28 PM, Vinod Koul wrote:
> On 23-03-21, 08:44, Dave Jiang wrote:
>> On 3/23/2021 4:56 AM, Jason Gunthorpe wrote:
>>> On Tue, Mar 23, 2021 at 05:15:30PM +0530, Vinod Koul wrote:
>>>>> Vinod,
>>>>> The series fixes the various 'struct device' lifetime handling in the
>>>>> idxd driver. The devm managed lifetime is incompatible with 'struct device'
>>>>> objects that resides in the idxd context. Tested with
>>>>> CONFIG_DEBUG_KOBJECT_RELEASE and address all issues from that.
>>>> Sorry for not looking into this earlier.. But I would like to check on
>>>> the direction idxd is taking. Somehow I get the feel the dmaengine is
>>>> not the right place for this. Considering that now we have auxdev merged
>>>> in, should the idxd be spilt into smaller function and no dmaengine
>>>> parts moved away from dmaengine... I think we do lack a subsystem for
>>>> all things accelerator in kernel atm...
>>> auxdev shouldn't be over-used IMHO.
>>>
>>> If the main purpose of the driver is dma engine then it is OK if the
>>> "core" part lives there too.
>> Hi Vinod,
>>
>> So this patch series serves as the basis of getting the idxd dsa_bus_type
>> related bits fixed up so that auxdev is not necessary. When Jason reviewed
>> previous iterations of the mdev series, he noted that the mdev driver needs
>> to go under VFIO. After the auxdev conversion of the mdev series, Jason and
>> Dan after further review suggested that given there's an internal bus in
>> idxd driver already (dsa_bus_type), that can be used to load drivers rather
>> than needing to rely on auxiliary bus. But the implementation of the
>> dsa_bus_type needs some fixes. After this series, I have another series
>> that's going through internal review right now that will fixup the driver
>> setup and initialization of dsa_bus_type and allow us to load external
>> drivers for the wq. The in kernel use cases for DSA is still valid under
>> dmaengine so the core parts remains valid for dmaengine. The plan going
>> forward is after getting all the fixups in we are planning to:
>>
>> 1. Introduce UACCE framework support for idxd and have a wq driver resides
>> under drivers/misc/uacce/idxd to support the char device operations and
>> deprecate the current custom char dev in idxd. This should remove the burden
>> on you to deal with the char device.
>>
>> 2. Resubmit the mdev driver under drivers/vfio/mdev/idxd after Jason's
>> latest VFIO refactoring is done.
>>
>> 3. Introduce new kernel use cases with dmanegine API support for SVA
>> operations.
>>
>> Let me know if this sounds ok to you. Thanks!
> Yes that does sound reasonable to me, when should I expect this move to
> show up on list?

We will try to do this in stages. So first we need to get this 'struct 
device' lifetime fixes series into the kernel. Next I have the 'struct 
device_driver' setup/shutdown series fix that Dan is reviewing 
internally right now that I will post as soon as he okays it. I also 
have the uacce conversion series ready and pending Dan's review. The 
mdev series I need to rebase it to Jason's new VFIO code refactor. Some 
of that refactor code is not yet posted public (I think?). So that will 
take a little longer. For the kernel SVA support, I have internal code 
but need to discuss with Dan on the implementation. Also we need to 
measure performance on hardware to make our case for the new kernel 
usage enablings. So that will come later.