Re: [PATCH v7 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: edma->dw remains non-NULL after dw_pcie_edma_remove()
--

commit c234b2198cdf6bace1b7fdf24f49b374e8b18f28
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 descriptor memory regions through the EPC auxiliary resource API.
It also updates the auxiliary resource queries to reject VF requests and check
whether the local dw-edma device is registered before reporting resources.

> 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
> @@ -858,6 +858,15 @@ dw_pcie_ep_find_bar_rsvd_region(struct dw_pcie_ep *ep,
>  	return NULL;
>  }
>  
> +static int dw_pcie_ep_check_edma_vfunc(u8 vfunc_no)
> +{
> +	/*
> +	 * The DWC endpoint databook says it is not possible to assign the
> +	 * DMA/HDMA registers to any Virtual Function.
> +	 */
> +	return vfunc_no ? -EOPNOTSUPP : 0;
> +}
> +
>  static int
>  dw_pcie_ep_get_aux_resources_count(struct pci_epc *epc, u8 func_no,
>  				   u8 vfunc_no)
> @@ -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;

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

Both dw_pcie_ep_get_aux_resources_count() and dw_pcie_ep_get_aux_resources()
in drivers/pci/controller/dwc/pcie-designware-ep.c check edma->dw to verify
that the local dw-edma device is registered before reporting DMA resources.

However, when the eDMA device is unregistered via dw_pcie_edma_remove() ->
dw_edma_remove(), such as in dw_pcie_ep_cleanup() during a fundamental reset
or on the err_remove_edma error path in dw_pcie_ep_init_registers():

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

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

	return ret;
}

dw_edma_remove() in drivers/dma/dw-edma/dw-edma-core.c tears down the IRQs,
workqueue, and DMA device without clearing 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;
}

Because edma->dw and edma->ll_wr_cnt / edma->ll_rd_cnt remain populated after
dw_pcie_edma_remove(), won't subsequent calls to
dw_pcie_ep_get_aux_resources_count() and dw_pcie_ep_get_aux_resources() pass
the !edma->dw check and continue reporting PCI_EPC_AUX_DMA_CTRL_MMIO and
PCI_EPC_AUX_DMA_DESC_MEM resources for an unregistered dw-edma device?

[ ... ]

> @@ -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;

[ ... ]

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