Re: [PATCH v3 2/4] scsi: hisi_sas: Fix incorrect delay values from magic-number cleanup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: hisi_sas: 50ms busy-wait under IRQ-disabled spinlock in phy_disable
--

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

scsi: hisi_sas: Fix incorrect delay values from magic-number cleanup

Commit 4ca7fe99fc84 ("scsi: hisi_sas: Use macro instead of magic
number") replaced several delay constants with a single macro of
value 100. This commit adds separate macros for each delay to restore
the original values while avoiding magic numbers.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> index 8a2500993e19d..9c363e2373538 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c

[ ... ]

> @@ -1072,7 +1075,7 @@ static void disable_phy_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
>  	cfg &= ~PHY_CFG_ENA_MSK;
>  	hisi_sas_phy_write32(hisi_hba, phy_no, PHY_CFG, cfg);
>  
> -	mdelay(HISI_SAS_DELAY_FOR_PHY_DISABLE);
> +	mdelay(DISABLE_PHY_DELAY_MS);

[Severity: High]
Does this execute a 50ms busy-wait under an IRQ-disabled spinlock?

Looking at hisi_sas_phy_enable() in drivers/scsi/hisi_sas/hisi_sas_main.c,
it calls the hardware-specific PHY disable callback while holding the
IRQ-disabled spinlock:

hisi_sas_phy_enable() {
	...
	spin_lock_irqsave(&phy->lock, flags);

	if (enable) {
		...
	} else {
		sphy->negotiated_linkrate = SAS_PHY_DISABLED;
		hisi_hba->hw->phy_disable(hisi_hba, phy_no);
	}
	...
}

Because hisi_hba->hw->phy_disable maps to disable_phy_v3_hw(), this means
the 50ms mdelay() executes with local interrupts disabled.

This will happen on any path that disables a PHY, including
hisi_sas_stop_phys() which iteratively disables all PHYs, compounding the
delay linearly.

Can this be changed to avoid executing a massive busy-wait with interrupts
disabled?

>  
>  	state = hisi_sas_read32(hisi_hba, PHY_STATE);
>  	if (state & BIT(phy_no)) {

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