[PATCH 1/3] scsi: storvsc: Miscellaneous code cleanups

[PATCH 1/3] scsi: storvsc: Miscellaneous code cleanups

[Michael Kelley]

As general cleanup and in preparation for subsequent patches:
* Use min() instead of open coding
* Use set_host_byte() and status_byte() instead of open coding
  access to scsi_status field
* Collapse nested "if" statements to reduce indentation
* Fix other indentation
* Remove extra blank lines

No functional changes.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 drivers/scsi/storvsc_drv.c | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index e6718a7..9996e8b 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1160,17 +1160,16 @@ static void storvsc_on_io_completion(struct storvsc_device *stor_device,
 		vstor_packet->vm_srb.srb_status = SRB_STATUS_SUCCESS;
 	}
 
-
 	/* Copy over the status...etc */
 	stor_pkt->vm_srb.scsi_status = vstor_packet->vm_srb.scsi_status;
 	stor_pkt->vm_srb.srb_status = vstor_packet->vm_srb.srb_status;
 
-	/* Validate sense_info_length (from Hyper-V) */
-	if (vstor_packet->vm_srb.sense_info_length > sense_buffer_size)
-		vstor_packet->vm_srb.sense_info_length = sense_buffer_size;
-
-	stor_pkt->vm_srb.sense_info_length =
-	vstor_packet->vm_srb.sense_info_length;
+	/*
+	 * Copy over the sense_info_length, but limit to the known max
+	 * size if Hyper-V returns a bad value.
+	 */
+	stor_pkt->vm_srb.sense_info_length = min_t(u8, sense_buffer_size,
+		vstor_packet->vm_srb.sense_info_length);
 
 	if (vstor_packet->vm_srb.scsi_status != 0 ||
 	    vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS)
@@ -1180,33 +1179,26 @@ static void storvsc_on_io_completion(struct storvsc_device *stor_device,
 			vstor_packet->vm_srb.scsi_status,
 			vstor_packet->vm_srb.srb_status);
 
-	if ((vstor_packet->vm_srb.scsi_status & 0xFF) == 0x02) {
-		/* CHECK_CONDITION */
-		if (vstor_packet->vm_srb.srb_status &
-			SRB_STATUS_AUTOSENSE_VALID) {
-			/* autosense data available */
-
-			storvsc_log(device, STORVSC_LOGGING_WARN,
-				"stor pkt %p autosense data valid - len %d\n",
-				request, vstor_packet->vm_srb.sense_info_length);
+	if (status_byte(vstor_packet->vm_srb.scsi_status) == CHECK_CONDITION
+	   && (vstor_packet->vm_srb.srb_status & SRB_STATUS_AUTOSENSE_VALID)) {
 
-			memcpy(request->cmd->sense_buffer,
-			       vstor_packet->vm_srb.sense_data,
-			       vstor_packet->vm_srb.sense_info_length);
+		storvsc_log(device, STORVSC_LOGGING_WARN,
+			"stor pkt %p autosense data valid - len %d\n",
+			request, vstor_packet->vm_srb.sense_info_length);
 
-		}
+		memcpy(request->cmd->sense_buffer,
+		       vstor_packet->vm_srb.sense_data,
+		       stor_pkt->vm_srb.sense_info_length);
 	}
 
 	stor_pkt->vm_srb.data_transfer_length =
-	vstor_packet->vm_srb.data_transfer_length;
+			vstor_packet->vm_srb.data_transfer_length;
 
 	storvsc_command_completion(request, stor_device);
 
 	if (atomic_dec_and_test(&stor_device->num_outstanding_req) &&
 		stor_device->drain_notify)
 		wake_up(&stor_device->waiting_to_drain);
-
-
 }
 
 static void storvsc_on_receive(struct storvsc_device *stor_device,
@@ -1675,7 +1667,7 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd *scmnd)
 	 * this. So, don't send it.
 	 */
 	case SET_WINDOW:
-		scmnd->result = DID_ERROR << 16;
+		set_host_byte(scmnd, DID_ERROR);
 		allowed = false;
 		break;
 	default:
-- 
1.8.3.1

[PATCH 2/3] scsi: storvsc: Update error logging

[Michael Kelley]

When an I/O error is reported by the underlying Hyper-V host, current
code provides details only when the logging level is set to WARN, making
it more difficult to diagnose problems in live customer situations. Fix
this by reporting details at ERROR level, which is the default.  Also
add more information, including the Hyper-V error code, and the tag #
so that the message can be matched with messages at the SCSI and blk-mq
levels.

Also, sense information logging is inconsistent and duplicative. The
existence of sense info is first logged at WARN level, and then full
sense info is logged at ERROR level. Fix this by removing the logging
of the existence of sense info, and change the logging of full sense
info to WARN level in favor of letting the generic SCSI layer handle
such logging. With the change to WARN level, it's no longer necessary
to filter out as noise any NOT READY sense info generated by the
virtual DVD device.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 drivers/scsi/storvsc_drv.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 9996e8b..fff9441 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1090,6 +1090,7 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request,
 	struct Scsi_Host *host;
 	u32 payload_sz = cmd_request->payload_sz;
 	void *payload = cmd_request->payload;
+	bool sense_ok;
 
 	host = stor_dev->host;
 
@@ -1099,11 +1100,10 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request,
 	scmnd->result = vm_srb->scsi_status;
 
 	if (scmnd->result) {
-		if (scsi_normalize_sense(scmnd->sense_buffer,
-				SCSI_SENSE_BUFFERSIZE, &sense_hdr) &&
-		    !(sense_hdr.sense_key == NOT_READY &&
-				 sense_hdr.asc == 0x03A) &&
-		    do_logging(STORVSC_LOGGING_ERROR))
+		sense_ok = scsi_normalize_sense(scmnd->sense_buffer,
+				SCSI_SENSE_BUFFERSIZE, &sense_hdr);
+
+		if (sense_ok && do_logging(STORVSC_LOGGING_WARN))
 			scsi_print_sense_hdr(scmnd->device, "storvsc",
 					     &sense_hdr);
 	}
@@ -1173,23 +1173,19 @@ static void storvsc_on_io_completion(struct storvsc_device *stor_device,
 
 	if (vstor_packet->vm_srb.scsi_status != 0 ||
 	    vstor_packet->vm_srb.srb_status != SRB_STATUS_SUCCESS)
-		storvsc_log(device, STORVSC_LOGGING_WARN,
-			"cmd 0x%x scsi status 0x%x srb status 0x%x\n",
+		storvsc_log(device, STORVSC_LOGGING_ERROR,
+			"tag#%d cmd 0x%x status: scsi 0x%x srb 0x%x hv 0x%x\n",
+			request->cmd->request->tag,
 			stor_pkt->vm_srb.cdb[0],
 			vstor_packet->vm_srb.scsi_status,
-			vstor_packet->vm_srb.srb_status);
+			vstor_packet->vm_srb.srb_status,
+			vstor_packet->status);
 
 	if (status_byte(vstor_packet->vm_srb.scsi_status) == CHECK_CONDITION
-	   && (vstor_packet->vm_srb.srb_status & SRB_STATUS_AUTOSENSE_VALID)) {
-
-		storvsc_log(device, STORVSC_LOGGING_WARN,
-			"stor pkt %p autosense data valid - len %d\n",
-			request, vstor_packet->vm_srb.sense_info_length);
-
+	   && (vstor_packet->vm_srb.srb_status & SRB_STATUS_AUTOSENSE_VALID))
 		memcpy(request->cmd->sense_buffer,
 		       vstor_packet->vm_srb.sense_data,
 		       stor_pkt->vm_srb.sense_info_length);
-	}
 
 	stor_pkt->vm_srb.data_transfer_length =
 			vstor_packet->vm_srb.data_transfer_length;
-- 
1.8.3.1

[PATCH 3/3] scsi: storvsc: Correctly handle multiple flags in srb_status

[Michael Kelley]

Hyper-V is observed to sometimes set multiple flags in the
srb_status, such as ABORTED and ERROR. Current code in
storvsc_handle_error() handles only a single flag being set,
and does nothing when multiple flags are set.  Fix this by
changing the case statement into a series of "if" statements
testing individual flags. The functionality for handling each
flag is unchanged.

Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
 drivers/scsi/storvsc_drv.c | 61 +++++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index fff9441..e96d2aa 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1009,17 +1009,40 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb,
 	struct storvsc_scan_work *wrk;
 	void (*process_err_fn)(struct work_struct *work);
 	struct hv_host_device *host_dev = shost_priv(host);
-	bool do_work = false;
 
-	switch (SRB_STATUS(vm_srb->srb_status)) {
-	case SRB_STATUS_ERROR:
+	/*
+	 * In some situations, Hyper-V sets multiple bits in the
+	 * srb_status, such as ABORTED and ERROR. So process them
+	 * individually, with the most specific bits first.
+	 */
+
+	if (vm_srb->srb_status & SRB_STATUS_INVALID_LUN) {
+		set_host_byte(scmnd, DID_NO_CONNECT);
+		process_err_fn = storvsc_remove_lun;
+		goto do_work;
+	}
+
+	if (vm_srb->srb_status & SRB_STATUS_ABORTED) {
+		if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID &&
+		    /* Capacity data has changed */
+		    (asc == 0x2a) && (ascq == 0x9)) {
+			process_err_fn = storvsc_device_scan;
+			/*
+			 * Retry the I/O that triggered this.
+			 */
+			set_host_byte(scmnd, DID_REQUEUE);
+			goto do_work;
+		}
+	}
+
+	if (vm_srb->srb_status & SRB_STATUS_ERROR) {
 		/*
 		 * Let upper layer deal with error when
 		 * sense message is present.
 		 */
-
 		if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID)
-			break;
+			return;
+
 		/*
 		 * If there is an error; offline the device since all
 		 * error recovery strategies would have already been
@@ -1032,37 +1055,19 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb,
 			set_host_byte(scmnd, DID_PASSTHROUGH);
 			break;
 		/*
-		 * On Some Windows hosts TEST_UNIT_READY command can return
-		 * SRB_STATUS_ERROR, let the upper level code deal with it
-		 * based on the sense information.
+		 * On some Hyper-V hosts TEST_UNIT_READY command can
+		 * return SRB_STATUS_ERROR. Let the upper level code
+		 * deal with it based on the sense information.
 		 */
 		case TEST_UNIT_READY:
 			break;
 		default:
 			set_host_byte(scmnd, DID_ERROR);
 		}
-		break;
-	case SRB_STATUS_INVALID_LUN:
-		set_host_byte(scmnd, DID_NO_CONNECT);
-		do_work = true;
-		process_err_fn = storvsc_remove_lun;
-		break;
-	case SRB_STATUS_ABORTED:
-		if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID &&
-		    (asc == 0x2a) && (ascq == 0x9)) {
-			do_work = true;
-			process_err_fn = storvsc_device_scan;
-			/*
-			 * Retry the I/O that triggered this.
-			 */
-			set_host_byte(scmnd, DID_REQUEUE);
-		}
-		break;
 	}
+	return;
 
-	if (!do_work)
-		return;
-
+do_work:
 	/*
 	 * We need to schedule work to process this error; schedule it.
 	 */
-- 
1.8.3.1

RE: [PATCH 3/3] scsi: storvsc: Correctly handle multiple flags in srb_status

[Long Li]

> Subject: [PATCH 3/3] scsi: storvsc: Correctly handle multiple flags in
> srb_status
> 
> Hyper-V is observed to sometimes set multiple flags in the srb_status, such
> as ABORTED and ERROR. Current code in
> storvsc_handle_error() handles only a single flag being set, and does nothing
> when multiple flags are set.  Fix this by changing the case statement into a
> series of "if" statements testing individual flags. The functionality for handling
> each flag is unchanged.
> 
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
>  drivers/scsi/storvsc_drv.c | 61 +++++++++++++++++++++++++----------------
> -----
>  1 file changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c index
> fff9441..e96d2aa 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1009,17 +1009,40 @@ static void storvsc_handle_error(struct
> vmscsi_request *vm_srb,
>  	struct storvsc_scan_work *wrk;
>  	void (*process_err_fn)(struct work_struct *work);
>  	struct hv_host_device *host_dev = shost_priv(host);
> -	bool do_work = false;
> 
> -	switch (SRB_STATUS(vm_srb->srb_status)) {
> -	case SRB_STATUS_ERROR:
> +	/*
> +	 * In some situations, Hyper-V sets multiple bits in the
> +	 * srb_status, such as ABORTED and ERROR. So process them
> +	 * individually, with the most specific bits first.
> +	 */
> +
> +	if (vm_srb->srb_status & SRB_STATUS_INVALID_LUN) {
> +		set_host_byte(scmnd, DID_NO_CONNECT);
> +		process_err_fn = storvsc_remove_lun;
> +		goto do_work;
> +	}
> +
> +	if (vm_srb->srb_status & SRB_STATUS_ABORTED) {
> +		if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID
> &&
> +		    /* Capacity data has changed */
> +		    (asc == 0x2a) && (ascq == 0x9)) {
> +			process_err_fn = storvsc_device_scan;
> +			/*
> +			 * Retry the I/O that triggered this.
> +			 */
> +			set_host_byte(scmnd, DID_REQUEUE);
> +			goto do_work;
> +		}
> +	}
> +
> +	if (vm_srb->srb_status & SRB_STATUS_ERROR) {

I'm curious on SRB_STATUS_INVALID_LUN and SRB_STATUS_ABORTED, the actions on those two codes are different. 

It doesn't look like we can get those two in the same status code.

>  		/*
>  		 * Let upper layer deal with error when
>  		 * sense message is present.
>  		 */
> -
>  		if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID)
> -			break;
> +			return;
> +
>  		/*
>  		 * If there is an error; offline the device since all
>  		 * error recovery strategies would have already been @@ -
> 1032,37 +1055,19 @@ static void storvsc_handle_error(struct vmscsi_request
> *vm_srb,
>  			set_host_byte(scmnd, DID_PASSTHROUGH);
>  			break;
>  		/*
> -		 * On Some Windows hosts TEST_UNIT_READY command can
> return
> -		 * SRB_STATUS_ERROR, let the upper level code deal with it
> -		 * based on the sense information.
> +		 * On some Hyper-V hosts TEST_UNIT_READY command can
> +		 * return SRB_STATUS_ERROR. Let the upper level code
> +		 * deal with it based on the sense information.
>  		 */
>  		case TEST_UNIT_READY:
>  			break;
>  		default:
>  			set_host_byte(scmnd, DID_ERROR);
>  		}
> -		break;
> -	case SRB_STATUS_INVALID_LUN:
> -		set_host_byte(scmnd, DID_NO_CONNECT);
> -		do_work = true;
> -		process_err_fn = storvsc_remove_lun;
> -		break;
> -	case SRB_STATUS_ABORTED:
> -		if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID
> &&
> -		    (asc == 0x2a) && (ascq == 0x9)) {
> -			do_work = true;
> -			process_err_fn = storvsc_device_scan;
> -			/*
> -			 * Retry the I/O that triggered this.
> -			 */
> -			set_host_byte(scmnd, DID_REQUEUE);
> -		}
> -		break;
>  	}
> +	return;
> 
> -	if (!do_work)
> -		return;
> -
> +do_work:
>  	/*
>  	 * We need to schedule work to process this error; schedule it.
>  	 */
> --
> 1.8.3.1

RE: [PATCH 3/3] scsi: storvsc: Correctly handle multiple flags in srb_status

[Michael Kelley]

From: Long Li <longli@microsoft.com> Sent: Monday, June 7, 2021 3:25 PM
> >
> > Hyper-V is observed to sometimes set multiple flags in the srb_status, such
> > as ABORTED and ERROR. Current code in
> > storvsc_handle_error() handles only a single flag being set, and does nothing
> > when multiple flags are set.  Fix this by changing the case statement into a
> > series of "if" statements testing individual flags. The functionality for handling
> > each flag is unchanged.
> >
> > Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> > ---
> >  drivers/scsi/storvsc_drv.c | 61 +++++++++++++++++++++++++----------------
> > -----
> >  1 file changed, 33 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c index
> > fff9441..e96d2aa 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -1009,17 +1009,40 @@ static void storvsc_handle_error(struct
> > vmscsi_request *vm_srb,
> >  	struct storvsc_scan_work *wrk;
> >  	void (*process_err_fn)(struct work_struct *work);
> >  	struct hv_host_device *host_dev = shost_priv(host);
> > -	bool do_work = false;
> >
> > -	switch (SRB_STATUS(vm_srb->srb_status)) {
> > -	case SRB_STATUS_ERROR:
> > +	/*
> > +	 * In some situations, Hyper-V sets multiple bits in the
> > +	 * srb_status, such as ABORTED and ERROR. So process them
> > +	 * individually, with the most specific bits first.
> > +	 */
> > +
> > +	if (vm_srb->srb_status & SRB_STATUS_INVALID_LUN) {
> > +		set_host_byte(scmnd, DID_NO_CONNECT);
> > +		process_err_fn = storvsc_remove_lun;
> > +		goto do_work;
> > +	}
> > +
> > +	if (vm_srb->srb_status & SRB_STATUS_ABORTED) {
> > +		if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID &&
> > +		    /* Capacity data has changed */
> > +		    (asc == 0x2a) && (ascq == 0x9)) {
> > +			process_err_fn = storvsc_device_scan;
> > +			/*
> > +			 * Retry the I/O that triggered this.
> > +			 */
> > +			set_host_byte(scmnd, DID_REQUEUE);
> > +			goto do_work;
> > +		}
> > +	}
> > +
> > +	if (vm_srb->srb_status & SRB_STATUS_ERROR) {
> 
> I'm curious on SRB_STATUS_INVALID_LUN and SRB_STATUS_ABORTED, the actions on
> those two codes are different.
> 
> It doesn't look like we can get those two in the same status code.

Agreed.  I've only observed having ERROR and ABORTED in the same status code.
That happens when a bad PFN is passed on a I/O request, which can be easily
tested.  With the old code, having ERROR and ABORTED together resulted in doing
nothing.

The behavior of INVALID_LUN is unchanged by this patch.  But with this patch,
if INVALID_LUN and ABORTED should ever occur together, we would process the
INVALID_LUN error, and ignore ABORTED, which I think makes sense. 
INVALID_LUN is the most serious error, so we process it first.  Then ABORTED is
next in priority for the specific case of "Capacity data has changed".  Finally we
handle ERROR.

Michael

> 
> >  		/*
> >  		 * Let upper layer deal with error when
> >  		 * sense message is present.
> >  		 */
> > -
> >  		if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID)
> > -			break;
> > +			return;
> > +
> >  		/*
> >  		 * If there is an error; offline the device since all
> >  		 * error recovery strategies would have already been @@ -
> > 1032,37 +1055,19 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb,
> >  			set_host_byte(scmnd, DID_PASSTHROUGH);
> >  			break;
> >  		/*
> > -		 * On Some Windows hosts TEST_UNIT_READY command can
> > return
> > -		 * SRB_STATUS_ERROR, let the upper level code deal with it
> > -		 * based on the sense information.
> > +		 * On some Hyper-V hosts TEST_UNIT_READY command can
> > +		 * return SRB_STATUS_ERROR. Let the upper level code
> > +		 * deal with it based on the sense information.
> >  		 */
> >  		case TEST_UNIT_READY:
> >  			break;
> >  		default:
> >  			set_host_byte(scmnd, DID_ERROR);
> >  		}
> > -		break;
> > -	case SRB_STATUS_INVALID_LUN:
> > -		set_host_byte(scmnd, DID_NO_CONNECT);
> > -		do_work = true;
> > -		process_err_fn = storvsc_remove_lun;
> > -		break;
> > -	case SRB_STATUS_ABORTED:
> > -		if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID &&
> > -		    (asc == 0x2a) && (ascq == 0x9)) {
> > -			do_work = true;
> > -			process_err_fn = storvsc_device_scan;
> > -			/*
> > -			 * Retry the I/O that triggered this.
> > -			 */
> > -			set_host_byte(scmnd, DID_REQUEUE);
> > -		}
> > -		break;
> >  	}
> > +	return;
> >
> > -	if (!do_work)
> > -		return;
> > -
> > +do_work:
> >  	/*
> >  	 * We need to schedule work to process this error; schedule it.
> >  	 */
> > --
> > 1.8.3.1

Re: [PATCH 1/3] scsi: storvsc: Miscellaneous code cleanups

[Martin K. Petersen]

Michael,

> As general cleanup and in preparation for subsequent patches:

Applied 1-3 to 5.14/scsi-staging.

Since Hannes' series has deprecated status_byte() and CHECK_CONDITION I
had to tweak that portion. Please verify my conflict resolution.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH 1/3] scsi: storvsc: Miscellaneous code cleanups

[Michael Kelley]

From: Martin K. Petersen <martin.petersen@oracle.com> Sent: Tuesday, June 15, 2021 7:25 PM
> 
> Michael,
> 
> > As general cleanup and in preparation for subsequent patches:
> 
> Applied 1-3 to 5.14/scsi-staging.
> 
> Since Hannes' series has deprecated status_byte() and CHECK_CONDITION I
> had to tweak that portion. Please verify my conflict resolution.
> 

Unfortunately, it's not quite right.  The line of code in question needs to be

if ((vstor_packet->vm_srb.scsi_status & 0xFF) == SAM_STAT_CHECK_CONDITION &&

The status_byte() helper was doing the masking as well as the right shift, so the
masking will need to be open coded.  The replacement get_status_byte() helper 
won't work here because it's based on a scsi_cmnd structure.

Michael

Re: [PATCH 1/3] scsi: storvsc: Miscellaneous code cleanups

[Martin K. Petersen]

Hello Michael,

> Unfortunately, it's not quite right.  The line of code in question
> needs to be
>
> if ((vstor_packet->vm_srb.scsi_status & 0xFF) == SAM_STAT_CHECK_CONDITION &&

> The status_byte() helper was doing the masking as well as the right
> shift, so the masking will need to be open coded.

CHECK_CONDITION is obsolete so no shifting is required for the SAM
status. And as far as I can tell vm_srb.scsi_status is a u8:

struct vmscsi_request {
        u16 length;
        u8 srb_status;
        u8 scsi_status;
[...]

-- 
Martin K. Petersen	Oracle Linux Engineering

RE: [PATCH 1/3] scsi: storvsc: Miscellaneous code cleanups

[Michael Kelley]

From: Martin K. Petersen <martin.petersen@oracle.com> Sent: Wednesday, June 16, 2021 1:07 PM
> 
> > Unfortunately, it's not quite right.  The line of code in question
> > needs to be
> >
> > if ((vstor_packet->vm_srb.scsi_status & 0xFF) == SAM_STAT_CHECK_CONDITION &&
> 
> > The status_byte() helper was doing the masking as well as the right
> > shift, so the masking will need to be open coded.
> 
> CHECK_CONDITION is obsolete so no shifting is required for the SAM
> status. And as far as I can tell vm_srb.scsi_status is a u8:
> 
> struct vmscsi_request {
>         u16 length;
>         u8 srb_status;
>         u8 scsi_status;
> [...]
> 

Indeed, you are right!  I was trying to preserve the masking with 0xFF
from the original code before my patch, but that masking was
spurious.  :-(   So yes, it's good.

Thanks,

Michael