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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The parsed channel separation size (`ch_sep_sz`) is a dead store; it is never passed to the core DMA driver.
- [Medium] dw_edma_get_ch_sep_sz() silently falls back to 256 bytes for unsupported hardware channel separation values.
- [Medium] The newly added `ch_sep_sz` field is left uninitialized (implicitly 0) for statically defined devices.
--

--- Patch [2]: [PATCH v1 2/3] dmaengine: dw-edma: Enable Chan Separation via VSEC ---
commit 5d0ec977534da50b8d8937caafb8d91a21d54565
Author: Devendra K Verma <devverma@amd.com>

dmaengine: dw-edma: Enable Chan Separation via VSEC

This commit aims to enable channel separation via the VSEC capability for
DEVICE INFORMATION. It parses the channel separation size from the hardware
registers up to 4K.

> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> index ec5e057a0f11b..6295d01ba2f71 100644
> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
[ ... ]
> @@ -73,6 +74,7 @@ struct dw_edma_pcie_data {
>  	u16				wr_ch_cnt;
>  	u16				rd_ch_cnt;
>  	u64				devmem_phys_off;
> +	u32				ch_sep_sz;
>  };
>  
>  static const struct dw_edma_pcie_data snps_edda_data = {

[Severity: Medium]
Does this leave the newly added ch_sep_sz field uninitialized (implicitly 0)
for statically defined devices like snps_edda_data? 

If the core driver begins relying on this field to space out channel registers, 
will preexisting devices calculate an offset of 0 for all channels and break 
DMA functionality?

[ ... ]
> @@ -189,6 +191,23 @@ static int dw_edma_pcie_irq_vector(struct device *dev, unsigned int nr)
>  	return pci_irq_vector(to_pci_dev(dev), nr);
>  }
>  
> +static u32 dw_edma_get_ch_sep_sz(u32 ch_sep_val)
> +{
> +	switch (ch_sep_val) {
> +	default:
> +	case 0:
> +		return 256;

[Severity: Medium]
If a hardware device advertises a channel separation size greater than 4K
(e.g., 8K/0x5, 16K/0x6, 32K/0x7 as mentioned in the commit message), will
this switch statement silently hit the default case and return 256 bytes
instead of the actual hardware size, leading to incorrect memory offsets?

> +	case 1:
> +		return 512;
> +	case 2:
> +		return 1024;
> +	case 3:
> +		return 2048;
> +	case 4:
> +		return 4096;
> +	}
> +}
[ ... ]
> @@ -324,9 +345,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;
>  
>  	if (!pdata)
>  		return -ENODEV;

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

The VSEC capability is parsed into pdata->ch_sep_sz in this patch, but it
doesn't appear to be copied to the struct dw_edma_chip or used anywhere
to calculate register offsets, leaving the feature non-functional.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722111640.1146212-1-devverma@amd.com?part=2