libata DMADIR support

Jeff G:

I've now slowly and awkwardly synced as the following tty log of patch and a #define patch of my own show. Therefore I ask:

Have you considered an include/linux/libata.h approach like:

+ #undef ATA_FORCE_DMADIR /* give Si DMADIR to all SATA ATAPI DMA */

+ #include <linux/ata.h>
+ #ifdef ATA_FORCE_DMADIR
+ #undef ata_id_use_dmadir
+ #define ata_id_use_dmadir(dev) 1
+ #endif

I'll save time in the end if I volunteer to write such a patch, after you reply to say you would consider such a patch, if in fact you would.

Pat LaVarre

$ cd linux-2.6.6-bk3/
$
$ patch --dry-run -p1 <~/patch.2.1
patching file drivers/scsi/libata-core.c
patching file drivers/scsi/libata-scsi.c
$ patch --dry-run -p1 <~/patch.dmadir
patching file drivers/scsi/libata-core.c
patching file drivers/scsi/libata-scsi.c
patching file include/linux/ata.h
patching file include/linux/libata.h
$
$ patch -p1 <~/patch.2.1
patching file drivers/scsi/libata-core.c
patching file drivers/scsi/libata-scsi.c
$ patch -p1 <~/patch.dmadir
patching file drivers/scsi/libata-core.c
patching file drivers/scsi/libata-scsi.c
Hunk #1 succeeded at 927 (offset 7 lines).
patching file include/linux/ata.h
patching file include/linux/libata.h
$

diff -Nurp o/include/linux/ata.h linux-2.6.6-bk3-pel/include/linux/ata.h

08:09:04.473768304 -0600
@@ -205,7 +205,7 @@ struct ata_taskfile {
#define ata_id_has_wcache(dev) ((dev)->id[82] & (1 << 5))
#define ata_id_has_lba(dev) ((dev)->id[49] & (1 << 8))
#define ata_id_has_dma(dev) ((dev)->id[49] & (1 << 9))
-#define ata_id_use_dmadir(dev) ((dev)->id[62] & (1 << 15))
+#define ata_id_use_dmadir(dev) 1 /* ((dev)->id[62] & (1 << 15)) */
#define ata_id_u32(dev,n) \
(((u32) (dev)->id[(n) + 1] << 16) | ((u32) (dev)->id[(n)]))
#define ata_id_u64(dev,n) \
diff -Nurp o/include/linux/libata.h linux-2.6.6-bk3-pel/include/linux/libata.h
--- o/include/linux/libata.h 2004-05-16 08:06:23.000000000 -0600
+++ linux-2.6.6-bk3-pel/include/linux/libata.h 2004-05-16
08:09:22.811980472 -0600
@@ -33,11 +33,11 @@
* compile-time options
*/
#undef ATA_FORCE_PIO /* do not configure or use DMA */
-#undef ATA_DEBUG /* debugging output */
-#undef ATA_VERBOSE_DEBUG /* yet more debugging output */
+#define ATA_DEBUG /* debugging output */
+#define ATA_VERBOSE_DEBUG /* yet more debugging output */
#undef ATA_IRQ_TRAP /* define to ack screaming irqs */
#undef ATA_NDEBUG /* define to disable quick runtime checks */
-#undef ATA_ENABLE_ATAPI /* define to enable ATAPI support */
+#define ATA_ENABLE_ATAPI /* define to enable ATAPI support */
#undef ATA_ENABLE_PATA /* define to enable PATA support in some
* low-level drivers */

diff -Nurp o/drivers/scsi/libata-core.c
linux-2.6.6-bk3-pel/drivers/scsi/libata-core.c
diff -Nurp o/drivers/scsi/libata-scsi.c
linux-2.6.6-bk3-pel/drivers/scsi/libata-scsi.c

Should be atapi patch #2.2, which I just commited and pushed upstream.

Jeff

I remember I promised cross-references into the t13.org theories. Over time I hope to walk back thru ATA/PI 7, 6, 5, and 4. Here first is most of a DMADIR grep in ATA/PI 7, quoted below.

I think I see that text:

(a) requires a DMADIR device to tell legacy hosts that the device talks PIO only

(b) requires a DMADIR device to tell new hosts that the device does not talk SWDMA/ MWDMA

(c) claims a DMADIR device should reward a host confused over direction with nothing more violent than SK 5 "ILLEGAL REQUEST", and

(d) lets a not-DMADIR device choose to rudely fail classic UDMA Data In commands, when a host chooses to help by setting the Features & x04 DMADIR bit.

Pat LaVarre

----- d1532v1r4b ATA-ATAPI-7.pdf
...

21 April 2004
...

[page 169 of 390]

6.18 IDENTIFY PACKET DEVICE [xA1]

...

6.18.18 Word 49: Capabilities

...

Bit 15 of word 49 is used to indicate that the device supports interleaved DMA data transfer for overlapped DMA commands. Devices which require the DMADIR bit in the Packet command shall clear this bit to 0.

...

Bit 8 of word 49 indicates that DMA is supported. Devices which require the DMADIR bit in the Packet command shall clear this bit to 0

...

6.18.24 Word 62: DMADIR

ATAPI devices that use a serial ATA bridge chip for connection to a serial ATA host may require use of the DMADIR bit to indicate transfer direction for Packet DMA commands. Word 62 is used to indicate if such support is required.

If bit 15 of word 62 is set to one, then DMADIR bit in the Packet Command is required by the device for Packet DMA and Bits 2:0 of word 63, bits 15 and 8 in word 49, and bits 6:0 of word 88 shall be cleared to 0,.

If bit 15 of word 62 is cleared to 0, DMADIR bit in the PACKET command is not required. If bit 15 of word 62 is cleared to zero, then all bits of word 62 shall be cleared to zero.

Bits (14:11) are reserved.

Bits (10:1) indicate DMA mode support. Since the DMADIR bit is only used for a Serial ATAPI device, all of these bits are set to 1.

...

6.18.25 Word 63: Multiword DMA transfer

...

If the serial interface is implemented, this bit shall be set to one except this bit shall be cleared 0 for Serial ATAPI devices requiring the DMADIR bit in the PACKET command.

...

6.18.41 Word 88:Ultra DMA modes

Word 88 shall have the content described for word 88 of the IDENTIFY DEVICE command, except bits 6:0 shall be cleared to 0 for Serial ATAPI devices which require the DMADIR bit in the Packet command.
...

[page 186 of 390]

6.25 PACKET [xA0]

...

6.25.4 Inputs

...

Features register -

DMADIR -

This bit indicates Packet DMA direction and is used only for devices that implement the Packet Command feature set with a Serial ATA bridge that require direction indication from the host. Support for this bit is determined by reading bit 15 of word 62 in the IDENTIFY PACKET DEVICE data. If bit 15 of word 62 is set to 1, the device requires the use of the DMADIR bit for Packet DMA commands.

If the device requires the DMADIR bit to be set for Packet DMA operations and the current operations is DMA (i.e. bit 0, the DMA bit, is set), this bit indicates the direction of data transfer (0 = transfer to the device; 1 = transfer to the host). If the device requires the DMADIR bit to be set for Packet DMA operations but the current operations is PIO (i.e. bit 0, the DMA bit, is cleared), this bit is ignored.

Since the data transfer direction will be set by the host as the command is constructed, the DMADIR bit should not conflict with the data transfer direction of the command. If a conflict between the command transfer direction and the DMADIR bit occurs, the device should return with an ABORTED command, and the sense key set to ILLEGAL REQUEST.

If the device does not require the DMADIR bit for Packet DMA operations, this bit should be cleared to 0.

A device that does not support the DMADIR feature may abort a command if the DMADIR bit is set to 1.

...

----

Under the "documents 2003" link on http://www.t13.org/ there is also the original Silicon Image proposal:

ATAPI DMA Direction issues Proposal 12/21/03 Hartney
http://www.t13.org/docs2003/e03132r3.pdf

Though the latest rev of ATA/ATAPI7 docs are probably a more up-to-date guide.

I bet your bridge doesn't _require_ DMADIR, therefore it doesn't bother with word 62? </wild guess>

Jeff

Sorry to say,

My op xA1 "IDENTIFY" bytes appear nybble-for-nybble identical, no matter if sampled via the SATA ATAPI of Si 3611CT80 r1.4 or if sampled via the original PATA ATAPI.

I quote:

$ cat /var/log/messages | grep ata_dump_id >sata
$ emacs sata
$ # reboot to disconnect bridge from host and from device
$ sudo cat /proc/ide/hdc/identify >pata
$ diff sata pata
12c12
< 203f 0000 0000 0000 0000 404f 0000 0000
---

$ #88 . 89 . 90 . 91 . 92 . 93 . 94 . 95 are the "word" indices
$

At first glance, you might think you just saw the upper halves of "word"s 88 and 93 change, after calculating 88 = 8 * (line 12 - 1)). But all that's actually changed in "word" 88 is the initially "selected" UDMA "mode". x20XX = UDMA 5, x04XX = UDMA 2. And all that's actually changed in "word" 93 is the CBLDID- detect of mask x2000 i.e. you caught me using a 40-pin PATA cable in place of an 80-pin PATA cable.

Bottom line, all the bits of the "F" set ("F" = "Fixed" = change no more often than discs change) have not changed.

I presume by intent as yet we don't mean to appear among the devices of /proc/ide/. I checked all /proc and all /sys, saw our SATA host appear there voluminously, but nothing that my newbie eye recognised as device data.

Pat LaVarre

P.S. As I wrote the trivial experimental patch below to gather op xA1 "IDENTIFY" data, I was painfully reminded that DPRINTK flushes per invocation, not only at EOL, and that /var/log/messages omits duplicate lines.

diff -Nurp linux-2.6.6-bk4/drivers/scsi/libata-core.c linux-2.6.6-bk4-pel/drivers/scsi/libata-core.c
@@ -974,6 +974,24 @@ static inline void ata_dump_id(struct at
 		"93==0x%04x\n",
 		dev->id[88],
 		dev->id[93]);
+
+	/* all "word"s, as in lk 2.4 `sudo cat /proc/ide/hd$v/identify` */
+	do {
+		int ix;
+		for (ix = 0; ix < 0x100; ix += 8) {
+			DPRINTK("0x%03X: "
+				"%04x %04x %04x %04x %04x %04x %04x %04x\n",
+				ix * 2,
+				dev->id[ix+0],
+				dev->id[ix+1],
+				dev->id[ix+2],
+				dev->id[ix+3],
+				dev->id[ix+4],
+				dev->id[ix+5],
+				dev->id[ix+6],
+				dev->id[ix+7]);
+		}
+	} while (0);
 }
 
 /**

Disappointing...

Jeff

Links! Thank you.

Your courage in aggressively sharing wild guesses, I appreciate, thank you, but in this instance:

My memory of my share of the SATA ATAPI UDMA flurry tells me that my Si 3611CT80 r1.4 bridge hangs with status = xD0 if we ask it to copy DMA Data In, without having prepared op xA0 "PACKET" features = x05 DMA In rather than x01 DMA Out.

Are you not convinced?

What further experiments should I run or repeat or reemphasise?

I agree, by connecting my compliant only-DMADIR-capable bridge to a compliant PATA DMA device I have ended up violating ATA/PI 6 by claiming classic UDMA in op xA1 "IDENTIFY" "word" 88 (line 12 offset 0), when in fact the abomination I built only actually supports the DMADIR that ATA/PI 7 claims will be described in "word" 62 (line 7 offset 6).

I see that as me falling into the pit that t13.org dug for me, while I admittedly wasn't watching closely enough to notice.

I imagine I was not the first and I will not be the last to fall into this spiked pit. Particularly vulnerable will be the people who resell compliant PATA ATAPI UDMA devices and then fall into erroneously believing that adding a compliant SATA/PATA bridge makes a compliant SATA ATAPI UDMA device.

Do you disagree?

Pat LaVarre

$ cat sata
85c0 0000 0000 0000 0000 0000 0000 0000
0000 0000 3139 3130 4343 3944 4345 3442
4646 4145 2020 2020 0000 0000 0000 3734
2e42 2020 2020 496f 6d65 6761 2020 5252
4420 2020 2020 2020 2020 2020 2020 2020
2020 2020 2020 2020 2020 2020 2020 0000
0000 0f00 0000 0200 0000 0006 0000 0000
0000 0000 0000 0000 0000 0000 0000 0007
0003 0078 0078 0078 0078 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0040 0000 4218 4000 4000 4218 0000 4000
203f 0000 0000 0000 0000 404f 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 436f 7079 7269 6768 7420 2863 2920
3230 3034 2049 6f6d 6567 6120 436f 7270
2e20 2041 6c6c 2072 6967 6874 7320 7265
7365 7276 6564 2e00 3032 2f32 372f 3034
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
0000 0000 0000 0000 0000 0000 0000 0000
$

I am convinced that your hardware requires DMADIR.

I am not convinced that there is no way to detect this condition at runtime, based on some hardware characteristic, or output.

Well, the expected behavior is proper implementation of word 62, bit 15 at the very least :)

If hardware requires the DMADIR bit, it should set the feature bit that says "I require DMADIR". If not, one could justifyably claim the hardware is not operating to spec.

<shrug> Honestly, I think the requirement of a data-direction bit was inevitable. I'm surprised you don't have one for ATA devices. Consider: what data direction will a vendor-reserved SCSI opcode require? The OS driver cannot know.

If you accept that premise, then these changes to ATA were required...

Jeff

s/OS driver/bridge/ obviously...

Me too.

I agree.

Shall we try timeout, reset, and retry of the initial DMA op x12 Inquiry?

If that fails for timeout with xD0 status, then we guess oh DMADIR and try again with Features = x05 Dma In, rather than our default of x01 Dma Out/classic?

Yes. We may have a phasing trouble here: the bridge appeared before the final definition of the relevant op xA1 "IDENTIFY" "word"s.

Ouch, ouch, ouch.

I just dug up the fact that mask x0707 of word 62 used to mean single word DMA e.g. in page 54 of 111, ATA-2 of 1996-03-18, d0948r4c.pdf.

t13.org ATA/PI 7 redefining word 62 to mean DMADIR may become a pernicious way of searching out anyone who still thinks SWDMA exists.

I agree, vehemently.

Indeed, we have more trouble coming eventually. If we compare the serial USB CBW to what we see on a serial SATA emulation of a parallel PATA cable, what's missing is:

bCBLength = length of the CDB bmCBWFlags.Direction = Data In/Out dCSWDataResidue = data bytes clocked but not copied

All the missing info causes trouble in one way or another. Also both USB and ATAPI die when we cross the 64-bit lba barrier that takes us past 16 bytes/CDB. Also both may break as aligning data only to 32-bit boundaries rather than 64-bit boundaries starts to matter.

I think you're saying that knowing the SCSI opcode does not decide the data copy direction.

I agree, vehemently.

By definition, knowing the opcode cannot decide the data copy direction of vendor-specific and newly standard commands.

Also in theory, the data copy direction of SCSI opcodes may vary in accord with PDT (= Peripheral Device Type = mask x1F of byte 0 of op x12 "INQUIRY" = x 0E/ 07/ 04/ 00 SBC of HDD/ Flash = x05 MMC of DVD/CD).

Also in practice, some (rudely designed) vendor-specific ops move data both ways.

None of those facts have kept people over the years from writing host software that thinks the op decides the direction. I think I remember seeing Linux-2.2 broken that way.

Pat LaVarre

I dunno. Feel free to play around. :)

Silicon Image is sending me a couple bridges, we'll see how they behave.

SWDMA never existed for ATAPI devices. Word 62 has -always- been reserved, until now.

Yep. Bart, our illustrious IDE maintainer, recently removed one such opcode table.

Jeff

To SFF we owe many perniciously slight binary incompatibilities. Re ops xEC/A1 "IDENTIFY" we have:

----- http://www.google.com/search?q=storage+cornucopia
----- http://www.bswd.com/cornucop.htm
----- SFF-8020i, Rev. 2.6: ATAPI for CD-ROMs (old and antiquated) ----- http://www.bswd.com/sff8020i.pdf

... January 22, 1996 ...

----- page 80 of 224

7.1.7.9 Single Word DMA Transfer (Word 62) The low order byte identifies by bit all of the modes which are supported, e.g., if Mode 0 is supported, bit 0 is set. The high order byte contains a single bit set to indicate which mode is active, e.g., if Word 0 is active, bit8 is set.

-----

Pat LaVarre

Heads up, I have a second sample of hardware now:

kernel: ata_exec_command_pio: ata2: cmd 0xA1
kernel: ata2: dev 0 cfg 49:0e00 82:4218 83:4000 84:4000 85:4218 86:0000 87:4000 88:0000

Its ata_piix.ko op x12 "INQUIRY" doesn't yet work at all, perhaps by way of more accurately simulating what we may see in mass distribution.

You'll remember, the first hardware sample I acquired was a massively-distributed PATA ATAPI DMA drive combined with an Si 3611CT80 r1.4 SATA/ PATA bridge.

My second sample is that same bridge, but now with a different drive behind it, specifically a drive whose firmware purportedly implements the "d1532v1r4b ATA-ATAPI-7.pdf" proposal to substitute:

PATA ATAPI:

id[49] = id[ 7 * 8 - 8 + 1] = x0F00 // DMA id[62] = id[ 8 * 8 - 8 + 6] = x0000 // SWDMA not id[63] = id[ 8 * 8 - 8 + 7] = x0007 // MWDMA 2 1 0 id[88] = id[12 * 8 - 8 + 0] = xXX3F // UDMA 5 4 2 1 0

SATA ATAPI per 2004-04-21 d1532v1r4b:

id[49] = id[ 7 * 8 - 8 + 1] = x0E00
id[62] = id[ 8 * 8 - 8 + 6] = x87FF // ATA-7: DMADIR, ATA-2: SWDMA 2 1 0 id[63] = id[ 8 * 8 - 8 + 7] = x0000
id[88] = id[12 * 8 - 8 + 0] = xXX00

I'll go digging now.

I imagine I'll find I can most concisely make this work by copying into words 49 63 88 the bits of word 62.

Pat LaVarre

Yep.

diff -Nurp linux-2.6.6-bk5/drivers/scsi/libata-core.c linux-2.6.6-bk5-pel/drivers/scsi/libata-core.c
@@ -1076,6 +1076,22 @@ retry:
 
 	ata_irq_on(ap);	/* re-enable interrupts */
 
+	/* see DMADIR as a variation on classic UDMA/ MWDMA */
+
+#ifdef ATAPI_ENABLE_DMADIR
+	printk(KERN_DEBUG "ata%u: dev %u cfg "
+	       "62:%04x\n",
+	       ap->id, device, dev->id[62]);
+	if (dev->id[62] & (1 << 15)) { /* 0x8000 DMADIR */
+		u16 * id = dev->id;
+		if (id[62] & (1 << 10)) { /* 0x0400 DMA */
+			id[49] |= (1 << 8);
+			id[63] |= ((id[62] >> 7) & 0x07); /* 0x0380 MWDMA */
+			id[88] |= ((id[62] >> 0) & 0x3F); /* 0x007F UDMA */
+		}
+	}
+#endif
+
 	/* print device capabilities */
 	printk(KERN_DEBUG "ata%u: dev %u cfg "
 	       "49:%04x 82:%04x 83:%04x 84:%04x 85:%04x 86:%04x 87:%04x 88:%04x\n",

Do you like FIXME? Merging the following would help me remember where to bring this up again after we get more of ATAPI_ENABLE_DMADIR working.

Pat LaVarre

diff -Nurp linux-2.6.6-bk6/drivers/scsi/libata-core.c linux-2.6.6-bk6-pel/drivers/scsi/libata-core.c
@@ -1059,6 +1059,8 @@ retry:
 
 	ata_irq_on(ap);	/* re-enable interrupts */
 
+	/* FIXME: infer UDMA/ MWDMA from new DMADIR dev->id[62] (was SWDMA) */
+
 	/* print device capabilities */
 	printk(KERN_DEBUG "ata%u: dev %u cfg "
 	       "49:%04x 82:%04x 83:%04x 84:%04x 85:%04x 86:%04x 87:%04x 88:%04x\n",

I don't necessarily expect this work, but we'll see...

===== drivers/scsi/libata-core.c 1.64 vs edited =====

@@ -2162,6 +2162,79 @@
 	}
 }
 
+static void atapi_error(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	struct ata_taskfile tf;
+	struct scsi_cmnd *cmd = qc->scsicmd;
+	u8 status;
+	u16 len;
+	const u8 request_sense_cdb[16] = {
+		REQUEST_SENSE, 0, 0, 0, SCSI_SENSE_BUFFERSIZE,
+	};
+
+	ata_tf_init(ap, &tf, qc->dev->devno);
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_ATAPI;
+	tf.ctl |= ATA_NIEN;
+	tf.lbam = SCSI_SENSE_BUFFERSIZE & 0xff;
+	tf.command = ATA_CMD_PACKET;
+	ata_tf_to_host(ap, &tf);
+
+	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB))
+		goto err_out;
+
+	status = ata_chk_status(ap);
+	if ((status & ATA_DRQ) == 0)
+		goto err_out;
+
+	/* FIXME: mmio-ize */
+	DPRINTK("send cdb\n");
+	outsl(ap->ioaddr.data_addr, request_sense_cdb,
+	      ap->host->max_cmd_len / 4);
+
+	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB))
+		goto err_out;
+
+	status = ata_chk_status(ap);
+	if ((status & ATA_DRQ) == 0)
+		goto err_out;
+
+	ap->ops->tf_read(ap, &tf);
+	len = tf.lbam;
+	len |= ((u16)tf.lbah) << 8;
+
+	if (len % 4)
+		printk(KERN_WARNING "ata%u: odd ATAPI length %u\n",
+		       ap->id, len);
+	memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));
+	insl(ap->ioaddr.data_addr, cmd->sense_buffer, len / 4);
+
+	if ((cmd->sense_buffer[0] & 0x7e) != 0x70) {
+		u8 err = ata_chk_err(ap);
+		cmd->sense_buffer[0] = 0xf0;
+		cmd->sense_buffer[2] = err >> 4;
+	}
+
+	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB))
+		goto err_out;
+
+	ata_irq_on(ap);
+
+out:
+	cmd->result = SAM_STAT_CHECK_CONDITION;
+	qc->scsidone(cmd);
+	return;
+
+err_out:
+	ata_irq_on(ap); /* re-enable interrupts */
+	cmd->sense_buffer[0] = 0xf0;
+	cmd->sense_buffer[2] = HARDWARE_ERROR;
+	cmd->sense_buffer[7] = 14 - 8;  /* addnl. sense len. FIXME: correct? */
+	cmd->sense_buffer[12] = 0x8; /* logical unit comm failure */
+	goto out;
+}
+
 /**
  *	ata_eng_timeout - Handle timeout of queued command
  *	@ap: Port on which timed-out command is active

@@ -2185,6 +2258,7 @@
 {
 	u8 host_stat, drv_stat;
 	struct ata_queued_cmd *qc;
+	int check_cond = 0;
 
 	DPRINTK("ENTER\n");
 

@@ -2195,16 +2269,30 @@
 		goto out;
 	}
 
-	/* hack alert!  We cannot use the supplied completion
-	 * function from inside the ->eh_strategy_handler() thread.
-	 * libata is the only user of ->eh_strategy_handler() in
-	 * any kernel, so the default scsi_done() assumes it is
-	 * not being called from the SCSI EH.
-	 */
-	qc->scsidone = scsi_finish_command;
+	if (qc->scsicmd) {
+		check_cond = !(qc->scsicmd->eh_eflags & SCSI_EH_CANCEL_CMD);
+
+		/* hack alert!  We cannot use the supplied completion
+		 * function from inside the ->eh_strategy_handler() thread.
+		 * libata is the only user of ->eh_strategy_handler() in
+		 * any kernel, so the default scsi_done() assumes it is
+		 * not being called from the SCSI EH.
+		 */
+		qc->scsidone = scsi_finish_command;
+	}
+
+	/* ATAPI devices */
+	if (qc->dev->class == ATA_DEV_ATAPI) {
+		if (check_cond) {
+			atapi_error(qc);
+			goto out;
+		}
+	}
 
+	/* ATA devices */
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
+	case ATA_PROT_ATAPI_DMA:
 		if (ap->flags & ATA_FLAG_MMIO) {
 			void *mmio = (void *) ap->ioaddr.bmdma_addr;
 			host_stat = readb(mmio + ATA_DMA_STATUS);

@@ -2217,6 +2305,7 @@
 		ata_dma_complete(qc, host_stat);
 		break;
 
+	case ATA_PROT_ATAPI:
 	case ATA_PROT_NODATA:
 		drv_stat = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
 

@@ -2317,15 +2406,23 @@
 	assert(qc != NULL);	/* ata_qc_from_tag _might_ return NULL */
 	assert(qc->flags & ATA_QCFLAG_ACTIVE);
 
-	if (likely(qc->flags & ATA_QCFLAG_SG))
+	if (likely(qc->flags & ATA_QCFLAG_SG)) {
 		ata_sg_clean(qc);
+		qc->flags &= ~ATA_QCFLAG_SG;
+	}
 
 	if (cmd) {
 		if (unlikely(drv_stat & (ATA_ERR | ATA_BUSY | ATA_DRQ))) {
-			if (is_atapi_taskfile(&qc->tf))
+			if (is_atapi_taskfile(&qc->tf)) {
 				cmd->result = SAM_STAT_CHECK_CONDITION;
-			else
-				ata_to_sense_error(qc);
+				qc->scsidone(cmd);
+				return;
+				/* error handler thread takes
+				 * over from here
+				 */
+			}
+
+			ata_to_sense_error(qc);
 		} else {
 			cmd->result = SAM_STAT_GOOD;
 		}

===== drivers/scsi/libata-scsi.c 1.37 vs edited =====

@@ -137,7 +137,7 @@
 
 	cmd->result = SAM_STAT_CHECK_CONDITION;
 
-	cmd->sense_buffer[0] = 0x70;
+	cmd->sense_buffer[0] = 0xf0;
 	cmd->sense_buffer[2] = MEDIUM_ERROR;
 	cmd->sense_buffer[7] = 14 - 8;	/* addnl. sense len. FIXME: correct? */
 

@@ -927,7 +927,7 @@
 	DPRINTK("ENTER\n");
 	cmd->result = SAM_STAT_CHECK_CONDITION;
 
-	cmd->sense_buffer[0] = 0x70;
+	cmd->sense_buffer[0] = 0xf0;
 	cmd->sense_buffer[2] = ILLEGAL_REQUEST;
 	cmd->sense_buffer[7] = 14 - 8;	/* addnl. sense len. FIXME: correct? */
 	cmd->sense_buffer[12] = asc;

===== drivers/scsi/libata.h 1.15 vs edited =====

@@ -28,6 +28,10 @@
 #define DRV_NAME	"libata"
 #define DRV_VERSION	"1.02"	/* must be exactly four chars */
 
+#ifndef SCSI_EH_CANCEL_CMD
+#define SCSI_EH_CANCEL_CMD	0x0001	/* Cancel this cmd */
+#endif /* SCSI_EH_CANCEL_CMD */
+
 struct ata_scsi_args {
 	struct ata_port		*ap;
 	struct ata_device	*dev;

Thanks I'll try -bk6 with my first device i.e.
my mass-produced PATA ATAPI DMA plus my mass-produced SATA adapter.

<shrug>

I don't remember anymore :)

Jeff

Many thanks again for restarting me. Looks to me like we're now trying to manually PIO an op x03 "REQUEST SENSE", analogous to how we manually make PIO op xA1 "IDENTIFY" work. I tried your patch as quoted below.

Consequently, I lost my display & mouse & keyboard. I'll repeat that test and report results. A significant variable could be how I provoke auto sense. I first tried "TEST UNIT READY":

modprobe ata-piix
plscsi /dev/sg0 -X time 5 0 -i 8 -x "00 00:00:00 00 00"

I'll next try "READ CAPACITY":

modprobe ata-piix
plscsi /dev/sg0 -X time 5 0 -i 8 -x "25 00 00:00:00:00 00 00:00 00"

And maybe I should try ATA_FORCE_PIO.

Pat LaVarre

$ cd linux-2.6.6-bk6
$ patch -p1 <~/patch.as1
patching file drivers/scsi/libata-core.c
Hunk #1 succeeded at 2144 (offset -18 lines).
Hunk #3 succeeded at 2251 (offset -18 lines).
Hunk #5 succeeded at 2388 (offset -18 lines).
patching file drivers/scsi/libata-scsi.c
Hunk #2 succeeded at 926 (offset -1 lines).
patching file drivers/scsi/libata.h
$

diff -Nurp linux-2.6.6-bk6/drivers/scsi/libata-core.c linux-2.6.6-bk6-pel/drivers/scsi/libata-core.c
@@ -2144,6 +2144,79 @@ static void ata_pio_task(void *_data)
 	}
 }
 
+static void atapi_error(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	struct ata_taskfile tf;
+	struct scsi_cmnd *cmd = qc->scsicmd;
+	u8 status;
+	u16 len;
+	const u8 request_sense_cdb[16] = {
+		REQUEST_SENSE, 0, 0, 0, SCSI_SENSE_BUFFERSIZE,
+	};
+
+	ata_tf_init(ap, &tf, qc->dev->devno);
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_ATAPI;
+	tf.ctl |= ATA_NIEN;
+	tf.lbam = SCSI_SENSE_BUFFERSIZE & 0xff;
+	tf.command = ATA_CMD_PACKET;
+	ata_tf_to_host(ap, &tf);
+
+	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB))
+		goto err_out;
+
+	status = ata_chk_status(ap);
+	if ((status & ATA_DRQ) == 0)
+		goto err_out;
+
+	/* FIXME: mmio-ize */
+	DPRINTK("send cdb\n");
+	outsl(ap->ioaddr.data_addr, request_sense_cdb,
+	      ap->host->max_cmd_len / 4);
+
+	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB))
+		goto err_out;
+
+	status = ata_chk_status(ap);
+	if ((status & ATA_DRQ) == 0)
+		goto err_out;
+
+	ap->ops->tf_read(ap, &tf);
+	len = tf.lbam;
+	len |= ((u16)tf.lbah) << 8;
+
+	if (len % 4)
+		printk(KERN_WARNING "ata%u: odd ATAPI length %u\n",
+		       ap->id, len);
+	memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));
+	insl(ap->ioaddr.data_addr, cmd->sense_buffer, len / 4);
+
+	if ((cmd->sense_buffer[0] & 0x7e) != 0x70) {
+		u8 err = ata_chk_err(ap);
+		cmd->sense_buffer[0] = 0xf0;
+		cmd->sense_buffer[2] = err >> 4;
+	}
+
+	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB))
+		goto err_out;
+
+	ata_irq_on(ap);
+
+out:
+	cmd->result = SAM_STAT_CHECK_CONDITION;
+	qc->scsidone(cmd);
+	return;
+
+err_out:
+	ata_irq_on(ap); /* re-enable interrupts */
+	cmd->sense_buffer[0] = 0xf0;
+	cmd->sense_buffer[2] = HARDWARE_ERROR;
+	cmd->sense_buffer[7] = 14 - 8;  /* addnl. sense len. FIXME: correct? */
+	cmd->sense_buffer[12] = 0x8; /* logical unit comm failure */
+	goto out;
+}
+
 /**
  *	ata_eng_timeout - Handle timeout of queued command
  *	@ap: Port on which timed-out command is active

@@ -2167,6 +2240,7 @@ void ata_eng_timeout(struct ata_port *ap
 {
 	u8 host_stat, drv_stat;
 	struct ata_queued_cmd *qc;
+	int check_cond = 0;
 
 	DPRINTK("ENTER\n");
 

@@ -2177,16 +2251,30 @@ void ata_eng_timeout(struct ata_port *ap
 		goto out;
 	}
 
-	/* hack alert!  We cannot use the supplied completion
-	 * function from inside the ->eh_strategy_handler() thread.
-	 * libata is the only user of ->eh_strategy_handler() in
-	 * any kernel, so the default scsi_done() assumes it is
-	 * not being called from the SCSI EH.
-	 */
-	qc->scsidone = scsi_finish_command;
+	if (qc->scsicmd) {
+		check_cond = !(qc->scsicmd->eh_eflags & SCSI_EH_CANCEL_CMD);
+
+		/* hack alert!  We cannot use the supplied completion
+		 * function from inside the ->eh_strategy_handler() thread.
+		 * libata is the only user of ->eh_strategy_handler() in
+		 * any kernel, so the default scsi_done() assumes it is
+		 * not being called from the SCSI EH.
+		 */
+		qc->scsidone = scsi_finish_command;
+	}
+
+	/* ATAPI devices */
+	if (qc->dev->class == ATA_DEV_ATAPI) {
+		if (check_cond) {
+			atapi_error(qc);
+			goto out;
+		}
+	}
 
+	/* ATA devices */
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
+	case ATA_PROT_ATAPI_DMA:
 		if (ap->flags & ATA_FLAG_MMIO) {
 			void *mmio = (void *) ap->ioaddr.bmdma_addr;
 			host_stat = readb(mmio + ATA_DMA_STATUS);

@@ -2199,6 +2287,7 @@ void ata_eng_timeout(struct ata_port *ap
 		ata_dma_complete(qc, host_stat);
 		break;
 
+	case ATA_PROT_ATAPI:
 	case ATA_PROT_NODATA:
 		drv_stat = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
 

@@ -2299,15 +2388,23 @@ void ata_qc_complete(struct ata_queued_c
 	assert(qc != NULL);	/* ata_qc_from_tag _might_ return NULL */
 	assert(qc->flags & ATA_QCFLAG_ACTIVE);
 
-	if (likely(qc->flags & ATA_QCFLAG_SG))
+	if (likely(qc->flags & ATA_QCFLAG_SG)) {
 		ata_sg_clean(qc);
+		qc->flags &= ~ATA_QCFLAG_SG;
+	}
 
 	if (cmd) {
 		if (unlikely(drv_stat & (ATA_ERR | ATA_BUSY | ATA_DRQ))) {
-			if (is_atapi_taskfile(&qc->tf))
+			if (is_atapi_taskfile(&qc->tf)) {
 				cmd->result = SAM_STAT_CHECK_CONDITION;
-			else
-				ata_to_sense_error(qc);
+				qc->scsidone(cmd);
+				return;
+				/* error handler thread takes
+				 * over from here
+				 */
+			}
+
+			ata_to_sense_error(qc);
 		} else {
 			cmd->result = SAM_STAT_GOOD;
 		}

diff -Nurp linux-2.6.6-bk6/drivers/scsi/libata-scsi.c linux-2.6.6-bk6-pel/drivers/scsi/libata-scsi.c
@@ -137,7 +137,7 @@ void ata_to_sense_error(struct ata_queue
 
 	cmd->result = SAM_STAT_CHECK_CONDITION;
 
-	cmd->sense_buffer[0] = 0x70;
+	cmd->sense_buffer[0] = 0xf0;
 	cmd->sense_buffer[2] = MEDIUM_ERROR;
 	cmd->sense_buffer[7] = 14 - 8;	/* addnl. sense len. FIXME: correct? */
 

@@ -926,7 +926,7 @@ void ata_scsi_badcmd(struct scsi_cmnd *c
 	DPRINTK("ENTER\n");
 	cmd->result = SAM_STAT_CHECK_CONDITION;
 
-	cmd->sense_buffer[0] = 0x70;
+	cmd->sense_buffer[0] = 0xf0;
 	cmd->sense_buffer[2] = ILLEGAL_REQUEST;
 	cmd->sense_buffer[7] = 14 - 8;	/* addnl. sense len. FIXME: correct? */
 	cmd->sense_buffer[12] = asc;

And /var/log/messages caught nothing:

kernel: ata_device_add: EXIT, returning 2
kernel: piix_init: done
syslogd 1.4.1: restart.
syslog: syslogd startup succeeded

This actually almost does work. Once via ssh I saw:

$ sudo modprobe ata-piix
$ plscsi /dev/sg0 -X time 5 0 -i 8 -x "25 00 00:00:00:00 00 00:00 00"
// x 6 29 sense
// -x0002 = -2 = plscsi.main exit int
$ plscsi /dev/sg0 -X time 5 0 -i 8 -x "25 00 00:00:00:00 00 00:00 00"
...

Argh I should have said -v to plscsi. Meanwhile, without complete proof, I believe that "// x 6 29 sense" line from plscsi means an auto sense once ran well enough to produce sense byte 0 = x70, a sane additional length at offset 7, and SK ASC ASCQ = x 6 29 00 i.e. auto sense once mostly worked.

But then the second try of that same command dumped an indefinitely long series of what by eye and hand I transcribe as:

ata_scsi_translate: ENTER
ata_scsi_dump_cdb: CDB(2:0,0,0) 25 00 00 00 00 00 00 00 00

Sorry to say now real life intrudes, I'll report again later/ tomorrow.

Pat LaVarre

Yes we can auto sense at least once per boot.

The extra ata_scsi_translate dmesg comes from a trivial recursion trap I added there, this time around. The 1 is the depth of recursion, the x25 is the opcode given as a parameter.

$ plscsi /dev/sg0 -X time 5 0 -i 8 -x "25 00 00:00:00:00 00 00:00 00" -vx 00000000 25 00 00:00:00:00 00 00:00 00 .. .. .. .. .. .. "%@@@@@@@@@"
x 00000000 00:00:00:00 00:00:00:00 .. .. .. .. .. .. .. .. "@@@@@@@@"
x 00000000 70:00:06:00 00:00:00:18 00:00:00:00 29:00 .. .. "p@F@@@@X@@@@)@"
// x 6 29 sense
// -x0002 = -2 = plscsi.main exit int
$

kernel: piix_init: done
kernel: Attached scsi generic sg0 at scsi1, channel 0, id 0, lun 0, type 5
kernel: ata_scsi_dump_cdb: CDB (2:0,0,0) 25 00 00 00 00 00 00 00 00
kernel: ata_scsi_translate: ENTER 1 0x25
kernel: ata_scsi_translate: ENTER
kernel: ata_sg_setup_one: mapped buffer of 8 bytes for read
kernel: ata_fill_sg: PRD[0] = (0xD440000, 0x8)
kernel: ata_dev_select: ENTER, ata2: device 0, wait 1
kernel: ata_tf_load_pio: feat 0x5 nsect 0x0 lba 0x0 0x0 0x0
kernel: ata_tf_load_pio: device 0xA0
kernel: ata_exec_command_pio: ata2: cmd 0xA0
kernel: ata_scsi_translate: EXIT
kernel: atapi_packet_task: busy wait
kernel: atapi_packet_task: send cdb
/sbin/hotplug: no runnable /etc/hotplug/scsi_generic.agent is installed
kernel: ata_host_intr: BUS_DMA (host_stat 0x25)
kernel: ata_dma_complete: ENTER
kernel: ata_dma_complete: host 2, host_stat==0x25, drv_stat==0x51
kernel: ata_sg_clean: unmapping 1 sg elements
kernel: ata_scsi_error: ENTER
kernel: ata_eng_timeout: ENTER
kernel: ata_tf_load_pio: feat 0x0 nsect 0x0 lba 0x0 0x60 0x0
kernel: ata_tf_load_pio: device 0xA0
kernel: ata_exec: ata2: cmd 0xA0
kernel: ata_exec_command_pio: ata2: cmd 0xA0
kernel: atapi_error: send cdb
kernel: ata_eng_timeout: EXIT
kernel: ata_scsi_error: EXIT

Pat LaVarre

I meant to say reentry trap.

(I'm too much of a newbie to know how to distinguish reentry from recursion in Linux.)

Anyhow, yes indeed, ata_scsi_translate re-entered itself at least 4551 times before my Ctrl+C and reboot took effect, when provoked by a standard "INQUIRY" after auto sense, specifically:

plscsi /dev/sg0 -X time 5 0 -v

The only -bk6 dmesg I get are an indefinitely long series of:

ata_scsi_dump_cdb: CDB (2:0,0,0) 12 00 00 00 24 00 00 00 00
kernel: ata_scsi_translate: ENTER

Pat LaVarre

printk tells me reentry occurs after ata_scsi_translate calls ata_scsi_qc_new, before ata_scsi_qc_new returns.

dump_stack tells me we're looking at indefinite serial reentry, not recursion.

Reworking ata_eng_timeout to call ata_dma_complete or ata_qc_complete even after atapi_error, same as when we need no auto sense, doesn't stop the indefinite reentry but does let me halt the indefinite reentry by counting reentries and then past some limit returning from ata_scsi_translate before calling ata_scsi_qc_new.

By contrast, with ata_eng_timeout as posted, that kind of reentry trap does not halt the reentry.

Pat LaVarre

Thanks for continuing to poke at this. FYI, I'm saving your results, but probably won't have time to "think" about ATAPI again for a week or so. Need to spend some time getting new SATA controllers working.

Keep posting, though, I'm still listening.

Thanks,

Jeff

Jeff G:

Thank you for this explicit encouragement. Knowing that you have heard me without speaking tells me my guesses at least appear sane at first glance. I deeply enjoy the learning, I only hesitate over how much of it to broadcast here. I can switch over to a less prominent blog whenever you like. I think next:

I will begin again in 2.6.6-bk8 without the auto sense patch of this thread.

Rather than branching on when auto sense is appropriate, I will try the simpler, intermediate step, of rudely adding an unsolicited auto sense to every command, by way of adding fragments of your auto sense patch.

That should let me see which fragment (or combination of fragments) first causes trouble.

I happen to know I have a robust PATA device: I believe often enough it will survive such abuses as unsolicited sense, reading x1F0 Data while BSY:DRQ != 0:1, etc.

Pat LaVarre

Keep posting... seconded.

I'm also reading this and linux-ide needs more traffic/people. ;-)

Cheers.

Hi!

Below inline is a trivial -bk8 patch that ducks ever trying PATAPI auto sense.

In a kernel shipping with ATA_ENABLE_ATAPI defined, this would be a very bad thing, because we thus falsely report success when in fact we have not copied read data in or write data out.

But at my desk I find this alternative more usable than vanilla 2.6.6-bk8, because this way I don't have to issue unsolicited op x03 "REQUEST SENSE" to clear out the always-initially-present unit attentions. Instead, I just ignore the first few garbage results I get.

For example:

$ plscsi /dev/sg0 -X time 5 0 -i 8 -x "25 00 00:00:00:00 00 00:00 00" -v
x 00000000 25 00 00:00:00:00 00 00:00 00 .. .. .. .. .. .. "%@@@@@@@@@"
x 00000000 00:00:00:00 00:00:00:00 .. .. .. .. .. .. .. .. "@@@@@@@@"
// 0 = plscsi.main exit int
$
$ plscsi /dev/sg0 -X time 5 0 -i 8 -x "25 00 00:00:00:00 00 00:00 00" -v
x 00000000 25 00 00:00:00:00 00 00:00 00 .. .. .. .. .. .. "%@@@@@@@@@"
x 00000000 01:04:C9:3F 00:00:08:00 .. .. .. .. .. .. .. .. "ADI?@@H@"
// 0 = plscsi.main exit int
$

See that? The first result is zeroed nonsense, courtesy the x 6 29 Reset unit attention we know was present. Only the second result is actually meaningful.

I can now proceed to test how well our DMA no-data/ data-in/ data-out protocols work apart from timeout, auto sense, and the rest of error recovery.

After that, I can return to inserting a PIO unsolicited op x03 "REQUEST SENSE" after every command, and then beyond that I can make the auto sense occur no more often than it should.

Pat LaVarre

diff -Nurp linux-2.6.6-bk8/drivers/scsi/libata-core.c linux-2.6.6-bk8-pel/drivers/scsi/libata-core.c
@@ -2303,6 +2303,9 @@ void ata_qc_complete(struct ata_queued_c
 		ata_sg_clean(qc);
 
 	if (cmd) {
+#if 1 /* pretend good */ 
+		cmd->result = SAM_STAT_GOOD;
+#else /* FIXME: admit lost write data, stale read data, etc. */
 		if (unlikely(drv_stat & (ATA_ERR | ATA_BUSY | ATA_DRQ))) {
 			if (is_atapi_taskfile(&qc->tf))
 				cmd->result = SAM_STAT_CHECK_CONDITION;

@@ -2311,6 +2314,7 @@ void ata_qc_complete(struct ata_queued_c
 		} else {
 			cmd->result = SAM_STAT_GOOD;
 		}
+#endif
 
 		qc->scsidone(cmd);
 	}

diff -Nurp linux-2.6.6-bk8/drivers/scsi/libata-scsi.c linux-2.6.6-bk8-pel/drivers/scsi/libata-scsi.c

Without auto sense, we read and write ok ...

But when I try a no-data op x00 "TEST UNIT READY", I get:

kernel: ata_scsi_dump_cdb: CDB (2:0,0,0) 00 00 00 00 00 00 47 c0 01
kernel: ata_scsi_translate: ENTER
kernel: ata_dev_select: ENTER, ata2: device 0, wait 1
kernel: ata_tf_load_pio: feat 0x0 nsect 0x0 lba 0x0 0x0 0x0
kernel: ata_tf_load_pio: device 0xA0
kernel: ata_exec_command_pio: ata2: cmd 0xA0
kernel: ata_scsi_translate: EXIT
kernel: atapi_packet_task: busy wait
kernel: atapi_packet_task: send cdb
kernel: ata_host_intr: BUS_NODATA (drv_stat 0x50)
kernel: irq 18: nobody cared!
kernel:  [<c01081d7>] __report_bad_irq+0x2a/0x8b
kernel:  [<c01082c1>] note_interrupt+0x6f/0x9f
kernel:  [<c01085f5>] do_IRQ+0x175/0x1a6
kernel:  [<c0103c9e>] default_idle+0x0/0x2d
kernel:  [<c01067b8>] common_interrupt+0x18/0x20
kernel:  [<c0103c9e>] default_idle+0x0/0x2d
kernel:  [<c0103cc8>] default_idle+0x2a/0x2d
kernel:  [<c0103d3c>] cpu_idle+0x37/0x40
kernel:  [<c011de99>] printk+0x198/0x1f1

kernel:
kernel: handlers:
kernel: [<df9986a8>] (ata_interrupt+0x0/0x1b9 [libata])
kernel: Disabling IRQ #18

Pat LaVarre

Well, it's a no-op on the ATA side, but not on the ATAPI side.

Thanks for testing though, I'll think about this one a bit. I'm surprised the interrupt wasn't handled correctly.

Jeff

For the record, ...

As of linux-2.6.6-bk8 plus my bk8.good.always.patch, ...

Commands that repeatably often DO complete rapidly include the following. Looks like we die only in Do < Ho (positive residue out) and Dn = Hn (thin diagonal no data).

Anybody who can make time to learn USB jargon could start with the Glossary at the end, the rest of you can delete the comments or indeed all of this post and read only the jargon-free discussions of specific trouble which follow.

Enjoy, Pat LaVarre

# Setup

export PLSCSI='/dev/sg0 -X time 5 0'

# Di == Hi // thin diagonal in

plscsi -i x24 -x "12 00:00:00 24 00" // "INQUIRY" plscsi -i x800 -x "28 00 00:00:00:10 00 00:01 00" // "READ" plscsi -i x20000 -x "28 00 00:00:00:00 00 00:40 00"

# Dn < Hi // max positive residue in

plscsi -i x24 -x "12 00:00:00 00 00"
plscsi -i x24 -x "00 00:00:00 00 00" // "TEST UNIT READY" plscsi -i x800 -x "28 00 00:00:00:10 00 00:00 00" plscsi -i x20000 -x "28 00 00:00:00:00 00 00:00 00"

# Di < Hi // positive residue in

plscsi -i xC0 -x "12 00:00:00 24 00" plscsi -i x1000 -x "28 00 00:00:00:10 00 00:01 00"

# Do == Ho // thin diagonal out

plscsi -o x24 -x "3B 02 00:00:00:00 00 00:24 00" // optional "WRITE BUFFER" plscsi -o x800 -x "2A 00 00:00:00:10 00 00:01 00" // "WRITE" plscsi -o x20000 -x "2A 00 00:00:00:00 00 00:40 00"

# Dn < Ho // max positive residue out

plscsi -o x24 -x "00 00:00:00 00 00"
plscsi -o x800 -x "2A 00 00:00:00:10 00 00:00 00" plscsi -o x20000 -x "2A 00 00:00:00:00 00 00:00 00"

# Glossary:

"The thirteen cases" are shown in the table:

http://images.google.com/images?q=Pat+LaVarre
http://members.aol.com/plscsi/20000125/13cases.gif

"The thirteen cases" are the thirteen commonly distinct combinations of:

Hn is an abbreviation for saying the Host expects to copy No data. Hi is the nonzero count of data bytes the Host expects to copy In. Ho is the nonzero count of data bytes the Host expects to copy Out.

Dn is an abbreviation for saying the Device actually agrees to copy No data. Di is the nonzero count of data bytes the Device actually agrees to copy In. Do is the nonzero count of data bytes the Device actually agrees to copy Out.

In linux-2.6.6-bk8 plus my bk8.good.always.patch,

One way to hang ata-piix.ko is ...

... for the device to refuse to copy the last bytes of a write.

In the example here, we tell the host to expect to copy out x1000 = 4 Ki bytes, but we tell the device to agree to copy out only x0001 blocks i.e. x800 = 2 Ki bytes.

The "ata2: unknown timeout, cmd 0xa0 stat 0xd0" message suggest we didn't end clean, but to confirm that appearance we follow up with a standard op x12 "INQUIRY" that indeed does not complete at all.

Pat LaVarre

P.S. This example I logged more carefully, but I first stumbled into this with stimulus more like:

plscsi -o xC0 -x "3B 02 00:00:00:00 00 00:24 00" // "WRITE BUFFER" // Do < Ho

// Stimulus:

export PLSCSI='/dev/sg0 -X time 5 0' plscsi -o x1000 -x "2A 00 00:00:00:10 00 00:01 00" plscsi -i x24 -x "12 00:00:00 24 00" Ctrl+C
sudo modprobe -r ata-piix
# FATAL: Module ata_piix is in use.
reboot

// Result:

kernel: ata_scsi_dump_cdb: CDB (2:0,0,0) 2a 00 00 00 00 10 00 00 01
kernel: ata_scsi_translate: ENTER
kernel: atapi_xlat: direction: write
kernel: ata_sg_setup_one: mapped buffer of 4096 bytes for write
kernel: ata_fill_sg: PRD[0] = (0x133B8000, 0x1000)
kernel: ata_dev_select: ENTER, ata2: device 0, wait 1
kernel: ata_tf_load_pio: feat 0x1 nsect 0x0 lba 0x0 0x0 0x0
kernel: ata_tf_load_pio: device 0xA0
kernel: ata_exec_command_pio: ata2: cmd 0xA0
kernel: ata_scsi_translate: EXIT
kernel: atapi_packet_task: busy wait
kernel: atapi_packet_task: send cdb
kernel: ata_scsi_error: ENTER
kernel: ata_eng_timeout: ENTER
kernel: ata2: unknown timeout, cmd 0xa0 stat 0xd0
kernel: ata_sg_clean: unmapping 1 sg elements
kernel: ata_eng_timeout: EXIT
kernel: ata_scsi_error: EXIT

kernel: ata_scsi_dump_cdb: CDB (2:0,0,0) 12 00 00 00 24 00 00 00 00
kernel: ata_scsi_translate: ENTER
kernel: ata_sg_setup_one: mapped buffer of 36 bytes for read
kernel: ata_fill_sg: PRD[0] = (0x133B8000, 0x24)
kernel: ata_dev_select: ENTER, ata2: device 0, wait 1
kernel: ATA: abnormal status 0xD0 on port 0xE007
kernel: ATA: abnormal status 0xD0 on port 0xE007
kernel: ata_tf_load_pio: feat 0x5 nsect 0x0 lba 0x0 0x0 0x0
kernel: ata_tf_load_pio: device 0xA0
kernel: ATA: abnormal status 0xD0 on port 0xE007
kernel: ata_exec_command_pio: ata2: cmd 0xA0
kernel: ata_scsi_translate: EXIT
kernel: atapi_packet_task: busy wait
kernel: ata2 is slow to respond, please be patient
kernel: ata2 failed to respond (30 secs)
kernel: ata_sg_clean: unmapping 1 sg elements

That's considered a "don't do that" condition :)

When the kernel generates an IO request, it always generates correctly-formed CDBs. Therefore, the only time this condition will occur is if a priveleged user intentionally generates an IO request that will kill the hardware.

It is not our intention to load down the kernel with all sorts of checks and balances, attempting to prevent certain types of priveleged-user insanity :) As an example, it is required to provide the data phase (pio-in, pio-out, dma-in, dma-out, etc.) for libata's taskfile interface, as it is for IDE's.

If the sysadmin supplies a PIO-in data phase for the WRITE DMA QUEUED command, the sysadmin will most likely kill the driver, or the hardware, or both.

The sysadmin can also use /dev/mem to randomly modify bits of kernel memory, or read/write to any IO location. They have plenty of power to kill the machine :)

Jeff

Is that not an accurate simulation of the real world case of a write error?

Pat LaVarre

Jeff G:

Thank you for making the time to question my newbie sanity: I do learn by having people find time to tell me what sounds reasonable or not.

Here now I can confirm,

In linux-2.6.6-bk8 plus my bk8.good.always.patch ...

One way to hang ata-piix.ko is ...

... to expect to copy zero bytes and have the device agree.

For example:

export PLSCSI='/dev/sg0 -X time 5 0'
plscsi -i x12 -x "03 00:00:00 12 00"
plscsi -x "00 00:00:00 00 00"

I then see "Disabling IRQ #18" happen even without my bk8.good.always.patch, by way of open /dev/scd0.

With /dev/sg0, the command completes, and modprobe -r works, but if I try modprobe again without reboot, then that hangs after again broadcasting the complaint: Disabling IRQ #18.

As already you saw in fragments, if I clear out the unit attentions with unsolicited request sense or with my bk8.good.always.patch, then I can even die despite having seen no error:

kernel: ata_host_intr: BUS_NODATA (drv_stat 0x50)
kernel: irq 18: nobody cared!

Pat LaVarre

Can you print out the BMDMA status register (often called 'host_stat' in the code)? I wonder if that indicates ATA_DMA_INTR for example.

Jeff

Thanks for asking.

I see ending host_stat = (x20 | ATA_DMA_INTR) same for ATA_PROT_ATAPI as for ATA_PROT_ATAPI_DMA. (Next I'll go look to see how hard ATA_PROT_ATAPI_DMA works to clear that.)

Specifically I see:

kernel: atapi_packet_task: busy wait
kernel: atapi_packet_task: send cdb
kernel: ata_host_intr: (host_stat 0x24)
kernel: ata_host_intr: BUS_NODATA (drv_stat 0x50)
kernel: irq 18: nobody cared!

when I apply the following patch (plus the usual #define's) and then retry the no data case.

Pat LaVarre

diff -Nurp linux-2.6.6-bk8/drivers/scsi/libata-core.c linux-2.6.6-bk8-pel/drivers/scsi/libata-core.c
@@ -2303,6 +2303,9 @@ void ata_qc_complete(struct ata_queued_c
 		ata_sg_clean(qc);
 
 	if (cmd) {
+#if 1 /* pretend good */ 
+		cmd->result = SAM_STAT_GOOD;
+#else /* FIXME: admit lost write data, stale read data, etc. */
 		if (unlikely(drv_stat & (ATA_ERR | ATA_BUSY | ATA_DRQ))) {
 			if (is_atapi_taskfile(&qc->tf))
 				cmd->result = SAM_STAT_CHECK_CONDITION;

@@ -2311,6 +2314,7 @@ void ata_qc_complete(struct ata_queued_c
 		} else {
 			cmd->result = SAM_STAT_GOOD;
 		}
+#endif
 
 		qc->scsidone(cmd);
 	}

@@ -2616,16 +2620,18 @@ inline unsigned int ata_host_intr (struc
 	u8 status, host_stat;
 	unsigned int handled = 0;
 
-	switch (qc->tf.protocol) {
-
-	/* BMDMA completion */
-	case ATA_PROT_DMA:
-	case ATA_PROT_ATAPI_DMA:
 		if (ap->flags & ATA_FLAG_MMIO) {
 			void *mmio = (void *) ap->ioaddr.bmdma_addr;
 			host_stat = readb(mmio + ATA_DMA_STATUS);
 		} else
 			host_stat = inb(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
+		VPRINTK("(host_stat 0x%X)\n", host_stat);
+
+	switch (qc->tf.protocol) {
+
+	/* BMDMA completion */
+	case ATA_PROT_DMA:
+	case ATA_PROT_ATAPI_DMA:
 		VPRINTK("BUS_DMA (host_stat 0x%X)\n", host_stat);
 
 		if (!(host_stat & ATA_DMA_INTR)) {

diff -Nurp linux-2.6.6-bk8/drivers/scsi/libata-scsi.c linux-2.6.6-bk8-pel/drivers/scsi/libata-scsi.c

Further munging of the code shows that ATA_PROT_ATAPI_DMA ordinarily ends by clearing x04 ATA_DMA_INTR.

I'll next try clearing x04 ATA_DMA_INTR for the no-data case too.

Pat LaVarre

Jeff G:

I recommend we apply the following patch, so that non-data commands no longer provoke "irq" "nobody cared!".

Open-ioctl-close of /dev/scd$n now mostly works.

(Read/ write still has issues I will report separately.)

Pat LaVarre

diff -Nurp linux-2.6.6-bk8/drivers/scsi/libata-core.c linux-2.6.6-bk8-pel/drivers/scsi/libata-core.c
@@ -2616,16 +2616,17 @@ inline unsigned int ata_host_intr (struc
 	u8 status, host_stat;
 	unsigned int handled = 0;
 
+	if (ap->flags & ATA_FLAG_MMIO) {
+		void *mmio = (void *) ap->ioaddr.bmdma_addr;
+		host_stat = readb(mmio + ATA_DMA_STATUS);
+	} else
+		host_stat = inb(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
+
 	switch (qc->tf.protocol) {
 
 	/* BMDMA completion */
 	case ATA_PROT_DMA:
 	case ATA_PROT_ATAPI_DMA:
-		if (ap->flags & ATA_FLAG_MMIO) {
-			void *mmio = (void *) ap->ioaddr.bmdma_addr;
-			host_stat = readb(mmio + ATA_DMA_STATUS);
-		} else
-			host_stat = inb(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
 		VPRINTK("BUS_DMA (host_stat 0x%X)\n", host_stat);
 
 		if (!(host_stat & ATA_DMA_INTR)) {

@@ -2648,8 +2649,9 @@ inline unsigned int ata_host_intr (struc
 	case ATA_PROT_ATAPI:
 	case ATA_PROT_NODATA:
 		status = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
-		DPRINTK("BUS_NODATA (drv_stat 0x%X)\n", status);
-		ata_qc_complete(qc, status);
+		DPRINTK("BUS_NODATA (drv_stat 0x%X host_stat 0x%X)\n",
+			status, host_stat);
+		ata_dma_complete(qc, host_stat); /* not ata_qc_complete */
 		handled = 1;
 		break;
 

diff -Nurp linux-2.6.6-bk8/drivers/scsi/libata-scsi.c linux-2.6.6-bk8-pel/drivers/scsi/libata-scsi.c

Jeff G:

Ah, never mind there for now. Digging a little, I see plug 'n play (e.g. the determination of DVD/ CD disc rewritable or not) may go weird while yet we neglect to auto sense. Moving on ...

To my newbie surprise, I have more good news. Teaching my -bk8 kernel toclear x04 ATA_DMA_INTR, means my kernel no longer needs my lab hack of the bk8.good.always.patch.

Failure is ok now, though not yet autosensed.

Sorry to say, failure is ok only if I do NOT apply your autosense patch. Still here applying that provokes an indefinite series of /var/log/messages that begin, for example

kernel: ata_sg_clean: unmapping 1 sg elements
kernel:   Vendor: Iomega    Model: RRD               Rev: 74.B
kernel:   Type:   CD-ROM                             ANSI SCSI revision: 00
kernel: ata_scsi_dump_cdb: CDB (2:0,0,0) 00 00 00 00 00 00 00 00 00
kernel: ata_scsi_translate: ENTER
kernel: ata_dev_select: ENTER, ata2: device 0, wait 1
kernel: ata_tf_load_pio: feat 0x0 nsect 0x0 lba 0x0 0x0 0x0
kernel: ata_tf_load_pio: device 0xA0
kernel: ata_exec_command_pio: ata2: cmd 0xA0
kernel: ata_scsi_translate: EXIT
kernel: atapi_packet_task: busy wait
kernel: atapi_packet_task: send cdb
kernel: ata_host_intr: BUS_NODATA (drv_stat 0x51 host_stat 0x24)
kernel: ata_dma_complete: ENTER
kernel: ata_dma_complete: host 2, host_stat==0x24, drv_stat==0x51
kernel: ata_scsi_error: ENTER
kernel: ata_eng_timeout: ENTER
kernel: ata_tf_load_pio: feat 0x0 nsect 0x0 lba 0x0 0x60 0x0
kernel: ata_tf_load_pio: device 0xA0
kernel: ata_exec: ata2: cmd 0xA0
kernel: ata_exec_command_pio: ata2: cmd 0xA0
kernel: atapi_error: send cdb
kernel: ata_eng_timeout: EXIT
kernel: ata_scsi_error: EXIT
kernel: ata_scsi_dump_cdb: CDB (2:0,0,0) 00 00 00 00 00 00 00 00 00
kernel: ata_scsi_translate: ENTER
kernel: ata_scsi_dump_cdb: CDB (2:0,0,0) 00 00 00 00 00 00 00 00 00
kernel: ata_scsi_translate: ENTER
kernel: ata_scsi_dump_cdb: CDB (2:0,0,0) 00 00 00 00 00 00 00 00 00
kernel: ata_scsi_translate: ENTER

...

I think a significant variable may be whether I have other /dev/scd* devices present and various modules more or less autoloaded. I don't imagine those variables change whether auto sense works or not, but I do think they change whether Linux itself catches a unit attention or whether I have to ask to capture a unit attention.

Real life intrudes, bye for now, I'll post again when next I make progress and/or feel stuck.

Pat LaVarre

Oi, I cannot repeat that result.

Sorry. I therefore suspect back then I lost control of my version.

I find now that I have to clear unit attentions via the abuse of unsolicited sense else { cmd->result = SAM_STAT_CHECK_CONDITION; } occurs, which in turn makes my open/ ioctl SG_IO/ close hang, which in turn denies me modprobe -r privilege.

In this thread I will continue working auto sense, but for the contemporaneous issue of expecting no data I began again from vanilla 2.6.7-rc1-bk3 at:

Subject: [PATCH] libata SATAPI no data
http://marc.theaimsgroup.com/?l=linux-ide&m=108560773104910

Pat LaVarre

Jeff G:

I've started again working towards auto sense.

You may remember, as a first step, I'm trying to inject a PIO "REQUEST SENSE" CDB after every ordinary CDB, no matter whether it fails or not. I resorted to that first step because Jeff G's auto sense patch gives me indefinite repeated dmesg, as discussed previously.

But the following naive variation on your auto sense patch took down my ATA_DEBUG ATA_VERBOSE_DEBUG ATA_ENABLE_ATAPI ATAPI_ENABLE_DMADIR kernel when I tried: modprobe ata-piix

I'll follow up with quoted dmesg when I can acquire them (they're missing from /var/log/messages). You might notice, I've left myself in 2.6.7-rc1-bk3, since -bk4 did not change ata.h libata.h libata-core.c libata-scsi.c.

Pat LaVarre

diff -Nurp linux-2.6.7-rc1-bk3/drivers/scsi/libata-core.c linux-2.6.7-rc1-bk3-pel/drivers/scsi/libata-core.c
@@ -2146,6 +2146,22 @@ static void ata_pio_task(void *_data)
 }
 
 /**
+ *	ata_check_bmdma - read PCI IDE BMDMA status
+ *	@ap: struct ata_port
+ */
+
+static u8 ata_check_bmdma(struct ata_port *ap)
+{
+	u8 host_stat;
+	if (ap->flags & ATA_FLAG_MMIO) {
+		void *mmio = (void *) ap->ioaddr.bmdma_addr;
+		host_stat = readb(mmio + ATA_DMA_STATUS);
+	} else
+		host_stat = inb(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
+	return host_stat;
+}
+
+/**
  *	ata_eng_timeout - Handle timeout of queued command
  *	@ap: Port on which timed-out command is active
  *

@@ -2188,11 +2204,7 @@ void ata_eng_timeout(struct ata_port *ap
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
-		if (ap->flags & ATA_FLAG_MMIO) {
-			void *mmio = (void *) ap->ioaddr.bmdma_addr;
-			host_stat = readb(mmio + ATA_DMA_STATUS);
-		} else
-			host_stat = inb(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
+		host_stat = ata_check_bmdma(ap);
 
 		printk(KERN_ERR "ata%u: DMA timeout, stat 0x%x\n",
 		       ap->id, host_stat);

@@ -2283,6 +2295,86 @@ struct ata_queued_cmd *ata_qc_new_init(s
 }
 
 /**
+ *	atapi_error - 
+ *	@qc:
+ */
+
+static void atapi_error(struct ata_queued_cmd *qc)
+{
+	struct ata_port *ap = qc->ap;
+	struct ata_taskfile tf;
+	struct scsi_cmnd *cmd = qc->scsicmd;
+	u8 status;
+	u8 max = min(0x12, SCSI_SENSE_BUFFERSIZE);
+	u16 len;
+	const u8 request_sense_cdb[16] = {
+		REQUEST_SENSE, 0, 0, 0, max, 0,
+	};
+	u8 err = ata_chk_err(ap); /* fetch before ata_tf_to_host */
+
+	ata_tf_init(ap, &tf, qc->dev->devno);
+	tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
+	tf.protocol = ATA_PROT_ATAPI;
+	tf.ctl |= ATA_NIEN;
+	tf.lbam = max;
+	tf.command = ATA_CMD_PACKET;
+	ata_tf_to_host(ap, &tf);
+
+	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB))
+		goto err_out;
+
+	status = ata_chk_status(ap);
+	if ((status & ATA_DRQ) == 0)
+		goto err_out;
+
+	/* FIXME: mmio-ize */
+	DPRINTK("send cdb\n");
+	outsl(ap->ioaddr.data_addr, request_sense_cdb,
+	      ap->host->max_cmd_len / 4);
+
+	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB))
+		goto err_out;
+
+	status = ata_chk_status(ap);
+	if ((status & ATA_DRQ) == 0)
+		goto err_out;
+
+	ap->ops->tf_read(ap, &tf);
+	len = tf.lbam;
+	len |= ((u16)tf.lbah) << 8;
+
+	if (!len || (len % 4) || (max < len))
+		printk(KERN_WARNING "ata%u: surprise ATAPI length %u\n",
+		       ap->id, len);
+	memset(cmd->sense_buffer, 0, sizeof(cmd->sense_buffer));
+	insl(ap->ioaddr.data_addr, cmd->sense_buffer, (len + 4 - 1) / 4);
+
+	if ((cmd->sense_buffer[0] & 0x7e) != 0x70) {
+		cmd->sense_buffer[0] = 0x70;
+		cmd->sense_buffer[2] = err >> 4;
+	}
+
+	if (ata_busy_sleep(ap, ATA_TMOUT_CDB_QUICK, ATA_TMOUT_CDB))
+		goto err_out;
+
+
+out:
+	/* ata_irq_on(ap); */
+	cmd->result = SAM_STAT_CHECK_CONDITION;
+	cmd->result = SAM_STAT_GOOD; /* FIXME */
+	qc->scsidone(cmd);
+	return;
+
+err_out:
+	cmd->sense_buffer[0] = 0x70;
+	cmd->sense_buffer[2] = HARDWARE_ERROR; /* SK, often 0x4 */
+	cmd->sense_buffer[7] = 14 - 7 - 1; /* total - offset - length */
+	cmd->sense_buffer[12] = 0x08; /* ASC, logical unit comm failure */
+	/* cmd->sense_buffer[13] = 0x03; // ASCQ, UDMA CRC */
+	goto out; /* above */
+}
+
+/**
  *	ata_qc_complete - Complete an active ATA command
  *	@qc: Command to complete
  *	@drv_stat: ATA status register contents

@@ -2312,6 +2404,8 @@ void ata_qc_complete(struct ata_queued_c
 		} else {
 			cmd->result = SAM_STAT_GOOD;
 		}
+		cmd->result = SAM_STAT_GOOD; /* FIXME */
+		atapi_error(qc); /* FIXME */
 
 		qc->scsidone(cmd);
 	}

@@ -2584,7 +2678,6 @@ static void ata_dma_complete(struct ata_
 		     ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
 	}
 
-
 	/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
 	ata_altstatus(ap);		/* dummy read */
 

@@ -2622,11 +2715,7 @@ inline unsigned int ata_host_intr (struc
 	/* BMDMA completion */
 	case ATA_PROT_DMA:
 	case ATA_PROT_ATAPI_DMA:
-		if (ap->flags & ATA_FLAG_MMIO) {
-			void *mmio = (void *) ap->ioaddr.bmdma_addr;
-			host_stat = readb(mmio + ATA_DMA_STATUS);
-		} else
-			host_stat = inb(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
+		host_stat = ata_check_bmdma(ap);
 		VPRINTK("BUS_DMA (host_stat 0x%X)\n", host_stat);
 
 		if (!(host_stat & ATA_DMA_INTR)) {

@@ -2650,11 +2739,14 @@ inline unsigned int ata_host_intr (struc
 	case ATA_PROT_NODATA:
 		status = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
 		DPRINTK("BUS_NODATA (drv_stat 0x%X)\n", status);
-		ata_qc_complete(qc, status);
+		host_stat = ata_check_bmdma(ap);
+		DPRINTK("BUS_NODATA (host_stat 0x%X)\n", host_stat);
+		ata_dma_complete(qc, host_stat);
 		handled = 1;
 		break;
 
 	default:
+		DPRINTK("irq trappable\n");
 		ap->stats.idle_irq++;
 
 #ifdef ATA_IRQ_TRAP

Will do now, except:

Sorry I do not know what "mainline" is. I will guess 2.6.7-rc1-bk4 unless I hear otherwise.

I will guess you want inline and attached and cc here unless I hear otherwise.

Pat LaVarre

That's fine, libata hasn't been updated since 2.6.7-rc1 IIRC.

Correct. Though, inline _and_ attached is a bit of overkill, assuming that your MUA does not word-wrap or otherwise mangle inline patches. Inline-only is typically preferred.

Jeff

Jeff G:

Here you go.

"Here I chose { inline } over { static inline } because lately I saw you counting libata.ko bytes."

"Here I chose u8 over int because lxr.linux.no/source/ suggests Linux readb and inb return u8."

This patch names the repeated five lines without changing where they are called. I have included it inline but not also attached. I confirmed the lines repeat via diff -b. I compiled, linked, cleared unit attentions by unsolicited sense, read, and wrote.

Sorry I have not yet understood all your English. I think/ hope this is close enough to help. I of course look forward to discovering if/ how I can help more.

Pat LaVarre

diff -Nurp linux-2.6.7-rc1-bk4/include/linux/ata.h linux-2.6.7-rc1-bk4-pel/include/linux/ata.h diff -Nurp linux-2.6.7-rc1-bk4/include/linux/libata.h linux-2.6.7-rc1-bk4-pel/include/linux/libata.h

diff -Nurp linux-2.6.7-rc1-bk4/drivers/scsi/libata-core.c linux-2.6.7-rc1-bk4-pel/drivers/scsi/libata-core.c
@@ -2146,6 +2146,22 @@ static void ata_pio_task(void *_data)
 }
 
 /**
+ *	ata_check_bmdma - read PCI IDE BMDMA status
+ *	@ap: struct ata_port
+ */
+
+static u8 ata_check_bmdma(struct ata_port *ap)
+{
+	u8 host_stat;
+	if (ap->flags & ATA_FLAG_MMIO) {
+		void *mmio = (void *) ap->ioaddr.bmdma_addr;
+		host_stat = readb(mmio + ATA_DMA_STATUS);
+	} else
+		host_stat = inb(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
+	return host_stat;
+}
+
+/**
  *	ata_eng_timeout - Handle timeout of queued command
  *	@ap: Port on which timed-out command is active
  *

@@ -2188,11 +2204,7 @@ void ata_eng_timeout(struct ata_port *ap
 
 	switch (qc->tf.protocol) {
 	case ATA_PROT_DMA:
-		if (ap->flags & ATA_FLAG_MMIO) {
-			void *mmio = (void *) ap->ioaddr.bmdma_addr;
-			host_stat = readb(mmio + ATA_DMA_STATUS);
-		} else
-			host_stat = inb(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
+		host_stat = ata_check_bmdma(ap);
 
 		printk(KERN_ERR "ata%u: DMA timeout, stat 0x%x\n",
 		       ap->id, host_stat);

@@ -2622,11 +2634,7 @@ inline unsigned int ata_host_intr (struc
 	/* BMDMA completion */
 	case ATA_PROT_DMA:
 	case ATA_PROT_ATAPI_DMA:
-		if (ap->flags & ATA_FLAG_MMIO) {
-			void *mmio = (void *) ap->ioaddr.bmdma_addr;
-			host_stat = readb(mmio + ATA_DMA_STATUS);
-		} else
-			host_stat = inb(ap->ioaddr.bmdma_addr + ATA_DMA_STATUS);
+		host_stat = ata_check_bmdma(ap);
 		VPRINTK("BUS_DMA (host_stat 0x%X)\n", host_stat);
 
 		if (!(host_stat & ATA_DMA_INTR)) {

diff -Nurp linux-2.6.7-rc1-bk4/drivers/scsi/libata-scsi.c linux-2.6.7-rc1-bk4-pel/drivers/scsi/libata-scsi.c

Deeply boring, I'm sorry to say:

ata_tf_load_pio: feat 0x0 nsect 0x0 lba 0x0 0x12 0x0
ata_tf_load_pio: device 0xA0
ata_exec: cmd 0xA0

That was the end of life.

Now of course I hope to abandon that naive approach in favour of naively misunderstanding what you're starting again with ata_check_bmdma near the intact inline patch of:

http://marc.theaimsgroup.com/?l=linux-ide&m=108570321913006

Pat LaVarre

Jeff G:

Nope, not that patch, that patch messes unjustifiably with the (rare or not) default case of ata_host_intr.

Naturally next I'll try naming the fragment of code that fetches the host_stat so I can concisely say do that only when appropriate.

Pat LaVarre

bk8.good.always.patch is the name I have stored locally for that patch.

Among our blogged results we of course still have the noise of brain-dead newbie errors by me.

I'll roll back to vanilla -bk8, apply my bk8.good.always.patch, and reconfirm results of:

#

# host expects no data, device agrees

sudo plscsi /dev/sg0 -v -X time 5 0 -x "12 00:00:00 00 00"

sudo plscsi /dev/sg0 -v -X time 5 0 -x "00 00:00:00 00 00"

#

# host expects one block in, device actually sources no data

sudo plscsi /dev/sg0 -v -X time 5 0 -i x800 -x "28 00 00:00:00:10 00 00:00 00"

#

# host expects one block out, device actually sinks no data

sudo plscsi /dev/sg0 -v -X time 5 0 -o x800 -x "2A 00 00:00:00:10 00 00:00 00"

Pat LaVarre

Bartlomiej,

Okay, here's some more ;-).

I've been lurking up to this point, but I've attached a patch that
fixes something that's been an irritation to me for awhile now (at least two years plus).

It's especially relevant now too, with the i386 4K stack in place. In
ide.c, ide_unregister(), old_hwif is allocated on the stack. The little sucker is over 1K all by itself! So, I've modified the code to use kmalloc().

It may not be right, but I'd still like to see some form of it used.
I'm still a newbie, so please don't flame me to a crisp if I've committed a grave sin. Thanks!

----------------------------------------------------------

@@ -615,11 +615,15 @@
 	ide_hwif_t *hwif, *g;
 	ide_hwgroup_t *hwgroup;
 	int irq_count = 0, unit, i;
-	ide_hwif_t old_hwif;
+	ide_hwif_t *old_hwif = NULL;
 
 	if (index >= MAX_HWIFS)
 		BUG();
 		
+	old_hwif = kmalloc(sizeof (*old_hwif), GFP_KERNEL);
+	if (old_hwif == NULL)
+		goto abort;
+
 	BUG_ON(in_interrupt());
 	BUG_ON(irqs_disabled());
 	down(&ide_cfg_sem);

@@ -765,120 +769,123 @@
 		hwif->dma_prdtable = 0;
 	}
 
-	old_hwif			= *hwif;
+
+	*old_hwif			= *hwif;
 
 	init_hwif_data(hwif, index);	/* restore hwif data to pristine status */
 	init_hwif_default(hwif, index);
 
-	hwif->hwgroup			= old_hwif.hwgroup;
+	hwif->hwgroup			= old_hwif->hwgroup;
 
-	hwif->gendev.parent		= old_hwif.gendev.parent;
+	hwif->gendev.parent		= old_hwif->gendev.parent;
 
-	hwif->proc			= old_hwif.proc;
+	hwif->proc			= old_hwif->proc;
 
-	hwif->major			= old_hwif.major;
-//	hwif->index			= old_hwif.index;
-//	hwif->channel			= old_hwif.channel;
-	hwif->straight8			= old_hwif.straight8;
-	hwif->bus_state			= old_hwif.bus_state;
+	hwif->major			= old_hwif->major;
+//	hwif->index			= old_hwif->index;
+//	hwif->channel			= old_hwif->channel;
+	hwif->straight8			= old_hwif->straight8;
+	hwif->bus_state			= old_hwif->bus_state;
 
-	hwif->atapi_dma			= old_hwif.atapi_dma;
-	hwif->ultra_mask		= old_hwif.ultra_mask;
-	hwif->mwdma_mask		= old_hwif.mwdma_mask;
-	hwif->swdma_mask		= old_hwif.swdma_mask;
+	hwif->atapi_dma			= old_hwif->atapi_dma;
+	hwif->ultra_mask		= old_hwif->ultra_mask;
+	hwif->mwdma_mask		= old_hwif->mwdma_mask;
+	hwif->swdma_mask		= old_hwif->swdma_mask;
 
-	hwif->chipset			= old_hwif.chipset;
-	hwif->hold			= old_hwif.hold;
+	hwif->chipset			= old_hwif->chipset;
+	hwif->hold			= old_hwif->hold;
 
 #ifdef CONFIG_BLK_DEV_IDEPCI
-	hwif->pci_dev			= old_hwif.pci_dev;
-	hwif->cds			= old_hwif.cds;
+	hwif->pci_dev			= old_hwif->pci_dev;
+	hwif->cds			= old_hwif->cds;
 #endif /* CONFIG_BLK_DEV_IDEPCI */
 
 #if 0
-	hwif->hwifops			= old_hwif.hwifops;
+	hwif->hwifops			= old_hwif->hwifops;
 #else
-	hwif->identify			= old_hwif.identify;
-	hwif->tuneproc			= old_hwif.tuneproc;
-	hwif->speedproc			= old_hwif.speedproc;
-	hwif->selectproc		= old_hwif.selectproc;
-	hwif->reset_poll		= old_hwif.reset_poll;
-	hwif->pre_reset			= old_hwif.pre_reset;
-	hwif->resetproc			= old_hwif.resetproc;
-	hwif->intrproc			= old_hwif.intrproc;
-	hwif->maskproc			= old_hwif.maskproc;
-	hwif->quirkproc			= old_hwif.quirkproc;
-	hwif->busproc			= old_hwif.busproc;
+	hwif->identify			= old_hwif->identify;
+	hwif->tuneproc			= old_hwif->tuneproc;
+	hwif->speedproc			= old_hwif->speedproc;
+	hwif->selectproc		= old_hwif->selectproc;
+	hwif->reset_poll		= old_hwif->reset_poll;
+	hwif->pre_reset			= old_hwif->pre_reset;
+	hwif->resetproc			= old_hwif->resetproc;
+	hwif->intrproc			= old_hwif->intrproc;
+	hwif->maskproc			= old_hwif->maskproc;
+	hwif->quirkproc			= old_hwif->quirkproc;
+	hwif->busproc			= old_hwif->busproc;
 #endif
 
 #if 0
-	hwif->pioops			= old_hwif.pioops;
+	hwif->pioops			= old_hwif->pioops;
 #else
-	hwif->ata_input_data		= old_hwif.ata_input_data;
-	hwif->ata_output_data		= old_hwif.ata_output_data;
-	hwif->atapi_input_bytes		= old_hwif.atapi_input_bytes;
-	hwif->atapi_output_bytes	= old_hwif.atapi_output_bytes;
+	hwif->ata_input_data		= old_hwif->ata_input_data;
+	hwif->ata_output_data		= old_hwif->ata_output_data;
+	hwif->atapi_input_bytes		= old_hwif->atapi_input_bytes;
+	hwif->atapi_output_bytes	= old_hwif->atapi_output_bytes;
 #endif
 
 #if 0
-	hwif->dmaops			= old_hwif.dmaops;
+	hwif->dmaops			= old_hwif->dmaops;
 #else
-	hwif->ide_dma_read		= old_hwif.ide_dma_read;
-	hwif->ide_dma_write		= old_hwif.ide_dma_write;
-	hwif->ide_dma_begin		= old_hwif.ide_dma_begin;
-	hwif->ide_dma_end		= old_hwif.ide_dma_end;
-	hwif->ide_dma_check		= old_hwif.ide_dma_check;
-	hwif->ide_dma_on		= old_hwif.ide_dma_on;
-	hwif->ide_dma_off_quietly	= old_hwif.ide_dma_off_quietly;
-	hwif->ide_dma_test_irq		= old_hwif.ide_dma_test_irq;
-	hwif->ide_dma_host_on		= old_hwif.ide_dma_host_on;
-	hwif->ide_dma_host_off		= old_hwif.ide_dma_host_off;
-	hwif->ide_dma_verbose		= old_hwif.ide_dma_verbose;
-	hwif->ide_dma_lostirq		= old_hwif.ide_dma_lostirq;
-	hwif->ide_dma_timeout		= old_hwif.ide_dma_timeout;
+	hwif->ide_dma_read		= old_hwif->ide_dma_read;
+	hwif->ide_dma_write		= old_hwif->ide_dma_write;
+	hwif->ide_dma_begin		= old_hwif->ide_dma_begin;
+	hwif->ide_dma_end		= old_hwif->ide_dma_end;
+	hwif->ide_dma_check		= old_hwif->ide_dma_check;
+	hwif->ide_dma_on		= old_hwif->ide_dma_on;
+	hwif->ide_dma_off_quietly	= old_hwif->ide_dma_off_quietly;
+	hwif->ide_dma_test_irq		= old_hwif->ide_dma_test_irq;
+	hwif->ide_dma_host_on		= old_hwif->ide_dma_host_on;
+	hwif->ide_dma_host_off		= old_hwif->ide_dma_host_off;
+	hwif->ide_dma_verbose		= old_hwif->ide_dma_verbose;
+	hwif->ide_dma_lostirq		= old_hwif->ide_dma_lostirq;
+	hwif->ide_dma_timeout		= old_hwif->ide_dma_timeout;
 #endif
 
 #if 0
-	hwif->iops			= old_hwif.iops;
+	hwif->iops			= old_hwif->iops;
 #else
-	hwif->OUTB		= old_hwif.OUTB;
-	hwif->OUTBSYNC		= old_hwif.OUTBSYNC;
-	hwif->OUTW		= old_hwif.OUTW;
-	hwif->OUTL		= old_hwif.OUTL;
-	hwif->OUTSW		= old_hwif.OUTSW;
-	hwif->OUTSL		= old_hwif.OUTSL;
-
-	hwif->INB		= old_hwif.INB;
-	hwif->INW		= old_hwif.INW;
-	hwif->INL		= old_hwif.INL;
-	hwif->INSW		= old_hwif.INSW;
-	hwif->INSL		= old_hwif.INSL;
-#endif
-
-	hwif->mmio			= old_hwif.mmio;
-	hwif->rqsize			= old_hwif.rqsize;
-	hwif->no_lba48			= old_hwif.no_lba48;
+	hwif->OUTB		= old_hwif->OUTB;
+	hwif->OUTBSYNC		= old_hwif->OUTBSYNC;
+	hwif->OUTW		= old_hwif->OUTW;
+	hwif->OUTL		= old_hwif->OUTL;
+	hwif->OUTSW		= old_hwif->OUTSW;
+	hwif->OUTSL		= old_hwif->OUTSL;
+
+	hwif->INB		= old_hwif->INB;
+	hwif->INW		= old_hwif->INW;
+	hwif->INL		= old_hwif->INL;
+	hwif->INSW		= old_hwif->INSW;
+	hwif->INSL		= old_hwif->INSL;
+#endif
+
+	hwif->mmio			= old_hwif->mmio;
+	hwif->rqsize			= old_hwif->rqsize;
+	hwif->no_lba48			= old_hwif->no_lba48;
 #ifndef CONFIG_BLK_DEV_IDECS
-	hwif->irq			= old_hwif.irq;
+	hwif->irq			= old_hwif->irq;
 #endif /* CONFIG_BLK_DEV_IDECS */
 
-	hwif->dma_base			= old_hwif.dma_base;
-	hwif->dma_master		= old_hwif.dma_master;
-	hwif->dma_command		= old_hwif.dma_command;
-	hwif->dma_vendor1		= old_hwif.dma_vendor1;
-	hwif->dma_status		= old_hwif.dma_status;
-	hwif->dma_vendor3		= old_hwif.dma_vendor3;
-	hwif->dma_prdtable		= old_hwif.dma_prdtable;
-
-	hwif->dma_extra			= old_hwif.dma_extra;
-	hwif->config_data		= old_hwif.config_data;
-	hwif->select_data		= old_hwif.select_data;
-	hwif->autodma			= old_hwif.autodma;
-	hwif->udma_four			= old_hwif.udma_four;
-	hwif->no_dsc			= old_hwif.no_dsc;
+	hwif->dma_base			= old_hwif->dma_base;
+	hwif->dma_master		= old_hwif->dma_master;
+	hwif->dma_command		= old_hwif->dma_command;
+	hwif->dma_vendor1		= old_hwif->dma_vendor1;
+	hwif->dma_status		= old_hwif->dma_status;
+	hwif->dma_vendor3		= old_hwif->dma_vendor3;
+	hwif->dma_prdtable		= old_hwif->dma_prdtable;
+
+	hwif->dma_extra			= old_hwif->dma_extra;
+	hwif->config_data		= old_hwif->config_data;
+	hwif->select_data		= old_hwif->select_data;
+	hwif->autodma			= old_hwif->autodma;
+	hwif->udma_four			= old_hwif->udma_four;
+	hwif->no_dsc			= old_hwif->no_dsc;
 
-	hwif->hwif_data			= old_hwif.hwif_data;
+	hwif->hwif_data			= old_hwif->hwif_data;
 abort:
+	if (old_hwif)
+		kfree(old_hwif);
 	spin_unlock_irq(&ide_lock);
 	up(&ide_cfg_sem);
 }

Daniel S. Cox
Electronic Commerce Systems

You do 'goto abort' while not holding locks.

Anyway this is the minor issue,
the major issue is that... you are late 52 hours... ;-)

Similar patch from Chris Wedgwood has been merged recently.

http://linus.bkbits.net:8080/linux-2.5/user=B.Zolnierkiewicz/cset@40ab7239LJqW0C3mhBU9Fv8BPbo0CA?nav=!-|index.html|stats|!+|index.html|ChangeSet@-3d

Bartlomiej,

Yes, you're correct. Should I be holding the locks? I didn't think
that you'd want to call kmalloc() with GFP_KERNEL inside a spinlock because it might sleep. So, I moved the kmalloc() to be one of the first things done to avoid being inside the lock(s). The comment at the top of the function is correct: it IS bonkers!

Cool! Like I said, I mainly wanted to get some form of this idea into
ide_unregister().

Thanks!

Daniel S. Cox
Electronic Commerce Systems

Bartlomiej,

Please disregard the previous email. I figured it out. Part of that
patch came from an earlier version where I was doing the kmalloc() later in the routine. I didn't reason very well when I still had the error flow going to the abort.

Sorry about the wasted bandwidth! Although it wasn't wasted entirely.
I learned a thing or two.

Thanks!

Daniel S. Cox
Electronic Commerce Systems

Good!

No problem. :-)