Re: [PATCH] Re: 2.6.19.1, sata_sil: sata dvd writer doesn't work

[Tejun Heo <htejun@gmail.com>]

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

Hello, Daniel.

Daniel Beichl wrote:
>> Okay, I've been thinking about it for some time.  I think I know what's
>> going on now.  Those authentication related commands are issued by
>> invoking ioctls on the device node.  The cdrom driver receives the
>> ioctls and issues respective SCSI commands for it.  All those DVD auth
>> commands either transfer or receive data from the device and thus
>> specifies certain data direction when they're issued.  libata selects
>> command protocol accordingly.
>>
>> It's all good and dandy till now but the problem is that those commands
>> can be issued with data length of zero!  This is allowed by the SCSI MMC
>> standard and used to probe whether the command succeeds or not before
>> taking further actions.  When this happens, libata chooses command
>> protocol according to the data direction, but has to feed 0-length data
>> to the DMA engine.  It seems sata_sil dma engine barfs if that happens.
>>  Can you please add the following code snippet at the head of
>> atapi_xlat()?
>>
>>     if (!nodata && !qc->nbytes) {
>>         printk("XXX forcing PIO for 0 length data cdb %02x\n",
>>             scmd->cmnd[0]);
>>         dump_stack();
>>         using_pio = 1;
>>     }
>>
>> Also, do you mind cc'ing linux-ide@vger.kernel.org?
>>
>>   
> Hi Tejun,
> 
> 
> i inserted the code snippet you mentioned to atapi_xlat() in libata-scsi.c
> and the dvd authentication succeeds.
> 
> Please see the attached dmesg file for the generated stack dumps.

Good to know it works.  :-)

> The question is whether this is the right place to fix it, as this seems
> to limit all
> drivers to pio if a zero data length command is transferred.
> It would make more sense to me if the individual driver decides this by
> performing a similar check in the check_atapi_dma function implemented
> by the
> individual driver. I did this for the sata_sil driver in the attached
> patch.

The thing is that those commands don't transfer any data at all, so it
doesn't really matter whether the data phase is specified as DMA or PIO.
 ATA_PROT_ATAPI degenerates into ATA_PROT_ATAPI_NODATA if there is no
data to transfer (in ATA spec they share the same protocol state machine
and the data request status bit dictates what actually happens).

I think this actually should be fixed in the cdrom driver.  It shouldn't
issue data command with 0 data length with data protocol.  Can you test
whether the attached patch works?  Also, it seems we'll need to add a
WARN_ON() in sr such that bugs like this can be caught more easily.

Thanks.

-- 
tejun

[-- Attachment #2: DATA_NONE-if-len-zero.patch --]
[-- Type: text/x-patch, Size: 511 bytes --]

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index b36f44d..5a5639d 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1533,7 +1533,10 @@ void init_cdrom_command(struct packet_co
 		memset(buf, 0, len);
 	cgc->buffer = (char *) buf;
 	cgc->buflen = len;
-	cgc->data_direction = type;
+	if (len)
+		cgc->data_direction = type;
+	else
+		cgc->data_direction = CGC_DATA_NONE;
 	cgc->timeout = CDROM_DEF_TIMEOUT;
 }