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

[sashiko-bot@kernel.org]

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

commit 220e302470d02f37d10b32b890cae93390897f07
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 via the EPC auxiliary resource API, and
gates auxiliary resource reporting on virtual function checks and
local dw-edma registration.

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

[Severity: Medium]
Can edma->dw remain non-NULL after the local dw-edma device is removed,
causing dw_pcie_ep_get_aux_resources_count() and
dw_pcie_ep_get_aux_resources() to report PCI_EPC_AUX_DMA_CTRL_MMIO and
PCI_EPC_AUX_DMA_DESC_MEM resources for an unregistered DMA engine?

dw_edma_probe() sets chip->dw = dw when registering the device, and
dw_edma_emul_irq_free() resets chip->db_offset = ~0 on teardown, but
neither dw_edma_remove() nor dw_pcie_edma_remove() clears chip->dw.
If dw_pcie_ep_init_registers() fails after dw_pcie_edma_detect() and
unwinds via err_remove_edma -> dw_pcie_edma_remove(pci), or if
dw_pcie_ep_cleanup() calls dw_pcie_edma_remove(pci) on a fundamental
reset, edma->dw is left holding a stale pointer and the !edma->dw check
still succeeds.

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