Re: [PATCH v2 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:
- [Critical] Missing device stop on activation timeout leads to a DMA Use-After-Free.
- [Critical] `sdxi_unregister()` ignores `sdxi_dev_stop()` timeout errors, leading to a DMA Use-After-Free.
- [High] `sdxi_dev_stop()` inadvertently resets the device if it is already in the `SDXI_GSV_STOP` state, leading to a silent initialization race condition.
--

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

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

Following admin context setup in the previous patch, drive each SDXI
function to active state during probe. Introduce sdxi_unregister() to
stop the function during remove and wire it up via the pci_driver
.remove callback.

> diff --git a/drivers/dma/sdxi/device.c b/drivers/dma/sdxi/device.c
> index 9d8729b626858..204841afa5b77 100644
> --- a/drivers/dma/sdxi/device.c
> +++ b/drivers/dma/sdxi/device.c
> @@ -89,6 +89,42 @@ static void sdxi_write_fn_gsr(struct sdxi_dev *sdxi, enum sdxi_fn_gsr cmd)
>  	sdxi_write64(sdxi, SDXI_MMIO_CTL0, ctl0);
>  }
>  
> +/*
> + * Transition the function from stopped state to active.
> + * See SDXI 1.0 4.1 SDXI Function State.
> + */
> +static int sdxi_dev_start(struct sdxi_dev *sdxi)
> +{
> +	enum sdxi_fn_gsv status = sdxi_dev_gsv(sdxi);
> +	int ret;
> +
> +	if (status != SDXI_GSV_STOP) {
> +		dev_err(sdxi->dev,
> +			"can't activate busy device (unexpected gsv: %s)\n",
> +			gsv_str(status));
> +		return -EBUSY;
> +	}
> +
> +	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;

If this activation times out, could the hardware still complete its
activation asynchronously?

Returning an error here causes the probe to fail, and the devres framework
will unwind allocations in LIFO order, freeing the DMA-mapped L1_table
and L2_table before the PCI device is disabled and its Bus Master bit
is cleared.

If the hardware eventually completes activation and is not explicitly halted
in this error path, could it perform DMA operations to the freed physical
pages?

> +	}
> +
> +	if (status == SDXI_GSV_ERROR) {
> +		dev_err(sdxi->dev, "went to error state during activation\n");
> +		return -EIO;
> +	}
> +
> +	dev_dbg(sdxi->dev, "activated\n");
> +	return 0;
> +}

[ ... ]

> @@ -281,3 +321,10 @@ int sdxi_register(struct device *dev, const struct sdxi_bus_ops *ops)
>  
>  	return sdxi_device_init(sdxi);
>  }
> +
> +void sdxi_unregister(struct device *dev)
> +{
> +	struct sdxi_dev *sdxi = dev_get_drvdata(dev);
> +
> +	sdxi_dev_stop(sdxi);

If the soft stop (SDXI_GSRV_STOP_SF) in sdxi_dev_stop() times out, it returns
an error without escalating to a hard stop or reset.

Since sdxi_unregister() ignores this return value, the removal process will
proceed and devres will free all DMA allocations while the device might remain
active with its Bus Master bit enabled.

Could this cause background DMA transfers into freed memory if the device
gets stuck in a state like SDXI_GSV_STOPG_SF?


Also looking at sdxi_dev_stop(), if the device is already in the
SDXI_GSV_STOP state, the switch statement lacks a case for it:

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

This causes execution to fall through to the default case and issue a
hardware reset. The subsequent polling loop immediately reads the
SDXI_GSV_STOP state and returns success before the hardware actually
begins processing the reset.

Because sdxi_fn_activate() calls sdxi_dev_stop() just before configuring
registers like SDXI_MMIO_CTL2, could the hardware reset asynchronously in
the background and wipe these configurations?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-sdxi-base-v2-0-889cfed17e3f@amd.com?part=9