Patchwork Do not send RW commands to locked disks.

login
register
mail settings
Submitter Gwendal Grignou
Date Sept. 2, 2009, 12:28 a.m.
Message ID <1251851285-9159-1-git-send-email-gwendal@google.com>
Download mbox | patch
Permalink /patch/32778/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Gwendal Grignou - Sept. 2, 2009, 12:28 a.m.
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(-)
Tejun Heo - Sept. 2, 2009, 1:39 a.m.
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.
Gwendal Grignou - Sept. 2, 2009, 9:43 p.m.
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Sept. 2, 2009, 11:03 p.m.
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.

Patch

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)
 {