diff mbox series

[v2,01/18] ata: libata: allow ata_scsi_set_sense() to not set CHECK_CONDITION

Message ID 20230112140412.667308-2-niklas.cassel@wdc.com (mailing list archive)
State Superseded
Headers show
Series Add Command Duration Limits support | expand

Commit Message

Niklas Cassel Jan. 12, 2023, 2:03 p.m. UTC 
Current ata_scsi_set_sense() calls scsi_build_sense(), which,
in addition to setting the sense data, unconditionally sets the
scsicmd->result to SAM_STAT_CHECK_CONDITION.

For Command Duration Limits policy 0xD:
The device shall complete the command without error (SAM_STAT_GOOD)
with the additional sense code set to DATA CURRENTLY UNAVAILABLE.

It is perfectly fine to have sense data for a command that returned
completion without error.

In order to support for CDL policy 0xD, we have to remove this
assumption that having sense data means that the command failed
(SAM_STAT_CHECK_CONDITION).

Add a new parameter to ata_scsi_set_sense() to allow us to set
sense data without unconditionally setting SAM_STAT_CHECK_CONDITION.
This new parameter will be used in a follow-up patch.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
---
 drivers/ata/libata-eh.c   |  3 ++-
 drivers/ata/libata-sata.c |  4 ++--
 drivers/ata/libata-scsi.c | 38 ++++++++++++++++++++------------------
 drivers/ata/libata.h      |  4 ++--
 4 files changed, 26 insertions(+), 23 deletions(-)

Comments

Hannes Reinecke Jan. 13, 2023, 7:49 a.m. UTC | #1 
On 1/12/23 15:03, Niklas Cassel wrote:
> Current ata_scsi_set_sense() calls scsi_build_sense(), which,
> in addition to setting the sense data, unconditionally sets the
> scsicmd->result to SAM_STAT_CHECK_CONDITION.
> 
> For Command Duration Limits policy 0xD:
> The device shall complete the command without error (SAM_STAT_GOOD)
> with the additional sense code set to DATA CURRENTLY UNAVAILABLE.
> 
> It is perfectly fine to have sense data for a command that returned
> completion without error.
> 
> In order to support for CDL policy 0xD, we have to remove this
> assumption that having sense data means that the command failed
> (SAM_STAT_CHECK_CONDITION).
> 
> Add a new parameter to ata_scsi_set_sense() to allow us to set
> sense data without unconditionally setting SAM_STAT_CHECK_CONDITION.
> This new parameter will be used in a follow-up patch.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
>   drivers/ata/libata-eh.c   |  3 ++-
>   drivers/ata/libata-sata.c |  4 ++--
>   drivers/ata/libata-scsi.c | 38 ++++++++++++++++++++------------------
>   drivers/ata/libata.h      |  4 ++--
>   4 files changed, 26 insertions(+), 23 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
Christoph Hellwig Jan. 17, 2023, 6:06 a.m. UTC | #2 
>  void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd,
> -			u8 sk, u8 asc, u8 ascq)
> +			bool check_condition, u8 sk, u8 asc, u8 ascq)
>  {
>  	bool d_sense = (dev->flags & ATA_DFLAG_D_SENSE);
>  
>  	if (!cmd)
>  		return;
>  
> -	scsi_build_sense(cmd, d_sense, sk, asc, ascq);
> +	scsi_build_sense_buffer(d_sense, cmd->sense_buffer, sk, asc, ascq);
> +	if (check_condition)
> +		set_status_byte(cmd, SAM_STAT_CHECK_CONDITION);
>  }

Adding a bool parameter do do something conditional at the end
of a function is always a bad idea.  Just split out a
__ata_scsi_set_sense helper that doesn't set the flag.
Damien Le Moal Jan. 17, 2023, 7:10 a.m. UTC | #3 
On 1/17/23 15:06, Christoph Hellwig wrote:
>>  void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd,
>> -			u8 sk, u8 asc, u8 ascq)
>> +			bool check_condition, u8 sk, u8 asc, u8 ascq)
>>  {
>>  	bool d_sense = (dev->flags & ATA_DFLAG_D_SENSE);
>>  
>>  	if (!cmd)
>>  		return;
>>  
>> -	scsi_build_sense(cmd, d_sense, sk, asc, ascq);
>> +	scsi_build_sense_buffer(d_sense, cmd->sense_buffer, sk, asc, ascq);
>> +	if (check_condition)
>> +		set_status_byte(cmd, SAM_STAT_CHECK_CONDITION);
>>  }
> 
> Adding a bool parameter do do something conditional at the end
> of a function is always a bad idea.  Just split out a
> __ata_scsi_set_sense helper that doesn't set the flag.

What about passing the SAM_STAT_XXX status to set as an argument ?
Most current call site will be modified to pass SAM_STAT_CHECK_CONDITION,
and we could add a wrapper ata_scsi_set_check_condition_sense() to
simplify this most common case.
Christoph Hellwig Jan. 17, 2023, 7:12 a.m. UTC | #4 
On Tue, Jan 17, 2023 at 04:10:15PM +0900, Damien Le Moal wrote:
> >> +			bool check_condition, u8 sk, u8 asc, u8 ascq)
> >>  {
> >>  	bool d_sense = (dev->flags & ATA_DFLAG_D_SENSE);
> >>  
> >>  	if (!cmd)
> >>  		return;
> >>  
> >> -	scsi_build_sense(cmd, d_sense, sk, asc, ascq);
> >> +	scsi_build_sense_buffer(d_sense, cmd->sense_buffer, sk, asc, ascq);
> >> +	if (check_condition)
> >> +		set_status_byte(cmd, SAM_STAT_CHECK_CONDITION);
> >>  }
> > 
> > Adding a bool parameter do do something conditional at the end
> > of a function is always a bad idea.  Just split out a
> > __ata_scsi_set_sense helper that doesn't set the flag.
> 
> What about passing the SAM_STAT_XXX status to set as an argument ?
> Most current call site will be modified to pass SAM_STAT_CHECK_CONDITION,
> and we could add a wrapper ata_scsi_set_check_condition_sense() to
> simplify this most common case.

What's the point of that?
Damien Le Moal Jan. 17, 2023, 7:15 a.m. UTC | #5 
On 1/17/23 16:12, Christoph Hellwig wrote:
> On Tue, Jan 17, 2023 at 04:10:15PM +0900, Damien Le Moal wrote:
>>>> +			bool check_condition, u8 sk, u8 asc, u8 ascq)
>>>>  {
>>>>  	bool d_sense = (dev->flags & ATA_DFLAG_D_SENSE);
>>>>  
>>>>  	if (!cmd)
>>>>  		return;
>>>>  
>>>> -	scsi_build_sense(cmd, d_sense, sk, asc, ascq);
>>>> +	scsi_build_sense_buffer(d_sense, cmd->sense_buffer, sk, asc, ascq);
>>>> +	if (check_condition)
>>>> +		set_status_byte(cmd, SAM_STAT_CHECK_CONDITION);
>>>>  }
>>>
>>> Adding a bool parameter do do something conditional at the end
>>> of a function is always a bad idea.  Just split out a
>>> __ata_scsi_set_sense helper that doesn't set the flag.
>>
>> What about passing the SAM_STAT_XXX status to set as an argument ?
>> Most current call site will be modified to pass SAM_STAT_CHECK_CONDITION,
>> and we could add a wrapper ata_scsi_set_check_condition_sense() to
>> simplify this most common case.
> 
> What's the point of that?

Trying to get to a pretty solution keeping a single line for setting sense
+ status. But sure, splitting out the status setting works too.
Christoph Hellwig Jan. 17, 2023, 7:23 a.m. UTC | #6 
On Tue, Jan 17, 2023 at 04:15:01PM +0900, Damien Le Moal wrote:
> Trying to get to a pretty solution keeping a single line for setting sense
> + status. But sure, splitting out the status setting works too.

I suggested just adding a wrapper above.  And while I tried to white
board code it here in the mail I think we can do even better than
that, and just drop this change and just open code the call to
scsi_build_sense_buffer for the CDL use case.

While we're at it:  That !cmd check in ata_scsi_set_sense looks
really odd, can someone clean that up and just remove the calls
where there is no command?
diff mbox series

Patch

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index a6c901811802..3521f3f67f5a 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1432,7 +1432,8 @@  static void ata_eh_request_sense(struct ata_queued_cmd *qc)
 	/* Ignore err_mask; ATA_ERR might be set */
 	if (tf.status & ATA_SENSE) {
 		if (ata_scsi_sense_is_valid(tf.lbah, tf.lbam, tf.lbal)) {
-			ata_scsi_set_sense(dev, cmd, tf.lbah, tf.lbam, tf.lbal);
+			ata_scsi_set_sense(dev, cmd, true, tf.lbah, tf.lbam,
+					   tf.lbal);
 			qc->flags |= ATA_QCFLAG_SENSE_VALID;
 		}
 	} else {
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index f3e7396e3191..414d7f8a95bf 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -1471,8 +1471,8 @@  void ata_eh_analyze_ncq_error(struct ata_link *link)
 		asc = (qc->result_tf.auxiliary >> 8) & 0xff;
 		ascq = qc->result_tf.auxiliary & 0xff;
 		if (ata_scsi_sense_is_valid(sense_key, asc, ascq)) {
-			ata_scsi_set_sense(dev, qc->scsicmd, sense_key, asc,
-					   ascq);
+			ata_scsi_set_sense(dev, qc->scsicmd, true, sense_key,
+					   asc, ascq);
 			ata_scsi_set_sense_information(dev, qc->scsicmd,
 						       &qc->result_tf);
 			qc->flags |= ATA_QCFLAG_SENSE_VALID;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index aa338f10cef2..6d4ee296450e 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -205,14 +205,16 @@  bool ata_scsi_sense_is_valid(u8 sk, u8 asc, u8 ascq)
 }
 
 void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd,
-			u8 sk, u8 asc, u8 ascq)
+			bool check_condition, u8 sk, u8 asc, u8 ascq)
 {
 	bool d_sense = (dev->flags & ATA_DFLAG_D_SENSE);
 
 	if (!cmd)
 		return;
 
-	scsi_build_sense(cmd, d_sense, sk, asc, ascq);
+	scsi_build_sense_buffer(d_sense, cmd->sense_buffer, sk, asc, ascq);
+	if (check_condition)
+		set_status_byte(cmd, SAM_STAT_CHECK_CONDITION);
 }
 
 void ata_scsi_set_sense_information(struct ata_device *dev,
@@ -235,7 +237,7 @@  void ata_scsi_set_sense_information(struct ata_device *dev,
 static void ata_scsi_set_invalid_field(struct ata_device *dev,
 				       struct scsi_cmnd *cmd, u16 field, u8 bit)
 {
-	ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST, 0x24, 0x0);
+	ata_scsi_set_sense(dev, cmd, true, ILLEGAL_REQUEST, 0x24, 0x0);
 	/* "Invalid field in CDB" */
 	scsi_set_sense_field_pointer(cmd->sense_buffer, SCSI_SENSE_BUFFERSIZE,
 				     field, bit, 1);
@@ -245,7 +247,7 @@  static void ata_scsi_set_invalid_parameter(struct ata_device *dev,
 					   struct scsi_cmnd *cmd, u16 field)
 {
 	/* "Invalid field in parameter list" */
-	ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST, 0x26, 0x0);
+	ata_scsi_set_sense(dev, cmd, true, ILLEGAL_REQUEST, 0x26, 0x0);
 	scsi_set_sense_field_pointer(cmd->sense_buffer, SCSI_SENSE_BUFFERSIZE,
 				     field, 0xff, 0);
 }
@@ -914,7 +916,7 @@  static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
 		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
 				   &sense_key, &asc, &ascq, verbose);
-		ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
+		ata_scsi_set_sense(qc->dev, cmd, true, sense_key, asc, ascq);
 	} else {
 		/*
 		 * ATA PASS-THROUGH INFORMATION AVAILABLE
@@ -1005,7 +1007,7 @@  static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
 	if (ata_dev_disabled(dev)) {
 		/* Device disabled after error recovery */
 		/* LOGICAL UNIT NOT READY, HARD RESET REQUIRED */
-		ata_scsi_set_sense(dev, cmd, NOT_READY, 0x04, 0x21);
+		ata_scsi_set_sense(dev, cmd, true, NOT_READY, 0x04, 0x21);
 		return;
 	}
 	/* Use ata_to_sense_error() to map status register bits
@@ -1015,12 +1017,12 @@  static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
 	    tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
 		ata_to_sense_error(qc->ap->print_id, tf->status, tf->error,
 				   &sense_key, &asc, &ascq, verbose);
-		ata_scsi_set_sense(dev, cmd, sense_key, asc, ascq);
+		ata_scsi_set_sense(dev, cmd, true, sense_key, asc, ascq);
 	} else {
 		/* Could not decode error */
 		ata_dev_warn(dev, "could not decode error status 0x%x err_mask 0x%x\n",
 			     tf->status, qc->err_mask);
-		ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
+		ata_scsi_set_sense(dev, cmd, true, ABORTED_COMMAND, 0, 0);
 		return;
 	}
 
@@ -1496,7 +1498,7 @@  static unsigned int ata_scsi_verify_xlat(struct ata_queued_cmd *qc)
 	return 1;
 
 out_of_range:
-	ata_scsi_set_sense(qc->dev, scmd, ILLEGAL_REQUEST, 0x21, 0x0);
+	ata_scsi_set_sense(qc->dev, scmd, true, ILLEGAL_REQUEST, 0x21, 0x0);
 	/* "Logical Block Address out of range" */
 	return 1;
 
@@ -1631,7 +1633,7 @@  static unsigned int ata_scsi_rw_xlat(struct ata_queued_cmd *qc)
 	return 1;
 
 out_of_range:
-	ata_scsi_set_sense(qc->dev, scmd, ILLEGAL_REQUEST, 0x21, 0x0);
+	ata_scsi_set_sense(qc->dev, scmd, true, ILLEGAL_REQUEST, 0x21, 0x0);
 	/* "Logical Block Address out of range" */
 	return 1;
 
@@ -2354,7 +2356,7 @@  static unsigned int ata_scsiop_mode_sense(struct ata_scsi_args *args, u8 *rbuf)
 	return 1;
 
 saving_not_supp:
-	ata_scsi_set_sense(dev, args->cmd, ILLEGAL_REQUEST, 0x39, 0x0);
+	ata_scsi_set_sense(dev, args->cmd, true, ILLEGAL_REQUEST, 0x39, 0x0);
 	 /* "Saving parameters not supported" */
 	return 1;
 }
@@ -3215,11 +3217,11 @@  static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 	return 1;
 invalid_param_len:
 	/* "Parameter list length error" */
-	ata_scsi_set_sense(dev, scmd, ILLEGAL_REQUEST, 0x1a, 0x0);
+	ata_scsi_set_sense(dev, scmd, true, ILLEGAL_REQUEST, 0x1a, 0x0);
 	return 1;
 invalid_opcode:
 	/* "Invalid command operation code" */
-	ata_scsi_set_sense(dev, scmd, ILLEGAL_REQUEST, 0x20, 0x0);
+	ata_scsi_set_sense(dev, scmd, true, ILLEGAL_REQUEST, 0x20, 0x0);
 	return 1;
 }
 
@@ -3446,7 +3448,7 @@  static unsigned int ata_scsi_zbc_in_xlat(struct ata_queued_cmd *qc)
 
 invalid_param_len:
 	/* "Parameter list length error" */
-	ata_scsi_set_sense(qc->dev, scmd, ILLEGAL_REQUEST, 0x1a, 0x0);
+	ata_scsi_set_sense(qc->dev, scmd, true, ILLEGAL_REQUEST, 0x1a, 0x0);
 	return 1;
 }
 
@@ -3524,7 +3526,7 @@  static unsigned int ata_scsi_zbc_out_xlat(struct ata_queued_cmd *qc)
 	return 1;
 invalid_param_len:
 	/* "Parameter list length error" */
-	ata_scsi_set_sense(qc->dev, scmd, ILLEGAL_REQUEST, 0x1a, 0x0);
+	ata_scsi_set_sense(qc->dev, scmd, true, ILLEGAL_REQUEST, 0x1a, 0x0);
 	return 1;
 }
 
@@ -3785,7 +3787,7 @@  static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 
  invalid_param_len:
 	/* "Parameter list length error" */
-	ata_scsi_set_sense(qc->dev, scmd, ILLEGAL_REQUEST, 0x1a, 0x0);
+	ata_scsi_set_sense(qc->dev, scmd, true, ILLEGAL_REQUEST, 0x1a, 0x0);
 	return 1;
 
  skip:
@@ -4141,7 +4143,7 @@  void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
 		break;
 
 	case REQUEST_SENSE:
-		ata_scsi_set_sense(dev, cmd, 0, 0, 0);
+		ata_scsi_set_sense(dev, cmd, true, 0, 0, 0);
 		break;
 
 	/* if we reach this, then writeback caching is disabled,
@@ -4173,7 +4175,7 @@  void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
 
 	/* all other commands */
 	default:
-		ata_scsi_set_sense(dev, cmd, ILLEGAL_REQUEST, 0x20, 0x0);
+		ata_scsi_set_sense(dev, cmd, true, ILLEGAL_REQUEST, 0x20, 0x0);
 		/* "Invalid command operation code" */
 		break;
 	}
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 2cd6124a01e8..5481d29bb273 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -115,8 +115,8 @@  extern int ata_scsi_add_hosts(struct ata_host *host,
 extern void ata_scsi_scan_host(struct ata_port *ap, int sync);
 extern int ata_scsi_offline_dev(struct ata_device *dev);
 extern bool ata_scsi_sense_is_valid(u8 sk, u8 asc, u8 ascq);
-extern void ata_scsi_set_sense(struct ata_device *dev,
-			       struct scsi_cmnd *cmd, u8 sk, u8 asc, u8 ascq);
+extern void ata_scsi_set_sense(struct ata_device *dev, struct scsi_cmnd *cmd,
+			       bool check_condition, u8 sk, u8 asc, u8 ascq);
 extern void ata_scsi_set_sense_information(struct ata_device *dev,
 					   struct scsi_cmnd *cmd,
 					   const struct ata_taskfile *tf);