[PATCH] tmscsim: INQUIRY always only untagged

[PATCH] tmscsim: INQUIRY always only untagged

[Guennadi Liakhovetski]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 302 bytes --]

Hello

The attached patch makes sure, all INQUIRY commands are only sent 
untagged. This way drives, affected by bug 2139, also survive further 
INQUIRYs, sent after the initial scan, e.g., by scsiinfo.

Hopefully, will be superseeded, when switching to tcq.

Thanks
Guennadi
---
Guennadi Liakhovetski

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2969 bytes --]

INQUIRY always send only untagged.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

diff -ur --exclude=CVS a/drivers/scsi/scsiiom.c b/drivers/scsi/scsiiom.c
--- a/drivers/scsi/scsiiom.c	Tue Aug 17 23:16:53 2004
+++ b/drivers/scsi/scsiiom.c	Sat Aug 21 23:42:11 2004
@@ -8,9 +8,9 @@
 static void __inline__
 dc390_freetag (struct dc390_dcb* pDCB, struct dc390_srb* pSRB)
 {
-	if (pSRB->TagNumber < 255) {
+	if (pSRB->TagNumber != (u8)SCSI_NO_TAG) {
 		pDCB->TagMask &= ~(1 << pSRB->TagNumber);   /* free tag mask */
-		pSRB->TagNumber = 255;
+		pSRB->TagNumber = (u8)SCSI_NO_TAG;
 	}
 }
 
@@ -67,7 +67,7 @@
     pSRB->MsgCnt = 0; cmd = SEL_W_ATN;
     DC390_write8 (ScsiFifo, IDENTIFY(disc_allowed, pDCB->TargetLUN));
     /* Change 99/05/31: Don't use tags when not disconnecting (BUSY) */
-    if ((pDCB->SyncMode & EN_TAG_QUEUEING) && disc_allowed)
+    if ((pDCB->SyncMode & EN_TAG_QUEUEING) && disc_allowed && pSRB->pcmd->cmnd[0] != INQUIRY)
       {
 	u8 tag_no = 0;
 	while ((1 << tag_no) & pDCB->TagMask) tag_no++;
@@ -903,18 +903,11 @@
 		printk(KERN_ERR "DC390: pSRB == pTmpSRB! (TagQ Error?) (%02i-%i)\n", pDCB->TargetID, pDCB->TargetLUN);
 	else
 		printk(KERN_ERR "DC390: pSRB == pTmpSRB! (TagQ Error?) (DCB 0!)\n");
-
-	/* Try to recover - some broken disks react badly to tagged INQUIRY */
-	if (pDCB && pACB->scan_devices && pDCB->GoingSRBCnt == 1) {
-		pSRB = pDCB->pGoingSRB;
-		pDCB->pActiveSRB = pSRB;
-	} else {
-		pSRB->pSRBDCB = pDCB;
-		dc390_EnableMsgOut_Abort(pACB, pSRB);
-		if (pDCB)
-			pDCB->DCBFlag |= ABORT_DEV;
-		return;
-	}
+	pSRB->pSRBDCB = pDCB;
+	dc390_EnableMsgOut_Abort(pACB, pSRB);
+	if (pDCB)
+		pDCB->DCBFlag |= ABORT_DEV;
+	return;
     }
 
     if( pSRB->SGIndex < pSRB->SGcount )
@@ -1257,7 +1250,7 @@
     }
     pACB->pActiveDCB = pDCB;
     /* TagQ: We expect a message soon, so never mind the exact SRB */
-    if( pDCB->SyncMode & EN_TAG_QUEUEING )
+    if ((pDCB->SyncMode & EN_TAG_QUEUEING) && (!pDCB->pActiveSRB || pDCB->pActiveSRB->TagNumber != (u8)SCSI_NO_TAG))
     {
 	pSRB = pACB->pTmpSRB;
 	pDCB->pActiveSRB = pSRB;
diff -ur --exclude=CVS a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
--- a/drivers/scsi/tmscsim.c	Sat Aug 21 23:07:42 2004
+++ b/drivers/scsi/tmscsim.c	Sat Aug 21 23:37:42 2004
@@ -245,6 +245,7 @@
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsicam.h>
+#include <scsi/scsi_tcq.h>
 
 
 #define DC390_BANNER "Tekram DC390/AM53C974"
@@ -861,6 +862,10 @@
 		goto host_busy;
 	if (acb->ACBFlag & (RESET_DETECT|RESET_DONE|RESET_DEV))
 		goto host_busy;
+	/* INQUIRY only untagged, do not accept more commands if the going one is untagged, e.g., INQUIRY */
+	if (unlikely((cmd->cmnd[0] == INQUIRY && dcb->pGoingSRB) ||
+		     (dcb->pGoingSRB && dcb->pGoingSRB->TagNumber == (u8)SCSI_NO_TAG)))
+		goto device_busy;
 
 	srb = acb->pFreeSRB;
 	if (unlikely(srb == NULL))

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Christoph Hellwig]

On Sat, Sep 04, 2004 at 12:41:00AM +0200, Guennadi Liakhovetski wrote:
> Hello
> 
> The attached patch makes sure, all INQUIRY commands are only sent 
> untagged. This way drives, affected by bug 2139, also survive further 
> INQUIRYs, sent after the initial scan, e.g., by scsiinfo.

Looks basically good to me, but if you want to store SCSI_NO_TAG in
->TagNumber I'd suggest that you make it an unsigned type.

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Guennadi Liakhovetski]

On Sat, 4 Sep 2004, Christoph Hellwig wrote:

> On Sat, Sep 04, 2004 at 12:41:00AM +0200, Guennadi Liakhovetski wrote:
>> Hello
>>
>> The attached patch makes sure, all INQUIRY commands are only sent
>> untagged. This way drives, affected by bug 2139, also survive further
>> INQUIRYs, sent after the initial scan, e.g., by scsiinfo.
>
> Looks basically good to me, but if you want to store SCSI_NO_TAG in
> ->TagNumber I'd suggest that you make it an unsigned type.

It is unsigned (u8) now. Or did you mean signed, since SCSI_NO_TAG is 
defined as (-1). I thought about making TagNumber an int, but I didn't 
bother, because it is anyway due removal...

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Christoph Hellwig]

On Sun, Sep 05, 2004 at 12:26:14AM +0200, Guennadi Liakhovetski wrote:
> On Sat, 4 Sep 2004, Christoph Hellwig wrote:
> 
> > On Sat, Sep 04, 2004 at 12:41:00AM +0200, Guennadi Liakhovetski wrote:
> >> Hello
> >>
> >> The attached patch makes sure, all INQUIRY commands are only sent
> >> untagged. This way drives, affected by bug 2139, also survive further
> >> INQUIRYs, sent after the initial scan, e.g., by scsiinfo.
> >
> > Looks basically good to me, but if you want to store SCSI_NO_TAG in
> > ->TagNumber I'd suggest that you make it an unsigned type.
> 
> It is unsigned (u8) now. Or did you mean signed, since SCSI_NO_TAG is 
> defined as (-1). I thought about making TagNumber an int, but I didn't 
> bother, because it is anyway due removal...

Umm, yes signed of course.

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Guennadi Liakhovetski]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 917 bytes --]

On Sat, 4 Sep 2004, Christoph Hellwig wrote:

> On Sun, Sep 05, 2004 at 12:26:14AM +0200, Guennadi Liakhovetski wrote:
>> On Sat, 4 Sep 2004, Christoph Hellwig wrote:
>>
>>> On Sat, Sep 04, 2004 at 12:41:00AM +0200, Guennadi Liakhovetski wrote:
>>>> Hello
>>>>
>>>> The attached patch makes sure, all INQUIRY commands are only sent
>>>> untagged. This way drives, affected by bug 2139, also survive further
>>>> INQUIRYs, sent after the initial scan, e.g., by scsiinfo.
>>>
>>> Looks basically good to me, but if you want to store SCSI_NO_TAG in
>>> ->TagNumber I'd suggest that you make it an unsigned type.
>>
>> It is unsigned (u8) now. Or did you mean signed, since SCSI_NO_TAG is
>> defined as (-1). I thought about making TagNumber an int, but I didn't
>> bother, because it is anyway due removal...
>
> Umm, yes signed of course.

Ok, how about the attached version?

Thanks
Guennadi
---
Guennadi Liakhovetski

[-- Attachment #2: Type: TEXT/PLAIN, Size: 5533 bytes --]

INQUIRY always send only untagged.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

diff -u a/drivers/scsi/scsiiom.c b/drivers/scsi/scsiiom.c
--- a/drivers/scsi/scsiiom.c	17 Aug 2004 21:16:53
+++ b/drivers/scsi/scsiiom.c	5 Sep 2004 11:13:15
@@ -8,9 +8,9 @@
 static void __inline__
 dc390_freetag (struct dc390_dcb* pDCB, struct dc390_srb* pSRB)
 {
-	if (pSRB->TagNumber < 255) {
+	if (pSRB->TagNumber != SCSI_NO_TAG) {
 		pDCB->TagMask &= ~(1 << pSRB->TagNumber);   /* free tag mask */
-		pSRB->TagNumber = 255;
+		pSRB->TagNumber = SCSI_NO_TAG;
 	}
 }
 
@@ -67,9 +67,9 @@
     pSRB->MsgCnt = 0; cmd = SEL_W_ATN;
     DC390_write8 (ScsiFifo, IDENTIFY(disc_allowed, pDCB->TargetLUN));
     /* Change 99/05/31: Don't use tags when not disconnecting (BUSY) */
-    if ((pDCB->SyncMode & EN_TAG_QUEUEING) && disc_allowed)
+    if ((pDCB->SyncMode & EN_TAG_QUEUEING) && disc_allowed && pSRB->pcmd->cmnd[0] != INQUIRY)
       {
-	u8 tag_no = 0;
+	int tag_no = 0;
 	while ((1 << tag_no) & pDCB->TagMask) tag_no++;
 	if (tag_no >= sizeof (pDCB->TagMask)*8 || tag_no >= pDCB->MaxCommand) { 
 		printk (KERN_WARNING "DC390: Out of tags for Dev. %02x %02x\n", pDCB->TargetID, pDCB->TargetLUN); 
@@ -79,7 +79,7 @@
 	DC390_write8 (ScsiFifo, SIMPLE_QUEUE_TAG);
 	pDCB->TagMask |= (1 << tag_no); pSRB->TagNumber = tag_no;
 	DC390_write8 (ScsiFifo, tag_no);
-	DEBUG1(printk (KERN_DEBUG "DC390: Select w/DisCn for Cmd %li (SRB %p), Using Tag %02x\n", pSRB->pcmd->pid, pSRB, tag_no));
+	DEBUG1(printk (KERN_DEBUG "DC390: Select w/DisCn for Cmd %li (SRB %p), Using Tag %d\n", pSRB->pcmd->pid, pSRB, tag_no));
 	cmd = SEL_W_ATN3;
       }
     else	/* No TagQ */
@@ -612,7 +612,7 @@
 }
 
 static struct dc390_srb*
-dc390_MsgIn_QTag (struct dc390_acb* pACB, struct dc390_dcb* pDCB, u8 tag)
+dc390_MsgIn_QTag (struct dc390_acb* pACB, struct dc390_dcb* pDCB, int tag)
 {
   struct dc390_srb* lastSRB = pDCB->pGoingLast;
   struct dc390_srb* pSRB = pDCB->pGoingSRB;
@@ -903,18 +903,11 @@
 		printk(KERN_ERR "DC390: pSRB == pTmpSRB! (TagQ Error?) (%02i-%i)\n", pDCB->TargetID, pDCB->TargetLUN);
 	else
 		printk(KERN_ERR "DC390: pSRB == pTmpSRB! (TagQ Error?) (DCB 0!)\n");
-
-	/* Try to recover - some broken disks react badly to tagged INQUIRY */
-	if (pDCB && pACB->scan_devices && pDCB->GoingSRBCnt == 1) {
-		pSRB = pDCB->pGoingSRB;
-		pDCB->pActiveSRB = pSRB;
-	} else {
-		pSRB->pSRBDCB = pDCB;
-		dc390_EnableMsgOut_Abort(pACB, pSRB);
-		if (pDCB)
-			pDCB->DCBFlag |= ABORT_DEV;
-		return;
-	}
+	pSRB->pSRBDCB = pDCB;
+	dc390_EnableMsgOut_Abort(pACB, pSRB);
+	if (pDCB)
+		pDCB->DCBFlag |= ABORT_DEV;
+	return;
     }
 
     if( pSRB->SGIndex < pSRB->SGcount )
@@ -1257,7 +1250,7 @@
     }
     pACB->pActiveDCB = pDCB;
     /* TagQ: We expect a message soon, so never mind the exact SRB */
-    if( pDCB->SyncMode & EN_TAG_QUEUEING )
+    if ((pDCB->SyncMode & EN_TAG_QUEUEING) && (!pDCB->pActiveSRB || pDCB->pActiveSRB->TagNumber != SCSI_NO_TAG))
     {
 	pSRB = pACB->pTmpSRB;
 	pDCB->pActiveSRB = pSRB;
@@ -1288,7 +1281,7 @@
 	}
     }
 
-    DEBUG1(printk (KERN_DEBUG "Resel SRB(%p): TagNum (%02x)\n", pSRB, pSRB->TagNumber));
+    DEBUG1(printk (KERN_DEBUG "Resel SRB(%p): TagNum %d\n", pSRB, pSRB->TagNumber));
     pSRB->ScsiPhase = SCSI_NOP0;
     DC390_write8 (Scsi_Dest_ID, pDCB->TargetID);
     DC390_write8 (Sync_Period, pDCB->SyncPeriod);
diff -u a/drivers/scsi/tmscsim.h b/drivers/scsi/tmscsim.h
--- a/drivers/scsi/tmscsim.h	17 Aug 2004 21:16:53
+++ b/drivers/scsi/tmscsim.h	5 Sep 2004 11:09:27
@@ -44,17 +44,15 @@
 struct scsi_cmnd	*pcmd;
 struct scatterlist	*pSegmentList;
 
-/* 0x10: */
 struct scatterlist Segmentx;	/* make a one entry of S/G list table */
 
-/* 0x1c: */
 unsigned long	SGBusAddr;	/*;a segment starting address as seen by AM53C974A*/
 unsigned long	SGToBeXferLen;	/*; to be xfer length */
 unsigned long	TotalXferredLen;
 unsigned long	SavedTotXLen;
+int		TagNumber;
 u32		SRBState;
 
-/* 0x30: */
 u8		SRBStatus;
 u8		SRBFlag;	/*; b0-AutoReqSense,b6-Read,b7-write */
 				/*; b4-settimeout,b5-Residual valid */
@@ -62,11 +60,9 @@
 u8		TargetStatus;
 
 u8		ScsiPhase;
-u8		TagNumber;
 u8		SGIndex;
 u8		SGcount;
 
-/* 0x38: */
 u8		MsgCnt;
 u8		EndMessage;
 u8		RetryCnt;
@@ -74,12 +70,10 @@
 
 unsigned long	Saved_Ptr;
 
-/* 0x40: */
 u8		MsgInBuf[6];
 u8		MsgOutBuf[6];
 
 //u8		IORBFlag;	/*;81h-Reset, 2-retry */
-/* 0x4c: */
 };
 
 
diff -u a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
--- a/drivers/scsi/tmscsim.c	3 Sep 2004 22:10:14
+++ b/drivers/scsi/tmscsim.c	5 Sep 2004 11:14:40
@@ -245,6 +245,7 @@
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsicam.h>
+#include <scsi/scsi_tcq.h>
 
 
 #define DC390_BANNER "Tekram DC390/AM53C974"
@@ -861,6 +862,10 @@
 		goto host_busy;
 	if (acb->ACBFlag & (RESET_DETECT|RESET_DONE|RESET_DEV))
 		goto host_busy;
+	/* INQUIRY only untagged, do not accept more commands if the going one is untagged, e.g., INQUIRY */
+	if (unlikely((cmd->cmnd[0] == INQUIRY && dcb->pGoingSRB) ||
+		     (dcb->pGoingSRB && dcb->pGoingSRB->TagNumber == SCSI_NO_TAG)))
+		goto device_busy;
 
 	srb = acb->pFreeSRB;
 	if (unlikely(srb == NULL))
@@ -892,7 +897,7 @@
 	srb->SGToBeXferLen = 0;
 	srb->ScsiPhase = 0;
 	srb->EndMessage = 0;
-	srb->TagNumber = 255;
+	srb->TagNumber = SCSI_NO_TAG;
 
 	if (dc390_StartSCSI(acb, dcb, srb)) {
 		dc390_Waiting_insert(dcb, srb);

Re: [PATCH] tmscsim: INQUIRY always only untagged

[James Bottomley]

On Sun, 2004-09-05 at 10:56, Guennadi Liakhovetski wrote:
> Ok, how about the attached version?

Well, it looks like it would cure the symptoms, and I can apply it as a
palliative.

However, the disease in this driver is that it snoops the inquiry data
and gets confused by the double inquiry we now do for certain drives.

What really needs to happen is

1) remove the inquiry snooping and believe the flags in struct
scsi_device instead
2) Don't turn on TCQ until slave_configure()
3) remove it's internal TCQ black list and put it at the mid-layer.

James

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Christoph Hellwig]

On Sun, Sep 05, 2004 at 11:42:58AM -0400, James Bottomley wrote:
> On Sun, 2004-09-05 at 10:56, Guennadi Liakhovetski wrote:
> > Ok, how about the attached version?
> 
> Well, it looks like it would cure the symptoms, and I can apply it as a
> palliative.
> 
> However, the disease in this driver is that it snoops the inquiry data
> and gets confused by the double inquiry we now do for certain drives.
> 
> What really needs to happen is
> 
> 1) remove the inquiry snooping and believe the flags in struct
> scsi_device instead
> 2) Don't turn on TCQ until slave_configure()
> 3) remove it's internal TCQ black list and put it at the mid-layer.

Umm yes.  But lets get the driver no oops first and then move forward.
There's still a lot of work to do on the driver.

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Guennadi Liakhovetski]

On Sun, 5 Sep 2004, Christoph Hellwig wrote:

> On Sun, Sep 05, 2004 at 11:42:58AM -0400, James Bottomley wrote:
>> On Sun, 2004-09-05 at 10:56, Guennadi Liakhovetski wrote:
>>> Ok, how about the attached version?
>>
>> Well, it looks like it would cure the symptoms, and I can apply it as a
>> palliative.
>>
>> However, the disease in this driver is that it snoops the inquiry data
>> and gets confused by the double inquiry we now do for certain drives.
>>
>> What really needs to happen is
>>
>> 1) remove the inquiry snooping and believe the flags in struct
>> scsi_device instead

...and use tags set up by the block layer...

>> 2) Don't turn on TCQ until slave_configure()

Well, this alone wouldn't help with this specific problem, exactly because 
INQUIRY can be sent later too. Or would the mid-layer completely disable 
tagging for such a disk?

>> 3) remove it's internal TCQ black list and put it at the mid-layer.
>
> Umm yes.  But lets get the driver no oops first and then move forward.
> There's still a lot of work to do on the driver.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] tmscsim: INQUIRY always only untagged

[James Bottomley]

On Sun, 2004-09-05 at 12:31, Christoph Hellwig wrote:
> Umm yes.  But lets get the driver no oops first and then move forward.
> There's still a lot of work to do on the driver.

Well, OK.  I agree to putting a palliative fix in, since what I
suggested will probably take more thought, but I don't think this one is
actually it.  Mixing tagged and untagged commands is illegal under the
SCSI spec.  Simply untagging the INQUIRY command may work to fix this
particular bug, but I bet it will introduce others when a scsi utility
sends an INQUIRY down to an in-use device.

I think perhaps the correct palliative fix is simply to list this device
(whatever it is, it's not listed in the bug report) as BLIST_NOTQ, then
the problem won't arise (and of course, tmscsim would have actually to
use the mid-layer black list rather than its own much shorter internal
list).

James

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Guennadi Liakhovetski]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1867 bytes --]

On Mon, 5 Sep 2004, James Bottomley wrote:

> On Sun, 2004-09-05 at 12:31, Christoph Hellwig wrote:
>> Umm yes.  But lets get the driver no oops first and then move forward.
>> There's still a lot of work to do on the driver.
>
> Well, OK.  I agree to putting a palliative fix in, since what I
> suggested will probably take more thought, but I don't think this one is
> actually it.  Mixing tagged and untagged commands is illegal under the
> SCSI spec.  Simply untagging the INQUIRY command may work to fix this
> particular bug, but I bet it will introduce others when a scsi utility
> sends an INQUIRY down to an in-use device.

I've seen this done, at least, in one more driver - 53c700.c. In its 
queuecommand it checks not to mix tagged and untagged commands, but allows 
both. That is, it checks, if there are currently (tagged) commands on the 
queue and if the new one is untagged, and if so, returns DEVICE_BUSY. So, 
it would accept an untagged command to a device, supporting tags, but 
currently having the queue empty. I am doing the same check in the latest 
version of my patch. Is it still bad?

> I think perhaps the correct palliative fix is simply to list this device
> (whatever it is, it's not listed in the bug report) as BLIST_NOTQ, then
> the problem won't arise (and of course, tmscsim would have actually to
> use the mid-layer black list rather than its own much shorter internal
> list).

I don't know what device the original reporter had, but the one I have 
here, which exibits the same behaviour, is a Seagate disk (see attached 
patch). So, if you still think, this device should not be allowed any tags 
(which is a pity, I would then again have no device with tags supported 
for tests here:-(), then, please, apply the attached patch. A patch to use 
the blacklist will follow shortly.

Thanks
Guennadi
---
Guennadi Liakhovetski

[-- Attachment #2: Type: TEXT/PLAIN, Size: 652 bytes --]

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

diff -u a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
--- a/drivers/scsi/scsi_devinfo.c	Mon Sep  6 10:37:08 2004
+++ b/drivers/scsi/scsi_devinfo.c	Mon Sep  6 10:36:30 2004
@@ -181,6 +181,7 @@
 	{"REGAL", "CDC-4X", NULL, BLIST_MAX5LUN | BLIST_SINGLELUN},
 	{"SanDisk", "ImageMate CF-SD1", NULL, BLIST_FORCELUN},
 	{"SEAGATE", "ST3390N", "9546", BLIST_NOTQ},
+	{"SEAGATE", "ST34555N", "0930", BLIST_NOTQ},	/* Chokes on tagged INQUIRY */
 	{"SGI", "RAID3", "*", BLIST_SPARSELUN},
 	{"SGI", "RAID5", "*", BLIST_SPARSELUN},
 	{"SGI", "TP9100", "*", BLIST_REPORTLUN2},

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Guennadi Liakhovetski]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1068 bytes --]

On Mon, 6 Sep 2004, Guennadi Liakhovetski wrote:

> On Mon, 5 Sep 2004, James Bottomley wrote:
>
>> On Sun, 2004-09-05 at 12:31, Christoph Hellwig wrote:
>>> Umm yes.  But lets get the driver no oops first and then move forward.
>>> There's still a lot of work to do on the driver.
>> 
>> I think perhaps the correct palliative fix is simply to list this device
>> (whatever it is, it's not listed in the bug report) as BLIST_NOTQ, then
>> the problem won't arise (and of course, tmscsim would have actually to
>> use the mid-layer black list rather than its own much shorter internal
>> list).
>
> I don't know what device the original reporter had, but the one I have here, 
> which exibits the same behaviour, is a Seagate disk (see attached patch). So, 
> if you still think, this device should not be allowed any tags (which is a 
> pity, I would then again have no device with tags supported for tests 
> here:-(), then, please, apply the attached patch. A patch to use the 
> blacklist will follow shortly.

Attached.

Thanks
Guennadi
---
Guennadi Liakhovetski

[-- Attachment #2: Type: TEXT/PLAIN, Size: 6909 bytes --]

Use mid-layer's decision wrt tag-support.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

diff -u a/drivers/scsi/scsiiom.c b/drivers/scsi/scsiiom.c
--- a/drivers/scsi/scsiiom.c	17 Aug 2004 21:16:53
+++ b/drivers/scsi/scsiiom.c	6 Sep 2004 09:32:53
@@ -8,9 +8,9 @@
 static void __inline__
 dc390_freetag (struct dc390_dcb* pDCB, struct dc390_srb* pSRB)
 {
-	if (pSRB->TagNumber < 255) {
+	if (pSRB->TagNumber != SCSI_NO_TAG) {
 		pDCB->TagMask &= ~(1 << pSRB->TagNumber);   /* free tag mask */
-		pSRB->TagNumber = 255;
+		pSRB->TagNumber = SCSI_NO_TAG;
 	}
 }
 
@@ -69,7 +69,7 @@
     /* Change 99/05/31: Don't use tags when not disconnecting (BUSY) */
     if ((pDCB->SyncMode & EN_TAG_QUEUEING) && disc_allowed)
       {
-	u8 tag_no = 0;
+	s8 tag_no = 0;
 	while ((1 << tag_no) & pDCB->TagMask) tag_no++;
 	if (tag_no >= sizeof (pDCB->TagMask)*8 || tag_no >= pDCB->MaxCommand) { 
 		printk (KERN_WARNING "DC390: Out of tags for Dev. %02x %02x\n", pDCB->TargetID, pDCB->TargetLUN); 
@@ -612,7 +612,7 @@
 }
 
 static struct dc390_srb*
-dc390_MsgIn_QTag (struct dc390_acb* pACB, struct dc390_dcb* pDCB, u8 tag)
+dc390_MsgIn_QTag (struct dc390_acb* pACB, struct dc390_dcb* pDCB, s8 tag)
 {
   struct dc390_srb* lastSRB = pDCB->pGoingLast;
   struct dc390_srb* pSRB = pDCB->pGoingSRB;
@@ -1308,38 +1308,6 @@
 	return 1;
    return 0;
 }
-   
-
-static void 
-dc390_disc_tagq_set (struct dc390_dcb* pDCB, PSCSI_INQDATA ptr)
-{
-   /* Check for SCSI format (ANSI and Response data format) */
-   if ( (ptr->Vers & 0x07) >= 2 || (ptr->RDF & 0x0F) == 2 )
-   {
-	if ( (ptr->Flags & SCSI_INQ_CMDQUEUE) &&
-	    (pDCB->DevMode & TAG_QUEUEING_) &&
-	    /* ((pDCB->DevType == TYPE_DISK) 
-		|| (pDCB->DevType == TYPE_MOD)) &&*/
-	    !dc390_tagq_blacklist (((char*)ptr)+8) )
-	  {
-	     if (pDCB->MaxCommand ==1) pDCB->MaxCommand = pDCB->pDCBACB->TagMaxNum;
-	     pDCB->SyncMode |= EN_TAG_QUEUEING /* | EN_ATN_STOP */;
-	     //pDCB->TagMask = 0;
-	  }
-	else
-	     pDCB->MaxCommand = 1;
-     }
-}
-
-
-static void 
-dc390_add_dev (struct dc390_acb* pACB, struct dc390_dcb* pDCB, PSCSI_INQDATA ptr)
-{
-   u8 bval1 = ptr->DevType & SCSI_DEVTYPE;
-   pDCB->DevType = bval1;
-   /* if (bval1 == TYPE_DISK || bval1 == TYPE_MOD) */
-	dc390_disc_tagq_set (pDCB, ptr);
-}
 
 
 static void __inline__
@@ -1552,15 +1520,11 @@
 	    pDCB->Inquiry7 = ptr->Flags;
 
 ckc_e:
-    if( pcmd->cmnd[0] == INQUIRY && 
-	(pcmd->result == (DID_OK << 16) || status_byte(pcmd->result) & CHECK_CONDITION) )
-     {
-	if ((ptr->DevType & SCSI_DEVTYPE) != TYPE_NODEV)
-	  {
-	     /* device found: add */ 
-	     dc390_add_dev (pACB, pDCB, ptr);
-	  }
-     }
+    if (pcmd->cmnd[0] == INQUIRY && 
+	(pcmd->result == (DID_OK << 16) || status_byte(pcmd->result) & CHECK_CONDITION) &&
+	(ptr->DevType & SCSI_DEVTYPE) != TYPE_NODEV)
+	/* device found: add */ 
+	pDCB->DevType = ptr->DevType & SCSI_DEVTYPE;
 
     pcmd->resid = pcmd->request_bufflen - pSRB->TotalXferredLen;
 
diff -u a/drivers/scsi/tmscsim.h b/drivers/scsi/tmscsim.h
--- a/drivers/scsi/tmscsim.h	17 Aug 2004 21:16:53
+++ b/drivers/scsi/tmscsim.h	6 Sep 2004 09:33:08
@@ -62,7 +62,7 @@
 u8		TargetStatus;
 
 u8		ScsiPhase;
-u8		TagNumber;
+s8		TagNumber;
 u8		SGIndex;
 u8		SGcount;
 
diff -u a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
--- a/drivers/scsi/tmscsim.c	3 Sep 2004 22:10:14
+++ b/drivers/scsi/tmscsim.c	6 Sep 2004 09:33:59
@@ -245,6 +245,7 @@
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsicam.h>
+#include <scsi/scsi_tcq.h>
 
 
 #define DC390_BANNER "Tekram DC390/AM53C974"
@@ -293,7 +294,6 @@
 static irqreturn_t do_DC390_Interrupt( int, void *, struct pt_regs *);
 
 static int    dc390_initAdapter(struct Scsi_Host *psh, unsigned long io_port, u8 Irq, u8 index );
-static void   dc390_updateDCB (struct dc390_acb* pACB, struct dc390_dcb* pDCB);
 
 static struct dc390_acb*	dc390_pACB_start= NULL;
 static struct dc390_acb*	dc390_pACB_current = NULL;
@@ -892,7 +892,7 @@
 	srb->SGToBeXferLen = 0;
 	srb->ScsiPhase = 0;
 	srb->EndMessage = 0;
-	srb->TagNumber = 255;
+	srb->TagNumber = SCSI_NO_TAG;
 
 	if (dc390_StartSCSI(acb, dcb, srb)) {
 		dc390_Waiting_insert(dcb, srb);
@@ -1069,35 +1069,6 @@
 
 #include "scsiiom.c"
 
-/***********************************************************************
- * Function : static void dc390_updateDCB()
- *
- * Purpose :  Set the configuration dependent DCB parameters
- ***********************************************************************/
-
-static void dc390_updateDCB (struct dc390_acb* pACB, struct dc390_dcb* pDCB)
-{
-  pDCB->SyncMode &= EN_TAG_QUEUEING | SYNC_NEGO_DONE /*| EN_ATN_STOP*/;
-  if (pDCB->DevMode & TAG_QUEUEING_) {
-	//if (pDCB->SyncMode & EN_TAG_QUEUEING) pDCB->MaxCommand = pACB->TagMaxNum;
-  } else {
-	pDCB->SyncMode &= ~EN_TAG_QUEUEING;
-	pDCB->MaxCommand = 1;
-  }
-
-  if( pDCB->DevMode & SYNC_NEGO_ )
-	pDCB->SyncMode |= SYNC_ENABLE;
-  else {
-	pDCB->SyncMode &= ~(SYNC_NEGO_DONE | SYNC_ENABLE);
-	pDCB->SyncOffset &= ~0x0f;
-  }
-
-  //if (! (pDCB->DevMode & EN_DISCONNECT_)) pDCB->SyncMode &= ~EN_ATN_STOP; 
-
-  pDCB->CtrlR1 = pACB->pScsiHost->this_id;
-  if( pDCB->DevMode & PARITY_CHK_ )
-	pDCB->CtrlR1 |= PARITY_ERR_REPO;
-}  
 
 /***********************************************************************
  * Function : static void dc390_initSRB()
@@ -1326,7 +1297,7 @@
 	 */
 	if (lun && (pDCB2 = dc390_findDCB(pACB, id, 0))) {
 		pDCB->DevMode = pDCB2->DevMode;
-		pDCB->SyncMode = pDCB2->SyncMode;
+		pDCB->SyncMode = pDCB2->SyncMode & SYNC_NEGO_DONE;
 		pDCB->SyncPeriod = pDCB2->SyncPeriod;
 		pDCB->SyncOffset = pDCB2->SyncOffset;
 		pDCB->NegoPeriod = pDCB2->NegoPeriod;
@@ -1347,7 +1318,16 @@
 			pDCB->CtrlR4 |= NEGATE_REQACKDATA | NEGATE_REQACK;
 	}
 
-	dc390_updateDCB(pACB, pDCB);
+	if (pDCB->DevMode & SYNC_NEGO_)
+		pDCB->SyncMode |= SYNC_ENABLE;
+	else {
+		pDCB->SyncMode = 0;
+		pDCB->SyncOffset &= ~0x0f;
+	}
+
+	pDCB->CtrlR1 = pACB->pScsiHost->this_id;
+	if (pDCB->DevMode & PARITY_CHK_)
+		pDCB->CtrlR1 |= PARITY_ERR_REPO;
 
 	pACB->scan_devices = 1;
 	scsi_device->hostdata = pDCB;
@@ -1395,10 +1375,16 @@
 	pACB->DCBCnt--;
 }
 
-static int dc390_slave_configure(struct scsi_device *scsi_device)
+static int dc390_slave_configure(struct scsi_device *sdev)
 {
-	struct dc390_acb* pACB = (struct dc390_acb*) scsi_device->host->hostdata;
-	pACB->scan_devices = 0;
+	struct dc390_acb *acb = (struct dc390_acb *)sdev->host->hostdata;
+	struct dc390_dcb *dcb = (struct dc390_dcb *)sdev->hostdata;
+
+	acb->scan_devices = 0;
+	if (sdev->tagged_supported && (dcb->DevMode & TAG_QUEUEING_)) {
+		dcb->SyncMode |= EN_TAG_QUEUEING;
+		dcb->MaxCommand = dcb->pDCBACB->TagMaxNum;
+	}
 	return 0;
 }
 

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Christoph Hellwig]

Patch looks good to me.  Two hints for a possible followon patch:

> @@ -1552,15 +1520,11 @@
>  	    pDCB->Inquiry7 = ptr->Flags;
>  
>  ckc_e:

Here we still have some INQUIRY snooping, and if you look how
ptr is set you'll see it'll OOPS for machines with more than 1GB of memory.

> -	dc390_updateDCB(pACB, pDCB);
> +	if (pDCB->DevMode & SYNC_NEGO_)
> +		pDCB->SyncMode |= SYNC_ENABLE;
> +	else {
> +		pDCB->SyncMode = 0;

pDCB is memset to 0 åt the start of the function, so the 0 initialization
here isn't nessecary strictly speaking.

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

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Guennadi Liakhovetski]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1076 bytes --]

On Mon, 6 Sep 2004, Christoph Hellwig wrote:

> Patch looks good to me.  Two hints for a possible followon patch:
>
>> @@ -1552,15 +1520,11 @@
>>  	    pDCB->Inquiry7 = ptr->Flags;
>>
>>  ckc_e:
>
> Here we still have some INQUIRY snooping, and if you look how
> ptr is set you'll see it'll OOPS for machines with more than 1GB of memory.

Right. Will think about a fix, thanks.

>> -	dc390_updateDCB(pACB, pDCB);
>> +	if (pDCB->DevMode & SYNC_NEGO_)
>> +		pDCB->SyncMode |= SYNC_ENABLE;
>> +	else {
>> +		pDCB->SyncMode = 0;
>
> pDCB is memset to 0 Еt the start of the function, so the 0 initialization
> here isn't nessecary strictly speaking.

Well, a few lines above on a multi-LUN device for LUN > 0 it will be set 
from LUN 0:

                 pDCB->SyncMode = pDCB2->SyncMode & SYNC_NEGO_DONE;

so, purely theoretically it could be non-zero at that point, at least, 
that's why I put

                 pDCB->SyncMode = 0;

there. But, DevMode is set from LUN 0, so, yes, you are right.

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] tmscsim: INQUIRY always only untagged

[James Bottomley]

On Sun, 2004-09-05 at 16:19, Guennadi Liakhovetski wrote:
> >> 1) remove the inquiry snooping and believe the flags in struct
> >> scsi_device instead
> 
> ...and use tags set up by the block layer...

Well, yes, that too ... it depends how much time anyone has to spend on
this driver.

> >> 2) Don't turn on TCQ until slave_configure()
> 
> Well, this alone wouldn't help with this specific problem, exactly because 
> INQUIRY can be sent later too. Or would the mid-layer completely disable 
> tagging for such a disk?

The bug report claims 2.4 works, so probably there the device itself
will still screw up if another inquiry were sent as part of the tagged
stream, I think (perhaps it doesn't, someone needs to check). However,
turning on TCQ in slave_configure() will mean that the two initial
inquiries are sent untagged, so it will only manifest in 2.6 in the same
circumstances as 2.4.

James

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Douglas Gilbert]

James Bottomley wrote:
> On Sun, 2004-09-05 at 12:31, Christoph Hellwig wrote:
> 
>>Umm yes.  But lets get the driver no oops first and then move forward.
>>There's still a lot of work to do on the driver.
> 
> 
> Well, OK.  I agree to putting a palliative fix in, since what I
> suggested will probably take more thought, but I don't think this one is
> actually it.  Mixing tagged and untagged commands is illegal under the
> SCSI spec.  Simply untagging the INQUIRY command may work to fix this
> particular bug, but I bet it will introduce others when a scsi utility
> sends an INQUIRY down to an in-use device.

Seems as though INQUIRY, REPORT LUNS and READ CAPACITY are special
in this respect since the task manager may (i.e. should) treat them
as implicit Head Of Queue. This proposal:
http://www.t10.org/ftp/t10/document.04/04-310r1.pdf
advises application clients not to send the "specials" with
ORDERED (i.e. tagged) task attribute.

My guess is that if any data that those "specials" monitor is changed
they will hit the next command status with an advisory
sense_key/asc/ascq sense. Then the application client should
send one of those "specials" out asap to find out what changed (and
not be held up by task manager).

Doug Gilbert

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Guennadi Liakhovetski]

On Mon, 6 Sep 2004, Guennadi Liakhovetski wrote:

> On Mon, 5 Sep 2004, James Bottomley wrote:
>
>> I think perhaps the correct palliative fix is simply to list this device
>> (whatever it is, it's not listed in the bug report) as BLIST_NOTQ, then
>> the problem won't arise (and of course, tmscsim would have actually to
>> use the mid-layer black list rather than its own much shorter internal
>> list).
>
> I don't know what device the original reporter had, but the one I have here, 
> which exibits the same behaviour, is a Seagate disk (see attached patch). So, 
> if you still think, this device should not be allowed any tags (which is a 
> pity, I would then again have no device with tags supported for tests 
> here:-(), then, please, apply the attached patch. A patch to use the 
> blacklist will follow shortly.

Looks like this patch didn't get applied until now. Have you changed your 
opinion about the proper handling of this disk, James, or it just somehow 
got lost? Attaching again below. Next patch to tmscsim will switch the 
driver to use the device information from the mid-layer, so, this patch is 
required.

Thanks
Guennadi
---
Guennadi Liakhovetski


Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

diff -u a/drivers/scsi/scsi_devinfo.c b/drivers/scsi/scsi_devinfo.c
--- a/drivers/scsi/scsi_devinfo.c	Mon Sep  6 10:37:08 2004
+++ b/drivers/scsi/scsi_devinfo.c	Mon Sep  6 10:36:30 2004
@@ -181,6 +181,7 @@
  	{"REGAL", "CDC-4X", NULL, BLIST_MAX5LUN | BLIST_SINGLELUN},
  	{"SanDisk", "ImageMate CF-SD1", NULL, BLIST_FORCELUN},
  	{"SEAGATE", "ST3390N", "9546", BLIST_NOTQ},
+	{"SEAGATE", "ST34555N", "0930", BLIST_NOTQ},	/* Chokes on tagged INQUIRY */
  	{"SGI", "RAID3", "*", BLIST_SPARSELUN},
  	{"SGI", "RAID5", "*", BLIST_SPARSELUN},
  	{"SGI", "TP9100", "*", BLIST_REPORTLUN2},

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Guennadi Liakhovetski]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 9000 bytes --]

Re-sending the long-outstanding patch.

On Mon, 6 Sep 2004, Guennadi Liakhovetski wrote:

> On Mon, 6 Sep 2004, Christoph Hellwig wrote:
>
>> Patch looks good to me.  Two hints for a possible followon patch:
>> 
>>> @@ -1552,15 +1520,11 @@
>>>  	    pDCB->Inquiry7 = ptr->Flags;
>>> 
>>>  ckc_e:
>> 
>> Here we still have some INQUIRY snooping, and if you look how
>> ptr is set you'll see it'll OOPS for machines with more than 1GB of 
>> memory.
>
> Right. Will think about a fix, thanks.

Will follow in a separate patch.

>>> -	dc390_updateDCB(pACB, pDCB);
>>> +	if (pDCB->DevMode & SYNC_NEGO_)
>>> +		pDCB->SyncMode |= SYNC_ENABLE;
>>> +	else {
>>> +		pDCB->SyncMode = 0;
>> 
>> pDCB is memset to 0 Еt the start of the function, so the 0 initialization
>> here isn't nessecary strictly speaking.
>
> Well, a few lines above on a multi-LUN device for LUN > 0 it will be set from 
> LUN 0:
>
>                pDCB->SyncMode = pDCB2->SyncMode & SYNC_NEGO_DONE;
>
> so, purely theoretically it could be non-zero at that point, at least, that's 
> why I put
>
>                pDCB->SyncMode = 0;
>
> there. But, DevMode is set from LUN 0, so, yes, you are right.

I am not sure any more if my analysis above is correct, so, I'd retain the 
explicit "= 0" for now.

Thanks
Guennadi
---
Guennadi Liakhovetski


Use mid-layer's decision wrt tag-support.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

diff -u a/drivers/scsi/scsiiom.c b/drivers/scsi/scsiiom.c
--- a/drivers/scsi/scsiiom.c	17 Aug 2004 21:16:53
+++ b/drivers/scsi/scsiiom.c	6 Sep 2004 09:32:53
@@ -8,9 +8,9 @@
  static void __inline__
  dc390_freetag (struct dc390_dcb* pDCB, struct dc390_srb* pSRB)
  {
-	if (pSRB->TagNumber < 255) {
+	if (pSRB->TagNumber != SCSI_NO_TAG) {
  		pDCB->TagMask &= ~(1 << pSRB->TagNumber);   /* free tag mask */
-		pSRB->TagNumber = 255;
+		pSRB->TagNumber = SCSI_NO_TAG;
  	}
  }

@@ -69,7 +69,7 @@
      /* Change 99/05/31: Don't use tags when not disconnecting (BUSY) */
      if ((pDCB->SyncMode & EN_TAG_QUEUEING) && disc_allowed)
        {
-	u8 tag_no = 0;
+	s8 tag_no = 0;
  	while ((1 << tag_no) & pDCB->TagMask) tag_no++;
  	if (tag_no >= sizeof (pDCB->TagMask)*8 || tag_no >= pDCB->MaxCommand) {
  		printk (KERN_WARNING "DC390: Out of tags for Dev. %02x %02x\n", pDCB->TargetID, pDCB->TargetLUN); 
@@ -604,7 +604,7 @@
  }

  static struct dc390_srb*
-dc390_MsgIn_QTag (struct dc390_acb* pACB, struct dc390_dcb* pDCB, u8 tag)
+dc390_MsgIn_QTag (struct dc390_acb* pACB, struct dc390_dcb* pDCB, s8 tag)
  {
    struct dc390_srb* lastSRB = pDCB->pGoingLast;
    struct dc390_srb* pSRB = pDCB->pGoingSRB;
@@ -1291,49 +1291,6 @@
      DC390_write8 (ScsiCmd, MSG_ACCEPTED_CMD);	/* ;to release the /ACK signal */
  }

-static u8 __inline__
-dc390_tagq_blacklist (char* name)
-{
-   u8 i;
-   for(i=0; i<BADDEVCNT; i++)
-     if (memcmp (name, dc390_baddevname1[i], 28) == 0)
-	return 1;
-   return 0;
-}
- 
-
-static void 
-dc390_disc_tagq_set (struct dc390_dcb* pDCB, PSCSI_INQDATA ptr)
-{
-   /* Check for SCSI format (ANSI and Response data format) */
-   if ( (ptr->Vers & 0x07) >= 2 || (ptr->RDF & 0x0F) == 2 )
-   {
-	if ( (ptr->Flags & SCSI_INQ_CMDQUEUE) &&
-	    (pDCB->DevMode & TAG_QUEUEING_) &&
-	    /* ((pDCB->DevType == TYPE_DISK) 
-		|| (pDCB->DevType == TYPE_MOD)) &&*/
-	    !dc390_tagq_blacklist (((char*)ptr)+8) )
-	  {
-	     if (pDCB->MaxCommand ==1) pDCB->MaxCommand = pDCB->pDCBACB->TagMaxNum;
-	     pDCB->SyncMode |= EN_TAG_QUEUEING /* | EN_ATN_STOP */;
-	     //pDCB->TagMask = 0;
-	  }
-	else
-	     pDCB->MaxCommand = 1;
-     }
-}
-
-
-static void 
-dc390_add_dev (struct dc390_acb* pACB, struct dc390_dcb* pDCB, PSCSI_INQDATA ptr)
-{
-   u8 bval1 = ptr->DevType & SCSI_DEVTYPE;
-   pDCB->DevType = bval1;
-   /* if (bval1 == TYPE_DISK || bval1 == TYPE_MOD) */
-	dc390_disc_tagq_set (pDCB, ptr);
-}
-
-
  static void __inline__
  dc390_RequestSense(struct dc390_acb* pACB, struct dc390_dcb* pDCB, struct dc390_srb* pSRB)
  {
@@ -1544,15 +1501,11 @@
  	    pDCB->Inquiry7 = ptr->Flags;

  ckc_e:
-    if( pcmd->cmnd[0] == INQUIRY && 
-	(pcmd->result == (DID_OK << 16) || status_byte(pcmd->result) & CHECK_CONDITION) )
-     {
-	if ((ptr->DevType & SCSI_DEVTYPE) != TYPE_NODEV)
-	  {
-	     /* device found: add */ 
-	     dc390_add_dev (pACB, pDCB, ptr);
-	  }
-     }
+    if (pcmd->cmnd[0] == INQUIRY && 
+	(pcmd->result == (DID_OK << 16) || status_byte(pcmd->result) & CHECK_CONDITION) &&
+	(ptr->DevType & SCSI_DEVTYPE) != TYPE_NODEV)
+	/* device found: add */ 
+	pDCB->DevType = ptr->DevType & SCSI_DEVTYPE;

      pcmd->resid = pcmd->request_bufflen - pSRB->TotalXferredLen;

diff -u a/drivers/scsi/tmscsim.h b/drivers/scsi/tmscsim.h
--- a/drivers/scsi/tmscsim.h	17 Aug 2004 21:16:53
+++ b/drivers/scsi/tmscsim.h	6 Sep 2004 09:33:08
@@ -62,7 +62,7 @@
  u8		TargetStatus;

  u8		ScsiPhase;
-u8		TagNumber;
+s8		TagNumber;
  u8		SGIndex;
  u8		SGcount;

diff -u a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
--- a/drivers/scsi/tmscsim.c	3 Sep 2004 22:10:14
+++ b/drivers/scsi/tmscsim.c	6 Sep 2004 09:33:59
@@ -242,6 +242,7 @@
  #include <scsi/scsi_device.h>
  #include <scsi/scsi_host.h>
  #include <scsi/scsicam.h>
+#include <scsi/scsi_tcq.h>


  #define DC390_BANNER "Tekram DC390/AM53C974"
@@ -277,8 +278,6 @@
  static void dc390_EnableMsgOut_Abort(struct dc390_acb*, struct dc390_srb*);
  static irqreturn_t do_DC390_Interrupt( int, void *, struct pt_regs *);

-static void   dc390_updateDCB (struct dc390_acb* pACB, struct dc390_dcb* pDCB);
-
  static u32	dc390_laststatus = 0;
  static u8	dc390_adapterCnt = 0;

@@ -343,12 +342,6 @@
         };
  #endif

-/* Devices erroneously pretending to be able to do TagQ */
-static u8  dc390_baddevname1[2][28] ={
-       "SEAGATE ST3390N         9546",
-       "HP      C3323-300       4269"};
-#define BADDEVCNT	2
-
  static u8  dc390_eepromBuf[MAX_ADAPTER_NUM][EE_LEN];
  static u8  dc390_clock_period1[] = {4, 5, 6, 7, 8, 10, 13, 20};
  static u8  dc390_clock_speed[] = {100,80,67,57,50, 40, 31, 20};
@@ -648,7 +641,7 @@
  	srb->SGToBeXferLen = 0;
  	srb->ScsiPhase = 0;
  	srb->EndMessage = 0;
-	srb->TagNumber = 255;
+	srb->TagNumber = SCSI_NO_TAG;

  	if (dc390_StartSCSI(acb, dcb, srb)) {
  		dc390_Waiting_insert(dcb, srb);
@@ -825,35 +818,6 @@

  #include "scsiiom.c"

-/***********************************************************************
- * Function : static void dc390_updateDCB()
- *
- * Purpose :  Set the configuration dependent DCB parameters
- ***********************************************************************/
-
-static void dc390_updateDCB (struct dc390_acb* pACB, struct dc390_dcb* pDCB)
-{
-  pDCB->SyncMode &= EN_TAG_QUEUEING | SYNC_NEGO_DONE /*| EN_ATN_STOP*/;
-  if (pDCB->DevMode & TAG_QUEUEING_) {
-	//if (pDCB->SyncMode & EN_TAG_QUEUEING) pDCB->MaxCommand = pACB->TagMaxNum;
-  } else {
-	pDCB->SyncMode &= ~EN_TAG_QUEUEING;
-	pDCB->MaxCommand = 1;
-  }
-
-  if( pDCB->DevMode & SYNC_NEGO_ )
-	pDCB->SyncMode |= SYNC_ENABLE;
-  else {
-	pDCB->SyncMode &= ~(SYNC_NEGO_DONE | SYNC_ENABLE);
-	pDCB->SyncOffset &= ~0x0f;
-  }
-
-  //if (! (pDCB->DevMode & EN_DISCONNECT_)) pDCB->SyncMode &= ~EN_ATN_STOP; 
-
-  pDCB->CtrlR1 = pACB->pScsiHost->this_id;
-  if( pDCB->DevMode & PARITY_CHK_ )
-	pDCB->CtrlR1 |= PARITY_ERR_REPO;
-}

  /**
   * dc390_slave_alloc - Called by the scsi mid layer to tell us about a new
@@ -894,7 +858,7 @@
  	 */
  	if (lun && (pDCB2 = dc390_findDCB(pACB, id, 0))) {
  		pDCB->DevMode = pDCB2->DevMode;
-		pDCB->SyncMode = pDCB2->SyncMode;
+		pDCB->SyncMode = pDCB2->SyncMode & SYNC_NEGO_DONE;
  		pDCB->SyncPeriod = pDCB2->SyncPeriod;
  		pDCB->SyncOffset = pDCB2->SyncOffset;
  		pDCB->NegoPeriod = pDCB2->NegoPeriod;
@@ -915,7 +879,16 @@
  			pDCB->CtrlR4 |= NEGATE_REQACKDATA | NEGATE_REQACK;
  	}

-	dc390_updateDCB(pACB, pDCB);
+	if (pDCB->DevMode & SYNC_NEGO_)
+		pDCB->SyncMode |= SYNC_ENABLE;
+	else {
+		pDCB->SyncMode = 0;
+		pDCB->SyncOffset &= ~0x0f;
+	}
+
+	pDCB->CtrlR1 = pACB->pScsiHost->this_id;
+	if (pDCB->DevMode & PARITY_CHK_)
+		pDCB->CtrlR1 |= PARITY_ERR_REPO;

  	pACB->scan_devices = 1;
  	scsi_device->hostdata = pDCB;
@@ -963,10 +936,16 @@
  	pACB->DCBCnt--;
  }

-static int dc390_slave_configure(struct scsi_device *scsi_device)
+static int dc390_slave_configure(struct scsi_device *sdev)
  {
-	struct dc390_acb* pACB = (struct dc390_acb*) scsi_device->host->hostdata;
-	pACB->scan_devices = 0;
+	struct dc390_acb *acb = (struct dc390_acb *)sdev->host->hostdata;
+	struct dc390_dcb *dcb = (struct dc390_dcb *)sdev->hostdata;
+
+	acb->scan_devices = 0;
+	if (sdev->tagged_supported && (dcb->DevMode & TAG_QUEUEING_)) {
+		dcb->SyncMode |= EN_TAG_QUEUEING;
+		dcb->MaxCommand = dcb->pDCBACB->TagMaxNum;
+	}
  	return 0;
  }

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Christoph Hellwig]

On Fri, Oct 01, 2004 at 11:00:23PM +0200, Guennadi Liakhovetski wrote:
> On Mon, 6 Sep 2004, Guennadi Liakhovetski wrote:
> 
> > On Mon, 5 Sep 2004, James Bottomley wrote:
> >
> >> I think perhaps the correct palliative fix is simply to list this device
> >> (whatever it is, it's not listed in the bug report) as BLIST_NOTQ, then
> >> the problem won't arise (and of course, tmscsim would have actually to
> >> use the mid-layer black list rather than its own much shorter internal
> >> list).
> >
> > I don't know what device the original reporter had, but the one I have here, 
> > which exibits the same behaviour, is a Seagate disk (see attached patch). So, 
> > if you still think, this device should not be allowed any tags (which is a 
> > pity, I would then again have no device with tags supported for tests 
> > here:-(), then, please, apply the attached patch. A patch to use the 
> > blacklist will follow shortly.
> 
> Looks like this patch didn't get applied until now. Have you changed your 
> opinion about the proper handling of this disk, James, or it just somehow 
> got lost? Attaching again below. Next patch to tmscsim will switch the 
> driver to use the device information from the mid-layer, so, this patch is 
> required.

I get rejects in every single hunk when trying to apply your patches.  It
looks like some kind of whitespace messup, but I don't see in what way.

Did you recently switch mailers?

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Christoph Hellwig]

> +    if (pcmd->cmnd[0] == INQUIRY && 
> +	(pcmd->result == (DID_OK << 16) || status_byte(pcmd->result) & CHECK_CONDITION) &&
> +	(ptr->DevType & SCSI_DEVTYPE) != TYPE_NODEV)
> +	/* device found: add */ 
> +	pDCB->DevType = ptr->DevType & SCSI_DEVTYPE;

instead of ptr->DevType you can use the 'device' field in struct scsi_device.
In fact the dcb DevType field should go away aswell in favour of that one.

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Christoph Hellwig]

On Fri, Oct 01, 2004 at 11:35:54PM +0100, Christoph Hellwig wrote:
> > +    if (pcmd->cmnd[0] == INQUIRY && 
> > +	(pcmd->result == (DID_OK << 16) || status_byte(pcmd->result) & CHECK_CONDITION) &&
> > +	(ptr->DevType & SCSI_DEVTYPE) != TYPE_NODEV)
> > +	/* device found: add */ 
> > +	pDCB->DevType = ptr->DevType & SCSI_DEVTYPE;
> 
> instead of ptr->DevType you can use the 'device' field in struct scsi_device.
> In fact the dcb DevType field should go away aswell in favour of that one.

And while we're at it: The whole inquiry sniffing in dc390_SRBdone (aka all
uses of ptr and ptr) can easily go away if you kill pDCB->Inquiry7 and check
sdev->sdtr instead.

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Guennadi Liakhovetski]

On Fri, 1 Oct 2004, Christoph Hellwig wrote:

> I get rejects in every single hunk when trying to apply your patches.  It
> looks like some kind of whitespace messup, but I don't see in what way.
>
> Did you recently switch mailers?

Ouch... I thought it did work eventually, apparently, not:-( I didn't 
switch - I sent patches as attachments up to now - will have to do this in 
the future too. Will re-send. Thanks for pointing out.

Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Guennadi Liakhovetski]

On Fri, 1 Oct 2004, Christoph Hellwig wrote:

> And while we're at it: The whole inquiry sniffing in dc390_SRBdone (aka all
> uses of ptr and ptr) can easily go away if you kill pDCB->Inquiry7 and check
> sdev->sdtr instead.

You once already suggested that - I didn't forget, it's a separate 
patch:-) It'll follow, but after "internal queuing removal."

Thanks
Guennadi
---
Guennadi Liakhovetski

Re: [PATCH] tmscsim: INQUIRY always only untagged

[Guennadi Liakhovetski]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 711 bytes --]

On Fri, 1 Oct 2004, Christoph Hellwig wrote:

>> +    if (pcmd->cmnd[0] == INQUIRY &&
>> +	(pcmd->result == (DID_OK << 16) || status_byte(pcmd->result) & CHECK_CONDITION) &&
>> +	(ptr->DevType & SCSI_DEVTYPE) != TYPE_NODEV)
>> +	/* device found: add */
>> +	pDCB->DevType = ptr->DevType & SCSI_DEVTYPE;
>
> instead of ptr->DevType you can use the 'device' field in struct scsi_device.
> In fact the dcb DevType field should go away aswell in favour of that one.

Ok, as I just realised, this can be "naturally" addressed by the next 
patch, removing internal queues. So, for now I am just re-sending 
yesterday's patch as attachments, if there are no further comments.

Thanks
Guennadi
---
Guennadi Liakhovetski

[-- Attachment #2: Type: TEXT/PLAIN, Size: 7605 bytes --]

Use mid-layer's decision wrt tag-support.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

diff -u a/drivers/scsi/scsiiom.c b/drivers/scsi/scsiiom.c
--- a/drivers/scsi/scsiiom.c	17 Aug 2004 21:16:53
+++ b/drivers/scsi/scsiiom.c	6 Sep 2004 09:32:53
@@ -8,9 +8,9 @@
 static void __inline__
 dc390_freetag (struct dc390_dcb* pDCB, struct dc390_srb* pSRB)
 {
-	if (pSRB->TagNumber < 255) {
+	if (pSRB->TagNumber != SCSI_NO_TAG) {
 		pDCB->TagMask &= ~(1 << pSRB->TagNumber);   /* free tag mask */
-		pSRB->TagNumber = 255;
+		pSRB->TagNumber = SCSI_NO_TAG;
 	}
 }
 
@@ -69,7 +69,7 @@
     /* Change 99/05/31: Don't use tags when not disconnecting (BUSY) */
     if ((pDCB->SyncMode & EN_TAG_QUEUEING) && disc_allowed)
       {
-	u8 tag_no = 0;
+	s8 tag_no = 0;
 	while ((1 << tag_no) & pDCB->TagMask) tag_no++;
 	if (tag_no >= sizeof (pDCB->TagMask)*8 || tag_no >= pDCB->MaxCommand) { 
 		printk (KERN_WARNING "DC390: Out of tags for Dev. %02x %02x\n", pDCB->TargetID, pDCB->TargetLUN); 
@@ -604,7 +604,7 @@
 }
 
 static struct dc390_srb*
-dc390_MsgIn_QTag (struct dc390_acb* pACB, struct dc390_dcb* pDCB, u8 tag)
+dc390_MsgIn_QTag (struct dc390_acb* pACB, struct dc390_dcb* pDCB, s8 tag)
 {
   struct dc390_srb* lastSRB = pDCB->pGoingLast;
   struct dc390_srb* pSRB = pDCB->pGoingSRB;
@@ -1291,49 +1291,6 @@
     DC390_write8 (ScsiCmd, MSG_ACCEPTED_CMD);	/* ;to release the /ACK signal */
 }
 
-static u8 __inline__
-dc390_tagq_blacklist (char* name)
-{
-   u8 i;
-   for(i=0; i<BADDEVCNT; i++)
-     if (memcmp (name, dc390_baddevname1[i], 28) == 0)
-	return 1;
-   return 0;
-}
-   
-
-static void 
-dc390_disc_tagq_set (struct dc390_dcb* pDCB, PSCSI_INQDATA ptr)
-{
-   /* Check for SCSI format (ANSI and Response data format) */
-   if ( (ptr->Vers & 0x07) >= 2 || (ptr->RDF & 0x0F) == 2 )
-   {
-	if ( (ptr->Flags & SCSI_INQ_CMDQUEUE) &&
-	    (pDCB->DevMode & TAG_QUEUEING_) &&
-	    /* ((pDCB->DevType == TYPE_DISK) 
-		|| (pDCB->DevType == TYPE_MOD)) &&*/
-	    !dc390_tagq_blacklist (((char*)ptr)+8) )
-	  {
-	     if (pDCB->MaxCommand ==1) pDCB->MaxCommand = pDCB->pDCBACB->TagMaxNum;
-	     pDCB->SyncMode |= EN_TAG_QUEUEING /* | EN_ATN_STOP */;
-	     //pDCB->TagMask = 0;
-	  }
-	else
-	     pDCB->MaxCommand = 1;
-     }
-}
-
-
-static void 
-dc390_add_dev (struct dc390_acb* pACB, struct dc390_dcb* pDCB, PSCSI_INQDATA ptr)
-{
-   u8 bval1 = ptr->DevType & SCSI_DEVTYPE;
-   pDCB->DevType = bval1;
-   /* if (bval1 == TYPE_DISK || bval1 == TYPE_MOD) */
-	dc390_disc_tagq_set (pDCB, ptr);
-}
-
-
 static void __inline__
 dc390_RequestSense(struct dc390_acb* pACB, struct dc390_dcb* pDCB, struct dc390_srb* pSRB)
 {
@@ -1544,15 +1501,11 @@
 	    pDCB->Inquiry7 = ptr->Flags;
 
 ckc_e:
-    if( pcmd->cmnd[0] == INQUIRY && 
-	(pcmd->result == (DID_OK << 16) || status_byte(pcmd->result) & CHECK_CONDITION) )
-     {
-	if ((ptr->DevType & SCSI_DEVTYPE) != TYPE_NODEV)
-	  {
-	     /* device found: add */ 
-	     dc390_add_dev (pACB, pDCB, ptr);
-	  }
-     }
+    if (pcmd->cmnd[0] == INQUIRY && 
+	(pcmd->result == (DID_OK << 16) || status_byte(pcmd->result) & CHECK_CONDITION) &&
+	(ptr->DevType & SCSI_DEVTYPE) != TYPE_NODEV)
+	/* device found: add */ 
+	pDCB->DevType = ptr->DevType & SCSI_DEVTYPE;
 
     pcmd->resid = pcmd->request_bufflen - pSRB->TotalXferredLen;
 
diff -u a/drivers/scsi/tmscsim.h b/drivers/scsi/tmscsim.h
--- a/drivers/scsi/tmscsim.h	17 Aug 2004 21:16:53
+++ b/drivers/scsi/tmscsim.h	6 Sep 2004 09:33:08
@@ -62,7 +62,7 @@
 u8		TargetStatus;
 
 u8		ScsiPhase;
-u8		TagNumber;
+s8		TagNumber;
 u8		SGIndex;
 u8		SGcount;
 
diff -u a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
--- a/drivers/scsi/tmscsim.c	3 Sep 2004 22:10:14
+++ b/drivers/scsi/tmscsim.c	6 Sep 2004 09:33:59
@@ -242,6 +242,7 @@
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
 #include <scsi/scsicam.h>
+#include <scsi/scsi_tcq.h>
 
 
 #define DC390_BANNER "Tekram DC390/AM53C974"
@@ -277,8 +278,6 @@
 static void dc390_EnableMsgOut_Abort(struct dc390_acb*, struct dc390_srb*);
 static irqreturn_t do_DC390_Interrupt( int, void *, struct pt_regs *);
 
-static void   dc390_updateDCB (struct dc390_acb* pACB, struct dc390_dcb* pDCB);
-
 static u32	dc390_laststatus = 0;
 static u8	dc390_adapterCnt = 0;
 
@@ -343,12 +342,6 @@
        };
 #endif   
 
-/* Devices erroneously pretending to be able to do TagQ */
-static u8  dc390_baddevname1[2][28] ={
-       "SEAGATE ST3390N         9546",
-       "HP      C3323-300       4269"};
-#define BADDEVCNT	2
-
 static u8  dc390_eepromBuf[MAX_ADAPTER_NUM][EE_LEN];
 static u8  dc390_clock_period1[] = {4, 5, 6, 7, 8, 10, 13, 20};
 static u8  dc390_clock_speed[] = {100,80,67,57,50, 40, 31, 20};
@@ -648,7 +641,7 @@
 	srb->SGToBeXferLen = 0;
 	srb->ScsiPhase = 0;
 	srb->EndMessage = 0;
-	srb->TagNumber = 255;
+	srb->TagNumber = SCSI_NO_TAG;
 
 	if (dc390_StartSCSI(acb, dcb, srb)) {
 		dc390_Waiting_insert(dcb, srb);
@@ -825,35 +818,6 @@
 
 #include "scsiiom.c"
 
-/***********************************************************************
- * Function : static void dc390_updateDCB()
- *
- * Purpose :  Set the configuration dependent DCB parameters
- ***********************************************************************/
-
-static void dc390_updateDCB (struct dc390_acb* pACB, struct dc390_dcb* pDCB)
-{
-  pDCB->SyncMode &= EN_TAG_QUEUEING | SYNC_NEGO_DONE /*| EN_ATN_STOP*/;
-  if (pDCB->DevMode & TAG_QUEUEING_) {
-	//if (pDCB->SyncMode & EN_TAG_QUEUEING) pDCB->MaxCommand = pACB->TagMaxNum;
-  } else {
-	pDCB->SyncMode &= ~EN_TAG_QUEUEING;
-	pDCB->MaxCommand = 1;
-  }
-
-  if( pDCB->DevMode & SYNC_NEGO_ )
-	pDCB->SyncMode |= SYNC_ENABLE;
-  else {
-	pDCB->SyncMode &= ~(SYNC_NEGO_DONE | SYNC_ENABLE);
-	pDCB->SyncOffset &= ~0x0f;
-  }
-
-  //if (! (pDCB->DevMode & EN_DISCONNECT_)) pDCB->SyncMode &= ~EN_ATN_STOP; 
-
-  pDCB->CtrlR1 = pACB->pScsiHost->this_id;
-  if( pDCB->DevMode & PARITY_CHK_ )
-	pDCB->CtrlR1 |= PARITY_ERR_REPO;
-}  
 
 /**
  * dc390_slave_alloc - Called by the scsi mid layer to tell us about a new
@@ -894,7 +858,7 @@
 	 */
 	if (lun && (pDCB2 = dc390_findDCB(pACB, id, 0))) {
 		pDCB->DevMode = pDCB2->DevMode;
-		pDCB->SyncMode = pDCB2->SyncMode;
+		pDCB->SyncMode = pDCB2->SyncMode & SYNC_NEGO_DONE;
 		pDCB->SyncPeriod = pDCB2->SyncPeriod;
 		pDCB->SyncOffset = pDCB2->SyncOffset;
 		pDCB->NegoPeriod = pDCB2->NegoPeriod;
@@ -915,7 +879,16 @@
 			pDCB->CtrlR4 |= NEGATE_REQACKDATA | NEGATE_REQACK;
 	}
 
-	dc390_updateDCB(pACB, pDCB);
+	if (pDCB->DevMode & SYNC_NEGO_)
+		pDCB->SyncMode |= SYNC_ENABLE;
+	else {
+		pDCB->SyncMode = 0;
+		pDCB->SyncOffset &= ~0x0f;
+	}
+
+	pDCB->CtrlR1 = pACB->pScsiHost->this_id;
+	if (pDCB->DevMode & PARITY_CHK_)
+		pDCB->CtrlR1 |= PARITY_ERR_REPO;
 
 	pACB->scan_devices = 1;
 	scsi_device->hostdata = pDCB;
@@ -963,10 +936,16 @@
 	pACB->DCBCnt--;
 }
 
-static int dc390_slave_configure(struct scsi_device *scsi_device)
+static int dc390_slave_configure(struct scsi_device *sdev)
 {
-	struct dc390_acb* pACB = (struct dc390_acb*) scsi_device->host->hostdata;
-	pACB->scan_devices = 0;
+	struct dc390_acb *acb = (struct dc390_acb *)sdev->host->hostdata;
+	struct dc390_dcb *dcb = (struct dc390_dcb *)sdev->hostdata;
+
+	acb->scan_devices = 0;
+	if (sdev->tagged_supported && (dcb->DevMode & TAG_QUEUEING_)) {
+		dcb->SyncMode |= EN_TAG_QUEUEING;
+		dcb->MaxCommand = dcb->pDCBACB->TagMaxNum;
+	}
 	return 0;
 }