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

[Niklas Cassel <cassel@kernel.org>]

On Fri, Jun 07, 2024 at 10:24:04AM +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>
> ---
> 
> Changes from v1:
>  - Moved scsih_ncq_prio_supp() to be a function in scsi_transport_sas.c
>    and renamed that function to sas_ata_ncq_prio_supported().
> 
>  drivers/scsi/mpi3mr/mpi3mr_app.c     | 62 ++++++++++++++++++++++++++++
>  drivers/scsi/mpt3sas/mpt3sas_base.h  |  3 --
>  drivers/scsi/mpt3sas/mpt3sas_ctl.c   |  4 +-
>  drivers/scsi/mpt3sas/mpt3sas_scsih.c | 23 -----------
>  drivers/scsi/scsi_transport_sas.c    | 22 ++++++++++
>  include/scsi/scsi_transport_sas.h    |  2 +
>  6 files changed, 88 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index 1638109a68a0..cd261b48eb46 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -2163,10 +2163,72 @@ persistent_id_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(persistent_id);
>  
> +/**
> + * sas_ncq_prio_supported_show - Indicate if device supports NCQ priority
> + * @dev: pointer to embedded device
> + * @attr: sas_ncq_prio_supported attribute descriptor
> + * @buf: the buffer returned
> + *
> + * A sysfs 'read-only' sdev attribute, only works with SATA devices
> + */
> +static ssize_t
> +sas_ncq_prio_supported_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	struct scsi_device *sdev = to_scsi_device(dev);
> +
> +	return sysfs_emit(buf, "%d\n", sas_ata_ncq_prio_supported(sdev));
> +}
> +static DEVICE_ATTR_RO(sas_ncq_prio_supported);
> +
> +/**
> + * sas_ncq_prio_enable_show - send prioritized io commands to device
> + * @dev: pointer to embedded device
> + * @attr: sas_ncq_prio_enable attribute descriptor
> + * @buf: the buffer returned
> + *
> + * A sysfs 'read/write' sdev attribute, only works with SATA devices
> + */
> +static ssize_t
> +sas_ncq_prio_enable_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct scsi_device *sdev = to_scsi_device(dev);
> +	struct mpi3mr_sdev_priv_data *sdev_priv_data =  sdev->hostdata;
> +
> +	if (!sdev_priv_data)
> +		return 0;
> +
> +	return sysfs_emit(buf, "%d\n", sdev_priv_data->ncq_prio_enable);
> +}
> +
> +static ssize_t
> +sas_ncq_prio_enable_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct scsi_device *sdev = to_scsi_device(dev);
> +	struct mpi3mr_sdev_priv_data *sdev_priv_data =  sdev->hostdata;
> +	bool ncq_prio_enable = 0;
> +
> +	if (kstrtobool(buf, &ncq_prio_enable))
> +		return -EINVAL;
> +
> +	if (!sas_ata_ncq_prio_supported(sdev))
> +		return -EINVAL;
> +
> +	sdev_priv_data->ncq_prio_enable = ncq_prio_enable;
> +
> +	return strlen(buf);
> +}
> +static DEVICE_ATTR_RW(sas_ncq_prio_enable);
> +
>  static struct attribute *mpi3mr_dev_attrs[] = {
>  	&dev_attr_sas_address.attr,
>  	&dev_attr_device_handle.attr,
>  	&dev_attr_persistent_id.attr,
> +	&dev_attr_sas_ncq_prio_supported.attr,
> +	&dev_attr_sas_ncq_prio_enable.attr,
>  	NULL,
>  };
>  
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
> index bf100a4ebfc3..fe1e96fda284 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.h
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
> @@ -2048,9 +2048,6 @@ void
>  mpt3sas_setup_direct_io(struct MPT3SAS_ADAPTER *ioc, struct scsi_cmnd *scmd,
>  	struct _raid_device *raid_device, Mpi25SCSIIORequest_t *mpi_request);
>  
> -/* NCQ Prio Handling Check */
> -bool scsih_ncq_prio_supp(struct scsi_device *sdev);
> -
>  void mpt3sas_setup_debugfs(struct MPT3SAS_ADAPTER *ioc);
>  void mpt3sas_destroy_debugfs(struct MPT3SAS_ADAPTER *ioc);
>  void mpt3sas_init_debugfs(void);
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index 1c9fd26195b8..87784c96249a 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -4088,7 +4088,7 @@ sas_ncq_prio_supported_show(struct device *dev,
>  {
>  	struct scsi_device *sdev = to_scsi_device(dev);
>  
> -	return sysfs_emit(buf, "%d\n", scsih_ncq_prio_supp(sdev));
> +	return sysfs_emit(buf, "%d\n", sas_ata_ncq_prio_supported(sdev));
>  }
>  static DEVICE_ATTR_RO(sas_ncq_prio_supported);
>  
> @@ -4123,7 +4123,7 @@ sas_ncq_prio_enable_store(struct device *dev,
>  	if (kstrtobool(buf, &ncq_prio_enable))
>  		return -EINVAL;
>  
> -	if (!scsih_ncq_prio_supp(sdev))
> +	if (!sas_ata_ncq_prio_supported(sdev))
>  		return -EINVAL;
>  
>  	sas_device_priv_data->ncq_prio_enable = ncq_prio_enable;
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_scsih.c b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> index 89ef43a5ef86..3a1ed6a4f370 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_scsih.c
> @@ -12571,29 +12571,6 @@ scsih_pci_mmio_enabled(struct pci_dev *pdev)
>  	return PCI_ERS_RESULT_RECOVERED;
>  }
>  
> -/**
> - * scsih_ncq_prio_supp - Check for NCQ command priority support
> - * @sdev: scsi device struct
> - *
> - * This is called when a user indicates they would like to enable
> - * ncq command priorities. This works only on SATA devices.
> - */
> -bool scsih_ncq_prio_supp(struct scsi_device *sdev)
> -{
> -	struct scsi_vpd *vpd;
> -	bool ncq_prio_supp = false;
> -
> -	rcu_read_lock();
> -	vpd = rcu_dereference(sdev->vpd_pg89);
> -	if (!vpd || vpd->len < 214)
> -		goto out;
> -
> -	ncq_prio_supp = (vpd->data[213] >> 4) & 1;
> -out:
> -	rcu_read_unlock();
> -
> -	return ncq_prio_supp;
> -}
>  /*
>   * The pci device ids are defined in mpi/mpi2_cnfg.h.
>   */
> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> index 424a89513814..17a72fcab210 100644
> --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -416,6 +416,28 @@ unsigned int sas_is_tlr_enabled(struct scsi_device *sdev)
>  }
>  EXPORT_SYMBOL_GPL(sas_is_tlr_enabled);
>  
> +/**
> + * sas_ata_ncq_prio_supported - Check for NCQ command priority support
> + * @sdev: SCSI device
> + *
> + * Check if an ATA device supports NCQ priority. For non-ATA devices,
> + * this always return false.

Nit:
"this will always return false"
or
"this always returns false"
or perhaps even better:
"the vpd page 89 is not implemented, so this function will return false"


With the nit fixed:
Reviewed-by: Niklas Cassel <cassel@kernel.org>