Re: [PATCH] scsi: mpi3mr: use DevicePage0 link rate only for direct-attached targets

[Thorsten Leemhuis]

On 8/4/26 16:28, Ilya Khomyakov wrote:
> This patch fixes a regression in the Broadcom MPI3 Storage Controller
> driver under drivers/scsi/mpi3mr/.

Ilya, what's the status of this regression fix? It looks like it fall
through the cracks. Or was there some progress that I missed (which can
easily happen!)

Ciao, Thorsten
> Commit c273c14b0294 ("scsi: mpi3mr: Use negotiated link rate from
> DevicePage0") changed mpi3mr to prefer the cached DevicePage0 rate.
> 
> DevicePage0 stores negotiated_link_rate as one standalone MPI3 SAS
> link-rate code, while SAS PHY Page 0 and SAS Expander Page 1 store logical
> and physical rates as two nibbles of one packed byte.
> 
> mpi3mr_get_sas_negotiated_logical_linkrate() currently sends both formats
> through the same high-nibble extraction at its common exit. A valid
> DevicePage0 value of 0x0b or 0x0c is therefore converted to zero. The later
> minimum-rate safeguard then publishes the value as 1.5 Gbit/s.
> 
> For example:
> 
>   DevicePage0 value:            0x0b
>   packed-field extraction:      (0x0b & 0xf0) >> 4 = 0x00
>   minimum-rate substitution:    0x00 -> 0x08
>   Linux SAS transport result:   1.5 Gbit/s
> 
> There is also a topology distinction. The caller uses the returned value to
> update the parent sas_phy. In an expander topology with the HBA-to-expander
> link limited to 12 Gbit/s, DevicePage0 reported 0x0b for every tested
> target, while Expander Page 1 reported 0xcc for several disk-facing PHYs
> that were operating at 22.5 Gbit/s. The cached target value therefore
> cannot unconditionally replace the local parent expander PHY value.
> 
> The original failure was reproduced on an eHBA 9600 controller with a
> SAS4016 IOC and a 46-PHY Microchip expander. Before the fix, a target with
> DevicePage0 negotiated_link_rate 0x0b could remain visible in sysfs as
> 1.5 Gbit/s when a replayed topology event contained current=0x0b and
> previous=0x0b and therefore skipped the Linux-side update.
> 
> Validate the standalone DevicePage0 code and use it directly only for a
> directly attached target. For an expander-attached target, retain the
> existing Expander Page 1 path because it describes the local parent
> expander PHY being updated. Retain the SAS PHY Page 0 fallback when a
> directly attached cached value is invalid.
> 
> The topology-aware variant was tested in an out-of-tree mpi3mr 8.17.1.0.
> build. After boot, Linux reported:
> 
>   PHY 18: 12.0 Gbit/s
>   PHY 19: 12.0 Gbit/s
>   PHY 24: 22.5 Gbit/s
>   PHY 25: 22.5 Gbit/s
>   PHY 26: 22.5 Gbit/s
>   PHY 27: 22.5 Gbit/s
>   PHY 28: 12.0 Gbit/s
>   PHY 30: 22.5 Gbit/s
> 
> No tested PHY was incorrectly reported as 1.5 Gbit/s, and the 22.5 Gbit/s
> disk-facing rates were preserved even though DevicePage0 contained 0x0b.
> 
> Fixes: c273c14b0294 ("scsi: mpi3mr: Use negotiated link rate from DevicePage0")
> Signed-off-by: Ilya Khomyakov <khomyakovilya@xxxxxxxxx>
> 
> ---
>  drivers/scsi/mpi3mr/mpi3mr_transport.c | 46 ++++++++++++++++++++++----
>  1 file changed, 39 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> index 240f67a..740fccc 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> @@ -587,6 +587,30 @@ static enum sas_linkrate mpi3mr_convert_phy_link_rate(u8 link_rate)
>  	return rc;
>  }
>  
> +/**
> + * mpi3mr_sas_link_rate_valid - validate a standalone SAS link-rate code
> + * @link_rate: MPI3 standalone SAS negotiated link-rate code
> + *
> + * DevicePage0 stores one plain negotiated link-rate code. Accept only
> + * negotiated data rates supported by the SAS transport conversion code;
> + * reserved or transitional values must use the existing PHY-page path.
> + *
> + * Return: true for a supported negotiated data rate, false otherwise.
> + */
> +static bool mpi3mr_sas_link_rate_valid(u8 link_rate)
> +{
> +	switch (link_rate) {
> +	case MPI3_SAS_NEG_LINK_RATE_1_5:
> +	case MPI3_SAS_NEG_LINK_RATE_3_0:
> +	case MPI3_SAS_NEG_LINK_RATE_6_0:
> +	case MPI3_SAS_NEG_LINK_RATE_12_0:
> +	case MPI3_SAS_NEG_LINK_RATE_22_5:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  /**
>   * mpi3mr_delete_sas_phy - Remove a single phy from port
>   * @mrioc: Adapter instance reference
> @@ -2292,18 +2316,26 @@ void mpi3mr_expander_remove(struct mpi3mr_ioc *mrioc, u64 sas_address,
>  static u8 mpi3mr_get_sas_negotiated_logical_linkrate(struct mpi3mr_ioc *mrioc,
>  	struct mpi3mr_tgt_dev *tgtdev)
>  {
> -	u8 link_rate = MPI3_SAS_NEG_LINK_RATE_1_5, phy_number;
> +	u8 cached_link_rate, link_rate = MPI3_SAS_NEG_LINK_RATE_1_5;
> +	u8 phy_number;
>  	struct mpi3_sas_expander_page1 expander_pg1;
>  	struct mpi3_sas_phy_page0 phy_pg0;
>  	u32 phynum_handle;
>  	u16 ioc_status;
>  
> -	/* First, try to use link rate from DevicePage0 (populated by firmware) */
> -	if (tgtdev->dev_spec.sas_sata_inf.negotiated_link_rate >=
> -	    MPI3_SAS_NEG_LINK_RATE_1_5) {
> -		link_rate = tgtdev->dev_spec.sas_sata_inf.negotiated_link_rate;
> -		goto out;
> -	}
> +	cached_link_rate =
> +		tgtdev->dev_spec.sas_sata_inf.negotiated_link_rate;
> +
> +	/*
> +	 * For a directly attached target, DevicePage0 and the parent host PHY
> +	 * describe the same link, so the standalone cached code can be used
> +	 * without packed-field decoding. For an expander-attached target, the
> +	 * caller updates the parent expander PHY and DevicePage0 can differ from
> +	 * that local segment; retain the Expander Page 1 read in that case.
> +	 */
> +	if ((tgtdev->devpg0_flag & MPI3_DEVICE0_FLAGS_ATT_METHOD_DIR_ATTACHED) &&
> +	    mpi3mr_sas_link_rate_valid(cached_link_rate))
> +		return cached_link_rate;
>  
>  	/* Fallback to reading from phy pages if DevicePage0 value not available */
>  	phy_number = tgtdev->dev_spec.sas_sata_inf.phy_id;