Re: [edk2-devel] [PATCH v5 06/12] OvmfPkg/MptScsiDxe: Report targets and one LUN

["Laszlo Ersek" <lersek@redhat.com>]

On 04/29/20 16:45, Liran Alon wrote:
> 
> On 29/04/2020 16:39, Laszlo Ersek wrote:
>> On 04/29/20 15:38, Laszlo Ersek wrote:
>>> On 04/24/20 19:59, Nikita Leshenko wrote:
>>>> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
>>>> b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
>>>> index 9f7c98829ee1..4862ff9dd497 100644
>>>> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
>>>> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
>>>> @@ -24,6 +24,7 @@ [Packages]
>>>>     OvmfPkg/OvmfPkg.dec
>>>>     [LibraryClasses]
>>>> +  BaseMemoryLib
>>>>     DebugLib
>>>>     MemoryAllocationLib
>>>>     UefiBootServicesTableLib
>>>> @@ -33,3 +34,6 @@ [LibraryClasses]
>>>>   [Protocols]
>>>>     gEfiExtScsiPassThruProtocolGuid        ## BY_START
>>>>     gEfiPciIoProtocolGuid                  ## TO_START
>>>> +
>>>> +[FixedPcd]
>>>> +  gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit   ## CONSUMES
>>>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>>>> index 28030391cff2..2d09444bbb16 100644
>>>> --- a/OvmfPkg/OvmfPkg.dec
>>>> +++ b/OvmfPkg/OvmfPkg.dec
>>>> @@ -163,6 +163,10 @@ [PcdsFixedAtBuild]
>>>>     #  polling loop iteration.
>>>>    
>>>> gUefiOvmfPkgTokenSpaceGuid.PcdPvScsiWaitForCmpStallInUsecs|5|UINT32|0x38
>>>>
>>>>   +  ## Set the *inclusive* number of targets that MptScsi exposes
>>>> for scan
>>>> +  #  by ScsiBusDxe.
>>>> +  gUefiOvmfPkgTokenSpaceGuid.PcdMptScsiMaxTargetLimit|7|UINT8|0x39
>>>> +
>>>>    
>>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
>>>>
>>>>    
>>>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
>>>>
>>>>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
>>>>
>>> (1) <PcdLib.h> should be #include'd in this patch, and PcdLib should be
>>> listed under [LibraryClasses].
>>>
>>> On one hand, that's a minor wart. The driver (and the OVMF platform(s))
>>> build OK at this stage, in practice. (I tested that.) So this, per se,
>>> does not justify a v6.
>>>
>>> On the other hand, even at the end of the series, the module does not
>>> spell out the PcdLib dependency (neither #include nor [LibraryClasses]).
>>> That's not so nice.
>>>
>>> But, I'll fix that up for you, if there's not going to be another reason
>>> for a v6.
>>>
>>> With (1) addressed (by you, or by me):
>>>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> Thanks,
>>> Laszlo
>>>
>> ... BTW I missed the same in the PvScsiDxe driver :/
>>
>> Liran, can you post a patch for that, please?
>>
>> Thanks,
>> Laszlo
> 
> 
> Sure. Will do. Quite surprised it builds successfully without it.
> BTW, VirtioScsi DXE driver also seems to be missing PcdLib dependency
> both in #include and [LibraryClasses] as-well.
> I can submit a patch for that as-well if you like.

Yes please!

Such dependencies are easy to miss, on commonly used / low-level lib
classes. Usually one of the higher-level lib class headers pulls in the
lib class header in question. And the [LibraryClasses] part remains
hidden because a lib instance already (explicitly or imlicitly) consumed
makes the consumer module inherit the [LibraryClasses] dependency too.

Thanks
Laszlo