Re: [PATCH v3 04/23] dmaengine: sdxi: Feature discovery and initial configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing explicit case for `SDXI_GSV_STOP` in `sdxi_dev_stop()`, causing an unintended hardware reset and a race condition during initialization.
- [Medium] Failure to escalate soft stop to hard stop on timeout breaks kexec recovery.
--

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

dmaengine: sdxi: Feature discovery and initial configuration

This commit discovers device capabilities, introduces register definitions,
and performs initial configuration. It also attempts to force the SDXI
function to a stopped state during initialization to recover from kexec or
prior driver bugs.

> diff --git a/drivers/dma/sdxi/device.c b/drivers/dma/sdxi/device.c
> index 0974a83bb45ca..7c6652f9c3c04 100644
> --- a/drivers/dma/sdxi/device.c
> +++ b/drivers/dma/sdxi/device.c
[ ... ]
> +/* Get the device to the GSV_STOP state. */
> +static int sdxi_dev_stop(struct sdxi_dev *sdxi)
> +{
> +	enum sdxi_fn_gsv status = sdxi_dev_gsv(sdxi);
> +	int ret;
> +
> +	dev_dbg(sdxi->dev, "attempting stop, current state: %s\n",
> +		gsv_str(status));
> +
> +	switch (status) {
> +	case SDXI_GSV_INIT:
> +	case SDXI_GSV_ACTIVE:
> +		sdxi_write_fn_gsr(sdxi, SDXI_GSRV_STOP_SF);
> +		break;
> +	case SDXI_GSV_STOPG_SF:
> +		sdxi_write_fn_gsr(sdxi, SDXI_GSRV_STOP_HD);
> +		break;
> +	case SDXI_GSV_STOPG_HD:
> +	case SDXI_GSV_ERROR:
> +		/*
> +		 * If hard-stopping, there's nothing to do but wait.
> +		 * If in error state, the reset is issued below.
> +		 */
> +		break;
> +	default:
> +		/* Unrecognized state; try a reset. */
> +		sdxi_write_fn_gsr(sdxi, SDXI_GSRV_RESET);
> +		break;
> +	}

[Severity: High]
Does this switch statement need an explicit case for SDXI_GSV_STOP?

If the device is already in the stopped state (which is the expected
post-reset state), the switch appears to fall through to the default block
and unnecessarily issues a hardware reset.

Immediately after the reset command is written, sdxi_dev_gsv_poll() reads
the status register. Since hardware resets take time to execute, the very
first read will likely still sample the old SDXI_GSV_STOP state. This
could cause the poll loop to exit immediately with success while the reset
is still actively running in the background.

Since the caller proceeds to configure critical control registers right
after this returns, could this lead to lost writes or corrupted state while
the hardware is resetting?

> +
> +	/* Wait for transition to either stop or error state. */
> +	ret = sdxi_dev_gsv_poll(sdxi, status,
> +				status == SDXI_GSV_STOP ||
> +				status == SDXI_GSV_ERROR);
> +
> +	if (ret == 0 && status == SDXI_GSV_ERROR) {
> +		sdxi_write_fn_gsr(sdxi, SDXI_GSRV_RESET);
> +		ret = sdxi_dev_gsv_poll(sdxi, status, status == SDXI_GSV_STOP);
> +	}
> +
> +	if (ret) {
> +		dev_err(sdxi->dev, "stop timed out, current state: %s\n",
> +			gsv_str(status));
> +		return ret;
> +	}

[Severity: Medium]
If a soft stop times out, should this escalate to a hard stop or reset
instead of returning an error?

The commit message mentions forcing the device to a stopped state to
recover from kexec or driver bugs. However, if the device is in
SDXI_GSV_ACTIVE and the soft stop (SDXI_GSRV_STOP_SF) times out (perhaps
due to pending PCIe transactions hanging after a crash), this simply returns
-ETIMEDOUT.

Since probe is not automatically retried, does this leave the hardware
stuck in SDXI_GSV_STOPG_SF and break the intended kexec recovery?

> +
> +	return 0;
> +}
[ ... ]

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