Re: [PATCH] Do not send RW commands to locked disks.

[Tejun Heo <tj@kernel.org>]

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