Re: [PATCH v3 1/2] dmaengine: dw-edma: Enable Chan Separation via VSEC

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The parsed channel separation size (`ch_space_sz`) is never propagated to the core driver, rendering the commit functionally dead.
- [Medium] The newly added `ch_space_sz` field implicitly defaults to 0 instead of the documented 256 bytes for all non-B00F devices.
--

--- Patch [1]: [PATCH v3 1/2] dmaengine: dw-edma: Enable Chan Separation via VSEC ---
commit 49b6417fbdbe084d5839664db3abac6f79f818d2
Author: Devendra K Verma <[email protected]>

dmaengine: dw-edma: Enable Chan Separation via VSEC

This commit adds support for parsing the channel separation size from the
DEVICE INFORMATION VSEC capability on the Xilinx B00F device. It aims to
allow the user to configure the separation between DMA channel registers.

> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> index 791c46e8ae4c4..5d7507a640565 100644
> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c

[ ... ]

> @@ -73,6 +75,7 @@ struct dw_edma_pcie_data {
>  	u16				wr_ch_cnt;
>  	u16				rd_ch_cnt;
>  	u64				devmem_phys_off;
> +	u32				ch_space_sz;
>  };

[Severity: Medium]
Does this new field implicitly default to 0 for non-B00F devices?

The commit message states "Default supported size is 256B", but static
structs like snps_edda_data and xilinx_mdb_data lack explicit
initialization for ch_space_sz, meaning they will be zero-initialized.

If this value is later used for channel offset calculations, could a 0-byte
separation cause all DMA channels to map to the exact same memory address?

[ ... ]

> @@ -279,6 +289,10 @@ static void dw_edma_pcie_get_xilinx_dma_data(struct pci_dev *pdev,
>  	pdata->mf = map;
>  	pdata->rg.bar = FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_BAR, val);
>  
> +	if (pdev->device == PCI_DEVICE_ID_XILINX_B00F)
> +		pdata->ch_space_sz = dw_edma_get_ch_space_sz
> +					(FIELD_GET(DW_PCIE_XILINX_CPM6_VSEC_CH_SEP, val));
> +

[Severity: Medium]
Is this parsed value supposed to be propagated to the core driver?

It appears pdata->ch_space_sz is populated here, but looking at
dw_edma_pcie_probe(), it doesn't seem to be passed along to
struct dw_edma_chip before the VSEC data goes out of scope:

drivers/dma/dw-edma/dw-edma-pcie.c:dw_edma_pcie_probe() {
	...
	chip->mf = vsec_data->mf;
	chip->nr_irqs = nr_irqs;
	chip->ops = &dw_edma_pcie_plat_ops;
	chip->cfg_non_ll = non_ll;
	/* missing chip->ch_space_sz = vsec_data->ch_space_sz ? */
	...
}

Without this propagation, does the channel separation configuration
actually take effect?

>  	pci_read_config_dword(pdev, vsec + 0xc, &val);
>  	pdata->wr_ch_cnt = min(pdata->wr_ch_cnt,
>  			       FIELD_GET(DW_PCIE_XILINX_MDB_VSEC_DMA_WR_CH, val));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1