Skip to contentSkip to conversation

hptiop: HighPoint RocketRAID 3xxx controller driver

PATCH New HighPoint Linux Team 3 participants 7 messages Jun 11, 2006 – Jul 24, 2006

Subsystems (2)
SCSI Highpoint rocketraid 3xxx RAID driver
lore
Share & tools
lore

HighPoint Linux Team

author maintainer
#0 lore raw
Quoted 11 lines · James Bottomley
	_req = get_req(hba);
	if (_req == NULL) {
		dprintk("hptiop_queuecmd : no free req\n");
		scp->result = DID_BUS_BUSY << 16;
		goto cmd_done;
	}

This should be doing a return SCSI_MLQUEUE_HOST_BUSY. DID_BUS_BUSY doesn't do the resource contention counting that you want (MLQUEUE_HOST_BUSY will wait until a command returns ... presumably freeing up resources before trying another).

Right, this should be modified.

Quoted 10 lines
	/*
	 * hptiop_shutdown will flash controller cache.
	 */
	if (scp->cmnd[0] == SYNCHRONIZE_CACHE)  {
		scp->result = DID_OK<<16;
		goto cmd_done;
	}

Are you really sure you want to do this? It looks like we'll be doing this in cases where shutdown won't be called (like suspend).

These lines should be removed. The controller firmware will response to SYNCHRONIZE_CACHE command.

Quoted 6 lines
	host->can_queue = le32_to_cpu(iop_config.max_requests);
	host->cmd_per_lun = le32_to_cpu(iop_config.max_requests);

You might want to think about adjusting this. For the single LUN case, it's fine. For the multi-lun case it may allow commands to a single LUN to starve everything else.

There will be no multi-lun support for the controller so this is not an issue.

Signed-off-by HighPoint Linux Team <[email protected]>

---

drivers/scsi/hptiop.c +1 -9
index 8302f3b..7806c45 100644
@@ -504,18 +504,10 @@ static int hptiop_queuecommand(struct sc
 	BUG_ON(!done);
 	scp->scsi_done = done;
 
-	/*
-	 * hptiop_shutdown will flash controller cache.
-	 */
-	if (scp->cmnd[0] == SYNCHRONIZE_CACHE)  {
-		scp->result = DID_OK<<16;
-		goto cmd_done;
-	}
-
 	_req = get_req(hba);
 	if (_req == NULL) {
 		dprintk("hptiop_queuecmd : no free req\n");
-		scp->result = DID_BUS_BUSY << 16;
+		scp->result = SCSI_MLQUEUE_HOST_BUSY;
 		goto cmd_done;
 	}
author maintainer
#1 lore raw
Quoted 10 lines · HighPoint Linux Team
Quoted 7 lines
	host->can_queue = le32_to_cpu(iop_config.max_requests);
	host->cmd_per_lun = le32_to_cpu(iop_config.max_requests);

You might want to think about adjusting this. For the single LUN case, it's fine. For the multi-lun case it may allow commands to a single LUN to starve everything else.

There will be no multi-lun support for the controller so this is not an issue.

Sorry, a mistake. Multi-lun is supported.
Should host->can_queue be set to (cmd_per_lun * max_lun) ?

#2 lore raw

drivers/scsi/hptiop.c static int hptiop_queuecommand(struct sc · line 511

Quoted 4 lines · HighPoint Linux Team
                dprintk("hptiop_queuecmd : no free req\n");
drivers/scsi/hptiop.c +1 -1
-               scp->result = DID_BUS_BUSY << 16;
+               scp->result = SCSI_MLQUEUE_HOST_BUSY;
                goto cmd_done;

As jeff said, this needs to become a return SCSI_MLQUEUE_HOST_BUSY.

Quoted 9 lines · HighPoint Linux Team
Quoted 8 lines
Quoted 7 lines
	host->can_queue = le32_to_cpu(iop_config.max_requests);
	host->cmd_per_lun = le32_to_cpu(iop_config.max_requests);

You might want to think about adjusting this. For the single LUN case, it's fine. For the multi-lun case it may allow commands to a single LUN to starve everything else.

Quoted 5 lines
Quoted 2 lines

There will be no multi-lun support for the controller so this is not an issue.

Sorry, a mistake. Multi-lun is supported.
Should host->can_queue be set to (cmd_per_lun * max_lun) ?

It depends on how the controller behaves. Setting can_queue and cmd_per_lun tends to work for most SCSI controllers because the actual scsi devices have queue depths << this number. If your controller will behave like this (i.e. will not allow the internal queue for a single lun to fill up to this depth) then you can keep this setting (although, again, since this is probably fixed by the firmware, you want to set cmd_per_lun to this value to avoid excessive QUEUE_FULL returns). If the controller will happily load all the available slots up for a single lun, then you might want to think about reducing cmd_per_lun to some fraction of can_queue (you could even set it to can_queue - 2 like the 3ware controller).

Most of the other actual raid controllers seem to have much smaller per-lun queues, so they mainly set smaller, fixed size values for cmd_per_lun.

James

author maintainer
#3 lore raw
Quoted 13 lines · James Bottomley
Quoted 1 line

Should host->can_queue be set to (cmd_per_lun * max_lun) ?

It depends on how the controller behaves. Setting can_queue and cmd_per_lun tends to work for most SCSI controllers because the actual scsi devices have queue depths << this number. If your controller will behave like this (i.e. will not allow the internal queue for a single lun to fill up to this depth) then you can keep this setting (although, again, since this is probably fixed by the firmware, you want to set cmd_per_lun to this value to avoid excessive QUEUE_FULL returns). If the controller will happily load all the available slots up for a single lun, then you might want to think about reducing cmd_per_lun to some fraction of can_queue (you could even set it to can_queue - 2 like the 3ware controller).

The hptiop firmware works as the latter case. Should it be modified like this:
host->can_queue = le32_to_cpu(iop_config.max_requests);
- host->cmd_per_lun = le32_to_cpu(iop_config.max_requests);
+ host->cmd_per_lun = host->can_queue - 2;

While the 3ware driver sets both can_queue and cmd_per_lun to 254:

(3w-9xxx.h)
#define TW_Q_LENGTH 256
#define TW_MAX_CMDS_PER_LUN 254

(3w-9xxx.c)
.can_queue = TW_Q_LENGTH-2,
.cmd_per_lun = TW_MAX_CMDS_PER_LUN

Should can_queue be set to TW_Q_LENGTH ?

HighPoint Linux Team

#4 lore raw
In reply to HighPoint Linux Team
Quoted 11 lines · HighPoint Linux Team
drivers/scsi/hptiop.c +0 -5
@@ -504,18 +504,10 @@ static int hptiop_queuecommand(struct sc
 	BUG_ON(!done);
 	scp->scsi_done = done;
 
-	/*
-	 * hptiop_shutdown will flash controller cache.
-	 */
-	if (scp->cmnd[0] == SYNCHRONIZE_CACHE)  {
-		scp->result = DID_OK<<16;

drivers/scsi/hptiop.c static int hptiop_queuecommand(struct sc · line 512

Quoted 10 lines
drivers/scsi/hptiop.c +1 -4
-		goto cmd_done;
-	}
-
 	_req = get_req(hba);
 	if (_req == NULL) {
 		dprintk("hptiop_queuecmd : no free req\n");
-		scp->result = DID_BUS_BUSY << 16;
+		scp->result = SCSI_MLQUEUE_HOST_BUSY;
 		goto cmd_done;
 	}

Close. For the last bit of code quoted above, you should return SCSI_MLQUEUE_HOST_BUSY as your ->queuecommand() return code, rather than setting scp->result and calling the done() hook.

Jeff

author maintainer
#5 lore raw
In reply to HighPoint Linux Team

Updates:
- don't bypass SYNCHRONIZE_CACHE command
- return SCSI_MLQUEUE_HOST_BUSY when no free request slots
- move scsi_remove_host() to the begin of hpt_remove(), or it will not work after resources being released.

Signed-off-by HighPoint Linux Team <[email protected]>

---

drivers/scsi/hptiop.c +3 -11
index 8302f3b..a96751c 100644
@@ -504,19 +504,10 @@ static int hptiop_queuecommand(struct sc
 	BUG_ON(!done);
 	scp->scsi_done = done;
 
-	/*
-	 * hptiop_shutdown will flash controller cache.
-	 */
-	if (scp->cmnd[0] == SYNCHRONIZE_CACHE)  {
-		scp->result = DID_OK<<16;
-		goto cmd_done;
-	}
-
 	_req = get_req(hba);
 	if (_req == NULL) {
 		dprintk("hptiop_queuecmd : no free req\n");
-		scp->result = DID_BUS_BUSY << 16;
-		goto cmd_done;
+		return SCSI_MLQUEUE_HOST_BUSY;
 	}
 
 	_req->scp = scp;

@@ -1429,6 +1420,8 @@ static void hptiop_remove(struct pci_dev
 
 	dprintk("scsi%d: hptiop_remove\n", hba->host->host_no);
 
+	scsi_remove_host(host);
+
 	spin_lock(&hptiop_hba_list_lock);
 	list_del_init(&hba->link);
 	spin_unlock(&hptiop_hba_list_lock);

@@ -1448,7 +1441,6 @@ static void hptiop_remove(struct pci_dev
 	pci_set_drvdata(hba->pcidev, NULL);
 	pci_disable_device(hba->pcidev);
 
-	scsi_remove_host(host);
 	scsi_host_put(host);
 }
author maintainer
#6 lore raw

PATCH 1/1 hptiop: wrong register used in hptiop_reset_hba()

IOP reset message should be posted to inbound message register instead of outbound message register.

Signed-off-by HighPoint Linux Team <[email protected]>

---

drivers/scsi/hptiop.c +1 -1
index ab2f8b2..74d4d22 100644
@@ -577,7 +577,7 @@ static int hptiop_reset_hba(struct hptio
 	if (atomic_xchg(&hba->resetting, 1) == 0) {
 		atomic_inc(&hba->reset_count);
 		writel(IOPMU_INBOUND_MSG0_RESET,
-				&hba->iop->outbound_msgaddr0);
+				&hba->iop->inbound_msgaddr0);
 		hptiop_pci_posting_flush(hba->iop);
 	}