Re: [PATCH 1/2] scsi: scsi_lib: add spinup_notify callback for ASC/ASCQ=0x04/0x11

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `spinup_notify` API lacks context documentation, risking `scheduling while atomic` bugs in hardware drivers.
- [High] Unmitigated concurrent `spinup_notify` invocations could flood LLDD hardware queues.
--

commit 171fafeb5fbd92de731525186cc8f26d24a7be8e
Author: Xingui Yang <yangxingui@huawei.com>

scsi: scsi_lib: add spinup_notify callback for ASC/ASCQ=0x04/0x11

When a SCSI device returns NOT_READY with ASC/ASCQ = 0x04/0x11 ("notify
(enable spinup) required"), the device is in Active_Wait or Idle_Wait power
state. Add an optional spinup_notify callback to struct scsi_host_template
that is invoked before ACTION_DELAYED_RETRY.

> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index daeb3693fe559..140c41a2fe03b 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -905,6 +905,10 @@ static void scsi_io_completion_action(struct scsi_cmnd *cmd, int result)
>  				case 0x1a: /* start stop unit in progress */
>  				case 0x1b: /* sanitize in progress */
>  				case 0x1d: /* configuration in progress */
> +					if (sshdr.ascq == 0x11 &&
> +					    cmd->device->host->hostt->spinup_notify)
> +						cmd->device->host->hostt->spinup_notify(
> +							cmd->device);

[Severity: High]
Could this cause a regression by flooding hardware queues with concurrent
spinup_notify invocations?

If multiple async commands are in flight when a device goes into a standby
state, they will all fail and return not ready with ascq == 0x11 almost
simultaneously.

Since scsi_io_completion_action() invokes spinup_notify unconditionally for
every command, this could cause a thundering herd.

For example, in the subsequent patch for hisi_sas, this triggers concurrent,
unlocked read-modify-write operations on the SL_CONTROL register:

hisi_sas_v3_hw.c:sl_notify_ssp_v3_hw() {
    ...
    sl_control |= SL_CONTROL_NOTIFY_EN_MSK;
    hisi_sas_phy_write32(hisi_hba, phy_no, SL_CONTROL, sl_control);
    ...
}

>  					action = ACTION_DELAYED_RETRY;
>  					break;
>  				case 0x0a: /* ALUA state transition */
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 7e2011830ba4b..500fad5ffab68 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -374,6 +374,15 @@ struct scsi_host_template {
>  #define SCSI_ADAPTER_RESET	1
>  #define SCSI_FIRMWARE_RESET	2
>  
> +	/*
> +	 * Optional callback invoked when a device returns NOT_READY with
> +	 * ASC/ASCQ = 0x04/0x11 ("notify (enable spinup) required").
> +	 * This allows LLDDs to perform controller-specific spinup
> +	 * notification before the mid-layer retries.
> +	 *
> +	 * Status: OPTIONAL
> +	 */
> +	void (*spinup_notify)(struct scsi_device *sdev);

[Severity: High]
Should this document the context constraints to prevent scheduling while
atomic regressions?

Since scsi_io_completion_action() is called from atomic context, any driver
implementing this callback must not sleep.

Without this documentation, it is easy to introduce bugs. For instance, the
subsequent patch in this series calls msleep(1) inside sl_notify_ssp_v3_hw()
via this callback:

hisi_sas_v3_hw.c:sl_notify_ssp_v3_hw() {
    ...
    msleep(1);
    ...
}

>  
>  	/*
>  	 * Name of proc directory

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702035724.2059166-1-yangxingui@huawei.com?part=1