On 24/03/2020 18:43, Laszlo Ersek wrote:
On 03/16/20 16:01, Liran Alon wrote:
+STATIC
+BOOLEAN
+PvScsiIsReqRingFull (
+ IN CONST PVSCSI_DEV *Dev
+ )
+{
+ PVSCSI_RINGS_STATE *RingsState;
+ UINT64 ReqNumEntries;
+
+ RingsState = Dev->RingDesc.RingState;
+ ReqNumEntries = 1 << RingsState->ReqNumEntriesLog2;
(5) Wrong for two reasons:
(5a) Based on ReqNumEntries having type UINT64, the shift count may
presumably be larger than 31. But the constant "1" has type "signed int"
(mapping to INT32 in edk2), and so we should never left-shift that by
even 31 positions (we should never shift bits into the sign bit). Let
alone by more than 31 positions.
In other words, the constant should be 1ULL.
(5b) Please use RShiftU64() from BaseLib.
Actually, I have noticed that ReqNumEntries should just be UINT32 and "1
<<" should be changed to "1U <<".
So I made these changes for v2. Thanks.
@@ -135,7 +492,70 @@ PvScsiPassThru (
IN EFI_EVENT Event OPTIONAL
)
{
...
+ Dev->RingDesc.RingState->ReqProdIdx++;
+
+ Status = PvScsiMmioWrite32 (Dev, PVSCSI_REG_OFFSET_KICK_RW_IO, 0);
+ if (EFI_ERROR (Status)) {
+ //
+ // If kicking the host fails, we must fake a host adapter error.
+ // EFI_NOT_READY would save us the effort, but it would also suggest that
+ // the caller retry.
+ //
+ goto FakeHostAdapterError;
+ }
(11) Hmmm. Not really happy about this. It doesn't feel like actual
error handling (= resource release / rollback); we're just factoring out
response composition. That's OK per se, but then it belongs to a helper
function, not a "function epilogue" here.
Ok. I will move it to a helper function in v2.
-Liran
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56229): https://edk2.groups.io/g/devel/message/56229
Mute This Topic: https://groups.io/mt/72001285/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
