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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The claimed 'Channel Separation' feature is completely unused and dead code, failing to pass the parsed separation size to the core DMA driver and causing hardware register offset mismatches on non-default configurations.
- [Medium] The `ch_sep_sz` field defaults to 0 for non-XILINX_B00F devices, violating the 256B default size specification.

Pre-existing issues:
- [High] Double free risk of IRQ vectors due to mixing managed `pcim_enable_device()` with manual `pci_free_irq_vectors()`.
--

--- Patch [2]: [PATCH v2 2/3] dmaengine: dw-edma: Enable Chan Separation via VSEC ---
commit c9662ac63af55f746e4aad35cf3b04bea4688738
Author: Devendra K Verma <[email protected]>

dmaengine: dw-edma: Enable Chan Separation via VSEC

As per, 'Designware Cores PCI Express DM Controller - Reference
Manual', section 3.2.34.3, VSEC for DEVICE INFORMATION supports
the channel separation mechanisms. Basically, the HDMA IP allows
the user to configure the separation between DMA channel
registers and retrieve it via the VSEC capability mentioned
above.

> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> index ec5e057a0f11b..d0f209082878d 100644
> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c

[ ... ]

> @@ -73,6 +76,7 @@ struct dw_edma_pcie_data {
>  	u16				wr_ch_cnt;
>  	u16				rd_ch_cnt;
>  	u64				devmem_phys_off;
> +	u32				ch_sep_sz;

[Severity: Medium]
Does this new field need to be initialized in the existing static
configuration structures?

For example, snps_edda_data and xilinx_mdb_data are not updated in this
patch to set ch_sep_sz. Because they are static structs, ch_sep_sz will
default to 0 for non-XILINX_B00F devices.

If ch_sep_sz is passed to the core driver, would a 0-byte separation cause
all channels to overlap in memory, violating the 256B default size
specification?

>  };
>  
>  static const struct dw_edma_pcie_data snps_edda_data = {

[ ... ]

> @@ -279,6 +290,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_VSEC_DMA_BAR, val);
>  
> +	if (pdev->device == PCI_DEVICE_ID_XILINX_B00F)
> +		pdata->ch_sep_sz = dw_edma_get_ch_sep_sz(FIELD_GET(DW_PCIE_XILINX_CPM6_VSEC_CH_SEP,
> +								   val));
> +

[Severity: High]
Is the parsed channel separation size actually used by the core driver?

It looks like ch_sep_sz is populated in vsec_data here, but is never
passed to the core DMA driver in dw_edma_pcie_probe():

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;
    chip->ll_wr_cnt = vsec_data->wr_ch_cnt;
    ...
}

Additionally, does the core driver support this dynamic separation? Looking at
__dw_ch_regs() in dw-hdma-v0-core.c, the channel register offsets appear to be
calculated using fixed C structure array indexing:

    if (dir == EDMA_DIR_WRITE)
        return &(__dw_regs(dw)->ch[ch].wr);
    else
        return &(__dw_regs(dw)->ch[ch].rd);

If a 32KB separation is configured, wouldn't the core driver still calculate
MMIO offsets assuming the hardcoded structure layout, potentially reading or
writing incorrect PCI memory on non-default configurations?

>  	pci_read_config_dword(pdev, vsec + 0xc, &val);

[ ... ]

> @@ -324,9 +339,9 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  	struct dw_edma_pcie_data *pdata = (void *)pid->driver_data;
>  	struct device *dev = &pdev->dev;
>  	struct dw_edma_chip *chip;
> +	bool non_ll = false;
>  	int err, nr_irqs;
>  	int i, mask;
> -	bool non_ll = false;
>  

[Severity: High]
This is a pre-existing issue, but I noticed a potential double free of the
IRQ vectors related to the device probing.

When pcim_enable_device() is used, IRQ allocations via
pci_alloc_irq_vectors() are managed by devres and automatically freed.

However, looking at the remove path in dw_edma_pcie_remove():

    /* Freeing IRQs */
    pci_free_irq_vectors(pdev);

Does calling pci_free_irq_vectors() manually conflict with the devres cleanup
registered automatically by pcim_enable_device(), leading to a double free
when the driver is unbound or fails to probe?

>  	if (!pdata)
>  		return -ENODEV;

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