Re: [PATCH v4] scsi: libsas: Fix SMP IO deadlock during HA resume

[John Garry]

On 9/28/26 05:02, Xingui Yang wrote:
> smp_execute_task_sg() calls pm_runtime_get_sync() on the host before
> issuing an SMP command. When that command is itself issued from the
> HA resume path, the get_sync() deadlocks: it waits for the ongoing
> resume (the device is RPM_RESUMING), while the resume is blocked in
> sas_drain_work() waiting for that same SMP IO to complete.
>
> The deadlock needs an expander-attached SATA disk. 

What do you mean by "deadlock needs an expander-attached SATA disk?

> During
> sas_resume_ha() -> sas_drain_work(), DISCE_RESUME ->
> sas_resume_sata() -> ata_sas_port_resume() requests ATA_EH_RESET,
> and the hard reset for such a disk is done via SMP PHY CONTROL
> (sas_ata_hard_reset() -> sas_phy_reset() -> sas_smp_phy_control() ->
> smp_execute_task_sg()). Direct-attached SATA resets through
> lldd_control_phy() and SSP devices use TMFs, so neither hits this.
>
> Replace the get_sync()/put_sync() pair with
> pm_runtime_get_noresume()/pm_runtime_put(). smp_execute_task_sg()
> only needs to hold off autosuspend while the SMP is in flight, and it
> must not try to resume the host: a sync resume issued from the HA
> resume path itself is what deadlocks, and by the time sas_resume_ha()
> runs, hw_init has already reinitialized the hardware, so the device
> is accessible without one.
>
> The usage reference is still required. 

Do you mean that usage reference from pm_runtime_get_noresume() is still 
required?

> Discovery work normally runs
> inside an event worker's PM reference, taken at
> sas_notify_port_event() notify time and held until the handler has
> flushed the disco queue. sas_rediscover_ex_phy() however requeues
> DISCE_REVALIDATE_DOMAIN from within the revalidation worker itself,
> and flush_workqueue() does not wait for work items queued during
> execution, so that chained revalidation runs with no outer PM
> reference - without the get_noresume(), its SMP could race
> autosuspend.
>
> For the BSG path, sas_smp_handler() is the only caller which may
> find the host autosuspended: expander SMP requests do not go through
> any SCSI device request queue, so nothing else in that path holds
> the host awake. Resume it there with pm_runtime_resume_and_get()
> and check the result.
>
> Fixes: 3dbbbf656b850 ("scsi: libsas: Fix HA resume deadlock and hisi_sas disk-wake race")
> Signed-off-by: Xingui Yang <yangxingui@xxxxxxxxxx>

Question: do you have a (non-hisi_sas) SAS HBA card whose driver uses 
libsas? pm8001 would be such an example. It would be nice to verify that 
all these and other non-rpm libsas changes does cause regression there.

> ---
> Changes since v3:
> - Move the host resume to sas_smp_handler(), the only caller which may
>    find the host autosuspended, as suggested by John Garry
> - Replace get_sync()/put_sync() with get_noresume()/put() in
>    smp_execute_task_sg(): a blocking resume issued from the HA resume
>    path itself is what deadlocks
> - Drop the racy SAS_HA_RESUMING check
>
> Changes since v2:
> - Drop the reference with pm_runtime_put() instead of
>    pm_runtime_put_noidle().
>
> Changes since v1:
> - Use pm_runtime_get_noresume()/put_noidle() during HA resume instead
>    of skipping the PM reference entirely, so an in-flight SMP IO always
>    keeps autosuspend away.
> - Convert pm_runtime_get_sync() to pm_runtime_resume_and_get() and
>    check the result (pre-existing issue flagged by sashiko).
>
>   drivers/scsi/libsas/sas_expander.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index 811c9eb4fef1..26c2099c28b9 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -62,7 +62,11 @@ static int smp_execute_task_sg(struct domain_device *dev,
>   		to_sas_internal(dev->port->ha->shost->transportt);
>   	struct sas_ha_struct *ha = dev->port->ha;
>   
> -	pm_runtime_get_sync(ha->dev);
> +	/*
> +	 * Non-blocking: a sync resume here would deadlock against
> +	 * sas_drain_work() during HA resume.
> +	 */
> +	pm_runtime_get_noresume(ha->dev);
>   	mutex_lock(&dev->ex_dev.cmd_mutex);
>   	for (retry = 0; retry < 3; retry++) {
>   		if (test_bit(SAS_DEV_GONE, &dev->state)) {
> @@ -135,7 +139,7 @@ static int smp_execute_task_sg(struct domain_device *dev,
>   		}
>   	}
>   	mutex_unlock(&dev->ex_dev.cmd_mutex);
> -	pm_runtime_put_sync(ha->dev);
> +	pm_runtime_put(ha->dev);
>   
>   	BUG_ON(retry == 3 && task != NULL);
>   	sas_free_task(task);
> @@ -2222,6 +2226,11 @@ void sas_smp_handler(struct bsg_job *job, struct Scsi_Host *shost,
>   		goto out;
>   	}
>   
> +	/* The host may have autosuspended, resume it here. */
> +	ret = pm_runtime_resume_and_get(dev->port->ha->dev);
> +	if (ret)
> +		goto out;
> +
>   	ret = smp_execute_task_sg(dev, job->request_payload.sg_list,
>   			job->reply_payload.sg_list);
>   	if (ret >= 0) {
> @@ -2229,6 +2238,7 @@ void sas_smp_handler(struct bsg_job *job, struct Scsi_Host *shost,
>   		rcvlen = job->reply_payload.payload_len - ret;
>   		ret = 0;
>   	}
> +	pm_runtime_put(dev->port->ha->dev);
>   
>   out:
>   	bsg_job_done(job, ret, rcvlen);