[PATCH v2 0/5] SCSI core patches

[PATCH v2 0/5] SCSI core patches

[Bart Van Assche]

Hi Martin,

Please consider these SCSI core patches for the next merge window.

Thanks,

Bart.

Changes compared to v1:
- Improved the SCSI tracing patch as requested by Steven Rostedt and
  Niklas Cassel.
- Added patch "scsi: core: Delay running the queue if the host is blocked"

Bart Van Assche (5):
  scsi: core: Use min() instead of open-coding it
  scsi: core: Update a source code comment
  scsi: core: Trace SCSI sense data
  scsi: core: Only kick the requeue list if necessary
  scsi: core: Delay running the queue if the host is blocked

 drivers/scsi/scsi_common.c  |  3 +--
 drivers/scsi/scsi_lib.c     | 20 ++++++++++++++------
 include/scsi/scsi_host.h    |  2 +-
 include/trace/events/scsi.h | 21 +++++++++++++++++++--
 4 files changed, 35 insertions(+), 11 deletions(-)

[PATCH v2 1/5] scsi: core: Use min() instead of open-coding it

[Bart Van Assche]

Use min() instead of open-coding it in scsi_normalize_sense().

Cc: Douglas Gilbert <dgilbert@interlog.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
index 6e50e81a8216..24dec80a6253 100644
--- a/drivers/scsi/scsi_common.c
+++ b/drivers/scsi/scsi_common.c
@@ -176,8 +176,7 @@ bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
 		if (sb_len > 2)
 			sshdr->sense_key = (sense_buffer[2] & 0xf);
 		if (sb_len > 7) {
-			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
-					 sb_len : (sense_buffer[7] + 8);
+			sb_len = min(sb_len, sense_buffer[7] + 8);
 			if (sb_len > 12)
 				sshdr->asc = sense_buffer[12];
 			if (sb_len > 13)

[PATCH v2 2/5] scsi: core: Update a source code comment

[Bart Van Assche]

The proc_name SCSI host template attribute is used for:
- The name of the /proc directory if CONFIG_SCSI_PROC_FS=y.
- The contents of the proc_name SCSI host sysfs attribute.

The current comment in include/scsi/scsi_host.h is not correct if
CONFIG_SCSI_PROC_FS=n. Hence, change that comment.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.g.garry@oracle.com>
Cc: Mike Christie <michael.christie@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/scsi/scsi_host.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 0f29799efa02..18632adca17e 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -365,7 +365,7 @@ struct scsi_host_template {
 
 
 	/*
-	 * Name of proc directory
+	 * Name reported via the proc_name SCSI host sysfs attribute.
 	 */
 	const char *proc_name;
 

[PATCH v2 3/5] scsi: core: Trace SCSI sense data

[Bart Van Assche]

If a command fails, SCSI sense data is essential to determine why it
failed. Hence make the sense key, ASC and ASCQ codes available in the
ftrace output.

Cc: Niklas Cassel <niklas.cassel@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.g.garry@oracle.com>
Cc: Mike Christie <michael.christie@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/trace/events/scsi.h | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h
index a2c7befd451a..6c4be1ebe268 100644
--- a/include/trace/events/scsi.h
+++ b/include/trace/events/scsi.h
@@ -269,9 +269,14 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
 		__field( unsigned int,	prot_sglen )
 		__field( unsigned char,	prot_op )
 		__dynamic_array(unsigned char,	cmnd, cmd->cmd_len)
+		__field( u8, sense_key )
+		__field( u8, asc )
+		__field( u8, ascq )
 	),
 
 	TP_fast_assign(
+		struct scsi_sense_hdr sshdr;
+
 		__entry->host_no	= cmd->device->host->host_no;
 		__entry->channel	= cmd->device->channel;
 		__entry->id		= cmd->device->id;
@@ -285,11 +290,22 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
 		__entry->prot_sglen	= scsi_prot_sg_count(cmd);
 		__entry->prot_op	= scsi_get_prot_op(cmd);
 		memcpy(__get_dynamic_array(cmnd), cmd->cmnd, cmd->cmd_len);
+		if (cmd->sense_buffer && SCSI_SENSE_VALID(cmd) &&
+		    scsi_command_normalize_sense(cmd, &sshdr)) {
+			__entry->sense_key = sshdr.sense_key;
+			__entry->asc = sshdr.asc;
+			__entry->ascq = sshdr.ascq;
+		} else {
+			__entry->sense_key = 0;
+			__entry->asc = 0;
+			__entry->ascq = 0;
+		}
 	),
 
 	TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u prot_sgl=%u " \
 		  "prot_op=%s driver_tag=%d scheduler_tag=%d cmnd=(%s %s raw=%s) " \
-		  "result=(driver=%s host=%s message=%s status=%s)",
+		  "result=(driver=%s host=%s message=%s status=%s "
+		  "sense_key=%u asc=%#x ascq=%#x))",
 		  __entry->host_no, __entry->channel, __entry->id,
 		  __entry->lun, __entry->data_sglen, __entry->prot_sglen,
 		  show_prot_op_name(__entry->prot_op), __entry->driver_tag,
@@ -299,7 +315,8 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
 		  "DRIVER_OK",
 		  show_hostbyte_name(((__entry->result) >> 16) & 0xff),
 		  "COMMAND_COMPLETE",
-		  show_statusbyte_name(__entry->result & 0xff))
+		  show_statusbyte_name(__entry->result & 0xff),
+		  __entry->sense_key, __entry->asc, __entry->ascq)
 );
 
 DEFINE_EVENT(scsi_cmd_done_timeout_template, scsi_dispatch_cmd_done,

[PATCH v2 4/5] scsi: core: Only kick the requeue list if necessary

[Bart Van Assche]

Instead of running the request queue of each device associated with a
host every 3 ms (BLK_MQ_RESOURCE_DELAY) while host error handling is in
progress, run the request queue after error handling has finished.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.g.garry@oracle.com>
Cc: Mike Christie <michael.christie@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_lib.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e59eb0cbfc83..a34390d35f1d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -122,11 +122,9 @@ static void scsi_mq_requeue_cmd(struct scsi_cmnd *cmd, unsigned long msecs)
 		WARN_ON_ONCE(true);
 	}
 
-	if (msecs) {
-		blk_mq_requeue_request(rq, false);
+	blk_mq_requeue_request(rq, false);
+	if (!scsi_host_in_recovery(cmd->device->host))
 		blk_mq_delay_kick_requeue_list(rq->q, msecs);
-	} else
-		blk_mq_requeue_request(rq, true);
 }
 
 /**
@@ -165,7 +163,8 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy)
 	 */
 	cmd->result = 0;
 
-	blk_mq_requeue_request(scsi_cmd_to_rq(cmd), true);
+	blk_mq_requeue_request(scsi_cmd_to_rq(cmd),
+			       !scsi_host_in_recovery(cmd->device->host));
 }
 
 /**
@@ -462,10 +461,16 @@ void scsi_requeue_run_queue(struct work_struct *work)
 	scsi_run_queue(q);
 }
 
+/*
+ * Transfer requests from the requeue_list to from where these can be dispatched
+ * and run the request queues.
+ */
 void scsi_run_host_queues(struct Scsi_Host *shost)
 {
 	struct scsi_device *sdev;
 
+	shost_for_each_device(sdev, shost)
+		blk_mq_kick_requeue_list(sdev->request_queue);
 	shost_for_each_device(sdev, shost)
 		scsi_run_queue(sdev->request_queue);
 }
@@ -499,6 +504,9 @@ static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd)
 
 static void scsi_run_queue_async(struct scsi_device *sdev)
 {
+	if (scsi_host_in_recovery(sdev->host))
+		return;
+
 	if (scsi_target(sdev)->single_lun ||
 	    !list_empty(&sdev->host->starved_list)) {
 		kblockd_schedule_work(&sdev->requeue_work);

[PATCH v2 5/5] scsi: core: Delay running the queue if the host is blocked

[Bart Van Assche]

Tell the block layer to rerun the queue after a delay instead of
immediately if the SCSI LLD decided to block the host.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: John Garry <john.g.garry@oracle.com>
Cc: Mike Christie <michael.christie@oracle.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a34390d35f1d..370be5994a45 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1772,7 +1772,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 		break;
 	case BLK_STS_RESOURCE:
 	case BLK_STS_ZONE_RESOURCE:
-		if (scsi_device_blocked(sdev))
+		if (scsi_device_blocked(sdev) || shost->host_self_blocked)
 			ret = BLK_STS_DEV_RESOURCE;
 		break;
 	case BLK_STS_AGAIN:

Re: [PATCH v2 4/5] scsi: core: Only kick the requeue list if necessary

[Ming Lei]

On Wed, May 03, 2023 at 04:06:53PM -0700, Bart Van Assche wrote:
> Instead of running the request queue of each device associated with a
> host every 3 ms (BLK_MQ_RESOURCE_DELAY) while host error handling is in
> progress, run the request queue after error handling has finished.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.g.garry@oracle.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/scsi_lib.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index e59eb0cbfc83..a34390d35f1d 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -122,11 +122,9 @@ static void scsi_mq_requeue_cmd(struct scsi_cmnd *cmd, unsigned long msecs)
>  		WARN_ON_ONCE(true);
>  	}
>  
> -	if (msecs) {
> -		blk_mq_requeue_request(rq, false);
> +	blk_mq_requeue_request(rq, false);
> +	if (!scsi_host_in_recovery(cmd->device->host))
>  		blk_mq_delay_kick_requeue_list(rq->q, msecs);
> -	} else
> -		blk_mq_requeue_request(rq, true);
>  }
>  
>  /**
> @@ -165,7 +163,8 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy)
>  	 */
>  	cmd->result = 0;
>  
> -	blk_mq_requeue_request(scsi_cmd_to_rq(cmd), true);
> +	blk_mq_requeue_request(scsi_cmd_to_rq(cmd),
> +			       !scsi_host_in_recovery(cmd->device->host));
>  }
>  
>  /**
> @@ -462,10 +461,16 @@ void scsi_requeue_run_queue(struct work_struct *work)
>  	scsi_run_queue(q);
>  }
>  
> +/*
> + * Transfer requests from the requeue_list to from where these can be dispatched
> + * and run the request queues.
> + */
>  void scsi_run_host_queues(struct Scsi_Host *shost)
>  {
>  	struct scsi_device *sdev;
>  
> +	shost_for_each_device(sdev, shost)
> +		blk_mq_kick_requeue_list(sdev->request_queue);
>  	shost_for_each_device(sdev, shost)
>  		scsi_run_queue(sdev->request_queue);

You may move blk_mq_kick_requeue_list() to the following loop, or even
inside scsi_run_queue(), which isn't called in fast patch.

Thanks,
Ming

Re: [PATCH v2 3/5] scsi: core: Trace SCSI sense data

[Hannes Reinecke]

On 5/4/23 01:06, Bart Van Assche wrote:
> If a command fails, SCSI sense data is essential to determine why it
> failed. Hence make the sense key, ASC and ASCQ codes available in the
> ftrace output.
> 
> Cc: Niklas Cassel <niklas.cassel@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.g.garry@oracle.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   include/trace/events/scsi.h | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h
> index a2c7befd451a..6c4be1ebe268 100644
> --- a/include/trace/events/scsi.h
> +++ b/include/trace/events/scsi.h
> @@ -269,9 +269,14 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>   		__field( unsigned int,	prot_sglen )
>   		__field( unsigned char,	prot_op )
>   		__dynamic_array(unsigned char,	cmnd, cmd->cmd_len)
> +		__field( u8, sense_key )
> +		__field( u8, asc )
> +		__field( u8, ascq )
>   	),
>   
>   	TP_fast_assign(
> +		struct scsi_sense_hdr sshdr;
> +
>   		__entry->host_no	= cmd->device->host->host_no;
>   		__entry->channel	= cmd->device->channel;
>   		__entry->id		= cmd->device->id;
> @@ -285,11 +290,22 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>   		__entry->prot_sglen	= scsi_prot_sg_count(cmd);
>   		__entry->prot_op	= scsi_get_prot_op(cmd);
>   		memcpy(__get_dynamic_array(cmnd), cmd->cmnd, cmd->cmd_len);
> +		if (cmd->sense_buffer && SCSI_SENSE_VALID(cmd) &&
> +		    scsi_command_normalize_sense(cmd, &sshdr)) {
> +			__entry->sense_key = sshdr.sense_key;
> +			__entry->asc = sshdr.asc;
> +			__entry->ascq = sshdr.ascq;
> +		} else {
> +			__entry->sense_key = 0;
> +			__entry->asc = 0;
> +			__entry->ascq = 0;
> +		}
>   	),
>   
Hmm. All this business with scsi_normalize_sense(); there are 15 
instances calling scsi_normalize_sense(), and half of it are calling it
with scmd->sense-buffer.
Makes one wonder if we shouldn't add fields in the scmd structure, and
do the decoding always...

But for another time, I guess.

>   	TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u prot_sgl=%u " \
>   		  "prot_op=%s driver_tag=%d scheduler_tag=%d cmnd=(%s %s raw=%s) " \
> -		  "result=(driver=%s host=%s message=%s status=%s)",
> +		  "result=(driver=%s host=%s message=%s status=%s "
> +		  "sense_key=%u asc=%#x ascq=%#x))",

Why two closing braces?

>   		  __entry->host_no, __entry->channel, __entry->id,
>   		  __entry->lun, __entry->data_sglen, __entry->prot_sglen,
>   		  show_prot_op_name(__entry->prot_op), __entry->driver_tag,
> @@ -299,7 +315,8 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>   		  "DRIVER_OK",
>   		  show_hostbyte_name(((__entry->result) >> 16) & 0xff),
>   		  "COMMAND_COMPLETE",
> -		  show_statusbyte_name(__entry->result & 0xff))
> +		  show_statusbyte_name(__entry->result & 0xff),
> +		  __entry->sense_key, __entry->asc, __entry->ascq)
>   );
>   
>   DEFINE_EVENT(scsi_cmd_done_timeout_template, scsi_dispatch_cmd_done,
Cheers,

Hannes

Re: [PATCH v2 3/5] scsi: core: Trace SCSI sense data

[Niklas Cassel]

On Wed, May 03, 2023 at 04:06:52PM -0700, Bart Van Assche wrote:
> If a command fails, SCSI sense data is essential to determine why it
> failed. Hence make the sense key, ASC and ASCQ codes available in the
> ftrace output.
> 
> Cc: Niklas Cassel <niklas.cassel@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.g.garry@oracle.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  include/trace/events/scsi.h | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h
> index a2c7befd451a..6c4be1ebe268 100644
> --- a/include/trace/events/scsi.h
> +++ b/include/trace/events/scsi.h
> @@ -269,9 +269,14 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>  		__field( unsigned int,	prot_sglen )
>  		__field( unsigned char,	prot_op )
>  		__dynamic_array(unsigned char,	cmnd, cmd->cmd_len)
> +		__field( u8, sense_key )
> +		__field( u8, asc )
> +		__field( u8, ascq )
>  	),
>  
>  	TP_fast_assign(
> +		struct scsi_sense_hdr sshdr;
> +
>  		__entry->host_no	= cmd->device->host->host_no;
>  		__entry->channel	= cmd->device->channel;
>  		__entry->id		= cmd->device->id;
> @@ -285,11 +290,22 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>  		__entry->prot_sglen	= scsi_prot_sg_count(cmd);
>  		__entry->prot_op	= scsi_get_prot_op(cmd);
>  		memcpy(__get_dynamic_array(cmnd), cmd->cmnd, cmd->cmd_len);
> +		if (cmd->sense_buffer && SCSI_SENSE_VALID(cmd) &&
> +		    scsi_command_normalize_sense(cmd, &sshdr)) {
> +			__entry->sense_key = sshdr.sense_key;
> +			__entry->asc = sshdr.asc;
> +			__entry->ascq = sshdr.ascq;
> +		} else {
> +			__entry->sense_key = 0;
> +			__entry->asc = 0;
> +			__entry->ascq = 0;
> +		}
>  	),
>  
>  	TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u prot_sgl=%u " \
>  		  "prot_op=%s driver_tag=%d scheduler_tag=%d cmnd=(%s %s raw=%s) " \
> -		  "result=(driver=%s host=%s message=%s status=%s)",
> +		  "result=(driver=%s host=%s message=%s status=%s "
> +		  "sense_key=%u asc=%#x ascq=%#x))",

With the extra parentheses removed (as mentioned by Hannes):

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH v2 1/5] scsi: core: Use min() instead of open-coding it

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 2/5] scsi: core: Update a source code comment

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 3/5] scsi: core: Trace SCSI sense data

[Christoph Hellwig]

On Wed, May 03, 2023 at 04:06:52PM -0700, Bart Van Assche wrote:
> If a command fails, SCSI sense data is essential to determine why it
> failed. Hence make the sense key, ASC and ASCQ codes available in the
> ftrace output.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Although, I'd also love to see pre-decoded ASC and ASCQ codes in the
scsi_cmnd at some point.

Re: [PATCH v2 4/5] scsi: core: Only kick the requeue list if necessary

[Christoph Hellwig]

Looks good modulo the comment from Ming.

Re: [PATCH v2 5/5] scsi: core: Delay running the queue if the host is blocked

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 3/5] scsi: core: Trace SCSI sense data

[Bart Van Assche]

On 5/4/23 01:07, Hannes Reinecke wrote:
> On 5/4/23 01:06, Bart Van Assche wrote:
>>       TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u prot_sgl=%u " \
>>             "prot_op=%s driver_tag=%d scheduler_tag=%d cmnd=(%s %s raw=%s) " \
>> -          "result=(driver=%s host=%s message=%s status=%s)",
>> +          "result=(driver=%s host=%s message=%s status=%s "
>> +          "sense_key=%u asc=%#x ascq=%#x))",
> 
> Why two closing braces?

There should only be one closing brace. I will fix this.

Thanks,

Bart.

Re: [PATCH v2 3/5] scsi: core: Trace SCSI sense data

[Martin K. Petersen]

Christoph,

> Although, I'd also love to see pre-decoded ASC and ASCQ codes in the
> scsi_cmnd at some point.

Yep! It would be nice to have these readily available.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 1/5] scsi: core: Use min() instead of open-coding it

[Benjamin Block]

On Wed, May 03, 2023 at 04:06:50PM -0700, Bart Van Assche wrote:
> Use min() instead of open-coding it in scsi_normalize_sense().
> 
> Cc: Douglas Gilbert <dgilbert@interlog.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/scsi_common.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
> index 6e50e81a8216..24dec80a6253 100644
> --- a/drivers/scsi/scsi_common.c
> +++ b/drivers/scsi/scsi_common.c
> @@ -176,8 +176,7 @@ bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
>  		if (sb_len > 2)
>  			sshdr->sense_key = (sense_buffer[2] & 0xf);
>  		if (sb_len > 7) {
> -			sb_len = (sb_len < (sense_buffer[7] + 8)) ?
> -					 sb_len : (sense_buffer[7] + 8);
> +			sb_len = min(sb_len, sense_buffer[7] + 8);
>  			if (sb_len > 12)
>  				sshdr->asc = sense_buffer[12];
>  			if (sb_len > 13)

Looks good to me.


Reviewed-by: Benjamin Block <bblock@linux.ibm.com>

-- 
Best Regards, Benjamin Block        /        Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH    /   https://www.ibm.com/privacy
Vors. Aufs.-R.: Gregor Pillen         /         Geschäftsführung: David Faller
Sitz der Ges.: Böblingen     /    Registergericht: AmtsG Stuttgart, HRB 243294

Re: [PATCH v2 2/5] scsi: core: Update a source code comment

[Benjamin Block]

On Wed, May 03, 2023 at 04:06:51PM -0700, Bart Van Assche wrote:
> The proc_name SCSI host template attribute is used for:
> - The name of the /proc directory if CONFIG_SCSI_PROC_FS=y.

But now you remove that case completely? It seems kinda strange to
bother touching the comment, but then only switching from one incomplete
form to an other?

> - The contents of the proc_name SCSI host sysfs attribute.
> 
> The current comment in include/scsi/scsi_host.h is not correct if
> CONFIG_SCSI_PROC_FS=n. Hence, change that comment.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.g.garry@oracle.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  include/scsi/scsi_host.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 0f29799efa02..18632adca17e 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -365,7 +365,7 @@ struct scsi_host_template {
>  
>  
>  	/*
> -	 * Name of proc directory
> +	 * Name reported via the proc_name SCSI host sysfs attribute.
>  	 */
>  	const char *proc_name;
>  

-- 
Best Regards, Benjamin Block        /        Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH    /   https://www.ibm.com/privacy
Vors. Aufs.-R.: Gregor Pillen         /         Geschäftsführung: David Faller
Sitz der Ges.: Böblingen     /    Registergericht: AmtsG Stuttgart, HRB 243294

Re: [PATCH v2 3/5] scsi: core: Trace SCSI sense data

[Benjamin Block]

On Wed, May 03, 2023 at 04:06:52PM -0700, Bart Van Assche wrote:
> If a command fails, SCSI sense data is essential to determine why it
> failed. Hence make the sense key, ASC and ASCQ codes available in the
> ftrace output.
> 
> Cc: Niklas Cassel <niklas.cassel@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.g.garry@oracle.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  include/trace/events/scsi.h | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h
> index a2c7befd451a..6c4be1ebe268 100644
> --- a/include/trace/events/scsi.h
> +++ b/include/trace/events/scsi.h
> @@ -269,9 +269,14 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>  		__field( unsigned int,	prot_sglen )
>  		__field( unsigned char,	prot_op )
>  		__dynamic_array(unsigned char,	cmnd, cmd->cmd_len)
> +		__field( u8, sense_key )
> +		__field( u8, asc )
> +		__field( u8, ascq )
>  	),
>  
>  	TP_fast_assign(
> +		struct scsi_sense_hdr sshdr;
> +
>  		__entry->host_no	= cmd->device->host->host_no;
>  		__entry->channel	= cmd->device->channel;
>  		__entry->id		= cmd->device->id;
> @@ -285,11 +290,22 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>  		__entry->prot_sglen	= scsi_prot_sg_count(cmd);
>  		__entry->prot_op	= scsi_get_prot_op(cmd);
>  		memcpy(__get_dynamic_array(cmnd), cmd->cmnd, cmd->cmd_len);
> +		if (cmd->sense_buffer && SCSI_SENSE_VALID(cmd) &&

Can't hurt to have these explicitly here, but these checks are also
done by `scsi_command_normalize_sense()` AFAICT.

> +		    scsi_command_normalize_sense(cmd, &sshdr)) {
> +			__entry->sense_key = sshdr.sense_key;
> +			__entry->asc = sshdr.asc;
> +			__entry->ascq = sshdr.ascq;
> +		} else {
> +			__entry->sense_key = 0;
> +			__entry->asc = 0;
> +			__entry->ascq = 0;
> +		}
>  	),
>  
>  	TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u prot_sgl=%u " \
>  		  "prot_op=%s driver_tag=%d scheduler_tag=%d cmnd=(%s %s raw=%s) " \
> -		  "result=(driver=%s host=%s message=%s status=%s)",
> +		  "result=(driver=%s host=%s message=%s status=%s "
> +		  "sense_key=%u asc=%#x ascq=%#x))",

In SPC, these are all in base 16, and some existing functions in
`scsi_logging.c` format Sense Key as base 16. We probably should keep
this consistent and also format Sense Key with `%#x`.

>  		  __entry->host_no, __entry->channel, __entry->id,
>  		  __entry->lun, __entry->data_sglen, __entry->prot_sglen,
>  		  show_prot_op_name(__entry->prot_op), __entry->driver_tag,
> @@ -299,7 +315,8 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>  		  "DRIVER_OK",
>  		  show_hostbyte_name(((__entry->result) >> 16) & 0xff),
>  		  "COMMAND_COMPLETE",
> -		  show_statusbyte_name(__entry->result & 0xff))
> +		  show_statusbyte_name(__entry->result & 0xff),
> +		  __entry->sense_key, __entry->asc, __entry->ascq)
>  );
>  
>  DEFINE_EVENT(scsi_cmd_done_timeout_template, scsi_dispatch_cmd_done,

-- 
Best Regards, Benjamin Block        /        Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH    /   https://www.ibm.com/privacy
Vors. Aufs.-R.: Gregor Pillen         /         Geschäftsführung: David Faller
Sitz der Ges.: Böblingen     /    Registergericht: AmtsG Stuttgart, HRB 243294

Re: [PATCH v2 2/5] scsi: core: Update a source code comment

[Bart Van Assche]

On 5/8/23 04:25, Benjamin Block wrote:
> On Wed, May 03, 2023 at 04:06:51PM -0700, Bart Van Assche wrote:
>> The proc_name SCSI host template attribute is used for:
>> - The name of the /proc directory if CONFIG_SCSI_PROC_FS=y.
> 
> But now you remove that case completely? It seems kinda strange to
> bother touching the comment, but then only switching from one incomplete
> form to an other?

Using procfs as an interface to other information than process information
is deprecated, so I'm not sure it's worth mentioning how this name is used
if CONFIG_SCSI_PROC_FS=y.

Bart.

Re: [PATCH v2 3/5] scsi: core: Trace SCSI sense data

[Bart Van Assche]

On 5/8/23 07:05, Benjamin Block wrote:
> On Wed, May 03, 2023 at 04:06:52PM -0700, Bart Van Assche wrote:
>> @@ -285,11 +290,22 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
>>   		__entry->prot_sglen	= scsi_prot_sg_count(cmd);
>>   		__entry->prot_op	= scsi_get_prot_op(cmd);
>>   		memcpy(__get_dynamic_array(cmnd), cmd->cmnd, cmd->cmd_len);
>> +		if (cmd->sense_buffer && SCSI_SENSE_VALID(cmd) &&
> 
> Can't hurt to have these explicitly here, but these checks are also
> done by `scsi_command_normalize_sense()` AFAICT.

Niklas requested these checks as a performance optimization.

> 
>> +		    scsi_command_normalize_sense(cmd, &sshdr)) {
>> +			__entry->sense_key = sshdr.sense_key;
>> +			__entry->asc = sshdr.asc;
>> +			__entry->ascq = sshdr.ascq;
>> +		} else {
>> +			__entry->sense_key = 0;
>> +			__entry->asc = 0;
>> +			__entry->ascq = 0;
>> +		}
>>   	),
>>   
>>   	TP_printk("host_no=%u channel=%u id=%u lun=%u data_sgl=%u prot_sgl=%u " \
>>   		  "prot_op=%s driver_tag=%d scheduler_tag=%d cmnd=(%s %s raw=%s) " \
>> -		  "result=(driver=%s host=%s message=%s status=%s)",
>> +		  "result=(driver=%s host=%s message=%s status=%s "
>> +		  "sense_key=%u asc=%#x ascq=%#x))",
> 
> In SPC, these are all in base 16, and some existing functions in
> `scsi_logging.c` format Sense Key as base 16. We probably should keep
> this consistent and also format Sense Key with `%#x`.

I can make this change.

Thanks,

Bart.

Re: [PATCH v2 3/5] scsi: core: Trace SCSI sense data

[Benjamin Block]

On Mon, May 08, 2023 at 07:10:31AM -0700, Bart Van Assche wrote:
> On 5/8/23 07:05, Benjamin Block wrote:
> > On Wed, May 03, 2023 at 04:06:52PM -0700, Bart Van Assche wrote:
> >> @@ -285,11 +290,22 @@ DECLARE_EVENT_CLASS(scsi_cmd_done_timeout_template,
> >>   		__entry->prot_sglen	= scsi_prot_sg_count(cmd);
> >>   		__entry->prot_op	= scsi_get_prot_op(cmd);
> >>   		memcpy(__get_dynamic_array(cmnd), cmd->cmnd, cmd->cmd_len);
> >> +		if (cmd->sense_buffer && SCSI_SENSE_VALID(cmd) &&
> > 
> > Can't hurt to have these explicitly here, but these checks are also
> > done by `scsi_command_normalize_sense()` AFAICT.
> 
> Niklas requested these checks as a performance optimization.

Oh okay, didn't remember that. Fair enough.

-- 
Best Regards, Benjamin Block        /        Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH    /   https://www.ibm.com/privacy
Vors. Aufs.-R.: Gregor Pillen         /         Geschäftsführung: David Faller
Sitz der Ges.: Böblingen     /    Registergericht: AmtsG Stuttgart, HRB 243294

Re: [PATCH v2 3/5] scsi: core: Trace SCSI sense data

[Bart Van Assche]

On 5/6/23 15:48, Martin K. Petersen wrote:
>> Although, I'd also love to see pre-decoded ASC and ASCQ codes in the
>> scsi_cmnd at some point.
> 
> Yep! It would be nice to have these readily available.

Implementing this seems nontrivial and risky to me. As an example, 
whether or not scsi_decide_disposition() calls scsi_check_sense() 
depends on the command result. Guaranteeing that 
scsi_command_normalize_sense() is called independent of the command 
result implies moving this call from scsi_check_sense() to somewhere 
else, e.g. scsi_decide_disposition(). Unfortunately scsi_check_sense() 
is not only called from the SCSI completion path but also by ATA code 
that is not related to the SCSI completion path (ata_eh_analyze_tf()). 
So I consider this change as out-of-scope for this patch series.

Bart.