Re: [PATCH v2 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 2 potential issue(s) to consider:
- [High] scsi: hisi_sas: 50ms busy-wait with local interrupts disabled in phy_disable
- [Low] scsi: hisi_sas: Unnecessary 10ms busy wait in sleepable contexts
--

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

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

This commit restores original delay values that were incorrectly changed to
100 by replacing a single macro with separate macros for each delay site.
This avoids magic numbers while fixing overly long delays on PHY disable
and host shutdown paths.

> 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 code execute a 50ms busy-wait with local interrupts disabled?

Looking at hisi_sas_phy_enable(), it acquires phy->lock using
spin_lock_irqsave(), disabling local hardware interrupts on the current CPU:

drivers/scsi/hisi_sas/hisi_sas_main.c:hisi_sas_phy_enable() {
    ...
    spin_lock_irqsave(&phy->lock, flags);
    ...
    if (enable)
        ...
    else
        hisi_hba->hw->phy_disable(hisi_hba, phy_no);
    ...
}

For v3 hardware, this callback resolves to disable_phy_v3_hw(), which
executes a 50ms busy loop via mdelay(DISABLE_PHY_DELAY_MS) while remaining
in atomic context.

Furthermore, hisi_sas_stop_phys() iterates over all PHYs and calls
hisi_sas_phy_enable(..., 0) sequentially. For a controller with multiple
PHYs, this multiplies the delay, resulting in hundreds of milliseconds
of CPU stall with interrupts disabled.

Executing massive busy loops with interrupts disabled blocks scheduling,
drops hardware interrupts, and causes massive system latency, risking soft
lockups. While this patch reduces the delay from 100ms back to 50ms, the
underlying architectural violation in the modified delay path remains.

>  
>  	state = hisi_sas_read32(hisi_hba, PHY_STATE);
>  	if (state & BIT(phy_no)) {
[ ... ]
> @@ -2755,7 +2758,7 @@ static int disable_host_v3_hw(struct hisi_hba *hisi_hba)
>  
>  	hisi_sas_stop_phys(hisi_hba);
>  
> -	mdelay(HISI_SAS_DELAY_FOR_PHY_DISABLE);
> +	mdelay(DISABLE_HOST_PHY_DELAY_MS);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Low]
Is it necessary to use a 10ms busy loop in sleepable contexts?

disable_host_v3_hw() executes mdelay(DISABLE_HOST_PHY_DELAY_MS), which
constitutes a 10ms CPU busy loop.

This function is called from paths like soft_reset_v3_hw(),
hisi_sas_reset_prepare_v3_hw(), and _suspend_v3_hw(). These callers
operate in sleepable contexts such as PM suspend/resume and device reset
paths where local CPU interrupts are not disabled.

Using mdelay() for a delay as long as 10ms on a sleepable code path wastes
CPU cycles and needlessly increases power consumption. Could this be
replaced with msleep() or usleep_range() instead?

>  
>  	reg_val = hisi_sas_read32(hisi_hba, AXI_MASTER_CFG_BASE +
>  				  AM_CTRL_GLOBAL);

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