Re: [PATCH] libata DMADIR support

Re: [PATCH] libata DMADIR support

[Pat LaVarre]

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
--- o/include/linux/ata.h	2004-05-16 08:06:04.000000000 -0600
+++ linux-2.6.6-bk3-pel/include/linux/ata.h	2004-05-16
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

Re: [PATCH] libata DMADIR support

[Jeff Garzik]

Pat LaVarre wrote:
> + #undef ATA_FORCE_DMADIR /* give Si DMADIR to all SATA ATAPI DMA */

> 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.


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

	Jeff

Re: [PATCH] libata DMADIR support

[Pat LaVarre]

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. 

...

----

Re: [PATCH] libata DMADIR support

[Jeff Garzik]

Pat LaVarre wrote:
> 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.

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.

> 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.

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

	Jeff

Re: [PATCH] libata DMADIR support

[Pat LaVarre]

> Re: [PATCH] libata-core when not ata_id_use_dmadir despite yes SiliconI
> From: Jeff Garzik <jgarzik () pobox ! com>
> Date: 2004-05-15 16:39:57
> ...
> > Do you know of some way to detect SI 3611CT80 1.4?
>
> Not right now.
>
> I need to see the full IDENTIFY PACKET DEVICE
> data to discover a way...  (and I need to
> write some code before libata dumps the full
> identify-device page)
>
> Other bridges add an identifying string to the
> -end- of the ATA device's model name, maybe
> SiI does too.

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
---
> 043f 0000 0000 0000 0000 604f 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
--- linux-2.6.6-bk4/drivers/scsi/libata-core.c	2004-05-17 12:50:15.000000000 -0600
+++ linux-2.6.6-bk4-pel/drivers/scsi/libata-core.c	2004-05-17 14:18:37.000000000 -0600
@@ -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);
 }
 
 /**

Re: [PATCH] libata DMADIR support

[Jeff Garzik]

Pat LaVarre wrote:
> 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.

Disappointing...

	Jeff

Re: [PATCH] libata DMADIR support

[Pat LaVarre]

> "documents 2003" link on http://www.t13.org/ ...
> ATAPI DMA Direction issues Proposal 12/21/03 Hartney
> http://www.t13.org/docs2003/e03132r3.pdf

Links!  Thank you.

> > 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.
> 
> I bet your bridge doesn't _require_ DMADIR, therefore it doesn't bother 
> with word 62?  </wild guess>

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
$

Re: [PATCH] libata DMADIR support

[Jeff Garzik]

Pat LaVarre wrote:
> 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?

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.



> 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).

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.


> 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.

<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

Re: [PATCH] libata DMADIR support

[Jeff Garzik]

Jeff Garzik wrote:
> Consider:  what data direction will a vendor-reserved SCSI opcode 
> require?  The OS driver cannot know.


s/OS driver/bridge/ obviously...

Re: [PATCH] libata DMADIR support

[Pat LaVarre]

> > my Si 3611CT80 r1.4 bridge ...
> 
> I am convinced that your hardware requires DMADIR.

Me too.

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

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?

> > 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.
> 
> Disappointing...

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

> word 62

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.

> <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...

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.

> > Consider:  what data direction will a vendor-reserved SCSI opcode 
> > require?  The OS driver cannot know.
> 
> s/OS driver/bridge/ obviously...

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

Re: [PATCH] libata DMADIR support

[Jeff Garzik]

Pat LaVarre wrote:
>>I am not convinced that there is no way to detect this condition at 
>>runtime, based on some hardware characteristic, or output.
> 
> 
> 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?

I dunno.  Feel free to play around.  :)

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


>>word 62
> 
> 
> 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.

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


> 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.

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

	Jeff

Re: [PATCH] libata DMADIR support

[Pat LaVarre]

>>> word 62
>> 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.
>
> SWDMA never existed for ATAPI devices.  Word 62 has -always- been 
> reserved, until now.

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

Re: [PATCH] libata DMADIR support

[Pat LaVarre]

> Feel free to play around.  :)
> 
> Silicon Image is sending me a couple bridges, we'll see how they
> behave.

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

Re: [PATCH] libata DMADIR support

[Pat LaVarre]

> a second sample of hardware ...
> 
> 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 ...
> 
> I imagine I'll find I can most concisely make this work by copying into
> words 49 63 88 the bits of word 62.

Yep.

diff -Nurp linux-2.6.6-bk5/drivers/scsi/libata-core.c linux-2.6.6-bk5-pel/drivers/scsi/libata-core.c
--- linux-2.6.6-bk5/drivers/scsi/libata-core.c	2004-05-18 14:20:05.000000000 -0600
+++ linux-2.6.6-bk5-pel/drivers/scsi/libata-core.c	2004-05-18 16:53:29.000000000 -0600
@@ -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",

Re: [PATCH] libata DMADIR support

[Jeff Garzik]

Pat LaVarre wrote:
> +#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


Yeah, something like this needs to be done for DMADIR bridges.

	Jeff

Re: [PATCH] libata DMADIR support

[Pat LaVarre]

> Yeah, something like this needs to be done for DMADIR bridges.

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
--- linux-2.6.6-bk6/drivers/scsi/libata-core.c	2004-05-19 13:35:25.000000000 -0600
+++ linux-2.6.6-bk6-pel/drivers/scsi/libata-core.c	2004-05-19 16:44:52.974294696 -0600
@@ -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",

[PATCH] atapi request sense work

[Jeff Garzik]

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

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


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 5523 bytes --]

===== drivers/scsi/libata-core.c 1.64 vs edited =====
--- 1.64/drivers/scsi/libata-core.c	Mon May 17 19:39:49 2004
+++ edited/drivers/scsi/libata-core.c	Tue May 18 19:45:10 2004
@@ -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 =====
--- 1.37/drivers/scsi/libata-scsi.c	Mon May 17 19:39:49 2004
+++ edited/drivers/scsi/libata-scsi.c	Tue May 18 19:43:54 2004
@@ -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 =====
--- 1.15/drivers/scsi/libata.h	Sun May 16 21:33:12 2004
+++ edited/drivers/scsi/libata.h	Tue May 18 19:02:12 2004
@@ -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;

Re: [PATCH] atapi request sense work

[Pat LaVarre]

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

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

>  	cmd->sense_buffer[7] = 14 - 8;	/* addnl. sense len. FIXME: correct? */
> ...
>  	cmd->sense_buffer[7] = 14 - 8;	/* addnl. sense len. FIXME: correct? */

What's in dispute for these FIXME?

At offset seven in a field whose length is one byte we find the
additional length, when the data comes from op x03 "REQUEST SENSE". 
Therefore the value we should put there is the total length minus the
offset seven minus the length one.

14 - 7 - 1 would be a plainer way of saying that, sure, but 14 - 8
always gives us the same answer.

A comment like /* total - offset - length */ might help.  I know source
code like 14 - 7 - 1 has a history of persuading future maintainers to
erroneously code (total - offset - 1) where (total - offset - length)
was meant.

"What's in dispute for these FIXME?"

Curiously yours, Pat LaVarre

Re: [PATCH] atapi request sense work

[Jeff Garzik]

Pat LaVarre wrote:
>> 	cmd->sense_buffer[7] = 14 - 8;	/* addnl. sense len. FIXME: correct? */
>>...
>> 	cmd->sense_buffer[7] = 14 - 8;	/* addnl. sense len. FIXME: correct? */
> 
> 
> What's in dispute for these FIXME?


<shrug>

I don't remember anymore :)

	Jeff

Re: [PATCH] atapi request sense work

[Pat LaVarre]

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

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
--- linux-2.6.6-bk6/drivers/scsi/libata-core.c	2004-05-19 13:35:25.000000000 -0600
+++ linux-2.6.6-bk6-pel/drivers/scsi/libata-core.c	2004-05-19 16:03:50.000000000 -0600
@@ -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
--- linux-2.6.6-bk6/drivers/scsi/libata-scsi.c	2004-05-19 13:35:25.000000000 -0600
+++ linux-2.6.6-bk6-pel/drivers/scsi/libata-scsi.c	2004-05-19 16:03:50.000000000 -0600
@@ -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;

Re: [PATCH] atapi request sense work

[Pat LaVarre]

> I lost my display & mouse & keyboard.

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

> @@ -137,7 +137,7 @@ void ata_to_sense_error(struct ata_queue
> ...
> @@ -926,7 +926,7 @@ void ata_scsi_badcmd(struct scsi_cmnd *c
> -	cmd->sense_buffer[0] = 0x70;
> +	cmd->sense_buffer[0] = 0xf0;

We're setting (bytes[0] & x80) INFO "Valid" why?

In hopes of soon filling in bytes[2:3:4:5] "INFO" with a nonzero
LBA-in-error?

Pat LaVarre

Re: [PATCH] atapi request sense work

[Pat LaVarre]

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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

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

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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

> a trivial recursion trap I added ..., this time around.

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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

> yes indeed, ata_scsi_translate re-entered itself at least 4551 times 
> ...

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

Re: [PATCH] atapi request sense work

[Jeff Garzik]

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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

Jeff G:

> Keep posting, ...

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

Re: [PATCH] atapi request sense work

[Bartlomiej Zolnierkiewicz]

On Friday 21 of May 2004 17:35, Pat LaVarre wrote:
> Jeff G:
> > Keep posting, ...
>
> 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:

Keep posting... seconded.

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

Cheers.

Re: [PATCH] atapi request sense work

[Pat LaVarre]

> Keep posting... seconded.

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
--- linux-2.6.6-bk8/drivers/scsi/libata-core.c	2004-05-21 09:44:22.000000000 -0600
+++ linux-2.6.6-bk8-pel/drivers/scsi/libata-core.c	2004-05-21 11:29:01.571933824 -0600
@@ -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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

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

Re: [PATCH] atapi request sense work

[Jeff Garzik]

Pat LaVarre wrote:
> Without auto sense, we read and write ok ...
> 
> But when I try a no-data op x00 "TEST UNIT READY", I get:

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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

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.

Re: [PATCH] atapi request sense work

[Pat LaVarre]

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

Re: [PATCH] atapi request sense work

[Jeff Garzik]

Pat LaVarre wrote:
> 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.


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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

> > 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.
>
> That's considered a "don't do that" condition :)

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

Pat LaVarre

Re: [PATCH] atapi request sense work

[Pat LaVarre]

Jeff G:

> > > Without auto sense, we read and write ok ...
> > > 
> > > But when I try a no-data op x00 "TEST UNIT READY", I get:
> > 
> > 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.
> ...
> I'll roll back to vanilla -bk8, apply my bk8.good.always.patch, and 
> reconfirm ...

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

Re: [PATCH] atapi request sense work

[Jeff Garzik]

Pat LaVarre wrote:
> kernel: ata_host_intr: BUS_NODATA (drv_stat 0x50)
> kernel: irq 18: nobody cared!


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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

> > kernel: ata_host_intr: BUS_NODATA (drv_stat 0x50)
> > kernel: irq 18: nobody cared!
>
> Can you print out the BMDMA status register (often called 'host_stat' in 
> the code)?

Thanks for asking.

> I wonder if that indicates ATA_DMA_INTR for example.

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
--- linux-2.6.6-bk8/drivers/scsi/libata-core.c	2004-05-21 09:44:22.000000000 -0600
+++ linux-2.6.6-bk8-pel/drivers/scsi/libata-core.c	2004-05-21 17:53:03.000000000 -0600
@@ -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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

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

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

Re: [PATCH] atapi request sense work

[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
--- linux-2.6.6-bk8/drivers/scsi/libata-core.c	2004-05-21 09:44:22.000000000 -0600
+++ linux-2.6.6-bk8-pel/drivers/scsi/libata-core.c	2004-05-21 18:17:57.228369336 -0600
@@ -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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

Jeff G:

> Open-ioctl-close of /dev/scd$n now mostly works.
> 
> (Read/ write still has issues I will report separately.)

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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

> To my newbie surprise, I have more good news.  Teaching my -bk8 kernel
> to clear 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.

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

Re: [PATCH] atapi request sense work

[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
--- linux-2.6.7-rc1-bk3/drivers/scsi/libata-core.c	2004-05-25 14:47:00.000000000 -0600
+++ linux-2.6.7-rc1-bk3-pel/drivers/scsi/libata-core.c	2004-05-27 16:56:27.000000000 -0600
@@ -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

Re: [PATCH] atapi request sense work

[Jeff Garzik]

Pat LaVarre wrote:
>  /**
> + *	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;
> +}
> +
> +/**


As a first step, would you be willing to create a patch versus mainline, 
that does nothing but add this function (and uses it)?

	Jeff

Re: [PATCH] atapi request sense work

[Pat LaVarre]

>> +static u8 ata_check_bmdma(struct ata_port *ap)
>> ...
>
> As a first step, would you be willing to create a patch versus 
> mainline, that does nothing but add this function (and uses it)?

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

Re: [PATCH] atapi request sense work

[Jeff Garzik]

Pat LaVarre wrote:
>>> +static u8 ata_check_bmdma(struct ata_port *ap)
>>> ...
>>
>>
>> As a first step, would you be willing to create a patch versus 
>> mainline, that does nothing but add this function (and uses it)?
> 
> 
> Will do now, except:
> 
> Sorry I do not know what "mainline" is.  I will guess 2.6.7-rc1-bk4 
> unless I hear otherwise.

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


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

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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

Jeff G:

> As a first step, ...
> create a patch versus mainline, that does
> nothing but add this function (and uses it)?

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
--- linux-2.6.7-rc1-bk4/drivers/scsi/libata-core.c	2004-05-27 16:40:42.000000000 -0600
+++ linux-2.6.7-rc1-bk4-pel/drivers/scsi/libata-core.c	2004-05-27 17:56:57.192826736 -0600
@@ -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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

> naive variation on your auto sense patch
> ...
> I'll follow up with quoted dmesg when I can ...

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

Re: [PATCH] atapi request sense work

[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.

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

Re: [PATCH] atapi request sense work

[Pat LaVarre]

> a trivial -bk8 patch that ducks ever trying PATAPI auto sense.

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

> > Without auto sense, we read and write ok ...
> > 
> > But when I try a no-data op x00 "TEST UNIT READY", I get:
> 
> 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.

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

Re: [PATCH] atapi request sense work

[Danny Cox]

Bartlomiej,

On Fri, 2004-05-21 at 11:46, Bartlomiej Zolnierkiewicz wrote:
> Keep posting... seconded.
> 
> I'm also reading this and linux-ide needs more traffic/people. ;-)

	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!

----------------------------------------------------------
--- drivers/ide/ide.c.old	2004-05-11 08:00:25.000000000 -0400
+++ drivers/ide/ide.c	2004-05-21 14:04:44.904273325 -0400
@@ -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

Re: [PATCH] atapi request sense work

[Bartlomiej Zolnierkiewicz]

On Friday 21 of May 2004 20:23, Danny Cox wrote:
> Bartlomiej,
>
> On Fri, 2004-05-21 at 11:46, Bartlomiej Zolnierkiewicz wrote:
> > Keep posting... seconded.
> >
> > I'm also reading this and linux-ide needs more traffic/people. ;-)
>
> 	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!

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

Re: [PATCH] kmalloc old_hwif

[Danny Cox]

Bartlomiej,

On Fri, 2004-05-21 at 14:39, Bartlomiej Zolnierkiewicz wrote:
> You do 'goto abort' while not holding locks.

	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!

> 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.

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

	Thanks!

-- 
Daniel S. Cox
Electronic Commerce Systems

Re: [PATCH] atapi request sense work

[Danny Cox]

Bartlomiej,

On Fri, 2004-05-21 at 14:39, Bartlomiej Zolnierkiewicz wrote:
> You do 'goto abort' while not holding locks.

	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

Re: [PATCH] atapi request sense work

[Bartlomiej Zolnierkiewicz]

On Friday 21 of May 2004 21:00, Danny Cox wrote:
> Bartlomiej,
>
> On Fri, 2004-05-21 at 14:39, Bartlomiej Zolnierkiewicz wrote:
> > You do 'goto abort' while not holding locks.
>
> 	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.

Good!

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

No problem. :-)