Re: [PATCH 2/2] scsi: hisi_sas: add spinup_notify callback to handle Active_Wait/Idle_Wait SSP devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Sleep in atomic context (softirq) leading to kernel panic.
- [High] Unprotected lockless read-modify-write on the per-PHY SL_CONTROL register.
--

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

scsi: hisi_sas: add spinup_notify callback to handle Active_Wait/Idle_Wait SSP devices

This patch adds a spinup_notify callback to the hisi_sas controller driver to
handle SSP devices in waiting states. When a device requires spinup, the driver
sends a NOTIFY(ENABLE SPINUP) primitive by toggling the SL_CONTROL.NOTIFY_EN
bit. The callback is registered across hardware versions v1, v2, and v3.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 944ce19ae2fc1..1a16d18db297a 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -2474,6 +2474,31 @@ int hisi_sas_get_fw_info(struct hisi_hba *hisi_hba)
>  }
>  EXPORT_SYMBOL_GPL(hisi_sas_get_fw_info);
>  
> +void hisi_sas_spinup_notify(struct scsi_device *sdev)
> +{
> +	struct domain_device *dev = sdev_to_domain_dev(sdev);
> +	struct sas_ha_struct *sha;
> +	struct hisi_hba *hisi_hba;
> +	struct sas_phy *local_phy;
> +	struct hisi_sas_phy *phy;
> +
> +	if (dev->parent && dev_is_expander(dev->parent->dev_type))
> +		return;
> +
> +	sha = SHOST_TO_SAS_HA(sdev->host);
> +	hisi_hba = sha->lldd_ha;
> +
> +	local_phy = sas_get_local_phy(dev);
> +	phy = &hisi_hba->phy[local_phy->number];
> +	if (phy->identify.target_port_protocols & SAS_PROTOCOL_SSP) {
> +		hisi_hba->hw->sl_notify_ssp(hisi_hba, local_phy->number);

[Severity: High]
Does calling sl_notify_ssp() here lead to a sleep in atomic context?

When the block layer completes a request and the device requires spinup,
the SCSI completion softirq invokes scsi_complete(), which calls
scsi_decide_disposition() -> scsi_check_sense() -> spinup_notify().

Since hisi_sas_spinup_notify() executes in BLOCK_SOFTIRQ context, calling
hisi_hba->hw->sl_notify_ssp() causes a panic on v3 hardware, as it contains
msleep(1):

drivers/scsi/hisi_sas/hisi_sas_v3_hw.c:sl_notify_ssp_v3_hw() {
	...
	sl_control = hisi_sas_phy_read32(hisi_hba, phy_no, SL_CONTROL);
	sl_control |= SL_CONTROL_NOTIFY_EN_MSK;
	hisi_sas_phy_write32(hisi_hba, phy_no, SL_CONTROL, sl_control);
	msleep(1);
	...
}

[Severity: High]
Can concurrent invocations of hisi_sas_spinup_notify() clobber the
SL_CONTROL register?

Because this lockless callback path is exposed to parallel softirq contexts,
multiple I/O completions for the same device can result in concurrent
read-modify-write operations in sl_notify_ssp_v3_hw():

drivers/scsi/hisi_sas/hisi_sas_v3_hw.c:sl_notify_ssp_v3_hw() {
	...
	sl_control = hisi_sas_phy_read32(hisi_hba, phy_no, SL_CONTROL);
	sl_control |= SL_CONTROL_NOTIFY_EN_MSK;
	hisi_sas_phy_write32(hisi_hba, phy_no, SL_CONTROL, sl_control);
	...
}

Could this unprotected read-modify-write race with itself across
different CPUs, potentially leaving NOTIFY_EN permanently asserted or
overwriting state changes made by concurrent hard IRQ handlers?

> +		dev_info(hisi_hba->dev, "spinup notify on phy%d for sdev %s\n",
> +			 local_phy->number, dev_name(&sdev->sdev_dev));
> +	}
> +	sas_put_local_phy(local_phy);
> +}
> +EXPORT_SYMBOL_GPL(hisi_sas_spinup_notify);

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