Re: [PATCH v2 4/4] scsi: hisi_sas: Fix spinup failure for SAS SSP devices in Active_Wait state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: hisi_sas: hardware access on suspended device via late CQ interrupt
- [Medium] pm: pm_runtime_get_noresume() used in IRQ handler instead of pm_runtime_get_if_active()
--

commit 8d6af7e7596b04b1c535c9ae957073a4a940e9cc
Author: Xingui Yang <yangxingui@huawei.com>

scsi: hisi_sas: Fix spinup failure for SAS SSP devices in Active_Wait state

This commit fixes a spinup failure for SAS SSP devices by parsing the sense
data in the slot completion path. When LU_NOT_READY_NOTIFY_REQUIRED is
returned, it queues a work item to send a spinup notify primitive while
taking a runtime PM reference.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 944ce19ae2fc1..f3b6bb510a71e 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -996,10 +996,25 @@ static void hisi_sas_phyup_pm_work(struct work_struct *work)
>  	pm_runtime_put_sync(dev);
>  }
>  
> +static void hisi_sas_spinup_notify_work(struct work_struct *work)
> +{
> +	struct hisi_sas_phy *phy =
> +		container_of(work, typeof(*phy), works[HISI_PHYE_SPINUP_NOTIFY]);
> +	struct hisi_hba *hisi_hba = phy->hisi_hba;
> +	struct device *dev = hisi_hba->dev;
> +	int phy_no = phy->sas_phy.id;
> +
> +	hisi_hba->hw->sl_notify_ssp(hisi_hba, phy_no);

[Severity: High]
Could this work item execute after the hardware is suspended? 

If a completion queue interrupt fires during suspend, it could happen in the
window between the workqueue flush and disabling interrupts:

_suspend_v3_hw()
    flush_workqueue(hisi_hba->wq);
    // interrupt fires -> hisi_sas_spinup_notify() queues this work
    interrupt_disable_v3_hw(hisi_hba);

Because the interrupt handler queues this work to a non-freezable workqueue, 
it can execute concurrently with or after the host is disabled. If the 
controller's bus is shut down by the time sl_notify_ssp() accesses MMIO 
registers, will this cause a system freeze or panic?

> +	dev_info(dev, "spinup notify primitive on phy%d\n", phy_no);
> +
> +	pm_runtime_put_sync(dev);
> +}

[ ... ]

> @@ -1639,6 +1654,33 @@ void hisi_sas_controller_reset_done(struct hisi_hba *hisi_hba)
>  }
>  EXPORT_SYMBOL_GPL(hisi_sas_controller_reset_done);
>  
> +void hisi_sas_spinup_notify(struct hisi_hba *hisi_hba,
> +			    struct sas_task *task)
> +{
> +	struct task_status_struct *ts = &task->task_status;
> +	struct domain_device *dev = task->dev;
> +	struct scsi_sense_hdr sshdr;
> +	struct sas_phy *local_phy;
> +	struct hisi_sas_phy *phy;
> +
> +	if (!scsi_normalize_sense(ts->buf, ts->buf_valid_size, &sshdr))
> +		return;
> +
> +	if (sshdr.sense_key != NOT_READY ||
> +	    sshdr.sense_code != LU_NOT_READY_NOTIFY_REQUIRED)
> +		return;
> +
> +	local_phy = sas_get_local_phy(dev);
> +	phy = &hisi_hba->phy[local_phy->number];
> +
> +	pm_runtime_get_noresume(hisi_hba->dev);

[Severity: Medium]
The PM subsystem guidelines state that IRQ handlers should use 
pm_runtime_get_if_active() rather than pm_runtime_get_noresume(). 

Since hisi_sas_spinup_notify() is called directly from hardware completion 
interrupt handlers, could this blindly increment the usage counter on a 
device that is already suspended or transitioning to suspend? 

Using pm_runtime_get_if_active() would verify the device is actually active 
and prevent queuing the spinup notify work when the controller is powering
down.

> +	if (!hisi_sas_notify_phy_event(phy, HISI_PHYE_SPINUP_NOTIFY))
> +		pm_runtime_put(hisi_hba->dev);
> +
> +	sas_put_local_phy(local_phy);
> +}
> +EXPORT_SYMBOL_GPL(hisi_sas_spinup_notify);

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