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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Log noise suppression logic incorrectly uses cumulative software counters, failing to suppress logs after the first error.
- [High] The patch fails to clear intermediate link establishment errors because the interrupt handler processes them before phyup.
- [Medium] Clearing CHL_INT2 interrupt statuses in phy_up_v3_hw races with the channel interrupt handler, causing spurious warnings.
- [Medium] Concurrent lockless reads of read-to-clear hardware registers cause non-deterministic error accounting.
--

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

scsi: hisi_sas: Clear PHY error counts on phyup

This commit clears PHY intermediate error counts generated during link
establishment upon a successful phyup. It aims to prevent logging of stale
error data by clearing hardware counters and only printing errors when they
are non-zero.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> index 9c363e2373538..f7e259aff3df1 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> @@ -1061,6 +1061,19 @@ 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)
> +{
> +	static const u32 msk = BIT(CHL_INT2_RX_DISP_ERR_OFF) |
> +			       BIT(CHL_INT2_RX_CODE_ERR_OFF) |
> +			       BIT(CHL_INT2_RX_INVLD_DW_OFF);
> +
> +	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);

[Severity: Medium]
Could these lockless reads of read-to-clear hardware registers cause
non-deterministic error accounting?

If this runs concurrently with the accounting reads in phy_get_events_v3_hw()
from the interrupt handler or sysfs, it seems one of the readers will consume
the register and get zero, leading to lost or corrupted statistics.

> +
> +	hisi_sas_phy_write32(hisi_hba, phy_no, CHL_INT2, msk);
> +}

[Severity: Medium]
Will clearing the CHL_INT2 interrupt statuses here race with the channel
interrupt handler?

When phy_up_v3_hw() calls this function, it writes to CHL_INT2 without
synchronization or masking. If handle_chl_int2_v3_hw() is executing
concurrently on another CPU, it might read CHL_INT2 immediately after it was
cleared. This would result in a read of zero and trigger the spurious warning:
"channel int 2 received with status bits cleared".

[ ... ]

> @@ -1673,6 +1682,8 @@ static irqreturn_t phy_up_v3_hw(int phy_no, struct hisi_hba *hisi_hba)
>  	phy->phy_attached = 1;
>  	spin_unlock(&phy->lock);
>  
> +	clear_phy_err_cnt_v3_hw(hisi_hba, phy_no);
> +

[Severity: High]
Does this clear the hardware counters too late in the process?

Since the CHL_INT2 error interrupts are unmasked by default during link
establishment, any intermediate error will immediately trigger an interrupt.
The handle_chl_int2_v3_hw() handler will process and log these errors before
phy_up_v3_hw() executes this cleanup, which appears to miss the goal of
suppressing those intermediate logs.

[ ... ]

> @@ -1926,15 +1937,18 @@ static void handle_chl_int2_v3_hw(struct hisi_hba *hisi_hba, int phy_no)
>  
>  		phy_get_events_v3_hw(hisi_hba, phy_no);
>  
> -		if (irq_value & BIT(CHL_INT2_RX_INVLD_DW_OFF))
> +		if ((irq_value & BIT(CHL_INT2_RX_INVLD_DW_OFF)) &&
> +		    sphy->invalid_dword_count > 0)

[Severity: High]
Does checking the cumulative software counter (sphy->invalid_dword_count)
break the log suppression logic?

Once a single link error occurs, this lifetime counter remains permanently
greater than zero. Subsequent link events will then evaluate this condition
to true and print the stale lifetime total, even if the new hardware error
count is zero. Should this be checking a newly read hardware count instead?

>  			dev_info(dev, "phy%d invalid dword cnt:   %u\n", phy_no,
>  				 sphy->invalid_dword_count);

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