[PATCH] libata: add ATAPI module option

[PATCH] libata: add ATAPI module option

[Jeff Garzik]

Though DMA alignment, CDB interrupt, DMADIR, and PIO support issues
keep libata's ATAPI support turned off by default, as of 2.6.13-git1
PATA users with non-ancient CDROM and DVD drives can start testing
the ATAPI code.

I just checked the following patch into the 'upstream' branch of
libata-dev.git, to be sent to Linus in a few days.

To emphasize, however: DO NOT use libata ATAPI in a production setting.



 [libata] allow ATAPI to be enabled with new atapi_enabled module option
    
 ATAPI is getting close to being ready.  To increase exposure, we enable
 the code in the upstream kernel, but default it to off (present
 behavior).  Users must pass atapi_enabled=1 as a module option (if
 module) or on the kernel command line (if built in) to turn on
 discovery of their ATAPI devices.

diff --git a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c
+++ b/drivers/scsi/libata-core.c
@@ -75,6 +75,10 @@ static void __ata_qc_complete(struct ata
 static unsigned int ata_unique_id = 1;
 static struct workqueue_struct *ata_wq;
 
+int atapi_enabled = 0;
+module_param(atapi_enabled, int, 0444);
+MODULE_PARM_DESC(atapi_enabled, "Enable discovery of ATAPI devices (0=off, 1=on)");
+
 MODULE_AUTHOR("Jeff Garzik");
 MODULE_DESCRIPTION("Library module for ATA devices");
 MODULE_LICENSE("GPL");
diff --git a/drivers/scsi/libata-scsi.c b/drivers/scsi/libata-scsi.c
--- a/drivers/scsi/libata-scsi.c
+++ b/drivers/scsi/libata-scsi.c
@@ -1470,10 +1470,10 @@ ata_scsi_find_dev(struct ata_port *ap, s
 	if (unlikely(!ata_dev_present(dev)))
 		return NULL;
 
-#ifndef ATA_ENABLE_ATAPI
-	if (unlikely(dev->class == ATA_DEV_ATAPI))
-		return NULL;
-#endif
+	if (atapi_enabled) {
+		if (unlikely(dev->class == ATA_DEV_ATAPI))
+			return NULL;
+	}
 
 	return dev;
 }
diff --git a/drivers/scsi/libata.h b/drivers/scsi/libata.h
--- a/drivers/scsi/libata.h
+++ b/drivers/scsi/libata.h
@@ -38,6 +38,7 @@ struct ata_scsi_args {
 };
 
 /* libata-core.c */
+extern int atapi_enabled;
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_port *ap,
 				      struct ata_device *dev);
 extern void ata_qc_free(struct ata_queued_cmd *qc);
diff --git a/include/linux/libata.h b/include/linux/libata.h
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -40,7 +40,6 @@
 #undef 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 */
 #undef ATA_ENABLE_PATA		/* define to enable PATA support in some
 				 * low-level drivers */
 #undef ATAPI_ENABLE_DMADIR	/* enables ATAPI DMADIR bridge support */

Re: [PATCH] libata: add ATAPI module option

[Mark Lord]

Jeff Garzik wrote:
>
> -#ifndef ATA_ENABLE_ATAPI
> -	if (unlikely(dev->class == ATA_DEV_ATAPI))
> -		return NULL;
> -#endif
> +	if (atapi_enabled) {
> +		if (unlikely(dev->class == ATA_DEV_ATAPI))
> +			return NULL;
> +	}
..

Is that if-stmt the right way around?
At first glance, I'd expect it to read:

      if (!atapi_enabled) {
      ...

Cheers!

Re: [PATCH] libata: add ATAPI module option

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
> 
>>
>> -#ifndef ATA_ENABLE_ATAPI
>> -    if (unlikely(dev->class == ATA_DEV_ATAPI))
>> -        return NULL;
>> -#endif
>> +    if (atapi_enabled) {
>> +        if (unlikely(dev->class == ATA_DEV_ATAPI))
>> +            return NULL;
>> +    }
> 
> ..
> 
> Is that if-stmt the right way around?
> At first glance, I'd expect it to read:
> 
>      if (!atapi_enabled) {
>      ...
> 
> Cheers!

Indeed, thanks, fixed.

	Jeff

Re: [PATCH] libata: add ATAPI module option

[Mark Lord]

Jeff Garzik wrote:
>
>  ATAPI is getting close to being ready.  To increase exposure, we enable
>  the code in the upstream kernel, but default it to off (present
>  behavior).  Users must pass atapi_enabled=1 as a module option (if
>  module) or on the kernel command line (if built in) to turn on
>  discovery of their ATAPI devices.

Okay, I give.  How do I specify this option for the case
of having libata + scsi BUILT-IN (no modules).

I tried the usual /boot/grub/menu.lst thing:

    kernel /boot/vmlinuz-2.6.14-rc5 root=/dev/sda5 ro atapi_enabled=1

But still no ATAPI drive.   ???

Hardcoding the modparm to "1" instead of "0" in libata-core.c works though.

Cheers
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

Re: [PATCH] libata: add ATAPI module option

[Jeff Garzik]

Mark Lord wrote:
> Jeff Garzik wrote:
> 
>>
>>  ATAPI is getting close to being ready.  To increase exposure, we enable
>>  the code in the upstream kernel, but default it to off (present
>>  behavior).  Users must pass atapi_enabled=1 as a module option (if
>>  module) or on the kernel command line (if built in) to turn on
>>  discovery of their ATAPI devices.
> 
> 
> Okay, I give.  How do I specify this option for the case
> of having libata + scsi BUILT-IN (no modules).
> 
> I tried the usual /boot/grub/menu.lst thing:
> 
>    kernel /boot/vmlinuz-2.6.14-rc5 root=/dev/sda5 ro atapi_enabled=1
> 
> But still no ATAPI drive.   ???
> 
> Hardcoding the modparm to "1" instead of "0" in libata-core.c works though.

For builtin, I think the format is <module name>.parameter, thus

	libata.atapi_enabled=1

should work.

	Jeff

Re: [PATCH] libata: add ATAPI module option

[Mark Lord]

Jeff Garzik wrote:
>
>     libata.atapi_enabled=1

Ahhh.. should have known.  Thanks.
-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com