Re: [PATCH v3 2/2] scsi: hisi_sas: Add lldd_notify_enable_spinup callback for SAS devices

Thank you for your contribution! Sashiko AI review found 2 potential issue(=
s) to consider:

New issues:
- [High] Missing check for controller reset state before accessing hardware=
 registers in the `hisi_sas_notify_enable_spinup` callback.

Pre-existing issues:
- [High] Race condition on SL_CONTROL register read-modify-write (RMW) oper=
ations between process and interrupt contexts without locking.
--

--- Patch [2]: [PATCH v3 2/2] scsi: hisi_sas: Add lldd_notify_enable_spinup=
 callback for SAS devices ---
commit 1a996ce0b10b1f8ffbc88dbdf06f98375a589a5c
Author: Xingui Yang <[email protected]>

scsi: hisi_sas: Add lldd_notify_enable_spinup callback for SAS devices
   =20
This commit registers the lldd_notify_enable_spinup callback for hisi_sas.
The callback sends a NOTIFY(ENABLE SPINUP) primitive to trigger media spinup
on SAS devices in Active_Wait or Idle_Wait state.

> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sa=
s/hisi_sas_main.c
> index 944ce19ae2fc1..a726d66803865 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -2081,6 +2081,16 @@ static int hisi_sas_write_gpio(struct sas_ha_struc=
t *sha, u8 reg_type,
>  				reg_index, reg_count, write_data);
>  }
> =20
> +static void hisi_sas_notify_enable_spinup(struct asd_sas_phy *phy)
> +{
> +	struct hisi_sas_phy *hisi_phy =3D
> +		container_of(phy, struct hisi_sas_phy, sas_phy);
> +	struct hisi_hba *hisi_hba =3D hisi_phy->hisi_hba;
> +
> +	hisi_hba->hw->sl_notify_ssp(hisi_hba, phy->id);

[Severity: High]
Should this code check the controller reset state before accessing the
hardware registers?

If a controller reset is triggered concurrently (e.g., via PCIe FLR) and
sets HISI_SAS_RESETTING_BIT, calling sl_notify_ssp() performs MMIO writes
while the hardware is resetting. This could trigger a Synchronous External
Abort (SEA) and kernel panic on ARM platforms.

Other callbacks like hisi_sas_phy_bcast() use
test_bit(HISI_SAS_RESETTING_BIT, &hisi_hba->flags) to prevent this. Would
it be appropriate to add a similar check here?


[Severity: High]
This is a pre-existing issue, but does calling sl_notify_ssp() here
expose an unprotected read-modify-write race on the SL_CONTROL register?

The hardware-specific callback sl_notify_ssp_v3_hw() runs in process
context and modifies the register:

drivers/scsi/hisi_sas/hisi_sas_v3_hw.c:sl_notify_ssp_v3_hw() {
    ...
    sl_control =3D hisi_sas_phy_read32(hisi_hba, phy_no, SL_CONTROL);
    sl_control |=3D SL_CONTROL_NOTIFY_EN_MSK;
    hisi_sas_phy_write32(hisi_hba, phy_no, SL_CONTROL, sl_control);
}

However, when a PHY goes down, phy_down_v3_hw() modifies the same register
in hardirq context:

drivers/scsi/hisi_sas/hisi_sas_v3_hw.c:phy_down_v3_hw() {
    ...
    sl_ctrl =3D hisi_sas_phy_read32(hisi_hba, phy_no, SL_CONTROL);
    hisi_sas_phy_write32(hisi_hba, phy_no, SL_CONTROL,
                         sl_ctrl&(~SL_CTA_MSK));
}

If the hardirq preempts the process-context worker exactly between the read
and write in sl_notify_ssp_v3_hw(), the worker will subsequently write back
its stale read value, undoing the interrupt's modification. Can this corrupt
the PHY state machine?

> +	dev_info(hisi_hba->dev, "phy%d notify enable spinup\n", phy->id);
> +}
> +

[ ... ]

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803020536.5465=
[email protected]?part=3D2