On 30/03/2020 18:54, Laszlo Ersek wrote:
On 03/28/20 21:00, Liran Alon wrote:
STATIC
EFI_STATUS
PvScsiInit (
@@ -466,6 +669,14 @@ PvScsiInit (
goto RestorePciAttributes;
}
+ //
+ // Init PVSCSI rings
+ //
+ Status = PvScsiInitRings (Dev);
+ if (EFI_ERROR (Status)) {
+ goto RestorePciAttributes;
+ }
+
//
// Populate the exported interface's attributes
//
@@ -509,6 +720,14 @@ PvScsiUninit (
IN OUT PVSCSI_DEV *Dev
)
{
+ //
+ // Reset device to stop device usage of the rings.
+ // This is required to safely free the rings.
+ //
+ PvScsiResetAdapter (Dev);
If I understand correctly (at the end of the series):
in order to save one of the (ultimately) two reset calls in
PvScsiUninit(), namely
- one for releasing the DMA buf
- and the other (internal to PvScsiFreeRings()) for releasing the rings,
you unnested the latter reset call from PvScsiFreeRings(), and added it
manually to the error path of PvScsiInit().
Right.
To me, that's more brittle and more difficult to reason about than what
I proposed, because now PvScsiFreeRings() does not completely undo
PvScsiInitRings().
Hmm right. Because PvScsiInitRings() also setup the ring to the device
with a command. Haven't thought about it.
I specifically requested that we please not try to
save on the two (seemingly) redundant reset calls in PvScsiUninit() --
see the paragraph containing "bad idea" here:
https://urldefense.com/v3/__http://mid.mail-archive.com/45e5950c-ec82-c24b-4c38-2912402747b2@redhat.com__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-Fn-EyPU$
https://urldefense.com/v3/__https://edk2.groups.io/g/devel/message/56425__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-4bfLR8U$
(Note: we could be tempted to somehow "centralize" all of these
Reset operations into a single spot. Bad idea. We are revoking the
device's access rights to different resources, so the revocation
operations will show up in different spots. It's a mere circumstance
that the revocations all happen to be Reset operations.)
I might be paranoid of course -- I just feel that maybe-superfluous
reset operations on error paths are much better than silently
corrupted guest memory and/or disk contents.
You reacted to that message of mine, but not this paragraph specifically
(it was snipped from your followup) -- so I thought you were OK with it.
I'm a bit disappointed that you disagreed with my request *silently*.
Oh now I got what you meant! I misinterpreted it. Not silently ignored
it of course!
I can change this in a v4 patch-series if you would like that. Sorry for
the misunderstanding.
I do agree with your statement that PvScsiFreeRings() is not completely
an undo of PvScsiInitRings().
And maybe for making code a little bit easier to reason about, it's
preferred to just do one additional unnecessary device reset.
To summarize: technically, your solution is correct; from a code hygiene
and resource ownership question, I'm convinced it is wrong.
I agree.
But, I'll live with it.
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
I can send a v4 patch-series just changing that if you would like.
Or submit a patch to change it after it will be merged.
Or that you will change it when applying. Or leave it as is.
I don't have any objection to any of the above.
(Also, you didn't set up the "diff.orderFile" knob the way I requested
in point (8):
I actually did set it up in my ~/.gitconfig. It seems that it doesn't
work from some reason...
https://urldefense.com/v3/__http://mid.mail-archive.com/0739202a-9b8a-759d-5809-2f2df69e9352@redhat.com__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-bs34O4s$
https://urldefense.com/v3/__https://edk2.groups.io/g/devel/message/56420__;!!GqivPVa7Brio!MtQ2Nlgza8eW9tXH6x5jj0EZQbLphEJgXBmIFjEur-nxyjxDufyi4uk-msgEmGo$
and so the header file changes are again at the end of the patch...
Another thing I'll have to live with, I guess.)
Laszlo
Thanks for the understanding,
-Liran
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56675): https://edk2.groups.io/g/devel/message/56675
Mute This Topic: https://groups.io/mt/72617147/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
