Re: [PATCH v3 0/4] scsi: hisi_sas: Some misc fixes

[yangxingui <yangxingui@huawei.com>]

Hi Martin, James,

We have gone through all of the Sashiko review comments on v2 and v3
one by one.  Of the six v2 findings, two were fixed in v3 (clearing
all five error counters and taking the runtime PM reference with
pm_runtime_get_if_active()), and one was answered in the v3 commit
message (the software counters are cumulative statistics and are not
reset by design).  The rest are pre-existing issues, summarized below
by trigger condition and impact:

- The delays in the phy disable and host shutdown paths predate this
   series.  The 50 ms wait for the PHY to go down is a hardware
   requirement, and a poll-based rework is planned.
- The pm8001 and mvsas response-IU findings are pre-existing issues
   in those drivers.  The libsas patch leaves these cases unchanged
   and strictly narrows the worst case, and fixing them is outside the
   scope of this series.
- The suspend flush-ordering window from the v2 review also predates
   this series.
- The suggested CHL_INT2 clear order would trade delayed reporting of
   error counters for actual data loss.
- The negative-return concern of pm_runtime_get_if_active() cannot be
   triggered on this driver, and the suggested check would disable the
   fix on !CONFIG_PM builds.

In short, none of the remaining findings is a regression introduced
by this series.

One concern to raise: Sashiko reviews every posting from scratch, and
its findings change from round to round.  Of the six v2 findings, one
was re-raised in v3 and three were not raised again, while six new
ones appeared (7 findings on all 4 patches for v3).  Iterating on its
comments alone will not converge.

Could you let us know whether this series is fine to be merged as is,
and how you would prefer the pre-existing findings to be handled: in
a follow-up series, or folded into this one?

Thanks,
Xingui

On 2026/10/9 11:32, Xingui Yang wrote:
> This series contains three independent fixes for the hisi_sas driver,
> plus a libsas fix found during review.
> 
> Patch 1 fixes an out-of-bounds memcpy in sas_ssp_task_response(): a
> device-provided sense_data_len >= 0x80000000 turns negative in the
> min_t(int, ...) clamp and becomes a huge memcpy length.
> 
> Patch 2 fixes incorrect delay values introduced by a previous
> magic-number cleanup commit. Three delay sites were changed to use
> a single macro (value 100) which did not match their original values,
> causing increased boot and shutdown time.
> 
> Patch 3 clears stale PHY error counts on phyup so that error detection
> is not misled by intermediate errors generated during link
> establishment.
> 
> Patch 4 fixes spinup failures of directly-attached SAS HDDs that power
> up in the Active_Wait state and wait for a NOTIFY(ENABLE SPINUP)
> primitive before becoming ready: parse the sense data in the slot
> completion path and send the primitive from deferred work.
> 
> Changes from v2:
> Addresses the Sashiko AI review of v2.
> - Clear all five error counter registers.
> - Use pm_runtime_get_if_active().
> 
> Changes from v1:
> Addresses the Sashiko AI review of v1.
> - Add Patch 1, found in review of the sense parsing of patch 4.
> - Clear the counters under phy->lock.
> - Take a runtime PM reference for the deferred work
> 
> The remaining review findings are pre-existing issues, left for
> separate fixes in the future.
> 
> Xingui Yang (4):
>    scsi: libsas: Fix out-of-bounds memcpy in sas_ssp_task_response()
>    scsi: hisi_sas: Fix incorrect delay values from magic-number cleanup
>    scsi: hisi_sas: Clear PHY error counts on phyup
>    scsi: hisi_sas: Fix spinup failure for SAS SSP devices in Active_Wait
>      state
> 
>   drivers/scsi/hisi_sas/hisi_sas.h       |  3 ++
>   drivers/scsi/hisi_sas/hisi_sas_main.c  | 43 ++++++++++++++++++++++
>   drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |  4 ++
>   drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  4 ++
>   drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 51 ++++++++++++++++++--------
>   drivers/scsi/libsas/sas_task.c         |  2 +-
>   6 files changed, 90 insertions(+), 17 deletions(-)
>