Re: [PATCH v3 09/23] dmaengine: sdxi: Start functions on probe, stop on remove

[sashiko-bot@kernel.org]

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

New issues:
- [High] Device continues DMA into freed memory if activation fails during probe or if soft-stop times out during driver removal.
- [High] Missing `pci_free_irq_vectors()` in the device remove callback and error paths leaks IRQ resources.

Pre-existing issues:
- [High] Missing switch case in `sdxi_dev_stop()` for `SDXI_GSV_STOP` causes a race condition and spurious reset.
--

commit fce324da74dfb9a79bfe266368af63dc843a0386
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: sdxi: Start functions on probe, stop on remove

This commit drives each SDXI function to the active state during probe by
writing to the GSR register and polling for the GSV_ACTIVE state. It also
introduces an unregister function to stop the device during driver removal.

> diff --git a/drivers/dma/sdxi/device.c b/drivers/dma/sdxi/device.c
> index 4d595e79b8ce5..e8f087f758fde 100644
> --- a/drivers/dma/sdxi/device.c
> +++ b/drivers/dma/sdxi/device.c

[ ... ]

> +static int sdxi_dev_start(struct sdxi_dev *sdxi)
> +{
> +	enum sdxi_fn_gsv status = sdxi_dev_gsv(sdxi);
> +	int ret;

[ ... ]

> +	sdxi_write_fn_gsr(sdxi, SDXI_GSRV_ACTIVE);
> +
> +	ret = sdxi_dev_gsv_poll(sdxi, status,
> +				status == SDXI_GSV_ACTIVE ||
> +				status == SDXI_GSV_ERROR);
> +	if (ret) {
> +		dev_err(sdxi->dev, "activation timed out, current state: %s\n",
> +			gsv_str(status));
> +		return ret;
> +	}

[Severity: High]
Is there a risk of DMA use-after-free here?

If the device fails to reach the ACTIVE state within the timeout, this returns
-ETIMEDOUT without forcing the device to a hard stop or reset.

Since the error propagates up and causes the probe to fail, devres will
subsequently free DMA-mapped memory such as the L1 and L2 tables. If the
hardware state machine is still running and eventually activates, could it
continue to perform DMA into the freed memory?

[ ... ]

> @@ -290,6 +330,14 @@ int sdxi_register(struct device *dev, const struct sdxi_bus_ops *ops)
>  }
>  EXPORT_SYMBOL_NS_GPL(sdxi_register, "SDXI");
>  
> +void sdxi_unregister(struct device *dev)
> +{
> +	struct sdxi_dev *sdxi = dev_get_drvdata(dev);
> +
> +	sdxi_dev_stop(sdxi);
> +}

[Severity: High]
Does this safely handle a timeout during stop?

Since the return value of sdxi_dev_stop() is ignored, the unbind process
will continue and free memory even if the hardware refused to stop. This
seems like it could also allow the device to continue DMA into freed memory.

[Severity: High]
This is a pre-existing issue, but looking at sdxi_dev_stop() itself, could
the missing SDXI_GSV_STOP case cause a spurious reset?

If sdxi_dev_stop() is called when the device is already in the STOP state:

drivers/dma/sdxi/device.c:sdxi_dev_stop() {
    ...
    switch (status) {
    case SDXI_GSV_INIT:
    case SDXI_GSV_ACTIVE:
        sdxi_write_fn_gsr(sdxi, SDXI_GSRV_STOP_SF);
        break;
    default:
        /* Unrecognized state; try a reset. */
        sdxi_write_fn_gsr(sdxi, SDXI_GSRV_RESET);
        break;
    }

    /* Wait for transition to either stop or error state. */
    ret = sdxi_dev_gsv_poll(sdxi, status,
                            status == SDXI_GSV_STOP ||
                            status == SDXI_GSV_ERROR);
    ...
}

The status falls through to the default case, writing SDXI_GSRV_RESET to the
hardware. The read_poll_timeout() then immediately reads the STOP state
(before the asynchronous reset takes effect) and returns success.

Could the subsequent initialization steps then execute concurrently with the
hardware reset, causing undefined device behavior?

> diff --git a/drivers/dma/sdxi/pci.c b/drivers/dma/sdxi/pci.c
> index 42e8af008b100..4d7d6812da6d8 100644
> --- a/drivers/dma/sdxi/pci.c
> +++ b/drivers/dma/sdxi/pci.c

[ ... ]

> +static void sdxi_pci_remove(struct pci_dev *pdev)
> +{
> +	pci_disable_sriov(pdev);
> +	sdxi_unregister(&pdev->dev);
> +}

[Severity: High]
Does this leak the IRQ vectors allocated during probe?

The driver allocates IRQ vectors using pci_alloc_irq_vectors() in
sdxi_pci_init(), but this remove callback completes without calling
pci_free_irq_vectors().

Similarly, if sdxi_device_init() fails in sdxi_register(), the error is
returned directly without freeing the IRQ vectors.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-sdxi-base-v3-0-4d38ca2bdffe@amd.com?part=9