* [PATCH] Do not send RW commands to locked disks.
@ 2009-09-02 0:28 Gwendal Grignou
2009-09-02 1:39 ` Tejun Heo
0 siblings, 1 reply; 4+ messages in thread
From: Gwendal Grignou @ 2009-09-02 0:28 UTC (permalink / raw)
To: tj; +Cc: linux-ide, Gwendal Grignou
Send a Read/Write command to a locked disk triggers the error handler.
Given the disk returns a generic device error code, the error handler can come
to the conclusion of reducing the link speed, which is bad.
Also, if drives are still locked at boot, this fix speeds up the boot process
by returning errors without invoquing the error handler thread.
Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
drivers/ata/libata-core.c | 2 ++
drivers/ata/libata-scsi.c | 6 ++++++
include/linux/ata.h | 3 +++
3 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 072ba5e..3f80570 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5023,6 +5023,8 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
qc->tf.feature != SETFEATURES_WC_OFF)
break;
/* fall through */
+ case ATA_CMD_SEC_UNLOCK: /* Read/Write access now work */
+ case ATA_CMD_SEC_ERASE_UNIT: /* Read/Write access now work */
case ATA_CMD_INIT_DEV_PARAMS: /* CHS translation changed */
case ATA_CMD_SET_MULTI: /* multi_count changed */
/* revalidate device */
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index d0dfeef..8a41767 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1658,6 +1658,12 @@ static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc)
u32 n_block;
int rc;
+ if (unlikely(ata_id_locked(qc->dev->id))) {
+ /* Terminate RW commands early when the disk is locked */
+ ata_scsi_set_sense(scmd, ABORTED_COMMAND, 0, 0);
+ return 1;
+ }
+
if (cdb[0] == WRITE_10 || cdb[0] == WRITE_6 || cdb[0] == WRITE_16)
tf_flags |= ATA_TFLAG_WRITE;
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 9c75921..5a5642d 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -247,7 +247,9 @@ enum {
ATA_CMD_PMP_READ = 0xE4,
ATA_CMD_PMP_WRITE = 0xE8,
ATA_CMD_CONF_OVERLAY = 0xB1,
+ ATA_CMD_SEC_ERASE_UNIT = 0xF4,
ATA_CMD_SEC_FREEZE_LOCK = 0xF5,
+ ATA_CMD_SEC_UNLOCK = 0xF2,
ATA_CMD_SMART = 0xB0,
ATA_CMD_MEDIA_LOCK = 0xDE,
ATA_CMD_MEDIA_UNLOCK = 0xDF,
@@ -536,6 +538,7 @@ static inline int ata_is_data(u8 prot)
((u64) (id)[(n) + 0]) )
#define ata_id_cdb_intr(id) (((id)[ATA_ID_CONFIG] & 0x60) == 0x20)
+#define ata_id_locked(id) (((id)[ATA_ID_DLF] & 0x7) == 0x7)
static inline bool ata_id_has_hipm(const u16 *id)
{
--
1.5.4.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] Do not send RW commands to locked disks.
2009-09-02 0:28 [PATCH] Do not send RW commands to locked disks Gwendal Grignou
@ 2009-09-02 1:39 ` Tejun Heo
2009-09-02 21:43 ` Gwendal Grignou
0 siblings, 1 reply; 4+ messages in thread
From: Tejun Heo @ 2009-09-02 1:39 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: linux-ide
Gwendal Grignou wrote:
> Send a Read/Write command to a locked disk triggers the error handler.
> Given the disk returns a generic device error code, the error handler can come
> to the conclusion of reducing the link speed, which is bad.
> Also, if drives are still locked at boot, this fix speeds up the boot process
> by returning errors without invoquing the error handler thread.
>
> Signed-off-by: Gwendal Grignou <gwendal@google.com>
> ---
> drivers/ata/libata-core.c | 2 ++
> drivers/ata/libata-scsi.c | 6 ++++++
> include/linux/ata.h | 3 +++
> 3 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 072ba5e..3f80570 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5023,6 +5023,8 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
> qc->tf.feature != SETFEATURES_WC_OFF)
> break;
> /* fall through */
> + case ATA_CMD_SEC_UNLOCK: /* Read/Write access now work */
> + case ATA_CMD_SEC_ERASE_UNIT: /* Read/Write access now work */
> case ATA_CMD_INIT_DEV_PARAMS: /* CHS translation changed */
> case ATA_CMD_SET_MULTI: /* multi_count changed */
> /* revalidate device */
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d0dfeef..8a41767 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1658,6 +1658,12 @@ static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc)
> u32 n_block;
> int rc;
>
> + if (unlikely(ata_id_locked(qc->dev->id))) {
> + /* Terminate RW commands early when the disk is locked */
> + ata_scsi_set_sense(scmd, ABORTED_COMMAND, 0, 0);
> + return 1;
> + }
> +
> if (cdb[0] == WRITE_10 || cdb[0] == WRITE_6 || cdb[0] == WRITE_16)
> tf_flags |= ATA_TFLAG_WRITE;
Hmmm... how sure are we that there aren't some braindead devices which
incorrectly would trigger the ata_id_locked() condition? I think it
would be safer to have an extra DFLAG, say ATA_DFLAG_LOCKED which gets
set when EH detects unknown device error && ata_id_locked(); then, in
the issue path, we can do
if (ATA_DFLAG_LOCKED is set) {
if (ata_id_locked())
Terminate early;
else
Clear ATA_DFLAG_LOCKED;
}
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Do not send RW commands to locked disks.
2009-09-02 1:39 ` Tejun Heo
@ 2009-09-02 21:43 ` Gwendal Grignou
2009-09-02 23:03 ` Tejun Heo
0 siblings, 1 reply; 4+ messages in thread
From: Gwendal Grignou @ 2009-09-02 21:43 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-ide
You have a point, but setting the flag LOCKED when an error happen
will mean that a braindead device [which always trigger
ata_id_locked()] will work until an internal disk error happens; that
will make debugging difficult.
Instead, I am working on adding the provision for a blacklist to allow
bad devices to bypass the test.
Gwendal.
On Tue, Sep 1, 2009 at 6:39 PM, Tejun Heo<tj@kernel.org> wrote:
> Gwendal Grignou wrote:
>> Send a Read/Write command to a locked disk triggers the error handler.
>> Given the disk returns a generic device error code, the error handler can come
>> to the conclusion of reducing the link speed, which is bad.
>> Also, if drives are still locked at boot, this fix speeds up the boot process
>> by returning errors without invoquing the error handler thread.
>>
>> Signed-off-by: Gwendal Grignou <gwendal@google.com>
>> ---
>> drivers/ata/libata-core.c | 2 ++
>> drivers/ata/libata-scsi.c | 6 ++++++
>> include/linux/ata.h | 3 +++
>> 3 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 072ba5e..3f80570 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -5023,6 +5023,8 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
>> qc->tf.feature != SETFEATURES_WC_OFF)
>> break;
>> /* fall through */
>> + case ATA_CMD_SEC_UNLOCK: /* Read/Write access now work */
>> + case ATA_CMD_SEC_ERASE_UNIT: /* Read/Write access now work */
>> case ATA_CMD_INIT_DEV_PARAMS: /* CHS translation changed */
>> case ATA_CMD_SET_MULTI: /* multi_count changed */
>> /* revalidate device */
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index d0dfeef..8a41767 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -1658,6 +1658,12 @@ static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc)
>> u32 n_block;
>> int rc;
>>
>> + if (unlikely(ata_id_locked(qc->dev->id))) {
>> + /* Terminate RW commands early when the disk is locked */
>> + ata_scsi_set_sense(scmd, ABORTED_COMMAND, 0, 0);
>> + return 1;
>> + }
>> +
>> if (cdb[0] == WRITE_10 || cdb[0] == WRITE_6 || cdb[0] == WRITE_16)
>> tf_flags |= ATA_TFLAG_WRITE;
>
> Hmmm... how sure are we that there aren't some braindead devices which
> incorrectly would trigger the ata_id_locked() condition? I think it
> would be safer to have an extra DFLAG, say ATA_DFLAG_LOCKED which gets
> set when EH detects unknown device error && ata_id_locked(); then, in
> the issue path, we can do
>
> if (ATA_DFLAG_LOCKED is set) {
> if (ata_id_locked())
> Terminate early;
> else
> Clear ATA_DFLAG_LOCKED;
> }
>
> Thanks.
>
> --
> tejun
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Do not send RW commands to locked disks.
2009-09-02 21:43 ` Gwendal Grignou
@ 2009-09-02 23:03 ` Tejun Heo
0 siblings, 0 replies; 4+ messages in thread
From: Tejun Heo @ 2009-09-02 23:03 UTC (permalink / raw)
To: Gwendal Grignou; +Cc: linux-ide
Hello, Gwendal.
Gwendal Grignou wrote:
> You have a point, but setting the flag LOCKED when an error happen
> will mean that a braindead device [which always trigger
> ata_id_locked()] will work until an internal disk error happens; that
> will make debugging difficult.
Yeah, right, unrelated error can set the LOCKED bit. Maybe we can
devise something intelligent there. A simple state machine to detect
those conditions?
> Instead, I am working on adding the provision for a blacklist to allow
> bad devices to bypass the test.
Blacklist wouldn't be too bad too but I really think we shouldn't be
believing the identify data at the face value. What if the device
sets the bit after security_lock but fails to fail commands which
wouldn't be all that surprising given the general quality of ATA
devices which has been diving rapidly with all those new vendors
jumping onto the SDD wagon? So, I think it should still be something
recognized and verified by EH before applying.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-09-02 23:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-09-02 0:28 [PATCH] Do not send RW commands to locked disks Gwendal Grignou
2009-09-02 1:39 ` Tejun Heo
2009-09-02 21:43 ` Gwendal Grignou
2009-09-02 23:03 ` Tejun Heo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).