[Hannes, James, Christoph: sorry for the extra copies -- my mail client
didn't automatically convert to plain-text for the list, so it bounced.]
Could someone enlighten me on when and where a driver might want to or
be required to set the "tagged_supported" flag?
A quick grep yields a number of places where the flag value is set or
cleared.
This looks like the mechanism which is supposed to set the flag (e.g.,
if the device/LUN characteristics suggest that it's appropriate), and so
I'm not sure when/why the driver would want to do it explicitly:
./drivers/scsi/scsi_scan.c: sdev->tagged_supported = 1; [in scsi_add_lun()]
These look like attempts to override the SCSI mid-layer:
./drivers/scsi/fnic/fnic_main.c: sdev->tagged_supported = 1; [in '_slave_alloc()]
./drivers/scsi/stex.c: sdev->tagged_supported = 1; [in '_slave_alloc()]
./drivers/scsi/stex.c: sdev->tagged_supported = 1; [in '_slave_configure()]
./drivers/scsi/qla4xxx/ql4_os.c: sdev->tagged_supported = 1; [in '_slave_alloc()]
./drivers/scsi/qla4xxx/ql4_os.c: sdev->tagged_supported = 1; [in '_slave_configure()]
./drivers/scsi/ufs/ufshcd.c: sdev->tagged_supported = 1; [in '_slave_alloc()]
./drivers/scsi/scsi_debug.c: sdp->tagged_supported = 1; [in '_slave_configure()]
However, it strikes me that it's unnecessary to set the flag in /both/
'_slave_alloc() and '_slave_configure() -- I think that either one
should work, and '_slave_configure() seems like the appropriate choice.
(But, I'm left wondering why these drivers need to override the SCSI
ML's choice....)
These are interesting, but not pertinent to the discussion at hand (they
seem to be fall-backs triggered during error recovery):
./drivers/scsi/atari_NCR5380.c: cmd->device->tagged_supported = 0; [error recovery?]
./drivers/scsi/53c700.c: SCp->device->tagged_supported = 0; [error recovery?]
./drivers/scsi/sun3_NCR5380.c: cmd->device->tagged_supported = 0; [error recovery?]
And, this one looks spurious (that is, I believe it's clearing the flag
when it's already clear):
./drivers/scsi/libsas/sas_scsi_host.c: scsi_dev->tagged_supported = 0; [looks spurious]
So, taking the patch below, for example, why is tagged_supported being
set in /both/ the '_slave_alloc() and the '_slave_configure() routines,
and, in fact, why is it being set at all -- why doesn't the SCSI
mid-layer set it correctly in this case?
Thanks,
Webb
On 9/29/14 7:47 AM, Hannes Reinecke wrote:
Megaraid_sas uses a shared pool of commands per HBA, so we
should be enabling a shared tag map.
This will allow the I/O scheduler to make better scheduling
decisions and will avoid BUSY states in the driver.
Signed-off-by: Hannes Reinecke<hare@xxxxxxx>
---
drivers/scsi/megaraid/megaraid_sas_base.c | 34 ++++++++++++++++++++++++++++++-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index f6a69a3..996fa9a 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -1659,7 +1659,7 @@ static int megasas_slave_configure(struct scsi_device *sdev)
*/
blk_queue_rq_timeout(sdev->request_queue,
MEGASAS_DEFAULT_CMD_TIMEOUT * HZ);
-
+ sdev->tagged_supported = 1;
return 0;
}
@@ -1667,6 +1667,10 @@ static int megasas_slave_alloc(struct scsi_device *sdev)
{
u16 pd_index = 0;
struct megasas_instance *instance ;
+
+ /* We have a shared tag map, so TCQ is always supported */
+ sdev->tagged_supported = 1;
+
instance = megasas_lookup_instance(sdev->host->host_no);
if (sdev->channel < MEGASAS_MAX_PD_CHANNELS) {
/*
@@ -1677,13 +1681,20 @@ static int megasas_slave_alloc(struct scsi_device *sdev)
sdev->id;
if (instance->pd_list[pd_index].driveState ==
MR_PD_STATE_SYSTEM) {
+ scsi_activate_tcq(sdev, MEGASAS_DEFAULT_CMD_PER_LUN);
return 0;
}
return -ENXIO;
}
+ scsi_activate_tcq(sdev, MEGASAS_DEFAULT_CMD_PER_LUN);
return 0;
}
+static void megasas_slave_destroy(struct scsi_device *sdev)
+{
+ scsi_deactivate_tcq(sdev, 1);
+}
+
void megaraid_sas_kill_hba(struct megasas_instance *instance)
{
if ((instance->pdev->device == PCI_DEVICE_ID_LSI_SAS0073SKINNY) ||
@@ -2746,6 +2757,21 @@ struct device_attribute *megaraid_host_attrs[] = {
NULL,
};
+static int megasas_change_queue_type(struct scsi_device *sdev, int tag_type)
+{
+ if (sdev->host->bqt && sdev->tagged_supported) {
+ scsi_set_tag_type(sdev, tag_type);
+
+ if (tag_type)
+ scsi_activate_tcq(sdev, sdev->queue_depth);
+ else
+ scsi_deactivate_tcq(sdev, sdev->queue_depth);
+ } else
+ tag_type = 0;
+
+ return tag_type;
+}
+
/*
* Scsi host template for megaraid_sas driver
*/
@@ -2756,6 +2782,7 @@ static struct scsi_host_template megasas_template = {
.proc_name = "megaraid_sas",
.slave_configure = megasas_slave_configure,
.slave_alloc = megasas_slave_alloc,
+ .slave_destroy = megasas_slave_destroy,
.queuecommand = megasas_queue_command,
.eh_device_reset_handler = megasas_reset_device,
.eh_bus_reset_handler = megasas_reset_bus_host,
@@ -2765,6 +2792,7 @@ static struct scsi_host_template megasas_template = {
.bios_param = megasas_bios_param,
.use_clustering = ENABLE_CLUSTERING,
.change_queue_depth = megasas_change_queue_depth,
+ .change_queue_type = megasas_change_queue_type,
.no_write_same = 1,
};
@@ -4909,6 +4937,10 @@ static int megasas_io_attach(struct megasas_instance *instance)
host->this_id = instance->init_id;
host->sg_tablesize = instance->max_num_sge;
+ if (!scsi_init_shared_tag_map(host, host->can_queue))
+ printk(KERN_WARNING "megasas: enable TCQ support, depth %d",
+ host->can_queue);
+
if (instance->fw_support_ieee)
instance->max_sectors_per_req = MEGASAS_MAX_SECTORS_IEEE;
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
