| Message ID | 1622827263-12516-1-git-send-email-mikelley@microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] scsi: storvsc: Miscellaneous code cleanups | expand |
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
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
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
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
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:
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(-)