[PATCH] block: add support for shared tag maps

[PATCH] block: add support for shared tag maps

[James Bottomley]

[PATCH] block: add support for shared tag maps

The current block queue implementation already contains most of the
machinery for shared tag maps.  The only remaining pieces are a way to
allocate and destroy a tag map independently of the queues (so that
the maps can be managed on the life cycle of the overseeing entity)


Index: linux-2.6/block/ll_rw_blk.c
===================================================================
--- linux-2.6.orig/block/ll_rw_blk.c
+++ linux-2.6/block/ll_rw_blk.c
@@ -848,6 +848,34 @@ struct request *blk_queue_find_tag(reque
 EXPORT_SYMBOL(blk_queue_find_tag);
 
 /**
+ * __blk_free_tags - release a given set of tag maintenance info
+ *
+ * @bqt:	the tag map to free
+ *
+ * Tries to free the specified @bqt@.  Returns true if it was
+ * actually freed and false if there are still references using it
+ */
+static int __blk_free_tags(struct blk_queue_tag *bqt)
+{
+	int retval = atomic_dec_and_test(&bqt->refcnt);
+	if (retval) {
+		BUG_ON(bqt->busy);
+		BUG_ON(!list_empty(&bqt->busy_list));
+
+		kfree(bqt->tag_index);
+		bqt->tag_index = NULL;
+
+		kfree(bqt->tag_map);
+		bqt->tag_map = NULL;
+
+		kfree(bqt);
+
+	}
+
+	return retval;
+}
+
+/**
  * __blk_queue_free_tags - release tag maintenance info
  * @q:  the request queue for the device
  *
@@ -862,23 +890,28 @@ static void __blk_queue_free_tags(reques
 	if (!bqt)
 		return;
 
-	if (atomic_dec_and_test(&bqt->refcnt)) {
-		BUG_ON(bqt->busy);
-		BUG_ON(!list_empty(&bqt->busy_list));
-
-		kfree(bqt->tag_index);
-		bqt->tag_index = NULL;
-
-		kfree(bqt->tag_map);
-		bqt->tag_map = NULL;
-
-		kfree(bqt);
-	}
+	__blk_free_tags(bqt);
 
 	q->queue_tags = NULL;
 	q->queue_flags &= ~(1 << QUEUE_FLAG_QUEUED);
 }
 
+
+/**
+ * blk_free_tags - release a given set of tag maintenance info
+ *
+ * @bqt:	the tag map to free
+ *
+ * For externally managed @bqt@ frees the map.  Callers of this
+ * function must guarantee to have released all the queues that
+ * might have been using this tag map.
+ */
+void blk_free_tags(struct blk_queue_tag *bqt)
+{
+	if (unlikely(!__blk_free_tags(bqt)))
+		BUG();
+}
+
 /**
  * blk_queue_free_tags - release tag maintenance info
  * @q:  the request queue for the device
@@ -901,7 +934,7 @@ init_tag_map(request_queue_t *q, struct 
 	unsigned long *tag_map;
 	int nr_ulongs;
 
-	if (depth > q->nr_requests * 2) {
+	if (q && depth > q->nr_requests * 2) {
 		depth = q->nr_requests * 2;
 		printk(KERN_ERR "%s: adjusted depth to %d\n",
 				__FUNCTION__, depth);
@@ -927,6 +960,38 @@ fail:
 	return -ENOMEM;
 }
 
+static struct blk_queue_tag *__blk_queue_init_tags(struct request_queue *q,
+						   int depth)
+{
+	struct blk_queue_tag *tags;
+
+	tags = kmalloc(sizeof(struct blk_queue_tag), GFP_ATOMIC);
+	if (!tags)
+		goto fail;
+
+	if (init_tag_map(q, tags, depth))
+		goto fail;
+
+	INIT_LIST_HEAD(&tags->busy_list);
+	tags->busy = 0;
+	atomic_set(&tags->refcnt, 1);
+	return tags;
+ fail:
+	return NULL;
+}
+
+/**
+ * blk_init_tags - initialize the tag info for an external tag map
+ *
+ * @depth:	the maximum queue depth supported
+ * @tags: the tag to use
+ **/
+struct blk_queue_tag *blk_init_tags(int depth)
+{
+	return __blk_queue_init_tags(NULL, depth);
+}
+EXPORT_SYMBOL(blk_init_tags);
+
 /**
  * blk_queue_init_tags - initialize the queue tag info
  * @q:  the request queue for the device
@@ -941,16 +1006,10 @@ int blk_queue_init_tags(request_queue_t 
 	BUG_ON(tags && q->queue_tags && tags != q->queue_tags);
 
 	if (!tags && !q->queue_tags) {
-		tags = kmalloc(sizeof(struct blk_queue_tag), GFP_ATOMIC);
-		if (!tags)
-			goto fail;
+		tags = __blk_queue_init_tags(q, depth);
 
-		if (init_tag_map(q, tags, depth))
+		if (!tags)
 			goto fail;
-
-		INIT_LIST_HEAD(&tags->busy_list);
-		tags->busy = 0;
-		atomic_set(&tags->refcnt, 1);
 	} else if (q->queue_tags) {
 		if ((rc = blk_queue_resize_tags(q, depth)))
 			return rc;
@@ -1002,6 +1061,13 @@ int blk_queue_resize_tags(request_queue_
 	}
 
 	/*
+	 * Currently cannot replace a shared tag map with a new
+	 * one, so error out if this is the case
+	 */
+	if (atomic_read(&bqt->refcnt) != 1)
+		return -EBUSY;
+
+	/*
 	 * save the old state info, so we can copy it back
 	 */
 	tag_index = bqt->tag_index;

Re: [PATCH] block: add support for shared tag maps

[Randy.Dunlap]

On Wed, 30 Aug 2006 09:44:18 -0400 James Bottomley wrote:

> [PATCH] block: add support for shared tag maps
> 
> The current block queue implementation already contains most of the
> machinery for shared tag maps.  The only remaining pieces are a way to
> allocate and destroy a tag map independently of the queues (so that
> the maps can be managed on the life cycle of the overseeing entity)
> 

kernel-doc only:  no blank line between the function name
and its parameter(s) list.

> Index: linux-2.6/block/ll_rw_blk.c
> ===================================================================
> --- linux-2.6.orig/block/ll_rw_blk.c
> +++ linux-2.6/block/ll_rw_blk.c
> @@ -848,6 +848,34 @@ struct request *blk_queue_find_tag(reque
>  EXPORT_SYMBOL(blk_queue_find_tag);
>  
>  /**
> + * __blk_free_tags - release a given set of tag maintenance info
> + *

drop

> + * @bqt:	the tag map to free
> + *
> + * Tries to free the specified @bqt@.  Returns true if it was
> + * actually freed and false if there are still references using it
> + */
> +static int __blk_free_tags(struct blk_queue_tag *bqt)
> +{
> +}
> +
> @@ -862,23 +890,28 @@ static void __blk_queue_free_tags(reques
>  
> +
> +/**
> + * blk_free_tags - release a given set of tag maintenance info
> + *

drop

> + * @bqt:	the tag map to free
> + *
> + * For externally managed @bqt@ frees the map.  Callers of this
> + * function must guarantee to have released all the queues that
> + * might have been using this tag map.
> + */
> +void blk_free_tags(struct blk_queue_tag *bqt)
> +{
> +	if (unlikely(!__blk_free_tags(bqt)))
> +		BUG();
> +}
> +
>  /**
>   * blk_queue_free_tags - release tag maintenance info
>   * @q:  the request queue for the device
> @@ -927,6 +960,38 @@ fail:
> +
> +/**
> + * blk_init_tags - initialize the tag info for an external tag map
> + *

drop

> + * @depth:	the maximum queue depth supported
> + * @tags: the tag to use
> + **/
> +struct blk_queue_tag *blk_init_tags(int depth)
> +{
> +	return __blk_queue_init_tags(NULL, depth);
> +}
> +EXPORT_SYMBOL(blk_init_tags);


---
~Randy

Re: [PATCH] block: add support for shared tag maps

[James Bottomley]

On Wed, 2006-08-30 at 08:31 -0700, Randy.Dunlap wrote:
> kernel-doc only:  no blank line between the function name
> and its parameter(s) list.

OK .. fixed.

James

Re: [PATCH] block: add support for shared tag maps

[Ed Lin]

======= On 2006-08-30 21:44:18, James Bottomley wrote:
>+
>+/**
>+ * blk_free_tags - release a given set of tag maintenance info
>+ *
>+ * @bqt:	the tag map to free
>+ *
>+ * For externally managed @bqt@ frees the map.  Callers of this
>+ * function must guarantee to have released all the queues that
>+ * might have been using this tag map.
>+ */
>+void blk_free_tags(struct blk_queue_tag *bqt)
>+{
>+	if (unlikely(!__blk_free_tags(bqt)))
>+		BUG();
>+}
>+

EXPORT_SYMBOL(blk_free_tags) ?
Also, add new function definitions in blkdev.h?

Re: [PATCH] block: add support for shared tag maps

[Jens Axboe]

On Thu, Aug 31 2006, Ed Lin wrote:
> 
> ======= On 2006-08-30 21:44:18, James Bottomley wrote:
> >+
> >+/**
> >+ * blk_free_tags - release a given set of tag maintenance info
> >+ *
> >+ * @bqt:	the tag map to free
> >+ *
> >+ * For externally managed @bqt@ frees the map.  Callers of this
> >+ * function must guarantee to have released all the queues that
> >+ * might have been using this tag map.
> >+ */
> >+void blk_free_tags(struct blk_queue_tag *bqt)
> >+{
> >+	if (unlikely(!__blk_free_tags(bqt)))
> >+		BUG();
> >+}
> >+
> 
> EXPORT_SYMBOL(blk_free_tags) ?
> Also, add new function definitions in blkdev.h?

Good point. James will be reposting it anyways, the patch needed a few
more fixes (memory leak, coding style).

-- 
Jens Axboe

Re: [PATCH] block: add support for shared tag maps

[James Bottomley]

On Thu, 2006-08-31 at 16:55 +0800, Ed Lin wrote:
> EXPORT_SYMBOL(blk_free_tags) ?
> Also, add new function definitions in blkdev.h?

Yes .. fixed that up when I tried to use it in a module.

So, given these two plus another patch that should fix all of the sync
cache issues, how about this as the final patch for stex (to replace the
[PATCH 3/3] stex: use block layer tagging)?

James

Index: scsi-misc-2.6/drivers/scsi/stex.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/stex.c
+++ scsi-misc-2.6/drivers/scsi/stex.c
@@ -34,6 +34,7 @@
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_tcq.h>
 
 #define DRV_NAME "stex"
 #define ST_DRIVER_VERSION "2.9.0.13"
@@ -95,7 +96,6 @@ enum {
 
 	/* request count, etc. */
 	MU_MAX_REQUEST				= 32,
-	TAG_BITMAP_LENGTH			= MU_MAX_REQUEST,
 
 	/* one message wasted, use MU_MAX_REQUEST+1
 		to handle MU_MAX_REQUEST messages */
@@ -265,7 +265,6 @@ struct st_hba {
 	struct Scsi_Host *host;
 	struct pci_dev *pdev;
 
-	u32 tag;
 	u32 req_head;
 	u32 req_tail;
 	u32 status_head;
@@ -309,40 +308,6 @@ static void stex_gettime(__le32 *time)
 	*(time + 1) = cpu_to_le32((tv.tv_sec >> 16) >> 16);
 }
 
-static u16 __stex_alloc_tag(unsigned long *bitmap)
-{
-	int i;
-	i = find_first_zero_bit(bitmap, TAG_BITMAP_LENGTH);
-	if (i < TAG_BITMAP_LENGTH)
-		__set_bit(i, bitmap);
-	return (u16)i;
-}
-
-static u16 stex_alloc_tag(struct st_hba *hba, unsigned long *bitmap)
-{
-	unsigned long flags;
-	u16 tag;
-
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	tag = __stex_alloc_tag(bitmap);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	return tag;
-}
-
-static void __stex_free_tag(unsigned long *bitmap, u16 tag)
-{
-	__clear_bit((int)tag, bitmap);
-}
-
-static void stex_free_tag(struct st_hba *hba, unsigned long *bitmap, u16 tag)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	__stex_free_tag(bitmap, tag);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-}
-
 static struct status_msg *stex_get_status(struct st_hba *hba)
 {
 	struct status_msg *status =
@@ -535,57 +500,31 @@ stex_send_cmd(struct st_hba *hba, struct
 }
 
 static int
+stex_slave_alloc(struct scsi_device *sdev)
+{
+	/* Cheat: usually extracted from Inquiry data */
+	sdev->tagged_supported = 1;
+
+	scsi_activate_tcq(sdev, sdev->host->can_queue);
+
+	return 0;
+}
+
+static int
 stex_slave_config(struct scsi_device *sdev)
 {
 	sdev->use_10_for_rw = 1;
 	sdev->use_10_for_ms = 1;
 	sdev->timeout = 60 * HZ;
+	sdev->tagged_supported = 1;
+
 	return 0;
 }
 
 static void
 stex_slave_destroy(struct scsi_device *sdev)
 {
-	struct st_hba *hba = (struct st_hba *) sdev->host->hostdata;
-	struct req_msg *req;
-	unsigned long flags;
-	unsigned long before;
-	u16 tag;
-
-	if (sdev->type != TYPE_DISK)
-		return;
-
-	before = jiffies;
-	while ((tag = stex_alloc_tag(hba, (unsigned long *)&hba->tag))
-		== TAG_BITMAP_LENGTH) {
-		if (time_after(jiffies, before + ST_INTERNAL_TIMEOUT * HZ))
-			return;
-		msleep(10);
-	}
-
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	req = stex_alloc_req(hba);
-	memset(req->cdb, 0, STEX_CDB_LENGTH);
-
-	req->target = sdev->id;
-	req->lun = sdev->channel; /* firmware lun issue work around */
-	req->cdb[0] = SYNCHRONIZE_CACHE;
-
-	hba->ccb[tag].cmd = NULL;
-	hba->ccb[tag].sg_count = 0;
-	hba->ccb[tag].sense_bufflen = 0;
-	hba->ccb[tag].sense_buffer = NULL;
-	hba->ccb[tag].req_type |= PASSTHRU_REQ_TYPE;
-
-	stex_send_cmd(hba, req, tag);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-
-	wait_event_timeout(hba->waitq,
-		!(hba->ccb[tag].req_type), ST_INTERNAL_TIMEOUT * HZ);
-	if (hba->ccb[tag].req_type & PASSTHRU_REQ_TYPE)
-		return;
-
-	stex_free_tag(hba, (unsigned long *)&hba->tag, tag);
+	scsi_deactivate_tcq(sdev, 1);
 }
 
 static int
@@ -650,8 +589,9 @@ stex_queuecommand(struct scsi_cmnd *cmd,
 
 	cmd->scsi_done = done;
 
-	if (unlikely((tag = __stex_alloc_tag((unsigned long *)&hba->tag))
-		== TAG_BITMAP_LENGTH))
+	tag = cmd->request->tag;
+	
+	if (unlikely(tag >= host->can_queue))
 		return SCSI_MLQUEUE_HOST_BUSY;
 
 	req = stex_alloc_req(hba);
@@ -771,26 +711,18 @@ static void stex_mu_intr(struct st_hba *
 	while (hba->status_tail != hba->status_head) {
 		resp = stex_get_status(hba);
 		tag = le16_to_cpu(resp->tag);
-		if (unlikely(tag >= TAG_BITMAP_LENGTH)) {
+		if (unlikely(tag >= hba->host->can_queue)) {
 			printk(KERN_WARNING DRV_NAME
 				"(%s): invalid tag\n", pci_name(hba->pdev));
 			continue;
 		}
-		if (unlikely((hba->tag & (1 << tag)) == 0)) {
-			printk(KERN_WARNING DRV_NAME
-				"(%s): null tag\n", pci_name(hba->pdev));
-			continue;
-		}
 
-		hba->out_req_cnt--;
 		ccb = &hba->ccb[tag];
 		if (hba->wait_ccb == ccb)
 			hba->wait_ccb = NULL;
 		if (unlikely(ccb->req == NULL)) {
 			printk(KERN_WARNING DRV_NAME
 				"(%s): lagging req\n", pci_name(hba->pdev));
-			__stex_free_tag((unsigned long *)&hba->tag, tag);
-			stex_unmap_sg(hba, ccb->cmd); /* ??? */
 			continue;
 		}
 
@@ -808,7 +740,15 @@ static void stex_mu_intr(struct st_hba *
 		ccb->srb_status = resp->srb_status;
 		ccb->scsi_status = resp->scsi_status;
 
-		if (ccb->req_type & PASSTHRU_REQ_TYPE) {
+		if (likely(ccb->cmd != NULL)) {
+			if (unlikely(ccb->cmd->cmnd[0] == PASSTHRU_CMD &&
+				ccb->cmd->cmnd[1] == PASSTHRU_GET_ADAPTER))
+				stex_controller_info(hba, ccb);
+			stex_unmap_sg(hba, ccb->cmd);
+			stex_scsi_done(ccb);
+			hba->out_req_cnt--;
+		} else if (ccb->req_type & PASSTHRU_REQ_TYPE) {
+			hba->out_req_cnt--;
 			if (ccb->req_type & PASSTHRU_REQ_NO_WAKEUP) {
 				ccb->req_type = 0;
 				continue;
@@ -816,14 +756,7 @@ static void stex_mu_intr(struct st_hba *
 			ccb->req_type = 0;
 			if (waitqueue_active(&hba->waitq))
 				wake_up(&hba->waitq);
-			continue;
 		}
-		if (ccb->cmd->cmnd[0] == PASSTHRU_CMD &&
-			ccb->cmd->cmnd[1] == PASSTHRU_GET_ADAPTER)
-			stex_controller_info(hba, ccb);
-		__stex_free_tag((unsigned long *)&hba->tag, tag);
-		stex_unmap_sg(hba, ccb->cmd);
-		stex_scsi_done(ccb);
 	}
 
 update_status:
@@ -933,21 +866,24 @@ static int stex_abort(struct scsi_cmnd *
 {
 	struct Scsi_Host *host = cmd->device->host;
 	struct st_hba *hba = (struct st_hba *)host->hostdata;
-	u16 tag;
+	u16 tag = cmd->request->tag;
 	void __iomem *base;
 	u32 data;
 	int result = SUCCESS;
 	unsigned long flags;
 	base = hba->mmio_base;
 	spin_lock_irqsave(host->host_lock, flags);
-
-	for (tag = 0; tag < MU_MAX_REQUEST; tag++)
-		if (hba->ccb[tag].cmd == cmd && (hba->tag & (1 << tag))) {
-			hba->wait_ccb = &(hba->ccb[tag]);
-			break;
-		}
-	if (tag >= MU_MAX_REQUEST)
-		goto out;
+	if (tag < host->can_queue && hba->ccb[tag].cmd == cmd)
+		hba->wait_ccb = &hba->ccb[tag];
+	else {
+		for (tag = 0; tag < host->can_queue; tag++)
+			if (hba->ccb[tag].cmd == cmd) {
+				hba->wait_ccb = &hba->ccb[tag];
+				break;
+			}
+		if (tag >= host->can_queue)
+			goto out;
+	}
 
 	data = readl(base + ODBL);
 	if (data == 0 || data == 0xffffffff)
@@ -965,6 +901,7 @@ static int stex_abort(struct scsi_cmnd *
 	}
 
 fail_out:
+	stex_unmap_sg(hba, cmd);
 	hba->wait_ccb->req = NULL; /* nullify the req's future return */
 	hba->wait_ccb = NULL;
 	result = FAILED;
@@ -1025,7 +962,6 @@ static int stex_reset(struct scsi_cmnd *
 		return FAILED;
 	}
 	spin_lock_irqsave(hba->host->host_lock, flags);
-	hba->tag = 0;
 	hba->req_head = 0;
 	hba->req_tail = 0;
 	hba->status_head = 0;
@@ -1061,6 +997,7 @@ static struct scsi_host_template driver_
 	.proc_name			= DRV_NAME,
 	.bios_param			= stex_biosparam,
 	.queuecommand			= stex_queuecommand,
+	.slave_alloc			= stex_slave_alloc,
 	.slave_configure		= stex_slave_config,
 	.slave_destroy			= stex_slave_destroy,
 	.eh_abort_handler		= stex_abort,
@@ -1180,6 +1117,8 @@ stex_probe(struct pci_dev *pdev, const s
 		goto out_free_irq;
 	}
 
+	scsi_init_shared_tag_map(host, ST_CAN_QUEUE);
+
 	scsi_scan_host(host);
 
 	return 0;
@@ -1201,46 +1140,6 @@ out_disable:
 	return err;
 }
 
-static void stex_hba_stop(struct st_hba *hba)
-{
-	struct req_msg *req;
-	unsigned long flags;
-	unsigned long before;
-	u16 tag;
-
-	before = jiffies;
-	while ((tag = stex_alloc_tag(hba, (unsigned long *)&hba->tag))
-		== TAG_BITMAP_LENGTH) {
-		if (time_after(jiffies, before + ST_INTERNAL_TIMEOUT * HZ))
-			return;
-		msleep(10);
-	}
-
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	req = stex_alloc_req(hba);
-	memset(req->cdb, 0, STEX_CDB_LENGTH);
-
-	req->cdb[0] = CONTROLLER_CMD;
-	req->cdb[1] = CTLR_POWER_STATE_CHANGE;
-	req->cdb[2] = CTLR_POWER_SAVING;
-
-	hba->ccb[tag].cmd = NULL;
-	hba->ccb[tag].sg_count = 0;
-	hba->ccb[tag].sense_bufflen = 0;
-	hba->ccb[tag].sense_buffer = NULL;
-	hba->ccb[tag].req_type |= PASSTHRU_REQ_TYPE;
-
-	stex_send_cmd(hba, req, tag);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-
-	wait_event_timeout(hba->waitq,
-		!(hba->ccb[tag].req_type), ST_INTERNAL_TIMEOUT * HZ);
-	if (hba->ccb[tag].req_type & PASSTHRU_REQ_TYPE)
-		return;
-
-	stex_free_tag(hba, (unsigned long *)&hba->tag, tag);
-}
-
 static void stex_hba_free(struct st_hba *hba)
 {
 	free_irq(hba->pdev->irq, hba);
@@ -1261,8 +1160,6 @@ static void stex_remove(struct pci_dev *
 
 	pci_set_drvdata(pdev, NULL);
 
-	stex_hba_stop(hba);
-
 	stex_hba_free(hba);
 
 	scsi_host_put(hba->host);
@@ -1270,13 +1167,6 @@ static void stex_remove(struct pci_dev *
 	pci_disable_device(pdev);
 }
 
-static void stex_shutdown(struct pci_dev *pdev)
-{
-	struct st_hba *hba = pci_get_drvdata(pdev);
-
-	stex_hba_stop(hba);
-}
-
 static struct pci_device_id stex_pci_tbl[] = {
 	{ 0x105a, 0x8350, PCI_ANY_ID, PCI_ANY_ID, 0, 0, st_shasta },
 	{ 0x105a, 0xc350, PCI_ANY_ID, PCI_ANY_ID, 0, 0, st_shasta },
@@ -1295,7 +1185,6 @@ static struct pci_driver stex_pci_driver
 	.id_table	= stex_pci_tbl,
 	.probe		= stex_probe,
 	.remove		= __devexit_p(stex_remove),
-	.shutdown	= stex_shutdown,
 };
 
 static int __init stex_init(void)

Re: [PATCH] block: add support for shared tag maps

[Ed Lin]

======= On 2006-09-01 06:21:45, James Bottomley wrote:

>On Thu, 2006-08-31 at 16:55 +0800, Ed Lin wrote:
>> EXPORT_SYMBOL(blk_free_tags) ?
>> Also, add new function definitions in blkdev.h?
>
>Yes .. fixed that up when I tried to use it in a module.
>
>So, given these two plus another patch that should fix all of the sync
>cache issues, how about this as the final patch for stex (to replace the
>[PATCH 3/3] stex: use block layer tagging)?
>

Thanks. It's good. But here are a few issues.

1.Maybe we need to check the result of scsi_init_shared_tag_map.
It's unlikely to fail. In case it really fails, we can simply exit
since we need tagging. Just checking.

2.During shutdown/unload, we need to notify firmware to stop some
background activities, that we are going to shutdown. If we don't do this,
and the power is switched off when firmware is not ready, there may be
error. It's possible the firmware is doing something that need to be
stopped before shutdown. So it's different from a simple cache issue.

So it's better to keep the stex_hba_stop function and the .shutdown
entry. In normal cases, there is no outside command at the stage when
shutdown functions are called. So we can safely assume the tag is 0,
and issue the notifying command, and then exit. It's only one and it
will be issued only once. This way we can guarantee data integrity,
and remove internal tag handling functions at the same time.

I agree with the other parts of the patch. So maybe we can consider
something like the following?



diff --git a/drivers/scsi/stex.c b/drivers/scsi/stex.c
index fd09330..15fb99f 100644
--- a/drivers/scsi/stex.c
+++ b/drivers/scsi/stex.c
@@ -34,6 +34,7 @@ #include <scsi/scsi.h>
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_tcq.h>
 
 #define DRV_NAME "stex"
 #define ST_DRIVER_VERSION "2.9.0.13"
@@ -95,7 +96,6 @@ enum {
 
 	/* request count, etc. */
 	MU_MAX_REQUEST				= 32,
-	TAG_BITMAP_LENGTH			= MU_MAX_REQUEST,
 
 	/* one message wasted, use MU_MAX_REQUEST+1
 		to handle MU_MAX_REQUEST messages */
@@ -265,7 +265,6 @@ struct st_hba {
 	struct Scsi_Host *host;
 	struct pci_dev *pdev;
 
-	u32 tag;
 	u32 req_head;
 	u32 req_tail;
 	u32 status_head;
@@ -309,40 +308,6 @@ static void stex_gettime(__le32 *time)
 	*(time + 1) = cpu_to_le32((tv.tv_sec >> 16) >> 16);
 }
 
-static u16 __stex_alloc_tag(unsigned long *bitmap)
-{
-	int i;
-	i = find_first_zero_bit(bitmap, TAG_BITMAP_LENGTH);
-	if (i < TAG_BITMAP_LENGTH)
-		__set_bit(i, bitmap);
-	return (u16)i;
-}
-
-static u16 stex_alloc_tag(struct st_hba *hba, unsigned long *bitmap)
-{
-	unsigned long flags;
-	u16 tag;
-
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	tag = __stex_alloc_tag(bitmap);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-	return tag;
-}
-
-static void __stex_free_tag(unsigned long *bitmap, u16 tag)
-{
-	__clear_bit((int)tag, bitmap);
-}
-
-static void stex_free_tag(struct st_hba *hba, unsigned long *bitmap, u16 tag)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	__stex_free_tag(bitmap, tag);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-}
-
 static struct status_msg *stex_get_status(struct st_hba *hba)
 {
 	struct status_msg *status =
@@ -535,57 +500,31 @@ stex_send_cmd(struct st_hba *hba, struct
 }
 
 static int
+stex_slave_alloc(struct scsi_device *sdev)
+{
+	/* Cheat: usually extracted from Inquiry data */
+	sdev->tagged_supported = 1;
+
+	scsi_activate_tcq(sdev, sdev->host->can_queue);
+
+	return 0;
+}
+
+static int
 stex_slave_config(struct scsi_device *sdev)
 {
 	sdev->use_10_for_rw = 1;
 	sdev->use_10_for_ms = 1;
 	sdev->timeout = 60 * HZ;
+	sdev->tagged_supported = 1;
+
 	return 0;
 }
 
 static void
 stex_slave_destroy(struct scsi_device *sdev)
 {
-	struct st_hba *hba = (struct st_hba *) sdev->host->hostdata;
-	struct req_msg *req;
-	unsigned long flags;
-	unsigned long before;
-	u16 tag;
-
-	if (sdev->type != TYPE_DISK)
-		return;
-
-	before = jiffies;
-	while ((tag = stex_alloc_tag(hba, (unsigned long *)&hba->tag))
-		== TAG_BITMAP_LENGTH) {
-		if (time_after(jiffies, before + ST_INTERNAL_TIMEOUT * HZ))
-			return;
-		msleep(10);
-	}
-
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	req = stex_alloc_req(hba);
-	memset(req->cdb, 0, STEX_CDB_LENGTH);
-
-	req->target = sdev->id;
-	req->lun = sdev->channel; /* firmware lun issue work around */
-	req->cdb[0] = SYNCHRONIZE_CACHE;
-
-	hba->ccb[tag].cmd = NULL;
-	hba->ccb[tag].sg_count = 0;
-	hba->ccb[tag].sense_bufflen = 0;
-	hba->ccb[tag].sense_buffer = NULL;
-	hba->ccb[tag].req_type |= PASSTHRU_REQ_TYPE;
-
-	stex_send_cmd(hba, req, tag);
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-
-	wait_event_timeout(hba->waitq,
-		!(hba->ccb[tag].req_type), ST_INTERNAL_TIMEOUT * HZ);
-	if (hba->ccb[tag].req_type & PASSTHRU_REQ_TYPE)
-		return;
-
-	stex_free_tag(hba, (unsigned long *)&hba->tag, tag);
+	scsi_deactivate_tcq(sdev, 1);
 }
 
 static int
@@ -650,8 +589,9 @@ stex_queuecommand(struct scsi_cmnd *cmd,
 
 	cmd->scsi_done = done;
 
-	if (unlikely((tag = __stex_alloc_tag((unsigned long *)&hba->tag))
-		== TAG_BITMAP_LENGTH))
+	tag = cmd->request->tag;
+
+	if (unlikely(tag >= host->can_queue))
 		return SCSI_MLQUEUE_HOST_BUSY;
 
 	req = stex_alloc_req(hba);
@@ -771,26 +711,18 @@ static void stex_mu_intr(struct st_hba *
 	while (hba->status_tail != hba->status_head) {
 		resp = stex_get_status(hba);
 		tag = le16_to_cpu(resp->tag);
-		if (unlikely(tag >= TAG_BITMAP_LENGTH)) {
+		if (unlikely(tag >= hba->host->can_queue)) {
 			printk(KERN_WARNING DRV_NAME
 				"(%s): invalid tag\n", pci_name(hba->pdev));
 			continue;
 		}
-		if (unlikely((hba->tag & (1 << tag)) == 0)) {
-			printk(KERN_WARNING DRV_NAME
-				"(%s): null tag\n", pci_name(hba->pdev));
-			continue;
-		}
 
-		hba->out_req_cnt--;
 		ccb = &hba->ccb[tag];
 		if (hba->wait_ccb == ccb)
 			hba->wait_ccb = NULL;
 		if (unlikely(ccb->req == NULL)) {
 			printk(KERN_WARNING DRV_NAME
 				"(%s): lagging req\n", pci_name(hba->pdev));
-			__stex_free_tag((unsigned long *)&hba->tag, tag);
-			stex_unmap_sg(hba, ccb->cmd); /* ??? */
 			continue;
 		}
 
@@ -808,7 +740,15 @@ static void stex_mu_intr(struct st_hba *
 		ccb->srb_status = resp->srb_status;
 		ccb->scsi_status = resp->scsi_status;
 
-		if (ccb->req_type & PASSTHRU_REQ_TYPE) {
+		if (likely(ccb->cmd != NULL)) {
+			if (unlikely(ccb->cmd->cmnd[0] == PASSTHRU_CMD &&
+				ccb->cmd->cmnd[1] == PASSTHRU_GET_ADAPTER))
+				stex_controller_info(hba, ccb);
+			stex_unmap_sg(hba, ccb->cmd);
+			stex_scsi_done(ccb);
+			hba->out_req_cnt--;
+		} else if (ccb->req_type & PASSTHRU_REQ_TYPE) {
+			hba->out_req_cnt--;
 			if (ccb->req_type & PASSTHRU_REQ_NO_WAKEUP) {
 				ccb->req_type = 0;
 				continue;
@@ -816,14 +756,7 @@ static void stex_mu_intr(struct st_hba *
 			ccb->req_type = 0;
 			if (waitqueue_active(&hba->waitq))
 				wake_up(&hba->waitq);
-			continue;
 		}
-		if (ccb->cmd->cmnd[0] == PASSTHRU_CMD &&
-			ccb->cmd->cmnd[1] == PASSTHRU_GET_ADAPTER)
-			stex_controller_info(hba, ccb);
-		__stex_free_tag((unsigned long *)&hba->tag, tag);
-		stex_unmap_sg(hba, ccb->cmd);
-		stex_scsi_done(ccb);
 	}
 
 update_status:
@@ -933,21 +866,24 @@ static int stex_abort(struct scsi_cmnd *
 {
 	struct Scsi_Host *host = cmd->device->host;
 	struct st_hba *hba = (struct st_hba *)host->hostdata;
-	u16 tag;
+	u16 tag = cmd->request->tag;
 	void __iomem *base;
 	u32 data;
 	int result = SUCCESS;
 	unsigned long flags;
 	base = hba->mmio_base;
 	spin_lock_irqsave(host->host_lock, flags);
-
-	for (tag = 0; tag < MU_MAX_REQUEST; tag++)
-		if (hba->ccb[tag].cmd == cmd && (hba->tag & (1 << tag))) {
-			hba->wait_ccb = &(hba->ccb[tag]);
-			break;
-		}
-	if (tag >= MU_MAX_REQUEST)
-		goto out;
+	if (tag < host->can_queue && hba->ccb[tag].cmd == cmd)
+		hba->wait_ccb = &hba->ccb[tag];
+	else {
+		for (tag = 0; tag < host->can_queue; tag++)
+			if (hba->ccb[tag].cmd == cmd) {
+				hba->wait_ccb = &hba->ccb[tag];
+				break;
+			}
+		if (tag >= host->can_queue)
+			goto out;
+	}
 
 	data = readl(base + ODBL);
 	if (data == 0 || data == 0xffffffff)
@@ -965,6 +901,7 @@ static int stex_abort(struct scsi_cmnd *
 	}
 
 fail_out:
+	stex_unmap_sg(hba, cmd);
 	hba->wait_ccb->req = NULL; /* nullify the req's future return */
 	hba->wait_ccb = NULL;
 	result = FAILED;
@@ -1025,7 +962,6 @@ static int stex_reset(struct scsi_cmnd *
 		return FAILED;
 	}
 	spin_lock_irqsave(hba->host->host_lock, flags);
-	hba->tag = 0;
 	hba->req_head = 0;
 	hba->req_tail = 0;
 	hba->status_head = 0;
@@ -1061,6 +997,7 @@ static struct scsi_host_template driver_
 	.proc_name			= DRV_NAME,
 	.bios_param			= stex_biosparam,
 	.queuecommand			= stex_queuecommand,
+	.slave_alloc			= stex_slave_alloc,
 	.slave_configure		= stex_slave_config,
 	.slave_destroy			= stex_slave_destroy,
 	.eh_abort_handler		= stex_abort,
@@ -1171,6 +1108,14 @@ stex_probe(struct pci_dev *pdev, const s
 	if (err)
 		goto out_free_irq;
 
+	scsi_init_shared_tag_map(host, ST_CAN_QUEUE);
+	if (host->bqt == NULL) {
+		err = -ENOMEM;
+		printk(KERN_ERR DRV_NAME "(%s): init shared queue failed\n",
+			pci_name(pdev));
+		goto out_free_irq;
+	}
+
 	pci_set_drvdata(pdev, hba);
 
 	err = scsi_add_host(host, &pdev->dev);
@@ -1206,15 +1151,7 @@ static void stex_hba_stop(struct st_hba 
 	struct req_msg *req;
 	unsigned long flags;
 	unsigned long before;
-	u16 tag;
-
-	before = jiffies;
-	while ((tag = stex_alloc_tag(hba, (unsigned long *)&hba->tag))
-		== TAG_BITMAP_LENGTH) {
-		if (time_after(jiffies, before + ST_INTERNAL_TIMEOUT * HZ))
-			return;
-		msleep(10);
-	}
+	u16 tag = 0;
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
 	req = stex_alloc_req(hba);
@@ -1233,12 +1170,12 @@ static void stex_hba_stop(struct st_hba 
 	stex_send_cmd(hba, req, tag);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
-	wait_event_timeout(hba->waitq,
-		!(hba->ccb[tag].req_type), ST_INTERNAL_TIMEOUT * HZ);
-	if (hba->ccb[tag].req_type & PASSTHRU_REQ_TYPE)
-		return;
-
-	stex_free_tag(hba, (unsigned long *)&hba->tag, tag);
+	before = jiffies;
+	while (hba->ccb[tag].req_type & PASSTHRU_REQ_TYPE) {
+		if (time_after(jiffies, before + ST_INTERNAL_TIMEOUT * HZ))
+			return;
+		msleep(10);
+	}
 }
 
 static void stex_hba_free(struct st_hba *hba)

Re: [PATCH] block: add support for shared tag maps

[James Bottomley]

On Fri, 2006-09-01 at 14:31 +0800, Ed Lin wrote:
> ======= On 2006-09-01 06:21:45, James Bottomley wrote:
> 
> >On Thu, 2006-08-31 at 16:55 +0800, Ed Lin wrote:
> >> EXPORT_SYMBOL(blk_free_tags) ?
> >> Also, add new function definitions in blkdev.h?
> >
> >Yes .. fixed that up when I tried to use it in a module.
> >
> >So, given these two plus another patch that should fix all of the sync
> >cache issues, how about this as the final patch for stex (to replace the
> >[PATCH 3/3] stex: use block layer tagging)?
> >
> 
> Thanks. It's good. But here are a few issues.
> 
> 1.Maybe we need to check the result of scsi_init_shared_tag_map.
> It's unlikely to fail. In case it really fails, we can simply exit
> since we need tagging. Just checking.

Yes, we should.  I generalised your error checking below.

> 2.During shutdown/unload, we need to notify firmware to stop some
> background activities, that we are going to shutdown. If we don't do this,
> and the power is switched off when firmware is not ready, there may be
> error. It's possible the firmware is doing something that need to be
> stopped before shutdown. So it's different from a simple cache issue.
> 
> So it's better to keep the stex_hba_stop function and the .shutdown
> entry. In normal cases, there is no outside command at the stage when
> shutdown functions are called. So we can safely assume the tag is 0,
> and issue the notifying command, and then exit. It's only one and it
> will be issued only once. This way we can guarantee data integrity,
> and remove internal tag handling functions at the same time.
> 
> I agree with the other parts of the patch. So maybe we can consider
> something like the following?

Yes, I think we have a winner.

James

Index: scsi-misc-2.6/drivers/scsi/stex.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/stex.c
+++ scsi-misc-2.6/drivers/scsi/stex.c
@@ -1108,9 +1108,8 @@ stex_probe(struct pci_dev *pdev, const s
 	if (err)
 		goto out_free_irq;
 
-	scsi_init_shared_tag_map(host, ST_CAN_QUEUE);
-	if (host->bqt == NULL) {
-		err = -ENOMEM;
+	err = scsi_init_shared_tag_map(host, ST_CAN_QUEUE);
+	if (err) {
 		printk(KERN_ERR DRV_NAME "(%s): init shared queue failed\n",
 			pci_name(pdev));
 		goto out_free_irq;
Index: scsi-misc-2.6/include/scsi/scsi_tcq.h
===================================================================
--- scsi-misc-2.6.orig/include/scsi/scsi_tcq.h
+++ scsi-misc-2.6/include/scsi/scsi_tcq.h
@@ -138,9 +138,10 @@ static inline struct scsi_cmnd *scsi_fin
  * @shost:	the host to share the tag map among all devices
  * @depth:	the total depth of the map
  */
-static inline void scsi_init_shared_tag_map(struct Scsi_Host *shost, int depth)
+static inline int scsi_init_shared_tag_map(struct Scsi_Host *shost, int depth)
 {
 	shost->bqt = blk_init_tags(depth);
+	return shost->bqt ? 0 : -ENOMEM;
 }
 
 #endif /* _SCSI_SCSI_TCQ_H */

Re: [PATCH] block: add support for shared tag maps

[Doug Maxey]

Ravi,

While working on a patch to add shared tags to qla4xxx was looking at
the shost->can_queue settings, I see the value is set pretty high:

qla4xxx_probe()
...
	host->can_queue = REQUEST_QUEUE_DEPTH + 128;

where REQUEST_QUEUE_DEPTH works out to be 1024.

My question:
what is the relationship between the can_queue and the
setting in
qla4xxx_slave_configure()
	if (sdev->tagged_supported)
		scsi_activate_tcq(sdev, 32);
	else
		scsi_deactivate_tcq(sdev, 32);

Does this imply that the firmware can ultimately track more requests
than we can possibly stuff in it?  Where do the other 1012 requests get
queued, in the block layer?

James' shared tag patch for stex has the following
+stex_slave_alloc(struct scsi_device *sdev)
+{
+	/* Cheat: usually extracted from Inquiry data */
+	sdev->tagged_supported = 1;
+
+	scsi_activate_tcq(sdev, sdev->host->can_queue);

where in this case the can_queue is set to ST_CAN_QUEUE, which works
out to 32.

Is my concern misplaced?

Re: [PATCH] block: add support for shared tag maps

[Mike Anderson]

Doug Maxey <dwm@enoyolf.org> wrote:
> 
> Ravi,
> 
> While working on a patch to add shared tags to qla4xxx was looking at
> the shost->can_queue settings, I see the value is set pretty high:
> 
> qla4xxx_probe()
> ...
> 	host->can_queue = REQUEST_QUEUE_DEPTH + 128;
> 
> where REQUEST_QUEUE_DEPTH works out to be 1024.
> 
> My question:
> what is the relationship between the can_queue and the
> setting in
> qla4xxx_slave_configure()
> 	if (sdev->tagged_supported)
> 		scsi_activate_tcq(sdev, 32);
> 	else
> 		scsi_deactivate_tcq(sdev, 32);
> 
> Does this imply that the firmware can ultimately track more requests
> than we can possibly stuff in it?  Where do the other 1012 requests get
> queued, in the block layer?

Maybe I misreading your question. host->can_queue is the per host instance
(adapter) limit and scsi_activate_tcq will set the per dev (lun) limit.

host->can_queue being large is a good thing (if it is backed by real
resources). In theory can_queue should be scaled to support a hosts
${max_number_of_devices} * ${max_queue_depth}.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [PATCH] block: add support for shared tag maps

[Ravi Anand]

>On Fri, 01 Sep 2006, Doug Maxey wrote:
> Ravi,
> 
> While working on a patch to add shared tags to qla4xxx was looking at
> the shost->can_queue settings, I see the value is set pretty high:
> 
> qla4xxx_probe()
> ...
> 	host->can_queue = REQUEST_QUEUE_DEPTH + 128;
> 
> where REQUEST_QUEUE_DEPTH works out to be 1024.
> 
> My question:
> what is the relationship between the can_queue and the
> setting in
> qla4xxx_slave_configure()
> 	if (sdev->tagged_supported)
> 		scsi_activate_tcq(sdev, 32);
> 	else
> 		scsi_deactivate_tcq(sdev, 32);
> 
> Does this imply that the firmware can ultimately track more requests
> than we can possibly stuff in it?  Where do the other 1012 requests get
> queued, in the block layer?

My understanding is that the "can_queue" is on a per HBA basis.
In other words maximum no of cmd's a give host adapter can
queue at any given point of time. 

While the setting the slave_configure() is on a per lun basis.
Which basically means max no of simultaneous cmd's that
can be outstanding for any given lun.

> 
> James' shared tag patch for stex has the following
> +stex_slave_alloc(struct scsi_device *sdev)
> +{
> +	/* Cheat: usually extracted from Inquiry data */
> +	sdev->tagged_supported = 1;
> +
> +	scsi_activate_tcq(sdev, sdev->host->can_queue);
> 
> where in this case the can_queue is set to ST_CAN_QUEUE, which works
> out to 32.

Dont know much about stex driver. So cant comment on it. 
James any thought ? 

Ravi






-- 
VGER BF report: H 4.75946e-10

Re: [PATCH] block: add support for shared tag maps

[James Bottomley]

On Fri, 2006-09-01 at 13:21 -0700, Ravi Anand wrote:
> Dont know much about stex driver. So cant comment on it. 
> James any thought ? 

The stex driver has a shared tag map (which is tiny:32) so it just sets
the can_queue and the tag depth to the same value.

James



-- 
VGER BF report: H 1.02918e-13

Re: [PATCH] block: add support for shared tag maps

[Christoph Hellwig]

On Fri, Sep 01, 2006 at 02:31:51PM +0800, Ed Lin wrote:
>  static int
> +stex_slave_alloc(struct scsi_device *sdev)
> +{
> +	/* Cheat: usually extracted from Inquiry data */
> +	sdev->tagged_supported = 1;
> +
> +	scsi_activate_tcq(sdev, sdev->host->can_queue);

these two calls look wrong here. scsi_activate_tcq is supposed to be
called from slave_configure.  similarly tagged_supported is probably
going to be overwriten as part of the scanning process, but you already
set it in slave_configure anyway.

> +	tag = cmd->request->tag;
> +
> +	if (unlikely(tag >= host->can_queue))
>  		return SCSI_MLQUEUE_HOST_BUSY;

Do we really need this check?  I'm pretty sure the block layer never
hands out tags bigger than what you told it to with scsi_activate_tcq.

Re: [PATCH] block: add support for shared tag maps

[James Bottomley]

On Mon, 2006-09-18 at 19:47 +0100, Christoph Hellwig wrote:
> these two calls look wrong here. scsi_activate_tcq is supposed to be
> called from slave_configure.  similarly tagged_supported is probably
> going to be overwriten as part of the scanning process, but you already
> set it in slave_configure anyway.

I agree in principle.  However, the driver is doing tags as indexes into
its own internal firmware sequencer.  Thus, the first inquiry that comes
down has to go into the sequencer with a tag.  Hence this charade to
turn on tagging in slave_alloc.


> > +     if (unlikely(tag >= host->can_queue))
> >               return SCSI_MLQUEUE_HOST_BUSY;
> 
> Do we really need this check?  I'm pretty sure the block layer never
> hands out tags bigger than what you told it to with scsi_activate_tcq.

Probably not.

James

Re: [PATCH] block: add support for shared tag maps

[Mike Christie]

On Mon, 2006-09-18 at 19:47 +0100, Christoph Hellwig wrote:
> On Fri, Sep 01, 2006 at 02:31:51PM +0800, Ed Lin wrote:
> >  static int
> > +stex_slave_alloc(struct scsi_device *sdev)
> > +{
> > +	/* Cheat: usually extracted from Inquiry data */
> > +	sdev->tagged_supported = 1;
> > +
> > +	scsi_activate_tcq(sdev, sdev->host->can_queue);
> 
> these two calls look wrong here. scsi_activate_tcq is supposed to be
> called from slave_configure.  similarly tagged_supported is probably
> going to be overwriten as part of the scanning process, but you already
> set it in slave_configure anyway.

If a lld is doing host wide tagging will they always need a tag? For
those drivers it seems like they are not doing tagging based on this in
scsi_scan

        if ((sdev->scsi_level >= SCSI_2) && (inq_result[7] & 2) &&
> >             !(*bflags & BLIST_NOTQ))
> >                 sdev->tagged_supported = 1;

Instead they need a tag because their FW or driver require it.

If so then should we just set this in scsi_alloc_sdev if shost->bqt is
set? For qla4xxx we are doing something like stex which is a little odd.
See patch below. It is untested and not even compile tested - just shows
what I mean and for Qlogic to test if it is ok with you guys.

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index fd9e281..2efd90c 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -206,6 +206,11 @@ static struct scsi_device *scsi_alloc_sd
 
 	scsi_sysfs_device_initialize(sdev);
 
+	if (shost->bqt) {
+		sdev->tagged_supported = 1;
+		scsi_activate_tcq(sdev, shost->can_queue);
+	}
+
 	if (shost->hostt->slave_alloc) {
 		ret = shost->hostt->slave_alloc(sdev);
 		if (ret) {

Re: [PATCH] block: add support for shared tag maps

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 3034 bytes --]

On Mon, 2006-09-18 at 14:25 -0500, Mike Christie wrote:
> On Mon, 2006-09-18 at 19:47 +0100, Christoph Hellwig wrote:
> > On Fri, Sep 01, 2006 at 02:31:51PM +0800, Ed Lin wrote:
> > >  static int
> > > +stex_slave_alloc(struct scsi_device *sdev)
> > > +{
> > > +	/* Cheat: usually extracted from Inquiry data */
> > > +	sdev->tagged_supported = 1;
> > > +
> > > +	scsi_activate_tcq(sdev, sdev->host->can_queue);
> > 
> > these two calls look wrong here. scsi_activate_tcq is supposed to be
> > called from slave_configure.  similarly tagged_supported is probably
> > going to be overwriten as part of the scanning process, but you already
> > set it in slave_configure anyway.
> 
> If a lld is doing host wide tagging will they always need a tag? For
> those drivers it seems like they are not doing tagging based on this in
> scsi_scan

Well, were I to ever convert the aic7xxx_old driver to mid level tags,
then the answer to this is partially yes.  We don't need to pass the tag
to the device, but since the firmware in the aic7xxx driver looks up the
tag of the used slot via a table for untagged commands, we are in danger
of having that tag passed back down to us for another device at another
time while this one is still active.  Therefore, we need to know which
tag slot the mid layer wants allocated even on untagged commands.  In
addition, the aic7xxx_old driver will, even on tagged devices, send some
commands as untagged when needed (some older devices would choke on both
a tag message and an SDTR message in the same command, so the driver
defaults to sending all WDTR/SDTR flagged commands as untagged).  On
those untagged commands, when the device reconnects, the driver looks
into a table indexed by target/lun to find the tag slot of the untagged
command and then DMAs it into card memory from the SCB array.  On tagged
commands, the tag itself was the index into the command array.  So,
that's a quick rundown of what the aic7xxx_old does.  That means that in
order to have the mid layer assign tags, the aic7xxx driver needs *all*
commands, untagged or tagged, to have a tag value and for that tag to be
reserved.  There would then need to be another something, such as tag
type, to indicate if the passed down tag is to be used solely to index
into the command array or if it should be passed to the device as the
tag value.  One possible way to implement this would be to have a flag
in the host struct registration that indicates that the host has a
shared tag array, the mid layer would allocate a static tag array of
can_queue tags, attach it to the host, then have all subsequent devices
in scsi_scan link to that array, and all commands would be sent with an
array slot allocated even if the command is untagged.  Problem solved.

-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: CFBFF194
              http://people.redhat.com/dledford

Infiniband specific RPMs available at
              http://people.redhat.com/dledford/Infiniband

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]