clean up some more tmscsim scan logic

- cleanup checks in ->queuecommand, we only get here either for inquiry or a found device now
- DCBmap in the acb is gone, was only used for debug prints after the prevous changes
- kill some more debugging keyed of by ->scan_devices

Currently SCSI drivers (tmscsim included) implement multiplecommand queues. Which, in part, makes sense, but the generic code changed, and not all drivers nave been converted to fully use it promptly. So, as a maximum, I saw (maybe you could add a couple:-)):

1) Queue in the block-layer
2) Incoming queue in a scsi LLD (recently removed in tmscsim)
3) Issue queue in LLD (Waiting in tmscsim)
4) Submitted tagged command queue or untagged 1 current command in LLD with a timer (Going in tmscsim)
5) tcq in SCSI mid-layer / block.

So, I can't figure out, nor find a sample implementation or a document about which queues should really be implemented in LLDs and how they should inter-operate with the mid- / block-layer. Say, a document, describing the (SCSI-)command flow would be very useful, like

n) scsi.c: LLD->queuecommand()
n+1) LLD: send out on the bus, insert on tcq / return
SCSI_MLQUEUE_DEVICE_BUSY
n+2) LLD: on command-completion, if successful, report (scsi_done()),
if not - auto request sense, put the request-sense command on internal "going" queue
...

Where n+2 is, actually, the second question - I've seen a comment somewhere, that LLDs should (not?) auto request-sense, rather than failing and letting the mid-layer issue one...

Is there a "perfect" LLD, I could look at for an example? I looked at 53c700.c, as it was until recently the only one using tcq, but it also has a few of those internal queues...

Quoted 12 lines · Guennadi Liakhovetski

Currently SCSI drivers (tmscsim included) implement multiplecommand queues. Which, in part, makes sense, but the generic code changed, and not all drivers nave been converted to fully use it promptly. So, as a maximum, I saw (maybe you could add a couple:-)):

1) Queue in the block-layer
2) Incoming queue in a scsi LLD (recently removed in tmscsim)
3) Issue queue in LLD (Waiting in tmscsim)
4) Submitted tagged command queue or untagged 1 current command in LLD with a timer (Going in tmscsim)
5) tcq in SCSI mid-layer / block.

Quoted 4 lines

So, I can't figure out, nor find a sample implementation or a document about which queues should really be implemented in LLDs and how they should inter-operate with the mid- / block-layer. Say, a document, describing the (SCSI-)command flow would be very useful, like

None should be in the LLDD. I have an experimental patch that doesn't remove the per-device queue yet but only does the easier part of returning MLQUEUE_DEVICE_BUSY in ->queuecommand but doesn't do the requeueing from the isr yet. I'll send it out ASAP.

Quoted 8 lines

n) scsi.c: LLD->queuecommand()
n+1) LLD: send out on the bus, insert on tcq / return
SCSI_MLQUEUE_DEVICE_BUSY
n+2) LLD: on command-completion, if successful, report (scsi_done()),
if not - auto request sense, put the request-sense command on internal "going" queue
...

Quoted 3 lines

Where n+2 is, actually, the second question - I've seen a comment somewhere, that LLDs should (not?) auto request-sense, rather than failing and letting the mid-layer issue one...

No, mid-layer auto request sense is deprecated and isn't that reliable. See the comment ontop of scsi_eh_get_sense() in scsi_error.c

Quoted 3 lines

Is there a "perfect" LLD, I could look at for an example? I looked at 53c700.c, as it was until recently the only one using tcq, but it also has a few of those internal queues...

that driver queues commands in HBA memory, not in the driver. It's one of the NCR scripts family HBAs where the firmware is written specificly to match the driver and they cooperate closely. (better ask James on the details..)

Hi

Quoted 3 lines · Christoph Hellwig

Quoted 2 lines · Guennadi Liakhovetski

Currently SCSI drivers (tmscsim included) implement multiplecommand queues. Which, in part, makes sense, but the generic code changed,

and not

Quoted 15 lines

Quoted 10 lines

all drivers nave been converted to fully use it promptly. So, as a maximum, I saw (maybe you could add a couple:-)):

1) Queue in the block-layer
2) Incoming queue in a scsi LLD (recently removed in tmscsim)
3) Issue queue in LLD (Waiting in tmscsim)
4) Submitted tagged command queue or untagged 1 current command in LLD with a timer (Going in tmscsim)
5) tcq in SCSI mid-layer / block.

Quoted 4 lines

So, I can't figure out, nor find a sample implementation or a document about which queues should really be implemented in LLDs and how they should inter-operate with the mid- / block-layer. Say, a document, describing the (SCSI-)command flow would be very useful, like

Quoted 4 lines

None should be in the LLDD. I have an experimental patch that doesn't remove the per-device queue yet but only does the easier part of returning MLQUEUE_DEVICE_BUSY in ->queuecommand but doesn't do the requeueing from the isr yet. I'll send it out ASAP.

Looks good, thanks! Compiles too. I'll test it on the hardware tomorrow, should work, still, I'd do a short test. And, yes, please, feel free to offload any part of that further cleanup on me - I would just need a couple of hints. I'll do it slower and you'll have to fix it afterwards and consult me before, but I want to have my piece of fun too!:-)

Quoted 4 lines

Quoted 4 lines

n) scsi.c: LLD->queuecommand()
n+1) LLD: send out on the bus, insert on tcq / return
SCSI_MLQUEUE_DEVICE_BUSY
n+2) LLD: on command-completion, if successful, report

(scsi_done()),

Quoted 6 lines

Quoted 6 lines

if not - auto request sense, put the request-sense command on internal "going" queue
...

Where n+2 is, actually, the second question - I've seen a comment somewhere, that LLDs should (not?) auto request-sense, rather than

failing

Quoted 4 lines

Quoted 1 line

and letting the mid-layer issue one...

No, mid-layer auto request sense is deprecated and isn't that reliable. See the comment ontop of scsi_eh_get_sense() in scsi_error.c

That's actually what I was reading and that's where I didn't understand:

* Notes:

  *    This has the unfortunate side effect that if a shost adapter does
  *    not automatically request sense information, that we end up shutting
  *    it down before we request it.  All shosts should be doing this
                                                                ^^^^
  *    anyways, so for now all I have to say is tough noogies if you end up
  *    in here.  On second thought, this is probably a good idea.  We
  *    *really* want to give authors an incentive to automatically request
  *    this.

So, I failed to understand if "this" above means "All shosts should be automatically requesting sense" or the opposite. Then the "second thought"... So, you mean LLDs should really auto-request sense on failures. Well, if we detect an error condition when we are CONNECTED to a host, it should be possible to directly send a REQUEST SENSE - the bus should be ours at this time, right?

Quoted 2 lines

Quoted 2 lines

Is there a "perfect" LLD, I could look at for an example? I looked at 53c700.c, as it was until recently the only one using tcq, but it

also has

Quoted 6 lines

Quoted 1 line

a few of those internal queues...

that driver queues commands in HBA memory, not in the driver. It's one of the NCR scripts family HBAs where the firmware is written specificly to match the driver and they cooperate closely. (better ask James on the details..)

Ok, I see (roughly), thanks.

Many thanks
Guennadi
---
Guennadi Liakhovetski

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

The device server should send sense information when the task finishes with CHECK CONDITION status _with_ the same transaction. (SAM-3r13, 5.9.6, SPC-3r18, 4.5.1). This used to be called "autosense" a couple of years back, but no more as it is now protocol.

This has been off-loaded to LLDD when the device doesn't support it, so that SCSI Core is basically SAM.

BTW, the state and task queue would most likely change between the time SCSI Core gets CHECK CONDITION (with no sense) until the time it issues REQUEST SENSE, to get the sense for _that_ command which failed. Thus we want LLDDs to emulate it for devices which do not support it, since it has better (closer) control of the device.

Quoted 4 lines · Christoph Hellwig

None should be in the LLDD. I have an experimental patch that doesn't remove the per-device queue yet but only does the easier part of returning MLQUEUE_DEVICE_BUSY in ->queuecommand but doesn't do the requeueing from the isr yet. I'll send it out ASAP.

Looks good, thanks! Compiles too. I'll test it on the hardware tomorrow,

That's actually what I was reading and that's where I didn't understand:

* Notes:

 *    This has the unfortunate side effect that if a shost adapter does
 *    not automatically request sense information, that we end up shutting
 *    it down before we request it.  All shosts should be doing this
								^^^^
 *    anyways, so for now all I have to say is tough noogies if you end up
 *    in here.  On second thought, this is probably a good idea.  We
 *    *really* want to give authors an incentive to automatically request
 *    this.

So, I failed to understand if "this" above means "All shosts should be automatically requesting sense" or the opposite.

Then the "second
thought"... So, you mean LLDs should really auto-request sense on failures. Well, if we detect an error condition when we are CONNECTED to a host, it should be possible to directly send a REQUEST SENSE - the bus should be ours at this time, right?

- cleanup checks in ->queuecommand, we only get here either for inquiry or a found device now
- DCBmap in the acb is gone, was only used for debug prints after the prevous changes
- kill some more debugging keyed of by ->scan_devices

drivers/scsi/scsiiom.c +1 -20

@@ -1398,26 +1398,7 @@
 	pSRB->SRBFlag &= ~AUTO_REQSENSE;
 	pSRB->AdaptStatus = 0;
 	pSRB->TargetStatus = CHECK_CONDITION << 1;
-#ifdef DC390_REMOVABLEDEBUG
-	switch (pcmd->sense_buffer[2] & 0x0f)
-	{
-	 case NOT_READY: printk (KERN_INFO "DC390: ReqSense: NOT_READY (Cmnd = 0x%02x, Dev = %i-%i, Stat = %i, Scan = %i)\n",
-				 pcmd->cmnd[0], pDCB->TargetID, pDCB->TargetLUN,
-				 status, pACB->scan_devices); break;
-	 case UNIT_ATTENTION: printk (KERN_INFO "DC390: ReqSense: UNIT_ATTENTION (Cmnd = 0x%02x, Dev = %i-%i, Stat = %i, Scan = %i)\n",
-				      pcmd->cmnd[0], pDCB->TargetID, pDCB->TargetLUN,
-				      status, pACB->scan_devices); break;
-	 case ILLEGAL_REQUEST: printk (KERN_INFO "DC390: ReqSense: ILLEGAL_REQUEST (Cmnd = 0x%02x, Dev = %i-%i, Stat = %i, Scan = %i)\n",
-				       pcmd->cmnd[0], pDCB->TargetID, pDCB->TargetLUN,
-				       status, pACB->scan_devices); break;
-	 case MEDIUM_ERROR: printk (KERN_INFO "DC390: ReqSense: MEDIUM_ERROR (Cmnd = 0x%02x, Dev = %i-%i, Stat = %i, Scan = %i)\n",
-				    pcmd->cmnd[0], pDCB->TargetID, pDCB->TargetLUN,
-				    status, pACB->scan_devices); break;
-	 case HARDWARE_ERROR: printk (KERN_INFO "DC390: ReqSense: HARDWARE_ERROR (Cmnd = 0x%02x, Dev = %i-%i, Stat = %i, Scan = %i)\n",
-				      pcmd->cmnd[0], pDCB->TargetID, pDCB->TargetLUN,
-				      status, pACB->scan_devices); break;
-	}
-#endif
+
 	//pcmd->result = MK_RES(DRIVER_SENSE,DID_OK,0,status);
 	if (status == (CHECK_CONDITION << 1))
 	{

@@ -1571,23 +1552,6 @@
 	    pDCB->Inquiry7 = ptr->Flags;
 

ckc_e:

drivers/scsi/scsiiom.c +0 -17

-    if( pACB->scan_devices )
-    {
-	if( pcmd->cmnd[0] == TEST_UNIT_READY ||
-	    pcmd->cmnd[0] == INQUIRY)
-	{
-#ifdef DC390_DEBUG0
-	    printk (KERN_INFO "DC390: %s: result: %08x",
-		    (pcmd->cmnd[0] == INQUIRY? "INQUIRY": "TEST_UNIT_READY"),
-		    pcmd->result);
-	    if (pcmd->result & (DRIVER_SENSE << 24)) printk (" (sense: %02x %02x %02x %02x)\n",
-				   pcmd->sense_buffer[0], pcmd->sense_buffer[1],
-				   pcmd->sense_buffer[2], pcmd->sense_buffer[3]);
-	    else printk ("\n");
-#endif
-	}
-    }
-
    if( pcmd->cmnd[0] == INQUIRY &&

	(pcmd->result == (DID_OK << 16) || status_byte(pcmd->result) & CHECK_CONDITION) )
     {

drivers/scsi/tmscsim.c +0 -36

@@ -612,11 +609,7 @@
     {
 	pDCB = pDCB->pNextDCB;
 	if (pDCB == pACB->pLinkDCB)
-	  {
-	     DCBDEBUG(printk (KERN_WARNING "DC390: DCB not found (DCB=%p, DCBmap[%2x]=%2x)\n",
-			      pDCB, id, pACB->DCBmap[id]));
 	     return 0;
-	  }
     }
   DCBDEBUG1( printk (KERN_DEBUG "DCB %p (%02x,%02x) found.\n",	\
 		      pDCB, pDCB->TargetID, pDCB->TargetLUN));

@@ -984,28 +977,6 @@
    struct dc390_srb*   pSRB;
    struct dc390_acb*   pACB = (struct dc390_acb*) cmd->device->host->hostdata;
 
-    DEBUG0(/*  if(pACB->scan_devices) */	\
-	printk(KERN_INFO "DC390: Queue Cmd=%02x,Tgt=%d,LUN=%d (pid=%li), buffer=%p\n",\
-	       cmd->cmnd[0],cmd->device->id,cmd->device->lun,cmd->pid, cmd->buffer));
-
-    /* TODO: Change the policy: Always accept TEST_UNIT_READY or INQUIRY
-     * commands and alloc a DCB for the device if not yet there. DCB will
-     * be removed in dc390_SRBdone if SEL_TIMEOUT */
-    if (!(pACB->scan_devices) && !(pACB->DCBmap[cmd->device->id] & (1 << cmd->device->lun))) {
-	printk(KERN_INFO "DC390: Ignore target %02x lun %02x\n",
-		cmd->device->id, cmd->device->lun);
-	goto fail;
-    }
-
-    /* Should it be: BUG_ON(!pDCB); ? */
-
-    if (!pDCB)
-    {  /* should never happen */
-	printk (KERN_ERR "DC390: no DCB found, target %02x lun %02x\n",
-		cmd->device->id, cmd->device->lun);
-	goto fail;
-    }
-
    pACB->Cmds++;
    cmd->scsi_done = done;
    cmd->result = 0;

@@ -1026,10 +997,6 @@
 
 requeue:
    return 1;
- fail:
-    cmd->result = DID_BAD_TARGET << 16;
-    done(cmd);
-    return 0;
 }
 
 /* We ignore mapping problems, as we expect everybody to respect

@@ -1394,7 +1361,6 @@
 static void __devinit dc390_initACB (struct Scsi_Host *psh, unsigned long io_port, u8 Irq, u8 index)
 {
    struct dc390_acb*    pACB;
-    u8   i;
 
    psh->can_queue = MAX_CMD_QUEUE;
    psh->cmd_per_lun = MAX_CMD_PER_LUN;

@@ -1441,8 +1407,6 @@
    dc390_linkSRB( pACB );
    pACB->pTmpSRB = &pACB->TmpSRB;
    dc390_initSRB( pACB->pTmpSRB );
-    for(i=0; i<MAX_SCSI_ID; i++)
-	pACB->DCBmap[i] = 0;
    pACB->sel_timeout = SEL_TIMEOUT;
    pACB->glitch_cfg = EATER_25NS;
    pACB->Cmds = pACB->CmdInQ = pACB->CmdOutOfSRB = 0;

@@ -1603,7 +1567,6 @@
 			pDCB->CtrlR4 |= NEGATE_REQACKDATA | NEGATE_REQACK;
 	}
 
-	pACB->DCBmap[id] |= (1 << lun);
 	dc390_updateDCB(pACB, pDCB);
 
 	pACB->scan_devices = 1;

@@ -1627,8 +1590,6 @@
 
 	BUG_ON(pDCB->GoingSRBCnt > 1);
 
-	pACB->DCBmap[pDCB->TargetID] &= ~(1 << pDCB->TargetLUN);
-
 	if (pDCB == pACB->pLinkDCB) {
 		if (pACB->pLastDCB == pDCB) {
 			pDCB->pNextDCB = NULL;

:q

Quoted 5 lines · Christoph Hellwig

- cleanup checks in ->queuecommand, we only get here either for inquiry or a found device now
- DCBmap in the acb is gone, was only used for debug prints after the prevous changes
- kill some more debugging keyed of by ->scan_devices

James, please, apply.