diff mbox series

[v4,8/8] ata: libata-scsi: Make ata_scsi_qc_complete() more readable

Message ID 20240701195758.1045917-9-ipylypiv@google.com
State New
Headers show
Series ATA PASS-THROUGH sense data fixes | expand

Commit Message

Igor Pylypiv July 1, 2024, 7:57 p.m. UTC 
The ATA PASS-THROUGH handling logic in ata_scsi_qc_complete() is hard
to read/understand. Improve the readability of the code by moving checks
into self-explanatory boolean variables.

Additionally, always set SAM_STAT_CHECK_CONDITION when CK_COND=1 because
SAT specification mandates that SATL shall return CHECK CONDITION if
the CK_COND bit is set.

Co-developed-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/ata/libata-scsi.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

Comments

Niklas Cassel July 1, 2024, 9:15 p.m. UTC | #1 
On Mon, Jul 01, 2024 at 07:57:58PM +0000, Igor Pylypiv wrote:
> The ATA PASS-THROUGH handling logic in ata_scsi_qc_complete() is hard
> to read/understand. Improve the readability of the code by moving checks
> into self-explanatory boolean variables.
> 
> Additionally, always set SAM_STAT_CHECK_CONDITION when CK_COND=1 because
> SAT specification mandates that SATL shall return CHECK CONDITION if
> the CK_COND bit is set.
> 
> Co-developed-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/ata/libata-scsi.c | 21 +++++++++++----------
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a66c177b6087..8f21b3b0bc75 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1659,26 +1659,27 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  {
>  	struct scsi_cmnd *cmd = qc->scsicmd;
>  	u8 *cdb = cmd->cmnd;
> -	int need_sense = (qc->err_mask != 0) &&
> -		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
> -	int need_passthru_sense = (qc->err_mask != 0) ||
> -		(qc->flags & ATA_QCFLAG_SENSE_VALID);
> +	bool have_sense = qc->flags & ATA_QCFLAG_SENSE_VALID;
> +	bool is_ata_passthru = cdb[0] == ATA_16 || cdb[0] == ATA_12;
> +	bool is_ck_cond_request = cdb[2] & 0x20;
> +	bool is_error = qc->err_mask != 0;
>  
>  	/* For ATA pass thru (SAT) commands, generate a sense block if
>  	 * user mandated it or if there's an error.  Note that if we
> -	 * generate because the user forced us to [CK_COND =1], a check
> +	 * generate because the user forced us to [CK_COND=1], a check
>  	 * condition is generated and the ATA register values are returned
>  	 * whether the command completed successfully or not. If there
> -	 * was no error, we use the following sense data:
> +	 * was no error, and CK_COND=1, we use the following sense data:
>  	 * sk = RECOVERED ERROR
>  	 * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
>  	 */
> -	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> -	    ((cdb[2] & 0x20) || need_passthru_sense)) {
> -		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> +	if (is_ata_passthru && (is_ck_cond_request || is_error || have_sense)) {
> +		if (!have_sense)
>  			ata_gen_passthru_sense(qc);
>  		ata_scsi_set_passthru_sense_fields(qc);
> -	} else if (need_sense) {
> +		if (is_ck_cond_request)
> +			set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
> +	} else if (is_error && !have_sense) {
>  		ata_gen_ata_sense(qc);
>  	} else {
>  		/* Keep the SCSI ML and status byte, clear host byte. */
> -- 
> 2.45.2.803.g4e1b14247a-goog
> 

Reviewed-by: Niklas Cassel <cassel@kernel.org>

However: I really think that this patch should be squashed with patch 2/8.

Sure, the changes in this patch will make it harder to backport...
but, even patch 2/8 will be a pain to backport...

And this patch will need to have CC: stable and be backported as well...
(such that we always set CHECK_CONDITION when CK_COND=1), so I strongly
suggest that we should squash these, since it will probably be way simpler
to backport the patch that is "patch 2/8 squashed with this patch",
compared to backporting patch 2/8, and then backporting this patch.
(That would just give two patches that will need manual backport, rather
than one patch that needs manual backport.)

Both of these are fixing incorrect sense data for ATA passthough commands
anyway.


Kind regards,
Niklas
Igor Pylypiv July 2, 2024, 2:50 a.m. UTC | #2 
On Mon, Jul 01, 2024 at 11:15:54PM +0200, Niklas Cassel wrote:
> On Mon, Jul 01, 2024 at 07:57:58PM +0000, Igor Pylypiv wrote:
> > The ATA PASS-THROUGH handling logic in ata_scsi_qc_complete() is hard
> > to read/understand. Improve the readability of the code by moving checks
> > into self-explanatory boolean variables.
> > 
> > Additionally, always set SAM_STAT_CHECK_CONDITION when CK_COND=1 because
> > SAT specification mandates that SATL shall return CHECK CONDITION if
> > the CK_COND bit is set.
> > 
> > Co-developed-by: Niklas Cassel <cassel@kernel.org>
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/ata/libata-scsi.c | 21 +++++++++++----------
> >  1 file changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index a66c177b6087..8f21b3b0bc75 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -1659,26 +1659,27 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  {
> >  	struct scsi_cmnd *cmd = qc->scsicmd;
> >  	u8 *cdb = cmd->cmnd;
> > -	int need_sense = (qc->err_mask != 0) &&
> > -		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
> > -	int need_passthru_sense = (qc->err_mask != 0) ||
> > -		(qc->flags & ATA_QCFLAG_SENSE_VALID);
> > +	bool have_sense = qc->flags & ATA_QCFLAG_SENSE_VALID;
> > +	bool is_ata_passthru = cdb[0] == ATA_16 || cdb[0] == ATA_12;
> > +	bool is_ck_cond_request = cdb[2] & 0x20;
> > +	bool is_error = qc->err_mask != 0;
> >  
> >  	/* For ATA pass thru (SAT) commands, generate a sense block if
> >  	 * user mandated it or if there's an error.  Note that if we
> > -	 * generate because the user forced us to [CK_COND =1], a check
> > +	 * generate because the user forced us to [CK_COND=1], a check
> >  	 * condition is generated and the ATA register values are returned
> >  	 * whether the command completed successfully or not. If there
> > -	 * was no error, we use the following sense data:
> > +	 * was no error, and CK_COND=1, we use the following sense data:
> >  	 * sk = RECOVERED ERROR
> >  	 * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
> >  	 */
> > -	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> > -	    ((cdb[2] & 0x20) || need_passthru_sense)) {
> > -		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> > +	if (is_ata_passthru && (is_ck_cond_request || is_error || have_sense)) {
> > +		if (!have_sense)
> >  			ata_gen_passthru_sense(qc);
> >  		ata_scsi_set_passthru_sense_fields(qc);
> > -	} else if (need_sense) {
> > +		if (is_ck_cond_request)
> > +			set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
> > +	} else if (is_error && !have_sense) {
> >  		ata_gen_ata_sense(qc);
> >  	} else {
> >  		/* Keep the SCSI ML and status byte, clear host byte. */
> > -- 
> > 2.45.2.803.g4e1b14247a-goog
> > 
> 
> Reviewed-by: Niklas Cassel <cassel@kernel.org>
> 
> However: I really think that this patch should be squashed with patch 2/8.
> 
> Sure, the changes in this patch will make it harder to backport...
> but, even patch 2/8 will be a pain to backport...
> 
> And this patch will need to have CC: stable and be backported as well...
> (such that we always set CHECK_CONDITION when CK_COND=1), so I strongly
> suggest that we should squash these, since it will probably be way simpler
> to backport the patch that is "patch 2/8 squashed with this patch",
> compared to backporting patch 2/8, and then backporting this patch.
> (That would just give two patches that will need manual backport, rather
> than one patch that needs manual backport.)
> 
> Both of these are fixing incorrect sense data for ATA passthough commands
> anyway.

Agreed, it makes more sense to squash. Squashed the patches in v5.

I really appreciate your thorough reviews and feedback, Niklas! Thank you!

Best,
Igor
> 
> 
> Kind regards,
> Niklas
diff mbox series

Patch

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index a66c177b6087..8f21b3b0bc75 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1659,26 +1659,27 @@  static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct scsi_cmnd *cmd = qc->scsicmd;
 	u8 *cdb = cmd->cmnd;
-	int need_sense = (qc->err_mask != 0) &&
-		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
-	int need_passthru_sense = (qc->err_mask != 0) ||
-		(qc->flags & ATA_QCFLAG_SENSE_VALID);
+	bool have_sense = qc->flags & ATA_QCFLAG_SENSE_VALID;
+	bool is_ata_passthru = cdb[0] == ATA_16 || cdb[0] == ATA_12;
+	bool is_ck_cond_request = cdb[2] & 0x20;
+	bool is_error = qc->err_mask != 0;
 
 	/* For ATA pass thru (SAT) commands, generate a sense block if
 	 * user mandated it or if there's an error.  Note that if we
-	 * generate because the user forced us to [CK_COND =1], a check
+	 * generate because the user forced us to [CK_COND=1], a check
 	 * condition is generated and the ATA register values are returned
 	 * whether the command completed successfully or not. If there
-	 * was no error, we use the following sense data:
+	 * was no error, and CK_COND=1, we use the following sense data:
 	 * sk = RECOVERED ERROR
 	 * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
 	 */
-	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
-	    ((cdb[2] & 0x20) || need_passthru_sense)) {
-		if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
+	if (is_ata_passthru && (is_ck_cond_request || is_error || have_sense)) {
+		if (!have_sense)
 			ata_gen_passthru_sense(qc);
 		ata_scsi_set_passthru_sense_fields(qc);
-	} else if (need_sense) {
+		if (is_ck_cond_request)
+			set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
+	} else if (is_error && !have_sense) {
 		ata_gen_ata_sense(qc);
 	} else {
 		/* Keep the SCSI ML and status byte, clear host byte. */