Re: [PATCH v2 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] Hardware state machine mismanagement and TOCTOU race during device initialization.
- [Low] Use of 64-bit MMIO accessors without proper fallback causes build failures on 32-bit architectures.
--

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

dmaengine: sdxi: Feature discovery and initial configuration

Discover via the capability registers the doorbell region stride, the
maximum supported context ID, the operation groups implemented, and
limits on buffer and control structure sizes. The driver has the
option of writing more conservative limits to the ctl2 register, but
it uses those supplied by the implementation for now.

> diff --git a/drivers/dma/sdxi/device.c b/drivers/dma/sdxi/device.c
> index b718ce04afa07..f9a9944ad8929 100644
> --- a/drivers/dma/sdxi/device.c
> +++ b/drivers/dma/sdxi/device.c
[ ... ]
> @@ -5,14 +5,180 @@
> +/* 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;
> +	}

If the device is already in the SDXI_GSV_STOP state, does the switch fall
through to the default case and unintentionally issue a reset?

The subsequent read_poll_timeout() check would then immediately return success
because the state evaluates as STOP before the background reset actually begins.
Could this allow the reset to proceed in the background while the driver
continues with initialization, potentially wiping the configuration?

[ ... ]
> +/*
> + * See SDXI 1.0 4.1.8 Activation of the SDXI Function by Software.
> + */
> +static int sdxi_fn_activate(struct sdxi_dev *sdxi)
> +{
> +	u64 version, cap0, cap1, ctl2;
> +	int err;
> +
> +	/*
> +	 * Clear any existing configuration from MMIO_CTL0 and ensure
> +	 * the function is in GSV_STOP state.
> +	 */
> +	sdxi_write64(sdxi, SDXI_MMIO_CTL0, 0);
> +	err = sdxi_dev_stop(sdxi);

Does writing 0 to SDXI_MMIO_CTL0 implicitly write SDXI_GSRV_RESET (value 0)
to the FN_GSR command field?

If so, sdxi_dev_stop() might read a stale device state while the hardware is
processing the reset, and overwrite the command field with a conflicting command
like SDXI_GSRV_STOP_SF. Can this lead to a hardware state machine conflict?

[ ... ]
> diff --git a/drivers/dma/sdxi/sdxi.h b/drivers/dma/sdxi/sdxi.h
> index d4c61ca2f875f..84b87066f438d 100644
> --- a/drivers/dma/sdxi/sdxi.h
> +++ b/drivers/dma/sdxi/sdxi.h
[ ... ]
> @@ -30,9 +34,24 @@ struct sdxi_dev {
> +static inline u64 sdxi_read64(const struct sdxi_dev *sdxi, enum sdxi_reg reg)
> +{
> +	return ioread64(sdxi->ctrl_regs + reg);
> +}
> +
> +static inline void sdxi_write64(struct sdxi_dev *sdxi, enum sdxi_reg reg, u64 val)
> +{
> +	iowrite64(val, sdxi->ctrl_regs + reg);
> +}

Can this cause build failures on 32-bit architectures?

The functions use ioread64() and iowrite64(), which typically rely on readq()
and writeq() that are not defined on 32-bit platforms.

Although linux/io-64-nonatomic-lo-hi.h is included at the top of the file,
the code does not use the macros actually provided by that header
(e.g., ioread64_lo_hi()).

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