* [PATCH 0/2] Fixes for ATA device queue depth control
@ 2022-09-24 6:29 Damien Le Moal
2022-09-24 6:29 ` [PATCH 1/2] ata: libata-scsi: Fix initialization of device queue depth Damien Le Moal
2022-09-24 6:29 ` [PATCH 2/2] ata: libata-sata: Fix device queue depth control Damien Le Moal
0 siblings, 2 replies; 5+ messages in thread
From: Damien Le Moal @ 2022-09-24 6:29 UTC (permalink / raw)
To: linux-ide, linux-scsi, Martin K . Petersen, John Garry
A couple of patches to fix maximum queue depth initialization and
control for ATA devices used through libsas.
Damien Le Moal (2):
ata: libata-scsi: Fix initialization of device queue depth
ata: libata-sata: Fix device queue depth control
drivers/ata/libata-sata.c | 24 ++++++++++++------------
drivers/ata/libata-scsi.c | 13 ++++++++-----
drivers/scsi/libsas/sas_scsi_host.c | 3 ++-
include/linux/libata.h | 4 ++--
4 files changed, 24 insertions(+), 20 deletions(-)
--
2.37.3
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] ata: libata-scsi: Fix initialization of device queue depth
2022-09-24 6:29 [PATCH 0/2] Fixes for ATA device queue depth control Damien Le Moal
@ 2022-09-24 6:29 ` Damien Le Moal
2022-09-24 6:29 ` [PATCH 2/2] ata: libata-sata: Fix device queue depth control Damien Le Moal
1 sibling, 0 replies; 5+ messages in thread
From: Damien Le Moal @ 2022-09-24 6:29 UTC (permalink / raw)
To: linux-ide, linux-scsi, Martin K . Petersen, John Garry
For SATA devices supporting NCQ, drivers using libsas first initialize a
scsi device queue depth based on the controller and device capabilities,
leading to the scsi device queue_depth field being 32 (ATA maximum queue
depth) for most setup. However, if libata was loaded using the
force=[ID]]noncq argument, the default queue depth should be set to 1 to
reflect the fact that queuable commands will never be used. This is
consistent with manually setting a device queue depth to 1 through sysfs
as that disables NCQ use for the device.
Fix ata_scsi_dev_config() to honor the noncq parameter by sertting the
device queue depth to 1 for devices that do not have the ATA_DFLAG_NCQ
flag set.
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
drivers/ata/libata-scsi.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 29e2f55c6faa..ff9602a0e54e 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1055,6 +1055,7 @@ EXPORT_SYMBOL_GPL(ata_scsi_dma_need_drain);
int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
{
struct request_queue *q = sdev->request_queue;
+ int depth = 1;
if (!ata_id_has_unload(dev->id))
dev->flags |= ATA_DFLAG_NO_UNLOAD;
@@ -1100,13 +1101,10 @@ 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);
- if (dev->flags & ATA_DFLAG_NCQ) {
- int 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);
- scsi_change_queue_depth(sdev, depth);
- }
+ depth = min(ATA_MAX_QUEUE, depth);
+ scsi_change_queue_depth(sdev, depth);
if (dev->flags & ATA_DFLAG_TRUSTED)
sdev->security_supported = 1;
--
2.37.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] ata: libata-sata: Fix device queue depth control
2022-09-24 6:29 [PATCH 0/2] Fixes for ATA device queue depth control Damien Le Moal
2022-09-24 6:29 ` [PATCH 1/2] ata: libata-scsi: Fix initialization of device queue depth Damien Le Moal
@ 2022-09-24 6:29 ` Damien Le Moal
2022-09-24 9:49 ` Sergey Shtylyov
1 sibling, 1 reply; 5+ messages in thread
From: Damien Le Moal @ 2022-09-24 6:29 UTC (permalink / raw)
To: linux-ide, linux-scsi, Martin K . Petersen, John Garry
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(-)
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);
--
2.37.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] ata: libata-sata: Fix device queue depth control
2022-09-24 6:29 ` [PATCH 2/2] ata: libata-sata: Fix device queue depth control Damien Le Moal
@ 2022-09-24 9:49 ` Sergey Shtylyov
2022-09-25 23:00 ` Damien Le Moal
0 siblings, 1 reply; 5+ messages in thread
From: Sergey Shtylyov @ 2022-09-24 9:49 UTC (permalink / raw)
To: Damien Le Moal, linux-ide, linux-scsi, Martin K . Petersen, John Garry
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] ata: libata-sata: Fix device queue depth control
2022-09-24 9:49 ` Sergey Shtylyov
@ 2022-09-25 23:00 ` Damien Le Moal
0 siblings, 0 replies; 5+ messages in thread
From: Damien Le Moal @ 2022-09-25 23:00 UTC (permalink / raw)
To: Sergey Shtylyov, linux-ide, linux-scsi, Martin K . Petersen, John Garry
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
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-09-25 23:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-24 6:29 [PATCH 0/2] Fixes for ATA device queue depth control Damien Le Moal
2022-09-24 6:29 ` [PATCH 1/2] ata: libata-scsi: Fix initialization of device queue depth Damien Le Moal
2022-09-24 6:29 ` [PATCH 2/2] ata: libata-sata: Fix device queue depth control Damien Le Moal
2022-09-24 9:49 ` Sergey Shtylyov
2022-09-25 23:00 ` Damien Le Moal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.