| Message ID | 20240606054749.55708-1-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: mpi3mr: Fix SATA NCQ priority support | expand |
On Thu, Jun 06, 2024 at 02:47:49PM +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 a SATA device should be > executed using NCQ high priority, if 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 > a 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 device function > scsi_ncq_prio_supported(). Nothing in that function is hardware > specific, so this function can be used for both the mpt3sas driver and > the mpi3mr driver. Shouldn't this move into the SAS transport class instead then? > +/** > + * scsi_ncq_prio_supported - Check for NCQ command priority support > + * @sdev: SCSI device > + * > + * Check if a (SATA) device supports NCQ priority. For non-SATA devices, > + * this always return false. > + */ > +bool scsi_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(scsi_ncq_prio_supported); This also feels kinda out of place in the core SCSI code and more in scope for the SAS transport class, even if the other code can't move there for whatever reason.
On 6/6/24 18:31, Christoph Hellwig wrote: > On Thu, Jun 06, 2024 at 02:47:49PM +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 a SATA device should be >> executed using NCQ high priority, if 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 >> a 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 device function >> scsi_ncq_prio_supported(). Nothing in that function is hardware >> specific, so this function can be used for both the mpt3sas driver and >> the mpi3mr driver. > > Shouldn't this move into the SAS transport class instead then? > >> +/** >> + * scsi_ncq_prio_supported - Check for NCQ command priority support >> + * @sdev: SCSI device >> + * >> + * Check if a (SATA) device supports NCQ priority. For non-SATA devices, >> + * this always return false. >> + */ >> +bool scsi_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(scsi_ncq_prio_supported); > > This also feels kinda out of place in the core SCSI code and more in > scope for the SAS transport class, even if the other code can't > move there for whatever reason. "also" ? your previous point was not about this function ? But I think I get it. I will move this to scsi_transport_sas.c and rename it to sas_ata_ncq_prio_supported().
On Thu, Jun 06, 2024 at 09:14:46PM +0900, Damien Le Moal wrote:
> "also" ? your previous point was not about this function ?
No, about all the attribute boilerplate code.
On 6/6/24 21:34, Christoph Hellwig wrote: > On Thu, Jun 06, 2024 at 09:14:46PM +0900, Damien Le Moal wrote: >> "also" ? your previous point was not about this function ? > > No, about all the attribute boilerplate code. Yeah. That boilerplate for the 2 drivers differ only with the internal data structure used to store the cdl_enable boolean. So I guess we could make these attributes more generic. Ideally though, we should do something similar to CDL and have scsi layer deal with that automatically to avoid SAS drivers to have to do that themselves. And libata also has the same attributes. I would like to get this fix in ASAP as I am getting reports back from the field of NCQ priority not working with mpi3mr. I can send a cleanup series for the attributes on top of this fix later if that is OK with you.
diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c index 1638109a68a0..d3736068ec3a 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", scsi_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 (!scsi_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..8759bc7b8056 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", scsi_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 (!scsi_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_lib.c b/drivers/scsi/scsi_lib.c index ec39acc986d6..a0b25c7156e5 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -3412,6 +3412,28 @@ int scsi_vpd_tpg_id(struct scsi_device *sdev, int *rel_id) } EXPORT_SYMBOL(scsi_vpd_tpg_id); +/** + * scsi_ncq_prio_supported - Check for NCQ command priority support + * @sdev: SCSI device + * + * Check if a (SATA) device supports NCQ priority. For non-SATA devices, + * this always return false. + */ +bool scsi_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(scsi_ncq_prio_supported); + /** * scsi_build_sense - build sense data for a command * @scmd: scsi command for which the sense should be formatted diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 9c540f5468eb..c00aefcb8fa3 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -555,6 +555,8 @@ extern void sdev_enable_disk_events(struct scsi_device *sdev); extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t); extern int scsi_vpd_tpg_id(struct scsi_device *, int *); +bool scsi_ncq_prio_supported(struct scsi_device *sdev); + #ifdef CONFIG_PM extern int scsi_autopm_get_device(struct scsi_device *); extern void scsi_autopm_put_device(struct scsi_device *);
The function mpi3mr_qcmd() of the mpi3mr driver is able to indicate to the HBA if a read or write command directed at a SATA device should be executed using NCQ high priority, if 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 a 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 device function scsi_ncq_prio_supported(). Nothing in that function is hardware specific, so this function can be used for 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> --- 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_lib.c | 22 ++++++++++ include/scsi/scsi_device.h | 2 + 6 files changed, 88 insertions(+), 28 deletions(-)