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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] scsi: hisi_sas: incomplete error clearance on phyup leaves software counters stale
- [Low] scsi: hisi_sas: missing clear of DWS_LOST and RESET_PROB error counts
--

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

scsi: hisi_sas: Clear PHY error counts on phyup

This patch attempts to clear intermediate link error counts generated during
link establishment in phyup. This is done to prevent stale error data from
polluting runtime statistics.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> index 9c363e2373538..a2011d77915bb 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
[ ... ]
> @@ -1061,16 +1064,29 @@ 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_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]
Will this leave the software counters stale? While the hardware registers are
cleared here, intermediate errors might have already been accumulated into
software counters during link establishment via phy_get_events_v3_hw():

handle_chl_int2_v3_hw()
  phy_get_events_v3_hw()
    reg_value = hisi_sas_phy_read32(hisi_hba, phy_no, ERR_CNT_INVLD_DW);
    sphy->invalid_dword_count += reg_value;

Because clear_phy_err_cnt_v3_hw() only reads and clears the hardware
registers, the corresponding software counters like sphy->invalid_dword_count
and phy->code_violation_err_count remain untouched. Should the software
counters also be reset here to completely clear the stale data?

[Severity: Low]
Does this function also need to clear ERR_CNT_DWS_LOST and ERR_CNT_RESET_PROB?

Since these registers are clear-on-read, and phy_get_events_v3_hw() reads all
five of them, any subsequent RX error could cause phy_get_events_v3_hw() to
read the uncleared, stale DWS_LOST and RESET_PROB counts, incorrectly adding
them to the running statistics.

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

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