| Message ID | 1251851285-9159-1-git-send-email-gwendal@google.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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.
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
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.
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) {
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(-)