[PATCH UPDATED] libata: add support for ATA_16 on ATAPI

2007-08-01 Thread Tejun Heo
From: Mark Lord <[EMAIL PROTECTED]>

Add support for issuing ATA_16 passthru commands to ATAPI devices
managed by libata.  It requires the previous CDB length fix patch.

A boot/module parameter, "atapi_scmd85=1" can be used to globally
disable this feature, if ever desired.

tj: renamed ata16_passthru module param to atapi_scmd85 to reduce
confusion

tj: restructured __ata_scsi_queuecmd() according to Jeff's suggestion.

Signed-off-by: Mark Lord <[EMAIL PROTECTED]>
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
Jeff, Mark, are you guys okay with the modified version?

Thanks.

 drivers/ata/libata-core.c |4 +++
 drivers/ata/libata-scsi.c |   52 +++---
 drivers/ata/libata.h  |1 
 3 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 6001aae..d8c6789 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -86,6 +86,10 @@ int atapi_dmadir = 0;
 module_param(atapi_dmadir, int, 0444);
 MODULE_PARM_DESC(atapi_dmadir, "Enable ATAPI DMADIR bridge support (0=off, 
1=on)");
 
+int atapi_scmd85 = 0;
+module_param(atapi_scmd85, int, 0444);
+MODULE_PARM_DESC(atapi_scmd85, "Enable relay of SCSI opcode 0x85 and disable 
ATA_16 passthrough 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 --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 12ac0b5..568180b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2746,28 +2746,48 @@ static inline int __ata_scsi_queuecmd(struct scsi_cmnd 
*scmd,
  void (*done)(struct scsi_cmnd *),
  struct ata_device *dev)
 {
+   u8 scsi_op = scmd->cmnd[0];
+   ata_xlat_func_t xlat_func;
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);
-   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 (unlikely(!scmd->cmd_len || scmd->cmd_len > dev->cdb_len))
+   goto bad_cdb_len;
 
-   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);
+   } else {
+   if (unlikely(!scmd->cmd_len))
+   goto bad_cdb_len;
+
+   xlat_func = NULL;
+   if (likely((scsi_op != ATA_16) || atapi_scmd85)) {
+   /* relay SCSI command to ATAPI device */
+   if (unlikely(scmd->cmd_len > dev->cdb_len))
+   goto bad_cdb_len;
+
+   xlat_func = atapi_xlat;
+   } else {
+   /* ATA_16 passthru, treat as an ATA command */
+   if (unlikely(scmd->cmd_len > 16))
+   goto bad_cdb_len;
+
+   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;
+
+ bad_cdb_len:
+   DPRINTK("bad CDB len=%u, scsi_op=0x%02x, max=%u\n",
+   scmd->cmd_len, scsi_op, dev->cdb_len);
+   scmd->result = DID_ERROR << 16;
+   done(scmd);
+   return 0;
 }
 
 /**
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 564cd23..59a71fc 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -56,6 +56,7 @@ extern unsigned int ata_print_id;
 extern struct workqueue_struct *ata_aux_wq;
 extern int atapi_enabled;
 extern int atapi_dmadir;
+extern int atapi_scmd85;
 extern int libata_fua;
 extern int libata_noacpi;
 extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev);
diff --git a/include/linux/libata.h b/include/linux/libata.h
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2007-08-01 Thread Tejun Heo
Jeff Garzik wrote:
> Tejun Heo wrote:
>> From: Mark Lord <[EMAIL PROTECTED]>
>>
>> Add support for issuing ATA_16 passthru commands to ATAPI devices
>> managed by libata.  It requires the previous CDB length fix patch.
>>
>> A boot/module parameter, "atapi_scmd85=1" can be used to globally
>> disable this feature, if ever desired.
>>
>> tj: renamed ata16_passthru module param to atapi_scmd85 to reduce
>> confusion
>>
>> tj: restructured __ata_scsi_queuecmd() according to Jeff's suggestion.
>>
>> Signed-off-by: Mark Lord <[EMAIL PROTECTED]>
>> Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
>> ---
>> Jeff, Mark, are you guys okay with the modified version?
> 
> Close!  Thanks for revising!
> 
> My only comment now is that I dislike atapi_scmd85.  That means nothing
> to me.
> 
> I liked the old name better.  Or maybe use atapi_passthru16.

The problem with ata16_passthru is that it suggests the opposite of what
it does.  The default value 0 allows ATA_16 passthrough command while
setting it to 1 disallows ATA_16 passthrough and passes through SCSI
Command 0x85 which shares command byte with ATA_16.  Maybe it's because
I'm not a native speaker but the last sentence is pretty difficult to
digest.  So, IMHO, atapi_scmd85 is slightly better in that it means
nothing rather than suggesting the opposite.

How about keeping the old name (or use atapi_passthru16) but flipping
the meaning so that it defaults to 1.  It just makes hell lot of sense
that "ata16_passthru=1" means allowing ATA_16 passthrough not the other
way around.

Thanks.

-- 
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2007-08-01 Thread Jeff Garzik

Tejun Heo wrote:

From: Mark Lord <[EMAIL PROTECTED]>

Add support for issuing ATA_16 passthru commands to ATAPI devices
managed by libata.  It requires the previous CDB length fix patch.

A boot/module parameter, "atapi_scmd85=1" can be used to globally
disable this feature, if ever desired.

tj: renamed ata16_passthru module param to atapi_scmd85 to reduce
confusion

tj: restructured __ata_scsi_queuecmd() according to Jeff's suggestion.

Signed-off-by: Mark Lord <[EMAIL PROTECTED]>
Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>
---
Jeff, Mark, are you guys okay with the modified version?


Close!  Thanks for revising!

My only comment now is that I dislike atapi_scmd85.  That means nothing 
to me.


I liked the old name better.  Or maybe use atapi_passthru16.

Jeff





-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2007-08-02 Thread Mark Lord

Tejun Heo wrote:

Jeff Garzik wrote:

Tejun Heo wrote:

Jeff, Mark, are you guys okay with the modified version?

Close!  Thanks for revising!

My only comment now is that I dislike atapi_scmd85.  That means nothing
to me.

I liked the old name better.  Or maybe use atapi_passthru16.


The problem with ata16_passthru is that it suggests the opposite of what
it does.  The default value 0 allows ATA_16 passthrough command while
setting it to 1 disallows ATA_16 passthrough and passes through SCSI
Command 0x85 which shares command byte with ATA_16.  Maybe it's because
I'm not a native speaker but the last sentence is pretty difficult to
digest.  So, IMHO, atapi_scmd85 is slightly better in that it means
nothing rather than suggesting the opposite.


Thanks for plugging away at this one, Tejun.

And I do agree that ata16_passthru sounds backwards of what it really does.
There's got to be a nicer name for this thing.

In practice, I doubt that anyone will ever use the parameter,
so it really doesn't matter a whole lot what we call it.

But something sensible would be nice.
There's got to be a shorter version of "atapi_pass_opcode_0x85_to_device=1".

Perhaps "atapi_op_85_passthru=1" or "atapi_passthru_op_85=1" ??

Also, this is an example of something that is really a "per device"
parameter, rather than a global.  But we haven't yet cracked a good way
to do that yet in libata.

Cheers
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html