Re: [PATCH v3] scsi: mpi3mr: Fix ATA NCQ priority support

[Niklas Cassel <cassel@kernel.org>]

On Mon, Jun 10, 2024 at 06:10:35PM +0900, Damien Le Moal wrote:
> The function mpi3mr_qcmd() of the mpi3mr driver is able to indicate to
> the HBA if a read or write command directed at an ATA device should be
> translated to an NCQ read/write command with the high prioiryt bit set
> when the request uses the RT priority class and the user has enabled NCQ
> priority through sysfs.
> 
> However, unlike the mpt3sas driver, the mpi3mr driver does not define
> the sas_ncq_prio_supported and sas_ncq_prio_enable sysfs attributes, so
> the ncq_prio_enable field of struct mpi3mr_sdev_priv_data is never
> actually set and NCQ Priority cannot ever be used.
> 
> Fix this by defining these missing atributes to allow a user to check if
> an ATA device supports NCQ priority and to enable/disable the use of NCQ
> priority. To do this, lift the function scsih_ncq_prio_supp() out of the
> mpt3sas driver and make it the generic scsi sas transport function
> sas_ata_ncq_prio_supported(). Nothing in that function is hardware
> specific, so this function can be used in both the mpt3sas driver and
> the mpi3mr driver.
> 
> Reported-by: Scott McCoy <scott.mccoy@wdc.com>
> Fixes: 023ab2a9b4ed ("scsi: mpi3mr: Add support for queue command processing")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> ---

(snip)

> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -416,6 +416,29 @@ unsigned int sas_is_tlr_enabled(struct scsi_device *sdev)
>  }
>  EXPORT_SYMBOL_GPL(sas_is_tlr_enabled);
>  
> +/**
> + * sas_ata_ncq_prio_supported - Check for ATA NCQ command priority support
> + * @sdev: SCSI device
> + *
> + * Check if an ATA device supports NCQ priority using VPD page 89h (ATA
> + * Information). Since this VPD page is implemented only for ATA devices,
> + * this function always returns false for SCSI devices."

The quote char (") at the end of the sentence looks lonely and out of place :)


> + */
> +bool sas_ata_ncq_prio_supported(struct scsi_device *sdev)
> +{
> +	struct scsi_vpd *vpd;
> +	bool ncq_prio_supported = false;
> +
> +	rcu_read_lock();
> +	vpd = rcu_dereference(sdev->vpd_pg89);
> +	if (vpd && vpd->len >= 214)
> +		ncq_prio_supported = (vpd->data[213] >> 4) & 1;
> +	rcu_read_unlock();
> +
> +	return ncq_prio_supported;
> +}
> +EXPORT_SYMBOL_GPL(sas_ata_ncq_prio_supported);
> +
>  /*
>   * SAS Phy attributes
>   */