On 03/30/20 19:24, Liran Alon wrote:
>
> On 30/03/2020 18:54, Laszlo Ersek wrote:
>> On 03/28/20 21:00, Liran Alon wrote:
>>> @@ -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:
>>
>> [...]
>>
>> (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.
OK. That's a relief to me, thank you! And I apologize for insinuating
that you deliberately & silently ignored said feedback from me. I tend
to be pedantic about email (it's not natural for me to think that
someone simply missed a comment) -- sorry about that!
Given that I've already merged v3 -- can you please post a followup
patch? Something like:
> diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c
> index 0a66c98421a9..3066b83b96fc 100644
> --- a/OvmfPkg/PvScsiDxe/PvScsi.c
> +++ b/OvmfPkg/PvScsiDxe/PvScsi.c
> @@ -1093,6 +1093,12 @@ PvScsiFreeRings (
> IN OUT PVSCSI_DEV *Dev
> )
> {
> + //
> + // Reset device to stop device usage of the rings.
> + // This is required to safely free the rings.
> + //
> + PvScsiResetAdapter (Dev);
> +
> PvScsiFreeSharedPages (
> Dev,
> 1,
> @@ -1199,12 +1205,6 @@ PvScsiInit (
> return EFI_SUCCESS;
>
> FreeRings:
> - //
> - // Reset device to stop device usage of the rings.
> - // This is required to safely free the rings.
> - //
> - PvScsiResetAdapter (Dev);
> -
> PvScsiFreeRings (Dev);
>
> RestorePciAttributes:
> @@ -1220,12 +1220,8 @@ PvScsiUninit (
> )
> {
> //
> - // Reset device to:
> - // - Make device stop processing all requests.
> - // - Stop device usage of the rings.
> - //
> - // This is required to safely free the DMA communication buffer
> - // and the rings.
> + // Reset device to make device stop processing all requests.
> + // This is required to safely free the DMA communication buffer.
> //
> PvScsiResetAdapter (Dev);
Thank you!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56683): https://edk2.groups.io/g/devel/message/56683
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]
-=-=-=-=-=-=-=-=-=-=-=-
