Re: [PATCH v1 1/4] smartpqi: Fix AIO retry marker cleared by SCSI core between dispatches.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] State leakage across blk-mq tag reuse leads to erroneous drive offlining. By registering `.init_cmd_priv`, the driver opts out of the SCSI core's per-dispatch zeroing of private command data, but fails to reliably clear `this_residual` on all completion paths.
--

--- Patch [1]: [PATCH v1 1/4] smartpqi: Fix AIO retry marker cleared by SCSI core between dispatches. ---
commit 594ef5db683084e131dde7930f0e556c7584b609
Author: David Strahan <David.Strahan@microchip.com>

smartpqi: Fix AIO retry marker cleared by SCSI core between dispatches.

This commit fixes an AIO retry loop issue by registering a per-command
initialization callback with the SCSI core to skip the per-dispatch clear.
This allows the retry marker to survive requeues, while the driver takes
over the responsibility of clearing it on terminal completion.

> diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
> index 5ec583dc2e7df..3a75b9fbedf45 100644
> --- a/drivers/scsi/smartpqi/smartpqi_init.c
> +++ b/drivers/scsi/smartpqi/smartpqi_init.c
> @@ -66,6 +66,12 @@ static struct pqi_cmd_priv *pqi_cmd_priv(struct scsi_cmnd *cmd)
>  	return scsi_cmd_priv(cmd);
>  }
>  
> +static int pqi_init_cmd_priv(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
> +{
> +	memset(pqi_cmd_priv(cmd), 0, sizeof(struct pqi_cmd_priv));
> +	return 0;
> +}
> +

[ ... ]

> @@ -5958,6 +5964,17 @@ void pqi_prep_for_scsi_done(struct scsi_cmnd *scmd)
>  	struct pqi_scsi_dev *device;
>  	struct completion *wait;
>  
> +	/*
> +	 * Clear the AIO-retry marker on final completion so the tag
> +	 * starts clean on its next dispatch.  On DID_IMM_RETRY leave
> +	 * it intact: pqi_aio_io_complete() sets DID_IMM_RETRY and
> +	 * bumps the marker to steer the requeue onto the RAID path,
> +	 * and pqi_process_raid_io_error() consumes the non-zero
> +	 * marker to offline a misbehaving drive.
> +	 */
> +	if (host_byte(scmd->result) != DID_IMM_RETRY)
> +		pqi_cmd_priv(scmd)->this_residual = 0;
> +

[Severity: High]
Does this reliably clear the marker on all completion paths?

If a command is aborted via the SCSI error handler, it can be terminated
directly by the SCSI midlayer via scsi_finish_command(), which bypasses
pqi_prep_for_scsi_done().

Because pqi_init_cmd_priv() prevents the core from zeroing the private data
on dispatch, a reused blk-mq tag could inherit this stale marker state.

If the dirty tag is subsequently used for a request to a physical drive and
encounters an ILLEGAL_REQUEST, it appears pqi_process_raid_io_error() could
misinterpret the leaked marker and erroneously offline a healthy drive:

pqi_process_raid_io_error() {
    ...
    if (pqi_cmd_priv(scmd)->this_residual &&
        !pqi_is_logical_device(scmd->device->hostdata) &&
        scsi_status == SAM_STAT_CHECK_CONDITION &&
        host_byte == DID_OK &&
        sense_data_length &&
        scsi_normalize_sense(error_info->data, sense_data_length, &sshdr) &&
        sshdr.sense_key == ILLEGAL_REQUEST &&
        sshdr.asc == 0x26 &&
        sshdr.ascq == 0x0) {
            host_byte = DID_NO_CONNECT;
            pqi_take_device_offline(scmd->device, "AIO");
    ...
}

Could this lead to unintended drive offlining if the tag is reused?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722220401.6357-1-david.strahan@microchip.com?part=1