Re: [PATCH v8 02/10] PCI: dwc: Expose endpoint DMA resources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] PCI: dwc: stale edma->dw in dw_pcie_ep_get_aux_resources() after eDMA removal
--

commit 9475ea2f70e5c9c7b09a2bc537c0f97a6e428f7f
Author: Koichiro Den <den@valinux.co.jp>

PCI: dwc: Expose endpoint DMA resources

This commit exposes the endpoint-integrated eDMA register window and
linked-list memory regions via the EPC auxiliary resource API. It also
checks that the local dw-edma device is registered and rejects virtual
function queries.

> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 630ba172280c2..7166ec7f212bd 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
[ ... ]
> @@ -865,14 +874,30 @@ dw_pcie_ep_get_aux_resources_count(struct pci_epc *epc, u8 func_no,
>  	struct dw_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>  	struct dw_edma_chip *edma = &pci->edma;
> +	u16 ll_wr_cnt, ll_rd_cnt;
> +	int count = 0;
> +	int ret;
>  
>  	if (!pci->edma_reg_size)
>  		return 0;
>  
> -	if (edma->db_offset == ~0)
> +	ret = dw_pcie_ep_check_edma_vfunc(vfunc_no);
> +	if (ret)
> +		return ret;
> +
> +	if (!edma->dw)
>  		return 0;
>  
> -	return 1;
> +	ll_wr_cnt = edma->ll_wr_cnt;
> +	ll_rd_cnt = edma->ll_rd_cnt;
> +
> +	if (!edma->cfg_non_ll)
> +		count += 1 + ll_wr_cnt + ll_rd_cnt;
> +
> +	if (edma->db_offset != ~0)
> +		count++;
> +
> +	return count;
>  }
>  
>  static int
> @@ -885,14 +910,32 @@ dw_pcie_ep_get_aux_resources(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>  	const struct pci_epc_bar_rsvd_region *rsvd;
>  	struct dw_edma_chip *edma = &pci->edma;
>  	enum pci_barno dma_ctrl_bar = NO_BAR;
> -	resource_size_t db_offset = edma->db_offset;
> +	resource_size_t db_offset;
>  	resource_size_t dma_ctrl_bar_offset = 0;
>  	resource_size_t dma_reg_size;
> -	int count;
> +	u16 ll_wr_cnt, ll_rd_cnt;
> +	bool has_ll;
> +	unsigned int i;
> +	int count, ret;
>  
> -	count = dw_pcie_ep_get_aux_resources_count(epc, func_no, vfunc_no);
> -	if (count < 0)
> -		return count;
> +	if (!pci->edma_reg_size)
> +		return 0;
> +
> +	ret = dw_pcie_ep_check_edma_vfunc(vfunc_no);
> +	if (ret)
> +		return ret;
> +
> +	if (!edma->dw)
> +		return 0;
> +
> +	ll_wr_cnt = edma->ll_wr_cnt;
> +	ll_rd_cnt = edma->ll_rd_cnt;
> +	db_offset = edma->db_offset;
> +	has_ll = !edma->cfg_non_ll;
> +
> +	count = db_offset != ~0;
> +	if (has_ll)
> +		count += 1 + ll_wr_cnt + ll_rd_cnt;

[Severity: Medium]
Can edma->dw remain a non-NULL stale pointer here after the local dw-edma
device has been unregistered?

Both dw_pcie_ep_get_aux_resources_count() and dw_pcie_ep_get_aux_resources()
check edma->dw to verify that the local dw-edma device is registered before
exposing PCI_EPC_AUX_DMA_CTRL_MMIO and PCI_EPC_AUX_DMA_DESC_MEM resources.

However, when the eDMA device is torn down via dw_pcie_edma_remove() ->
dw_edma_remove(), either on the err_remove_edma error path in
dw_pcie_ep_init_registers():

drivers/pci/controller/dwc/pcie-designware-ep.c:dw_pcie_ep_init_registers() {
	...
err_remove_edma:
	dw_pcie_edma_remove(pci);

	return ret;
}

or during fundamental reset in dw_pcie_ep_cleanup():

drivers/pci/controller/dwc/pcie-designware-ep.c:dw_pcie_ep_cleanup() {
	...
	dwc_pcie_debugfs_deinit(pci);
	dw_pcie_edma_remove(pci);
}

dw_edma_remove() in drivers/dma/dw-edma/dw-edma-core.c unregisters the DMA
device (and resets chip->db_offset = ~0 via dw_edma_emul_irq_free()), but
neither dw_edma_remove() nor dw_pcie_edma_remove() clears chip->dw:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_remove() {
	...
	/* Deregister eDMA device */
	dma_async_device_unregister(&dw->dma);
	list_for_each_entry_safe(chan, _chan, &dw->dma.channels,
				 vc.chan.device_node) {
		tasklet_kill(&chan->vc.task);
		list_del(&chan->vc.chan.device_node);
	}

	return err;
}

If an endpoint function driver subsequently queries
pci_epc_get_aux_resources_count() and pci_epc_get_aux_resources() while the
eDMA device is unregistered, wouldn't the !edma->dw check pass and expose
DMA control MMIO and linked-list descriptor resources for a removed eDMA
instance?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022082230.3164834-1-den@valinux.co.jp?part=2