ata: libata-scsi: do not raise UA for storage element depopulation and restoration

Currently, the translation of the trnalated ATA commands REMOVE ELEMENT AND TRUNCATE and RESTORE ELEMENTS AND REBUILD are completed using the function ata_scsi_depop_ua_cap_changed_complete(). This completion function raises a UNIT ATTENTION with the additional sense code CAPACITY DATA CHANGED. But doing so, the scsi layer seeing the UNIT ATTENTION sense key ends up failing the command, even if the command result is in fact OK.

The SAT specifications do provide more details about the capacity change should be notified, and that relies on the ACCESSIBLE CAPACITY field of the IDENTIFY DATA retrieved before or after the command is issued, and then raising a UNIT ATTENTION if the capacity has really changed. However, we do not have any simple mean to raise a unit attention from libata-scsi. So rather than seeing the REMOVE ELEMENT AND TRUNCATE and RESTORE ELEMENTS AND REBUILD commands failing, remove the function ata_scsi_depop_ua_cap_changed_complete() and rely on the regular completion callback. Since for now these commands can only be issued as passthrough commands, the user is responsible for revalidating the device capacity after executing these commands.

---
 drivers/ata/libata-scsi.c | 24 ------------------------
 1 file changed, 24 deletions(-)

index a7d667cfcfec..b3666519b648 100644
@@ -4823,28 +4823,6 @@ ata_scsi_get_phys_element_status_xlat(struct ata_queued_cmd *qc)
 	return 0;
 }
 
-static void ata_scsi_depop_ua_cap_changed_complete(struct ata_queued_cmd *qc)
-{
-	struct scsi_cmnd *scmd = qc->scsicmd;
-	u8 *cdb = scmd->cmnd;
-	bool is_ata_passthru = cdb[0] == ATA_16 || cdb[0] == ATA_12;
-	bool is_success = qc->err_mask == 0;
-
-	/*
-	 * For successful non-passthrough commands, raise a UNIT ATTENTION with
-	 * the additional sense code set to CAPACITY DATA HAS CHANGED to be
-	 * raised. Note that this should be done only if the capacity has
-	 * actually changed, which may not be the case if the element that was
-	 * specified for depopulation was already depopulated, or we did not
-	 * restore any removed element. But a capacity change unit attention is
-	 * harmless, so always raise the unit attention.
-	 */
-	if (is_success && !is_ata_passthru)
-		ata_scsi_set_sense(qc->dev, scmd, UNIT_ATTENTION,
-				   UA_CHANGED_ASC, CAPACITY_CHANGED_ASCQ);
-	ata_scsi_qc_complete(qc);
-}
-
 static unsigned int
 ata_scsi_remove_element_and_truncate_xlat(struct ata_queued_cmd *qc)
 {

@@ -4884,7 +4862,6 @@ ata_scsi_remove_element_and_truncate_xlat(struct ata_queued_cmd *qc)
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48;
 
 	qc->flags |= ATA_QCFLAG_RESULT_TF;
-	qc->complete_fn = ata_scsi_depop_ua_cap_changed_complete;
 
 	return 0;
 }

@@ -4937,7 +4914,6 @@ ata_scsi_restore_elements_and_rebuild_xlat(struct ata_queued_cmd *qc)
 	tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE | ATA_TFLAG_LBA48;
 
 	qc->flags |= ATA_QCFLAG_RESULT_TF;
-	qc->complete_fn = ata_scsi_depop_ua_cap_changed_complete;
 
 	return 0;
 }

2.55.0

Nit: trnalated / translated

I also think that we can drop "Currently, " as it provides no value.

Then it becomes:
"
The translation of the translated ATA commands REMOVE ELEMENT AND TRUNCATE and RESTORE ELEMENTS AND REBUILD are completed using the "

It sounds a bit weird to repeat translated twice. Perhaps something like:
"
The libata command completion for the ATA commands 'REMOVE ELEMENT AND TRUNCATE' and 'RESTORE ELEMENTS AND REBUILD' is handled using the "

No need to send a new patch, just tell me and I can fixup when applying.

Kind regards,
Niklas

Oops. I really should better re-read what I write :) Yes, please fix it up as above, it sounds way better. Thanks.

Damien Le Moal
Western Digital Research

I fixed up the commit message as suggested in the thread and also s/CAPACITY DATA CHANGED/CAPACITY DATA HAS CHANGED/

Applied to libata/linux.git (for-7.3-fixes), thanks!

[1/1] ata: libata-scsi: do not raise UA for storage element depopulation and restoration

      https://git.kernel.org/libata/linux/c/6d81700a

Kind regards,
Niklas