Re: Lost DVD-RW [Was Re: 2.6.18-rc5-mm1]

[Tejun Heo <htejun@gmail.com>]

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

J.A. Magallón wrote:
> libata version 2.00 loaded.
> ata_piix 0000:00:1f.1: version 2.00ac7
> ACPI: PCI Interrupt 0000:00:1f.1[A] -> GSI 18 (level, low) -> IRQ 16
> PCI: Setting latency timer of device 0000:00:1f.1 to 64
> ata1: PATA max UDMA/100 cmd 0x1F0 ctl 0x3F6 bmdma 0xF000 irq 14
> ata2: PATA max UDMA/100 cmd 0x170 ctl 0x376 bmdma 0xF008 irq 15
> scsi0 : ata_piix
> ata1.00: XXX class=3 is_ata=0 is_cfa=1
> ata1.00: failed to IDENTIFY (device reports illegal type, err_mask=0x0)

Magallón, does the attached patch fix the problem?

Alan, it seems that 0x848a indicates CFA device iff the ID data is from 
IDENTIFY DEVICE.  When the command is IDENTIFY PACKET DEVICE, 0x848a 
seems to indicate a valid ATAPI device.

 From ATA8-ACS, 7.17 is IDENTIFY DEVICE.

> 7.17.7.1 Word 0: General configuration
> 
> Devices that conform to this standard shall clear bit 15 to zero. If
> bit 7 is set to one, the device is a removable media device. Bit 6 is
> obsolete.
> 
> If bit 2 is set to one it indicates that the content of the IDENTIFY
> DEVICE data is incomplete. This will occur if the device supports the
> Power-up in Standby feature set and required data is contained on the
> device media. In this case the content of at least word 0 and word 2
> shall be valid.
> 
> Devices supporting the CFA feature set shall place the value 848Ah in
                                                                ^^^^^
> word 0. In this case, the above definitions for the bits in  word 0
> are not valid.

And, 7.18 is on IDENTIFY PACKET DEVICE.

> 7.18.6.2 Word 0: General configuration
> 
> Bits (15:14) of word 0 indicate the type of device. Bit 15 shall be
> set to one and bit 14 shall be cleared to zeroto indicate the device
> implements the PACKET Command feature set.
> 
> Bits (12:8) of word 0 indicate the command packet set implemented by
> the device. This value follows the peripheral device type value as
> defined in SCSI Primary Commands, ANSI INCITS 301:1997.
> 
> Bit 7 if set to one indicates that the device has removable media.
> 
> Bits (6:5) of word 0 indicate the DRQ response time when a PACKET
> command is received. A value of 00b indicates a maximum time of 3 ms
> from receipt of PACKET to the setting of DRQ to one. A value of 10b 
> indicates a maximum time of 50 μs from the receipt of PACKET to the
> setting of DRQ to one. The value 11b is reserved.
> 
> If bit 2 is set to one it indicates that the content of the IDENTIFY
> DEVICE data is incomplete. This will occur if the device supports the
> Power-up in Standby feature set and required data is contained on the
> device media. In this case the content of at least word 0 and word 2
> shall be valid.
> 
> Bits (1:0) of word 0 indicate the packet size the device supports. A
> value of 00b indicates that a 12-byte packet is supported; a value of
> 01b indicates a 16 byte packet. The values 10b and 11b are reserved.

So, when the output is from IDENTIFY PACKET DEVICE, 0x848a doesn't have 
any special meaning.  It indicates a valid write-once, removable media, 
ATAPI device with 16bytes CDB.

The attached patch makes sanity checking logic in ata_dev_read_id() 
check for CFA only if IDENTIFY DEVICE is used.

Thanks.

-- 
tejun

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

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
index 73dd6c8..427b73a 100644
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -1256,10 +1256,15 @@ int ata_dev_read_id(struct ata_device *d
 	swap_buf_le16(id, ATA_ID_WORDS);
 
 	/* sanity check */
-	if ((class == ATA_DEV_ATA) != (ata_id_is_ata(id) | ata_id_is_cfa(id))) {
-		rc = -EINVAL;
-		reason = "device reports illegal type";
-		goto err_out;
+	rc = -EINVAL;
+	reason = "device reports illegal type";
+
+	if (class == ATA_DEV_ATA) {
+		if (!ata_id_is_ata(id) && !ata_id_is_cfa(id))
+			goto err_out;
+	} else {
+		if (ata_id_is_ata(id))
+			goto err_out;
 	}
 
 	if (post_reset && class == ATA_DEV_ATA) {