[PATCH 0/4] usb: gadget: pch_udc: order IRQ and descriptor lifetimes

[PATCH 0/4] usb: gadget: pch_udc: order IRQ and descriptor lifetimes

[Myeonghun Pak]

These fixes are part of our ongoing research while reviewing kernel code.

Apply after the published probe-cleanup and VBUS-work patches:
https://lore.kernel.org/all/20260811-pch-v1-1-f79801c1cd91@gmail.com/
https://lore.kernel.org/all/CAGEsz8GQR2vJ8ddedaLg=Yq_RoeTxgoXTQ4-QS8mJpDXNn36qA@mail.gmail.com/

1. Drain works and release IRQ handlers before cleanup; establish a final
   controller interrupt mask after the PCI handler has finished.
2. Initialize DMA before IRQ registration; enable bus mastering afterwards.
3. Track successful EP0 mappings and return -EIO on mapping failure.
4. Return the EP0 data descriptor to its owning pool.

Compile-tested only.

[PATCH 1/4] usb: gadget: pch_udc: release IRQ handlers before DMA cleanup

[Myeonghun Pak]

Cleanup frees endpoint descriptors and DMA pools while the managed
PCI handler is still registered. Drain the VBUS works and release
the GPIO and PCI handlers before freeing DMA resources on removal
and probe failure. Initialize both works before GPIO registration
so every shared cleanup path can disable and drain them.

Mask the controller before releasing the PCI IRQ, then mask it again
after the handler has finished: an already running enumeration
handler can otherwise re-enable interrupt sources during teardown.
Record the PCI IRQ only after a successful request.

This issue was identified as part of our ongoing research while
reviewing kernel code.

Fixes: f646cf94520e ("USB device driver of Topcliff PCH")
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
Compile-tested only.

 drivers/usb/gadget/udc/pch_udc.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
index f5ac2b8056d2..3b5c5c41c38c 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -1372,8 +1372,6 @@ static int pch_vbus_gpio_init(struct pch_udc_dev *dev)
 
 	dev->vbus_gpio.port = NULL;
 	dev->vbus_gpio.intr = 0;
-	INIT_WORK(&dev->vbus_gpio.irq_work_fall, pch_vbus_gpio_work_fall);
-	INIT_WORK(&dev->vbus_gpio.irq_work_rise, pch_vbus_gpio_work_rise);
 
 	/* Retrieve the GPIO line from the USB gadget device */
 	gpiod = devm_gpiod_get_optional(d, NULL, GPIOD_IN);
@@ -2833,6 +2831,9 @@ static int pch_udc_pcd_init(struct pch_udc_dev *dev)
 	pch_udc_init(dev);
 	pch_udc_pcd_reinit(dev);
 
+	INIT_WORK(&dev->vbus_gpio.irq_work_fall, pch_vbus_gpio_work_fall);
+	INIT_WORK(&dev->vbus_gpio.irq_work_rise, pch_vbus_gpio_work_rise);
+
 	ret = pch_vbus_gpio_init(dev);
 	if (ret)
 		pch_udc_exit(dev);
@@ -2979,6 +2980,15 @@ static void pch_udc_shutdown(struct pci_dev *pdev)
 
 static void pch_udc_cleanup(struct pch_udc_dev *dev)
 {
+	pch_vbus_gpio_free(dev);
+	pch_udc_exit(dev);
+	if (dev->gadget.irq) {
+		devm_free_irq(&dev->pdev->dev, dev->gadget.irq, dev);
+		/* The handler may have re-enabled interrupt sources. */
+		pch_udc_exit(dev);
+		dev->gadget.irq = 0;
+	}
+
 	/* dma pool cleanup */
 	dma_pool_destroy(dev->data_requests);
 
@@ -3000,10 +3010,6 @@ static void pch_udc_cleanup(struct pch_udc_dev *dev)
 	if (dev->dma_addr)
 		dma_unmap_single(&dev->pdev->dev, dev->dma_addr,
 				 UDC_EP0OUT_BUFF_SIZE * 4, DMA_FROM_DEVICE);
-
-	pch_vbus_gpio_free(dev);
-
-	pch_udc_exit(dev);
 }
 
 static void pch_udc_remove(struct pci_dev *pdev)
@@ -3091,6 +3097,8 @@ static int pch_udc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 		goto finished;
 	}
 
+	dev->gadget.irq = pdev->irq;
+
 	pci_set_master(pdev);
 	pci_try_set_mwi(pdev);
 
@@ -3113,6 +3121,8 @@ static int pch_udc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	return 0;
 
 finished:
+	disable_work_sync(&dev->vbus_gpio.irq_work_fall);
+	disable_work_sync(&dev->vbus_gpio.irq_work_rise);
 	pch_udc_cleanup(dev);
 	return retval;
 }

[PATCH 2/4] usb: gadget: pch_udc: initialize DMA before registering interrupt sources

[Myeonghun Pak]

VBUS GPIO work can reconnect the controller before probe has
allocated its EP0 descriptors. Initialize the lock, endpoint state
and DMA resources before registering either interrupt source.
Keep PCI bus mastering disabled until the PCI IRQ is registered.

This issue was identified as part of our ongoing research while
reviewing kernel code.

Fixes: 637b78eb31e0 ("usb: gadget: pch_udc: Detecting VBUS through GPIO with interrupt")
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
Compile-tested only.

 drivers/usb/gadget/udc/pch_udc.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
index 3b5c5c41c38c..c360bc556cdc 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -2819,25 +2819,14 @@ static void pch_udc_pcd_reinit(struct pch_udc_dev *dev)
 /**
  * pch_udc_pcd_init() - This API initializes the driver structure
  * @dev:	Reference to the driver structure
- *
- * Return codes:
- *	0:		Success
- *	-ERRNO:		All kind of errors when retrieving VBUS GPIO
  */
-static int pch_udc_pcd_init(struct pch_udc_dev *dev)
+static void pch_udc_pcd_init(struct pch_udc_dev *dev)
 {
-	int ret;
-
 	pch_udc_init(dev);
 	pch_udc_pcd_reinit(dev);
 
 	INIT_WORK(&dev->vbus_gpio.irq_work_fall, pch_vbus_gpio_work_fall);
 	INIT_WORK(&dev->vbus_gpio.irq_work_rise, pch_vbus_gpio_work_rise);
-
-	ret = pch_vbus_gpio_init(dev);
-	if (ret)
-		pch_udc_exit(dev);
-	return ret;
 }
 
 /**
@@ -3082,10 +3071,15 @@ static int pch_udc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	dev->base_addr = pcim_iomap_table(pdev)[dev->bar];
 
+	spin_lock_init(&dev->lock);
+	dev->gadget.ops = &pch_udc_ops;
+
 	/* initialize the hardware */
-	retval = pch_udc_pcd_init(dev);
+	pch_udc_pcd_init(dev);
+
+	retval = init_dma_pools(dev);
 	if (retval)
-		return retval;
+		goto finished;
 
 	pci_enable_msi(pdev);
 
@@ -3102,11 +3096,7 @@ static int pch_udc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	pci_set_master(pdev);
 	pci_try_set_mwi(pdev);
 
-	/* device struct setup */
-	spin_lock_init(&dev->lock);
-	dev->gadget.ops = &pch_udc_ops;
-
-	retval = init_dma_pools(dev);
+	retval = pch_vbus_gpio_init(dev);
 	if (retval)
 		goto finished;
 

[PATCH 3/4] usb: gadget: pch_udc: track successful EP0 DMA mappings

[Myeonghun Pak]

DMA address zero is valid, and a failed dma_map_single() does not
create a mapping to unmap. Track successful mapping explicitly so
cleanup handles both cases correctly. Return -EIO on mapping failure
instead of returning the positive dma_mapping_error() result.

This issue was identified as part of our ongoing research while
reviewing kernel code.

Fixes: f646cf94520e ("USB device driver of Topcliff PCH")
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
Compile-tested only.

 drivers/usb/gadget/udc/pch_udc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
index c360bc556cdc..38b9159e2461 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -328,6 +328,7 @@ struct pch_vbus_gpio_data {
  * @data_requests:	DMA pool for data requests
  * @stp_requests:	DMA pool for setup requests
  * @dma_addr:		DMA pool for received
+ * @dma_mapped:		EP0 receive buffer has a valid DMA mapping
  * @setup_data:		Received setup data
  * @base_addr:		for mapped device memory
  * @bar:		PCI BAR used for mapped device memory
@@ -351,6 +352,7 @@ struct pch_udc_dev {
 	struct dma_pool		*data_requests;
 	struct dma_pool		*stp_requests;
 	dma_addr_t			dma_addr;
+	bool				dma_mapped;
 	struct usb_ctrlrequest		setup_data;
 	void __iomem			*base_addr;
 	unsigned short			bar;
@@ -2887,7 +2889,10 @@ static int init_dma_pools(struct pch_udc_dev *dev)
 	dev->dma_addr = dma_map_single(&dev->pdev->dev, ep0out_buf,
 				       UDC_EP0OUT_BUFF_SIZE * 4,
 				       DMA_FROM_DEVICE);
-	return dma_mapping_error(&dev->pdev->dev, dev->dma_addr);
+	if (dma_mapping_error(&dev->pdev->dev, dev->dma_addr))
+		return -EIO;
+	dev->dma_mapped = true;
+	return 0;
 }
 
 static int pch_udc_start(struct usb_gadget *g,
@@ -2996,7 +3001,7 @@ static void pch_udc_cleanup(struct pch_udc_dev *dev)
 		dma_pool_destroy(dev->stp_requests);
 	}
 
-	if (dev->dma_addr)
+	if (dev->dma_mapped)
 		dma_unmap_single(&dev->pdev->dev, dev->dma_addr,
 				 UDC_EP0OUT_BUFF_SIZE * 4, DMA_FROM_DEVICE);
 }

[PATCH 4/4] usb: gadget: pch_udc: free the EP0 data descriptor to its owning pool

[Myeonghun Pak]

The EP0 OUT data descriptor is allocated from data_requests, but
cleanup destroys that pool before passing the descriptor to
stp_requests. Return it to data_requests before destroying the
owning pool. Keep the setup descriptor on stp_requests.

This issue was identified as part of our ongoing research while
reviewing kernel code.

Fixes: f646cf94520e ("USB device driver of Topcliff PCH")
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
Compile-tested only.

 drivers/usb/gadget/udc/pch_udc.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
index 38b9159e2461..b07da0fdcdfc 100644
--- a/drivers/usb/gadget/udc/pch_udc.c
+++ b/drivers/usb/gadget/udc/pch_udc.c
@@ -2984,20 +2984,21 @@ static void pch_udc_cleanup(struct pch_udc_dev *dev)
 	}
 
 	/* dma pool cleanup */
-	dma_pool_destroy(dev->data_requests);
+	if (dev->data_requests) {
+		if (dev->ep[UDC_EP0OUT_IDX].td_data)
+			dma_pool_free(dev->data_requests,
+				      dev->ep[UDC_EP0OUT_IDX].td_data,
+				      dev->ep[UDC_EP0OUT_IDX].td_data_phys);
+		dma_pool_destroy(dev->data_requests);
+	}
 
 	if (dev->stp_requests) {
-		/* cleanup DMA desc's for ep0in */
+		/* Free the EP0 OUT setup descriptor. */
 		if (dev->ep[UDC_EP0OUT_IDX].td_stp) {
 			dma_pool_free(dev->stp_requests,
 				dev->ep[UDC_EP0OUT_IDX].td_stp,
 				dev->ep[UDC_EP0OUT_IDX].td_stp_phys);
 		}
-		if (dev->ep[UDC_EP0OUT_IDX].td_data) {
-			dma_pool_free(dev->stp_requests,
-				dev->ep[UDC_EP0OUT_IDX].td_data,
-				dev->ep[UDC_EP0OUT_IDX].td_data_phys);
-		}
 		dma_pool_destroy(dev->stp_requests);
 	}