| Message ID | 20220924062907.959856-3-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes for ATA device queue depth control | expand |
Hello! On 9/24/22 9:29 AM, Damien Le Moal wrote: > The function __ata_change_queue_depth() uses the helper > ata_scsi_find_dev() to get the ata_device structure of a scsi device and > set that device maximum queue depth. However, when the ata device is > managed by libsas, ata_scsi_find_dev() return NULL, turning Returns? > __ata_change_queue_depth() into a nop, which prevents the user from > setting the maximum queue depth of ATA devices used with libsas based > HBAs. > > Fix this by renaming __ata_change_queue_depth() to > ata_change_queue_depth() and adding a pointer to the ata_device > structure of the target device as argument. This pointer is provided by > ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case ofi Of? > a libata managed device and by sas_change_queue_depth() using > sas_to_ata_dev() in the case of a libsas managed ata device. > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> [...] > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index ff9602a0e54e..c63bb50323c1 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -1101,6 +1101,11 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev) > if (dev->flags & ATA_DFLAG_AN) > set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events); > > + pr_info("#### can queue %d, ata qd %d, scsi qd %d\n", Hm... isn't this a debugging pr_info() call you forgot to remove? > + sdev->host->can_queue, > + ata_id_queue_depth(dev->id), > + sdev->queue_depth); > + > if (dev->flags & ATA_DFLAG_NCQ) > depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id)); > depth = min(ATA_MAX_QUEUE, depth); [...] MBR, Sergey
On 9/24/22 18:49, Sergey Shtylyov wrote: > Hello! > > On 9/24/22 9:29 AM, Damien Le Moal wrote: > >> The function __ata_change_queue_depth() uses the helper >> ata_scsi_find_dev() to get the ata_device structure of a scsi device and >> set that device maximum queue depth. However, when the ata device is >> managed by libsas, ata_scsi_find_dev() return NULL, turning > > Returns? > >> __ata_change_queue_depth() into a nop, which prevents the user from >> setting the maximum queue depth of ATA devices used with libsas based >> HBAs. >> >> Fix this by renaming __ata_change_queue_depth() to >> ata_change_queue_depth() and adding a pointer to the ata_device >> structure of the target device as argument. This pointer is provided by >> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case ofi > > Of? > >> a libata managed device and by sas_change_queue_depth() using >> sas_to_ata_dev() in the case of a libsas managed ata device. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > [...] > >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index ff9602a0e54e..c63bb50323c1 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -1101,6 +1101,11 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev) >> if (dev->flags & ATA_DFLAG_AN) >> set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events); >> >> + pr_info("#### can queue %d, ata qd %d, scsi qd %d\n", > > Hm... isn't this a debugging pr_info() call you forgot to remove? Oops. Yes. Sending an update. Thanks for the review. > >> + sdev->host->can_queue, >> + ata_id_queue_depth(dev->id), >> + sdev->queue_depth); >> + >> if (dev->flags & ATA_DFLAG_NCQ) >> depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id)); >> depth = min(ATA_MAX_QUEUE, depth); > [...] > > MBR, Sergey
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index 7a5fe41aa5ae..13b9d0fdd42c 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -1018,26 +1018,25 @@ DEVICE_ATTR(sw_activity, S_IWUSR | S_IRUGO, ata_scsi_activity_show, EXPORT_SYMBOL_GPL(dev_attr_sw_activity); /** - * __ata_change_queue_depth - helper for ata_scsi_change_queue_depth - * @ap: ATA port to which the device change the queue depth + * ata_change_queue_depth - Set a device maximum queue depth + * @ap: ATA port of the target device + * @dev: target ATA device * @sdev: SCSI device to configure queue depth for * @queue_depth: new queue depth * - * libsas and libata have different approaches for associating a sdev to - * its ata_port. + * Helper to set a device maximum queue depth, usable with both libsas + * and libata. * */ -int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, - int queue_depth) +int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, + struct scsi_device *sdev, int queue_depth) { - struct ata_device *dev; unsigned long flags; - if (queue_depth < 1 || queue_depth == sdev->queue_depth) + if (!dev || !ata_dev_enabled(dev)) return sdev->queue_depth; - dev = ata_scsi_find_dev(ap, sdev); - if (!dev || !ata_dev_enabled(dev)) + if (queue_depth < 1 || queue_depth == sdev->queue_depth) return sdev->queue_depth; /* NCQ enabled? */ @@ -1059,7 +1058,7 @@ int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, return scsi_change_queue_depth(sdev, queue_depth); } -EXPORT_SYMBOL_GPL(__ata_change_queue_depth); +EXPORT_SYMBOL_GPL(ata_change_queue_depth); /** * ata_scsi_change_queue_depth - SCSI callback for queue depth config @@ -1080,7 +1079,8 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth) { struct ata_port *ap = ata_shost_to_port(sdev->host); - return __ata_change_queue_depth(ap, sdev, queue_depth); + return ata_change_queue_depth(ap, ata_scsi_find_dev(ap, sdev), + sdev, queue_depth); } EXPORT_SYMBOL_GPL(ata_scsi_change_queue_depth); diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index ff9602a0e54e..c63bb50323c1 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1101,6 +1101,11 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev) if (dev->flags & ATA_DFLAG_AN) set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events); + pr_info("#### can queue %d, ata qd %d, scsi qd %d\n", + sdev->host->can_queue, + ata_id_queue_depth(dev->id), + sdev->queue_depth); + if (dev->flags & ATA_DFLAG_NCQ) depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id)); depth = min(ATA_MAX_QUEUE, depth); diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index 9c82e5dc4fcc..a36fa1c128a8 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -872,7 +872,8 @@ int sas_change_queue_depth(struct scsi_device *sdev, int depth) struct domain_device *dev = sdev_to_domain_dev(sdev); if (dev_is_sata(dev)) - return __ata_change_queue_depth(dev->sata_dev.ap, sdev, depth); + return ata_change_queue_depth(dev->sata_dev.ap, + sas_to_ata_dev(dev), sdev, depth); if (!sdev->tagged_supported) depth = 1; diff --git a/include/linux/libata.h b/include/linux/libata.h index 698032e5ef2d..20765d1c5f80 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -1136,8 +1136,8 @@ extern int ata_scsi_slave_config(struct scsi_device *sdev); extern void ata_scsi_slave_destroy(struct scsi_device *sdev); extern int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth); -extern int __ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, - int queue_depth); +extern int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, + struct scsi_device *sdev, int queue_depth); extern struct ata_device *ata_dev_pair(struct ata_device *adev); extern int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev); extern void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap);
The function __ata_change_queue_depth() uses the helper ata_scsi_find_dev() to get the ata_device structure of a scsi device and set that device maximum queue depth. However, when the ata device is managed by libsas, ata_scsi_find_dev() return NULL, turning __ata_change_queue_depth() into a nop, which prevents the user from setting the maximum queue depth of ATA devices used with libsas based HBAs. Fix this by renaming __ata_change_queue_depth() to ata_change_queue_depth() and adding a pointer to the ata_device structure of the target device as argument. This pointer is provided by ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case ofi a libata managed device and by sas_change_queue_depth() using sas_to_ata_dev() in the case of a libsas managed ata device. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/ata/libata-sata.c | 24 ++++++++++++------------ drivers/ata/libata-scsi.c | 5 +++++ drivers/scsi/libsas/sas_scsi_host.c | 3 ++- include/linux/libata.h | 4 ++-- 4 files changed, 21 insertions(+), 15 deletions(-)