From 281bb7b3c5e216fb08449ec05cfe748462ab1ff5 Mon Sep 17 00:00:00 2001 From: benyamin-codez <115509179+benyamin-codez@users.noreply.github.com> Date: Thu, 19 Sep 2024 14:00:28 +1000 Subject: [PATCH 1/3] [vioscsi] SendSRB minor refactor The virtqueue_add_buf() routine will return 0 on SUCCESS or otherwise a negative number, usually -28 (ENOSPC), i.e no space for buffer. An inline comment is added for edification. Here we refactor virtqueue_add_buf() handling to only process the SRB if virtqueue_add_buf() returns SUCCESS. Presently, other positive return codes would be processed. Here we define VQ_ADD_BUFFER_SUCCESS in vioscsi\helper.h To ensure valid data is reported we also move the trace event on failure to above the call to CompleteRequest() and wrap the line for improved readability. Signed-off-by: benyamin-codez <115509179+benyamin-codez@users.noreply.github.com> --- vioscsi/helper.c | 7 +++++-- vioscsi/helper.h | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/vioscsi/helper.c b/vioscsi/helper.c index 9574a3111..1a08bd21f 100755 --- a/vioscsi/helper.c +++ b/vioscsi/helper.c @@ -109,7 +109,7 @@ ENTER_FN_SRB(); srbExt->out, srbExt->in, &srbExt->cmd, va, pa); - if (res >= 0) { + if (res == VQ_ADD_BUFFER_SUCCESS) { element = &adaptExt->processing_srbs[index]; InsertTailList(&element->srb_list, &srbExt->list_entry); element->srb_cnt++; @@ -120,13 +120,16 @@ ENTER_FN_SRB(); virtqueue_notify(adaptExt->vq[QueueNumber]); } } else { + // virtqueue_add_buf() returned -28 (ENOSPC), i.e. no space for buffer, or some other error virtqueue_notify(adaptExt->vq[QueueNumber]); ScsiStatus = SCSISTAT_QUEUE_FULL; SRB_SET_SRB_STATUS(Srb, SRB_STATUS_BUSY); SRB_SET_SCSI_STATUS(Srb, ScsiStatus); StorPortBusy(DeviceExtension, 10); + RhelDbgPrint(TRACE_LEVEL_WARNING, + " Could not put an SRB into a VQ, so complete it with SRB_STATUS_BUSY. QueueNumber = %lu, SRB = 0x%p, Lun = %d, TimeOut = %d.\n", + QueueNumber, srbExt->Srb, SRB_LUN(Srb), Srb->TimeOutValue); CompleteRequest(DeviceExtension, Srb); - RhelDbgPrint(TRACE_LEVEL_FATAL, " Could not put an SRB into a VQ, so complete it with SRB_STATUS_BUSY. QueueNumber = %d, SRB = 0x%p, Lun = %d, TimeOut = %d.\n", QueueNumber, srbExt->Srb, SRB_LUN(Srb), Srb->TimeOutValue); } EXIT_FN_SRB(); diff --git a/vioscsi/helper.h b/vioscsi/helper.h index ac38977f0..408b20b4a 100755 --- a/vioscsi/helper.h +++ b/vioscsi/helper.h @@ -49,6 +49,8 @@ #define CACHE_LINE_SIZE 64 #define ROUND_TO_CACHE_LINES(Size) (((ULONG_PTR)(Size) + CACHE_LINE_SIZE - 1) & ~(CACHE_LINE_SIZE - 1)) +#define VQ_ADD_BUFFER_SUCCESS 0 + #include // Note: SrbGetCdbLength is defined in srbhelper.h From c2240c846a63bb86b70f62db70ed6f2a4d8d9661 Mon Sep 17 00:00:00 2001 From: benyamin-codez <115509179+benyamin-codez@users.noreply.github.com> Date: Fri, 20 Sep 2024 12:12:06 +1000 Subject: [PATCH 2/3] [vioscsi] SendSRB additional minor refactor From @JonKohler review feedback: 1. Use VQ_ADD_BUFFER_SUCCESS definition in virtqueue_add_buf() status variable init 2. Add virtqueue_add_buf() status variable to trace output on error Also: 1. Cleaned up init table 2. Replaced mentions of index variable with vq_req_idx Signed-off-by: benyamin-codez <115509179+benyamin-codez@users.noreply.github.com> --- vioscsi/helper.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/vioscsi/helper.c b/vioscsi/helper.c index 1a08bd21f..62d90e7a5 100755 --- a/vioscsi/helper.c +++ b/vioscsi/helper.c @@ -54,10 +54,10 @@ SendSRB( STOR_LOCK_HANDLE LockHandle = { 0 }; ULONG status = STOR_STATUS_SUCCESS; UCHAR ScsiStatus = SCSISTAT_GOOD; - ULONG MessageID; - int res = 0; + ULONG MessageID; + INT add_buffer_req_status = VQ_ADD_BUFFER_SUCCESS; PREQUEST_LIST element; - ULONG index; + ULONG vq_req_idx; ENTER_FN_SRB(); if (!Srb) @@ -93,7 +93,7 @@ ENTER_FN_SRB(); } MessageID = QUEUE_TO_MESSAGE(QueueNumber); - index = QueueNumber - VIRTIO_SCSI_REQUEST_QUEUE_0; + vq_req_idx = QueueNumber - VIRTIO_SCSI_REQUEST_QUEUE_0; if (adaptExt->reset_in_progress) { RhelDbgPrint(TRACE_LEVEL_FATAL, " Reset is in progress, completing SRB 0x%p with SRB_STATUS_BUS_RESET.\n", Srb); @@ -104,13 +104,13 @@ ENTER_FN_SRB(); VioScsiVQLock(DeviceExtension, MessageID, &LockHandle, FALSE); SET_VA_PA(); - res = virtqueue_add_buf(adaptExt->vq[QueueNumber], - srbExt->psgl, - srbExt->out, srbExt->in, - &srbExt->cmd, va, pa); + add_buffer_req_status = virtqueue_add_buf(adaptExt->vq[QueueNumber], + srbExt->psgl, + srbExt->out, srbExt->in, + &srbExt->cmd, va, pa); - if (res == VQ_ADD_BUFFER_SUCCESS) { - element = &adaptExt->processing_srbs[index]; + if (add_buffer_req_status == VQ_ADD_BUFFER_SUCCESS) { + element = &adaptExt->processing_srbs[vq_req_idx]; InsertTailList(&element->srb_list, &srbExt->list_entry); element->srb_cnt++; } @@ -127,8 +127,8 @@ ENTER_FN_SRB(); SRB_SET_SCSI_STATUS(Srb, ScsiStatus); StorPortBusy(DeviceExtension, 10); RhelDbgPrint(TRACE_LEVEL_WARNING, - " Could not put an SRB into a VQ, so complete it with SRB_STATUS_BUSY. QueueNumber = %lu, SRB = 0x%p, Lun = %d, TimeOut = %d.\n", - QueueNumber, srbExt->Srb, SRB_LUN(Srb), Srb->TimeOutValue); + " Could not put an SRB into a VQ due to error %i. To be completed with SRB_STATUS_BUSY. QueueNumber = %lu, SRB = 0x%p, Lun = %d, TimeOut = %d.\n", + add_buffer_req_status, QueueNumber, srbExt->Srb, SRB_LUN(Srb), Srb->TimeOutValue); CompleteRequest(DeviceExtension, Srb); } From 5826297f611a56515fff4bb61e76d087dfa4a544 Mon Sep 17 00:00:00 2001 From: benyamin-codez <115509179+benyamin-codez@users.noreply.github.com> Date: Sat, 21 Sep 2024 03:38:30 +1000 Subject: [PATCH 3/3] [vioscsi] SendSRB minor refactor cleanup Revert variable add_buffer_req_status to res to complete build scripts Will raise new PR for these changes Signed-off-by: benyamin-codez <115509179+benyamin-codez@users.noreply.github.com> --- vioscsi/helper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/vioscsi/helper.c b/vioscsi/helper.c index 62d90e7a5..43e85f410 100755 --- a/vioscsi/helper.c +++ b/vioscsi/helper.c @@ -55,7 +55,7 @@ SendSRB( ULONG status = STOR_STATUS_SUCCESS; UCHAR ScsiStatus = SCSISTAT_GOOD; ULONG MessageID; - INT add_buffer_req_status = VQ_ADD_BUFFER_SUCCESS; + INT res = VQ_ADD_BUFFER_SUCCESS; PREQUEST_LIST element; ULONG vq_req_idx; ENTER_FN_SRB(); @@ -104,12 +104,12 @@ ENTER_FN_SRB(); VioScsiVQLock(DeviceExtension, MessageID, &LockHandle, FALSE); SET_VA_PA(); - add_buffer_req_status = virtqueue_add_buf(adaptExt->vq[QueueNumber], + res = virtqueue_add_buf(adaptExt->vq[QueueNumber], srbExt->psgl, srbExt->out, srbExt->in, &srbExt->cmd, va, pa); - if (add_buffer_req_status == VQ_ADD_BUFFER_SUCCESS) { + if (res == VQ_ADD_BUFFER_SUCCESS) { element = &adaptExt->processing_srbs[vq_req_idx]; InsertTailList(&element->srb_list, &srbExt->list_entry); element->srb_cnt++; @@ -128,7 +128,7 @@ ENTER_FN_SRB(); StorPortBusy(DeviceExtension, 10); RhelDbgPrint(TRACE_LEVEL_WARNING, " Could not put an SRB into a VQ due to error %i. To be completed with SRB_STATUS_BUSY. QueueNumber = %lu, SRB = 0x%p, Lun = %d, TimeOut = %d.\n", - add_buffer_req_status, QueueNumber, srbExt->Srb, SRB_LUN(Srb), Srb->TimeOutValue); + res, QueueNumber, srbExt->Srb, SRB_LUN(Srb), Srb->TimeOutValue); CompleteRequest(DeviceExtension, Srb); }