Re: [edk2-devel] [PATCH v5 10/12] OvmfPkg/MptScsiDxe: Initialize hardware

["Nikita Leshenko" <nikita.leshchenko@oracle.com>]

> On 29 Apr 2020, at 17:55, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 04/24/20 19:59, Nikita Leshenko wrote:
>> [...]
>> +STATIC
>> +EFI_STATUS
>> +MptScsiInit (
>> +  IN MPT_SCSI_DEV       *Dev
>> +  )
>> +{
>> +  EFI_STATUS                     Status;
>> +  MPT_IO_CONTROLLER_INIT_REQUEST Req;
> 
> [...]
> 
> You can either introduce a new typedef for the alignment / union under
> IndustryStandard, or just define an ad-hoc union here in this function,
> like PvScsiDxe does.
Yes you're right, I missed that, sorry.

> 
> [...]
> "Please use another STATIC_ASSERT here for expressing that the response
> structure size is an integer multiple of sizeof (UINT16)."
Missed that too, sorry
> 
>> [...]
>> //
>> // Ext SCSI Pass Thru
>> //
>> @@ -382,6 +557,11 @@ MptScsiControllerStart (
>>       ));
>>   }
>> 
>> +  Status = MptScsiInit (Dev);
>> +  if (EFI_ERROR (Status)) {
>> +    goto RestoreAttributes;
> 
> Hmmm, git-range-diff flags this as a v4->v5 change, and I don't
> understand why...
> 
> Ah, OK. In v4, we jumped to "RestorePciAttributes" -- which was a
> non-existent label. So I think the v4 variant of this patch didn't
> compile. I hope that's fixed now, with the above. :)
Yes, I ran `git rebase --exec` to make sure that all patches compiled and I noticed that.

Nikita
> 
> The rest of the updates / patch look fine to me.
> 
> Thanks!
> Laszlo
> 
> 
>> +  }
>> +
>>   //
>>   // Host adapter channel, doesn't exist
>>   //
>> @@ -406,11 +586,14 @@ MptScsiControllerStart (
>>                   &Dev->PassThru
>>                   );
>>   if (EFI_ERROR (Status)) {
>> -    goto RestoreAttributes;
>> +    goto UninitDev;
>>   }
>> 
>>   return EFI_SUCCESS;
>> 
>> +UninitDev:
>> +  MptScsiReset (Dev);
>> +
>> RestoreAttributes:
>>   Dev->PciIo->Attributes (
>>                 Dev->PciIo,
>> @@ -470,6 +653,8 @@ MptScsiControllerStop (
>>     return Status;
>>   }
>> 
>> +  MptScsiReset (Dev);
>> +
>>   Dev->PciIo->Attributes (
>>                 Dev->PciIo,
>>                 EfiPciIoAttributeOperationSet,
>> 
>