[Fwd: Re: PATCH: libata: add support for ATA_16 on ATAPI]

Jeff?
This has been pending for a very long time now.

This patch brings libata up to similar standards as Linux IDE with support for things like hdparm -I (identify) and -C (check power) among others on ATAPI drives.

Andrew, can you pick this up for -mm ??

???

-------- Original Message --------
Subject: Re: PATCH: libata: add support for ATA_16 on ATAPI
Date: Sun, 29 Apr 2007 16:46:15 +0900
From: Tejun Heo <[email protected]>
To: Mark Lord <[email protected]>
CC: Jeff Garzik <[email protected]>, Alan Cox <[email protected]>, IDE/ATA development list <[email protected]>
References: <[email protected]> <[email protected]>

diff -u --recursive --new-file --exclude-from=old/Documentation/dontdiff old/drivers/ata/libata-core.c new/drivers/ata/libata-core.c
@@ -82,6 +82,10 @@

module_param(atapi_dmadir, int, 0444);
MODULE_PARM_DESC(atapi_dmadir, "Enable ATAPI DMADIR bridge support (0=off, 1=on)");

+int ata16_passthru = 0;
+module_param(ata16_passthru, int, 0444);
+MODULE_PARM_DESC(ata16_passthru, "Enable passthru of SCSI opcode 0x85 to ATAPI devices (0=off, 1=on)"); +
int libata_fua = 0;
module_param_named(fua, libata_fua, int, 0444);
MODULE_PARM_DESC(fua, "FUA support (0=off, 1=on)");

diff -u --recursive --new-file --exclude-from=old/Documentation/dontdiff old/drivers/ata/libata-scsi.c new/drivers/ata/libata-scsi.c
@@ -2688,6 +2688,10 @@
 

static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd) {
+ if (dev->class == ATA_DEV_ATAPI)
+ if (cmd != ATA_16 || ata16_passthru)
+ return atapi_xlat;
+

	switch (cmd) {
	case READ_6:
	case READ_10:

@@ -2746,27 +2750,28 @@
void (*done)(struct scsi_cmnd *), struct ata_device *dev)
{
- int rc = 0;
-
- if (unlikely(!scmd->cmd_len || scmd->cmd_len > dev->cdb_len)) {
- DPRINTK("bad CDB len=%u, max=%u\n",
- scmd->cmd_len, dev->cdb_len);
+ ata_xlat_func_t xlat_func;
+ int rc = 0, max_len;
+ u8 scsi_op = scmd->cmnd[0];
+
+ if (scsi_op == ATA_16 && dev->class == ATA_DEV_ATAPI && !ata16_passthru)
+ max_len = 16;
+ else
+ max_len = dev->cdb_len;
+ + if (unlikely(!scmd->cmd_len || scmd->cmd_len > max_len)) {
+ DPRINTK("bad CDB len=%u, max=%u\n",

+			scmd->cmd_len, max_len);
		scmd->result = DID_ERROR << 16;
		done(scmd);
		return 0;
	}

- if (dev->class == ATA_DEV_ATA) {
- ata_xlat_func_t xlat_func = ata_get_xlat_func(dev,
- scmd->cmnd[0]);
-
- if (xlat_func)
- rc = ata_scsi_translate(dev, scmd, done, xlat_func);
- else
- ata_scsi_simulate(dev, scmd, done);
- } else
- rc = ata_scsi_translate(dev, scmd, done, atapi_xlat);
-
+ xlat_func = ata_get_xlat_func(dev, scsi_op);
+ if (xlat_func)
+ rc = ata_scsi_translate(dev, scmd, done, xlat_func);
+ else

+		ata_scsi_simulate(dev, scmd, done);
	return rc;
}

diff -u --recursive --new-file --exclude-from=old/Documentation/dontdiff old/drivers/ata/libata.h new/drivers/ata/libata.h
@@ -47,6 +47,7 @@

extern struct workqueue_struct *ata_aux_wq;
extern int atapi_enabled;
extern int atapi_dmadir;
+extern int ata16_passthru;
extern int libata_fua;
extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev); extern int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,

My reluctance stems mainly from the modifications to the hot path. ATA and ATAPI need wholly different command simulations, so I don't like your modification of sending both ATA and ATAPI devices to the same simulation function -- only to have to disambiguate with an ATA-or-ATAPI test.

I would rather keep the branch in the queuecommand hook the same. That should keep the ATA device callpath unchanged, while enabling what you want on the ATAPI callpath.

Jeff

Sure thing. Just elaborate a teensy bit more on what's going on inside Jeff's mind, and I'll code something up to mimic.

Thanks.