Re: [edk2-devel] [PATCH v3 13/17] OvmfPkg/PvScsiDxe: Setup requests and completions rings

["Liran Alon" <liran.alon@oracle.com>]

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 <lersek@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