Re: [PATCH v3 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 1 potential issue(s) to consider:
- [Medium] pm: usage count underflow due to incorrect pm_runtime_get_if_active() check in hisi_sas_spinup_notify()
--

commit 255dba6c6286916d717929164fd02a77f83d3168
Author: Xingui Yang <yangxingui@huawei.com>
Subject: scsi: hisi_sas: Fix spinup failure for SAS SSP devices in Active_Wait state

This commit addresses spinup failures for directly-attached SSP devices 
waiting in the Active_Wait state. It parses sense data during slot 
completion and queues work to send a NOTIFY(ENABLE SPINUP) primitive while 
managing the runtime PM state of the controller.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 944ce19ae2fc1..cf7aa0a699cde 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)
[ ... ]
> +static void hisi_sas_spinup_notify_work(struct work_struct *work)
> +{
[ ... ]
> +	hisi_hba->hw->sl_notify_ssp(hisi_hba, phy_no);
> +	dev_info(dev, "spinup notify primitive on phy%d\n", phy_no);
> +
> +	pm_runtime_put_sync(dev);

[Severity: Medium]
If the earlier pm_runtime_get_if_active() check bypassed a negative error
code, will this cause a runtime PM usage count underflow since the reference
was never successfully incremented?

> +}
[ ... ]
> @@ -1639,6 +1654,34 @@ void hisi_sas_controller_reset_done(struct hisi_hba *hisi_hba)
[ ... ]
> +void hisi_sas_spinup_notify(struct hisi_hba *hisi_hba,
> +			    struct sas_task *task)
> +{
[ ... ]
> +	if (!pm_runtime_get_if_active(hisi_hba->dev))
> +		return;

[Severity: Medium]
Does this check properly handle negative error codes?

pm_runtime_get_if_active() returns 1 if the device is active, 0 if inactive,
and a negative error code (like -EINVAL) if runtime PM is disabled. If it
returns -EINVAL, !(-EINVAL) evaluates to false, causing execution to proceed
without actually acquiring a PM reference.

Should this check be `if (pm_runtime_get_if_active(...) <= 0)` instead?

> +
> +	local_phy = sas_get_local_phy(dev);
> +	phy = &hisi_hba->phy[local_phy->number];
> +	if (!hisi_sas_notify_phy_event(phy, HISI_PHYE_SPINUP_NOTIFY))
> +		pm_runtime_put(hisi_hba->dev);

[Severity: Medium]
Similarly, if queueing fails here, but the initial pm_runtime_get_if_active()
returned a negative error code, does this cause an immediate usage count
underflow?

> +
> +	sas_put_local_phy(local_phy);
> +}

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