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@xxxxxxx>
> Fixes: 023ab2a9b4ed ("scsi: mpi3mr: Add support for queue command processing")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxx>
