Re: [PATCH v3 3/4] scsi: hisi_sas: Clear PHY error counts on phyup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] scsi: hisi_sas: Race condition in clear_phy_err_cnt_v3_hw loses interrupts
--

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

scsi: hisi_sas: Clear PHY error counts on phyup

Some disks generate link errors during link establishment but still
phy up successfully. This commit clears these intermediate error
counts on phyup to prevent leaving stale data for error detection.

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

[ ... ]

> @@ -1061,16 +1064,31 @@ static void enable_phy_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
>  	hisi_sas_phy_write32(hisi_hba, phy_no, PHY_CFG, cfg);
>  }
>  
> +static void clear_phy_err_cnt_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
> +{
> +	u32 irq_msk = hisi_sas_phy_read32(hisi_hba, phy_no, CHL_INT2_MSK);
> +
> +	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2_MSK,
> +			     CHL_INT2_RX_ERR_MSK | irq_msk);
> +
> +	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_DWS_LOST);
> +	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_RESET_PROB);
> +	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_INVLD_DW);
> +	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_DISP_ERR);
> +	hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_CODE_ERR);
> +
> +	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2, CHL_INT2_RX_ERR_MSK);

[Severity: Low]
Does this sequence create a race window where an error interrupt can be lost?

Because the error counters are clear-on-read, reading them before clearing the
Write-1-to-Clear interrupt status register CHL_INT2 could lead to a race.
If a new PHY error occurs in the window after the counter is read but before
CHL_INT2 is written, the hardware will increment the error counter and set the
corresponding interrupt status bit.

The subsequent write to CHL_INT2 will then clear this newly set interrupt
status bit, causing the interrupt for the new error to be permanently lost
until another independent error occurs.

Could the CHL_INT2 register be cleared before reading the clear-on-read
counters to prevent this race?

> +	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2_MSK, irq_msk);
> +}

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