Re: [PATCH v2 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless Read-Modify-Write (RMW) race on the `SL_CONTROL` hardware register between process context (workqueue) and hardirq context.
--

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

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

This commit adds a spinup_notify callback for waiting SSP devices. It queues
a work item to the driver's ordered workqueue to send a NOTIFY primitive by
pulsing the SL_CONTROL.NOTIFY_EN bit, which defers hardware access to process
context.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 944ce19ae2fc1..14cf01466a75d 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
[ ... ]
> @@ -996,10 +996,22 @@ 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;
> +	int phy_no = phy->sas_phy.id;
> +
> +	hisi_hba->hw->sl_notify_ssp(hisi_hba, phy_no);

[Severity: High]
This is a pre-existing issue, but does the ordered workqueue actually prevent
concurrent RMW races on SL_CONTROL against hardirq context here?

The commit message states that the ordered workqueue prevents concurrent RMW
races on SL_CONTROL, but looking at sl_notify_ssp_v3_hw(), the RMW operation
occurs in process context without interrupt disable or spinlock protection:

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);
        ...
}

If an interrupt fires exactly between the read32 and write32 operations,
phy_down_v3_hw() executes another RMW on the same register in hardirq
context:

drivers/scsi/hisi_sas/hisi_sas_v3_hw.c:phy_down_v3_hw() {
        ...
        sl_ctrl = hisi_sas_phy_read32(hisi_hba, phy_no, SL_CONTROL);
        hisi_sas_phy_write32(hisi_hba, phy_no, SL_CONTROL,
                                                sl_ctrl&(~SL_CTA_MSK));
        ...
}

If this preempts the workqueue RMW, wouldn't the process context write
revert the clearing of SL_CTA_MSK done by the interrupt, leading to
hardware state corruption and potentially causing the loss of the PHY/disk
connection?

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

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