[PATCH] libata atapi work #2.1

[PATCH] libata atapi work #2.1

[Jeff Garzik]

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


Patch based on libata 20040515-1 patch just posted.

Now that the previous patches are flushed into the for-upstream queue, 
this is the beginning of a new patch series.  To reduce confusion, all 
patches in this series are numbered 2.<n>.

If you have a bridge that requires DMADIR, you'll have to apply a 
variant of the DMADIR patch to get your setup working.



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

# ChangeSet
#   2004/05/15 18:07:32-04:00 jgarzik@redhat.com 
#   [libata] handle non-data ATAPI commands via interrupt
#   
#   It's easier to do it this way, than polling, at the moment.
#   
#   Also, fix a test in ata_scsi_translate that was incorrectly
#   erroring-out non-data commands.
# 
diff -Nru a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c	Sat May 15 18:09:46 2004
+++ b/drivers/scsi/libata-core.c	Sat May 15 18:09:46 2004
@@ -2677,6 +2677,7 @@
 		handled = 1;
 		break;
 
+	case ATA_PROT_ATAPI:
 	case ATA_PROT_NODATA:	/* command completion, but no data xfer */
 		status = ata_busy_wait(ap, ATA_BUSY | ATA_DRQ, 1000);
 		DPRINTK("BUS_NODATA (drv_stat 0x%X)\n", status);
@@ -2837,9 +2838,16 @@
 	      qc->scsicmd->cmnd, ap->host->max_cmd_len / 4);
 
 	/* if we are DMA'ing, irq handler takes over from here */
-	if (qc->tf.protocol == ATA_PROT_ATAPI_DMA) {
+	if (qc->tf.protocol == ATA_PROT_ATAPI_DMA)
 		ap->ops->bmdma_start(qc);	    /* initiate bmdma */
-	} else {
+
+	/* non-data commands are also handled via irq */
+	else if (qc->scsicmd->sc_data_direction == SCSI_DATA_NONE) {
+		/* do nothing */
+	}
+
+	/* PIO commands are handled by polling */
+	else {
 		ap->pio_task_state = PIO_ST;
 		queue_work(ata_wq, &ap->pio_task);
 	}
diff -Nru a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
--- a/drivers/scsi/libata-scsi.c	Sat May 15 18:09:46 2004
+++ b/drivers/scsi/libata-scsi.c	Sat May 15 18:09:46 2004
@@ -46,8 +46,8 @@
  *	@geom: location to which geometry will be output
  *
  *	Generic bios head/sector/cylinder calculator
- *	used by sd. Most BIOSes nowadays expect a XXX/255/16  (CHS) 
- *	mapping. Some situations may arise where the disk is not 
+ *	used by sd. Most BIOSes nowadays expect a XXX/255/16  (CHS)
+ *	mapping. Some situations may arise where the disk is not
  *	bootable if this is not used.
  *
  *	LOCKING:
@@ -57,7 +57,7 @@
  *	Zero.
  */
 int ata_std_bios_param(struct scsi_device *sdev, struct block_device *bdev,
-		       sector_t capacity, int geom[]) 
+		       sector_t capacity, int geom[])
 {
 	geom[0] = 255;
 	geom[1] = 63;
@@ -362,19 +362,20 @@
 
 	VPRINTK("ENTER\n");
 
-	if (unlikely(cmd->request_bufflen < 1)) {
-		printk(KERN_WARNING "ata%u(%u): empty request buffer\n",
-		       ap->id, dev->devno);
-		goto err_out;
-	}
-
 	qc = ata_scsi_qc_new(ap, dev, cmd, done);
 	if (!qc)
 		return;
 
 	if (cmd->sc_data_direction == SCSI_DATA_READ ||
-	    cmd->sc_data_direction == SCSI_DATA_WRITE)
+	    cmd->sc_data_direction == SCSI_DATA_WRITE) {
+		if (unlikely(cmd->request_bufflen < 1)) {
+			printk(KERN_WARNING "ata%u(%u): WARNING: zero len r/w req\n",
+			       ap->id, dev->devno);
+			goto err_out;
+		}
+
 		qc->flags |= ATA_QCFLAG_SG; /* data is present; dma-map it */
+	}
 
 	if (xlat_func(qc, scsicmd))
 		goto err_out;
@@ -910,12 +911,18 @@
 
 	qc->tf.command = ATA_CMD_PACKET;
 
-	if ((cmd->sc_data_direction == SCSI_DATA_NONE) ||
-	    ((qc->flags & ATA_QCFLAG_DMA) == 0)) {
+	/* no data - interrupt-driven */
+	if (cmd->sc_data_direction == SCSI_DATA_NONE)
+		qc->tf.protocol = ATA_PROT_ATAPI;
+
+	/* PIO data xfer - polling */
+	else if ((qc->flags & ATA_QCFLAG_DMA) == 0) {
 		ata_qc_set_polling(qc);
 		qc->tf.protocol = ATA_PROT_ATAPI;
 		qc->tf.lbam = (8 * 1024) & 0xff;
 		qc->tf.lbah = (8 * 1024) >> 8;
+
+	/* DMA data xfer - interrupt-driven */
 	} else {
 		qc->flags |= ATA_QCFLAG_SG; /* data is present; dma-map it */
 		qc->tf.protocol = ATA_PROT_ATAPI_DMA;

Re: [PATCH] libata atapi work #2.1

[Pat LaVarre]

Jeff G:

> Subject: Re: SATA ATAPI work in progress
> Date: 2004-05-15 16:32:56
> ...
> Seems like we are pretty close.  Once I add
> the code to issue a REQUEST SENSE, I bet
> things start working.

I agree!  I'm delighted to report,

I now see SATA stream up past 1.5 GB/min via ioctl SG_IO read & write of
the disc blocks, after I apply together all four of the patches you
first posted as:

> Subject: [PATCH] libata update 20040515-1
> Subject: [PATCH] libata atapi work #2.1
> Subject: [PATCH] libata DMADIR support
> Subject: [PATCH] Re: SATA ATAPI work in progress

Heads up, I was myself slow to appreciate:

Yes still in -bk3 after patch.2.1 we need to apply that fourth patch
i.e. what I had termed patch.7 i.e. ata_host_intr += ATA_PROT_ATAPI_DMA.

> Once I add the code to issue a REQUEST SENSE, ...

Also, of course, to avoid dying for the lack of auto sense, I have to
duck all errors.

Once I died by mistyping an LBA, off by one nybble.  Repeatedly I die
except when I remember to clear the x 6 29 Reset and x 6 28 GoneReady
unit attentions, via an abuse which works here i.e. repeatedly
unsolicited "REQUEST SENSE".  I notice in particular that, with this
device, modprobe seemingly provokes x 6 29 Reset.

Pat LaVarre


$ # demo of 1.5 GB/min, in or out
$
$ plscsi /dev/sg0 -i x12 -x "03 00:00:00 12 00"
$ plscsi /dev/sg0 -i x12 -x "03 00:00:00 12 00"
$
$ time pldd of if=/dev/sg0 bs=64k sbs=2k skip=0 count=1600
 
real    0m4.134s
user    0m0.002s
sys     0m0.155s
$
$ dc -e '64 1024 * 1600 * 4.134 / p 60 * p'
25364683
1521880980
$
$ time pldd of=/dev/sg0 if bs=64k sbs=2k seek=0 count=1600
 
real    0m4.125s
user    0m0.001s
sys     0m0.150s
$
$ dc -e '64 1024 * 1600 * 4.125 / p 60 * p'
25420024
1525201440
$


$ # demo of reset implicit in modprobe
$
$ plscsi /dev/sg0 -i x12 -x "03 00:00:00 12 00" -v
x 00000000 03 00:00:00 12 00 .. .. .. .. .. .. .. .. .. .. "C@@@R@"
x 00000000 70:00:00:00 00:00:00:18 00:00:00:00 00:00:00:00 "p@@@@@@X@@@@@@@@"
x 00000010 00:00 .. .. .. .. .. .. .. .. .. .. .. .. .. .. "@@"
// 0 = plscsi.main exit int
$
$ sudo modprobe -r ata-piix
$ sudo modprobe ata-piix
$
$ plscsi /dev/sg0 -i x12 -x "03 00:00:00 12 00" -v
x 00000000 03 00:00:00 12 00 .. .. .. .. .. .. .. .. .. .. "C@@@R@"
x 00000000 70:00:06:00 00:00:00:18 00:00:00:00 29:00:00:00 "p@F@@@@X@@@@)@@@"
x 00000010 00:00 .. .. .. .. .. .. .. .. .. .. .. .. .. .. "@@"
// 0 = plscsi.main exit int
$

Re: [PATCH] libata atapi work #2.1

[Jeff Garzik]

Pat LaVarre wrote:
> I now see SATA stream up past 1.5 GB/min via ioctl SG_IO read & write of
> the disc blocks, after I apply together all four of the patches you
> first posted as:

Sounds like something is working...


>>Subject: [PATCH] libata update 20040515-1
>>Subject: [PATCH] libata atapi work #2.1
>>Subject: [PATCH] libata DMADIR support
>>Subject: [PATCH] Re: SATA ATAPI work in progress
> 
> 
> Heads up, I was myself slow to appreciate:
> 
> Yes still in -bk3 after patch.2.1 we need to apply that fourth patch
> i.e. what I had termed patch.7 i.e. ata_host_intr += ATA_PROT_ATAPI_DMA.

Please let me know what patches (if any besides #define) are needed for 
setup, after you include patch #2.2 and #2.3 in the pile.


>>Once I add the code to issue a REQUEST SENSE, ...
> 
> 
> Also, of course, to avoid dying for the lack of auto sense, I have to
> duck all errors.

Yes.

Two main things on the SCSI end of ATAPI to do:
* issue REQUEST SENSE, to simulate an auto-sensing scsi device
* in INQUIRY output, simulate compliance with MMC-3.  Linux SCSI stack 
is not aware that all ATAPI devices report zeroes in the SCSI version 
field (as do some USB storage devices).  So, we fake it.

	Jeff

Re: [PATCH] libata atapi work #2.1

[Pat LaVarre]

Jeff G:

> something is working...
 
Yes!

In my one sample, even misaligning a dd-shattered stream costs me only
0.5 GB/min at the outside of the disc, so command/ status overhead is
running reasonably low.

> Please let me know what patches (if any
> besides #define) are needed for setup, after
> you include patch #2.2 and #2.3 in the pile.

I will revert to -bk3 vanilla, then apply 2.* from linux-ide, then
report back.

> Two main things on the SCSI end of ATAPI to
> do:
> * issue REQUEST SENSE, to simulate an
> auto-sensing scsi device

I will try to create a desired inference of ATAPI op x03 "REQUEST SENSE"
to immediately follow the failing command, after the pattern of the
existing inference of ATA op xA1 "IDENTIFY".

I will have to discover how to auto sense only after a plain failure,
not inappropriately after a timeout.  Last I checked, the code was
written to see any failure as if it were a timeout.

> * in INQUIRY output, simulate compliance with
> MMC-3.  Linux SCSI stack is not aware that all
> ATAPI devices report zeroes in the SCSI version
> field (as do some USB storage devices).  So,
> we fake it.

I will try to munge the otherwise transparent pass thru of op x12
"INQUIRY" after the pattern of:

http://lxr.linux.no/source/drivers/usb/storage/protocol.c?v=2.6.5#L62

62 static void fix_inquiry_data(Scsi_Cmnd *srb)
...
82         /* Change the SCSI revision number */
83         databuf[2] = (databuf[2] & ~7) | 2;

I suppose there is no saving the transparency of ioctl CDROM_SEND_PACKET
and ioctl SG_IO.  Because of this linux-scsi legacy, we have to lose the
bits that we read from the device.

Pat LaVarre

Re: [PATCH] libata atapi work #2.1

[Jeff Garzik]

Pat LaVarre wrote:
>>Please let me know what patches (if any
>>besides #define) are needed for setup, after
>>you include patch #2.2 and #2.3 in the pile.
> 
> 
> I will revert to -bk3 vanilla, then apply 2.* from linux-ide, then
> report back.

Last night's -bk4 should include everything except the obvious patch to 
include/linux/libata.h to define ATAPI_ENABLE_DMADIR.


> I will have to discover how to auto sense only after a plain failure,
> not inappropriately after a timeout.  Last I checked, the code was
> written to see any failure as if it were a timeout.

Good guess, very close:  the code is written to see any timeout as if it 
were a failure.  Calling ata_eng_timeout() function is unfortunately a 
bit misnamed.


>>* in INQUIRY output, simulate compliance with
>>MMC-3.  Linux SCSI stack is not aware that all
>>ATAPI devices report zeroes in the SCSI version
>>field (as do some USB storage devices).  So,
>>we fake it.
> 
> 
> I will try to munge the otherwise transparent pass thru of op x12
> "INQUIRY" after the pattern of:
> 
> http://lxr.linux.no/source/drivers/usb/storage/protocol.c?v=2.6.5#L62
> 
> 62 static void fix_inquiry_data(Scsi_Cmnd *srb)
> ...
> 82         /* Change the SCSI revision number */
> 83         databuf[2] = (databuf[2] & ~7) | 2;

Yes, something like this is needed.

We need to indicated MMC-3...

	Jeff

Re: [PATCH] libata atapi work #2.1

[Pat LaVarre]

(-: Be warned, this from me is all ack, ack, ack - no substance. :-)

> -bk4 should include everything
> except the obvious ... define ...

Yes, -bk4 works great here now, same as the collected patches did
before.

> Last night's -bk4

Thanks for the hint.

Independently at http://kernel.org I saw notice of 2004-05-17 11:46 UTC
-bk4 appear.  The actual patch did not arrive in the view I have here of
http://www.kernel.org/pub/linux/kernel/v2.6/snapshots/ til later, maybe
16:00 UTC.  Now is past 19:00 UTC.

Pat LaVarre

P.S. "Same as" "before" is of course my destructive exercise:

plscsi /dev/sg0 -i 0x12 -x "03 00:00:00 12 00" -v
plscsi /dev/sg0 -i 0x12 -x "03 00:00:00 12 00" -v
time pldd of if=/dev/sg0 bs=64k sbs=2k skip=0 count=1600
time pldd of=/dev/sg0 if bs=64k sbs=2k seek=0 count=1600

--- linux-2.6.6-bk4/include/linux/libata.h	2004-05-17 12:50:20.000000000 -0600
+++ linux-2.6.6-bk4-pel/include/linux/libata.h	2004-05-17 13:11:16.315019408 -0600
@@ -33,14 +33,14 @@
  * 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 */
-#undef ATAPI_ENABLE_DMADIR	/* enables ATAPI DMADIR bridge support */
+#define ATAPI_ENABLE_DMADIR	/* enables ATAPI DMADIR bridge support */
 
 
 /* note: prints function name for you */

Re: [PATCH] libata atapi work #2.1

[Pat LaVarre]

> > on the SCSI end of ATAPI ...
> > * issue REQUEST SENSE, to simulate an
> > auto-sensing scsi device
> ...
> I will have to discover how to auto sense only after a plain failure,
> not inappropriately after a timeout.
> ...
> I will try to create a desired inference of ATAPI op x03 "REQUEST SENSE"
> to immediately follow the failing command, after the pattern of the
> existing inference of ATA op xA1 "IDENTIFY".

Sorry, no good.

That won't work well til after someone finishes the "(TODO: investigate
using standard PIO-IN paths)" of ata_dev_identify of
drivers/scsi/libata-core.c to replace the "we hand-code it here" found
there now.

Also ATA folk might resist adding a for-them-unused { u8 sense[0x100]; }
member to the (struct ata_port), by way of analogy with the { u16
id[ATA_ID_WORDS]; } now found there in include/linux/libata.h.

Pat LaVarre

Re: [PATCH] libata atapi work #2.1

[Pat LaVarre]

Jeff G:

> > > on the SCSI end of ATAPI ...
> > > * issue REQUEST SENSE, to simulate an
> > > auto-sensing scsi device
> > ...
> > I will have to discover how to auto sense only after a plain failure,
> > not inappropriately after a timeout.

Is the following patch a miniscule step forward?

I guess it is because, in -bk5, same as before, I see the lack of auto
sense forces me to reboot before again I may modprobe:

kernel: atapi_packet_task: send cdb
kernel: ata_host_intr: BUS_NODATA (drv_stat 0x51)
kernel: atapi_autosense: FIXME: auto sense up to 0x12 of 0x60 @ 0xD2B33140
kernel: ata_scsi_error: ENTER
kernel: ata_eng_timeout: ENTER
/sbin/hotplug: no runnable /etc/hotplug/scsi_generic.agent is installed
kernel: ata2: BUG: timeout without command
kernel: ata_eng_timeout: EXIT
kernel: ata_scsi_error: EXIT
kernel: irq 18: nobody cared!
kernel:  [<c01081d7>] __report_bad_irq+0x2a/0x8b
...
kernel:
kernel: handlers:
kernel: [<df96750f>] (ata_interrupt+0x0/0x1b9 [libata])
kernel: Disabling IRQ #18

The "atapi_autosense: FIXME" message is new, it comes from this patch,
at I hope the right time.

In my newbie ignorance, at first I tried auto sensing in ata_scsi_error,
but the ata_qc_from_tag was null by then.

Next, the "irq 18: nobody cared!" provoked me to confirm that
ata_host_intr sees qc->tf.protocol = 5 = ATA_PROT_ATAPI (not
ATA_PROT_ATAPI_DMA) presumably because (cmd->sc_data_direction ==
SCSI_DATA_NONE).

Now I ask:

"Is the following patch a miniscule step forward?"

Pat LaVarre

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 14:40:01.000000000 -0600
@@ -2299,6 +2299,28 @@ struct ata_queued_cmd *ata_qc_new_init(s
 }
 
 /**
+ *	atapi_autosense -
+ *	@qc:
+ *	@drv_stat:
+ *	@cmd:
+ *
+ *	LOCKING:
+ *
+ */
+
+void atapi_autosense(struct ata_queued_cmd *qc, u8 drv_stat,
+		   struct scsi_cmnd *cmd)
+{
+	cmd->result = SAM_STAT_CHECK_CONDITION;
+	if (drv_stat & (ATA_BUSY | ATA_DRQ)) {
+		return;
+	}
+	DPRINTK("FIXME: auto sense up to 0x12 of 0x%X @ 0x%X\n",
+		(int) sizeof cmd->sense_buffer,
+		(int) &cmd->sense_buffer[0]);
+}
+
+/**
  *	ata_qc_complete -
  *	@qc:
  *	@drv_stat:
@@ -2322,7 +2344,7 @@ void ata_qc_complete(struct ata_queued_c
 	if (cmd) {
 		if (unlikely(drv_stat & (ATA_ERR | ATA_BUSY | ATA_DRQ))) {
 			if (is_atapi_taskfile(&qc->tf))
-				cmd->result = SAM_STAT_CHECK_CONDITION;
+				atapi_autosense(qc, drv_stat, cmd);
 			else
 				ata_to_sense_error(qc);
 		} else {