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

[Nathan Lynch <nathan.lynch@amd.com>]

sashiko-bot@kernel.org writes:
> 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?

No, this is ok as written; when the function is in GSV_STOP, it ignores
any value written to fn_gsr except for SDXI_GSRV_ACTIVE.

>> +/*
>> + * 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?

I don't think there's a state machine conflict (see Figure 4-1: SDXI
Function States and State Transitions in the spec). But we probably
shouldn't change the fn_pasid_vl or fn_err_intr_en fields in the
register unless the function is already in stopped state.

Another reason to be more careful here is that we shouldn't clear
MMIO_CTL0's fn_grp_id field, which is software-assignable for function
grouping, and is read-only for VFs.


>> @@ -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()).

I decided to just make the driver depend on 64BIT and use readq/writeq.
We can take up 32-bit build coverage later.